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

[Michael Niedermayer]

On Mon, Apr 07, 2008 at 11:40:38AM +0000, Bj?rn Axelsson wrote:
> 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.

Both patches look ok

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080408/90dd1d90/attachment.pgp>