[FFmpeg-devel] [RFC][PATCH] av_fifo_write_from_bytestream()

[Björn Axelsson]

On Fri, 2008-04-04 at 14:05 +0200, Michael Niedermayer wrote:

[...]

> [...]
> 
> > Index: libavutil/fifo.h
> > ===================================================================
> > --- libavutil/fifo.h.orig	2008-04-03 13:25:00.000000000 +0200
> > +++ libavutil/fifo.h	2008-04-04 13:04:37.092855408 +0200
> > @@ -76,7 +76,23 @@
> >   * @param *buf data source
> >   * @param size data size
> >   */
> > -void av_fifo_write(AVFifoBuffer *f, const uint8_t *buf, int size);
> > +attribute_deprecated void av_fifo_write(AVFifoBuffer *f, const uint8_t *buf, int size);
> > +
> > +/**
> > + * Feeds data from a user supplied callback to an AVFifoBuffer.
> > + * @param *f AVFifoBuffer to write to
> > + * @param size number of bytes to write
> 
> > + * @param *func generic write function. First parameter is src,
> > + * second is dest_buf, third is dest_buf_size.
> 
> memcpy(void *destination, void *source, size_t size)

See av_fifo_generic_read() which you pointed me at from the start, and
also get_buffer(ByteIOContext *s, unsigned char *buf, int size)

It seems to me that it is better not to require a wrapper for get_buffer
than to copy memcpy's parameter order.

Not changed in the new patch.

> > + * func must return the number of bytes written to dest_buf, or an AVERROR()
> > + * code in case of failure. A return value of 0 indicates no more data
> > + * available to write.
> > + * If func is NULL, src is interpreted as a simple byte array for source data.
> > + * @param *src data source
> > + * @return the number of bytes written to the fifo, or an AVERROR() code if no
> > + * data was written and an error occured.
> > + */
> > +int av_fifo_generic_write(AVFifoBuffer *f, int size, int (*func)(void*, void*, int), void* src);
> 
> please dont reorder the arguments
> (AVFifoBuffer *f, void* src, size_t size, int (*func)(void*, void*, size_t));
> is fine

Again, the order and parameter types are from av_fifo_generic_read(). So
whatever I do there will be inconsistent and confusing :-(
Anyway, parameter order changed in this patch. Let me know if you change
your mind.

> >  
> >  /**
> >   * Resizes an AVFifoBuffer.
> > Index: libavutil/fifo.c
> > ===================================================================
> > --- libavutil/fifo.c.orig	2008-04-03 13:24:58.000000000 +0200
> > +++ libavutil/fifo.c	2008-04-04 12:23:17.425391146 +0200
> > @@ -73,15 +73,30 @@
> >  
> >  void av_fifo_write(AVFifoBuffer *f, const uint8_t *buf, int size)
> >  {
> > +    av_fifo_generic_write(f, size, NULL, (void *)buf);
> > +}
> > +
> 
> > +int av_fifo_generic_write(AVFifoBuffer *f, int size, int (*func)(void*, void*, int), void* src)
> > +{
> > +    int total = size;
> >      do {
> >          int len = FFMIN(f->end - f->wptr, size);
> > +        if(func) {
> 
> > +            len = func(src, f->wptr, len);
> > +            if(len == 0 || (len < 0 && size != total))
> > +                return total - size;
> > +            else if(len < 0)
> > +                return len;
> 
> What is this good for? With that one has to
> x= av_fifo_generic_write
> if(x<0) x=0
> 
> or
> 
> x= av_fifo_generic_write
> if(x!=len) x=ctx->error;
> 
> depending on what one wants, if you would return the length it would be
> 
> x= av_fifo_generic_write
> 
> 
> and
> 
> av_fifo_generic_write
> x=ctx->error;
> 
> so please use:
> 
> len = func(src, f->wptr, len);
> if(len<=0)
>     break;
> 

Hmm. I was thinking that the error code would be lost if we don't return
it. But url_ferror() is of course ok for ByteIOContexts, which is good
enough for me.

Implementation changed and docs updated.

> > +        } else {
> 
> > +            memcpy(f->wptr, src, len);
> > -        memcpy(f->wptr, buf, len);
> 
> cosmetic

Hmm, I guess you mean that both the parameter name change and the
indentation are cosmetics. I moved both to a separate patch.

-- 
Bj?rn Axelsson                    Phone: +46-(0)90-18 98 97
Intinor AB                          Fax: +46-(0)920-757 10
www.intinor.se
Interactive Television & Digital Media Distribution
-------------- next part --------------
A non-text attachment was scrubbed...
Name: av_fifo_generic_write.diff
Type: text/x-patch
Size: 2064 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080407/b24f604b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: av_fifo_generic_write_cosmetic.diff
Type: text/x-patch
Size: 2134 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080407/b24f604b/attachment-0001.bin>