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

Björn Axelsson bjorn.axelsson
Fri Apr 4 13:11:32 CEST 2008


On Thu, 2008-04-03 at 21:08 +0200, Michael Niedermayer wrote:
> On Thu, Apr 03, 2008 at 02:57:07PM +0000, Bj?rn Axelsson wrote:
> > On Thu, 2008-04-03 at 14:31 +0200, Michael Niedermayer wrote:
> > > On Thu, Apr 03, 2008 at 11:41:23AM +0000, Bj?rn Axelsson wrote:
> > > > On Thu, 2008-04-03 at 13:06 +0200, Michael Niedermayer wrote:
> > > > > On Thu, Apr 03, 2008 at 12:14:36PM +0200, Bj?rn Axelsson wrote:
> > > > > > Hi list,
> > > > > > when changing the mms protocol over to use fifos I need a way to
> > > > > > efficiently read data from a ByteIOContext into an AVFifoBuffer. 
> > > > > > 
> > > > > > The attached patch adds such a function to fifo.(c|h) but also makes
> > > > > > those files depend on avformat.h
> > > > > 
> > > > > rejected
> > > > > see av_fifo_generic_read()
> > > > 
> > > > Right you are, and blind I am...
> > > > 
> > > > The attached patch implements av_fifo_generic_write() modeled after
> > > > av_fifo_generic_read().
> > > > 
> > > > I'll send a separate patch for reimplementing av_fifo_write() using
> > > > av_fifo_generic_write() when (if) this is accepted.
> > > 
> > > What about replacing av_fifo_write() by av_fifo_generic_write() and
> > > providing a wraper (maybe with attribute_deprecated)?
> > > If you now say thats what you are doing then look at your patch, you
> > > dont replace av_fifo_write() you add a new function.
> > > That is you add code duplication with the intent to remove the old code
> > > in a subsequent patch. This hides the changes between the old and new
> > > code.
> > 
> > ok, patches merged.
> > 
> > I don't know if an attribute_deprecated should be used on the old
> > av_fifo_write() 
> > Personally I think not, because it is a nice and simple convenience
> > function.
> 
> The only extra thing the new function needs is a NULL argument.
> There are just 5 calls to av_fifo_write() in the whole ffmpeg source.

Ok. attribute_deprecated added to av_fifo_write(), and the use of a NULL
func argument added to the documentation of av_fifo_generic_write().
Should I also change the old av_fifo_write() and include in the same
patch?

> > 
> > > > I must note that I am a bit unhappy with the error handling of this
> > > > implementation, but it is consistent with the rest of the AVFifoBuffer
> > > > API.
> > > 
> > > Consistency is no excuse to leave a problem unsolved.
> > 
> > If you insist =). Patch extended with what I consider minimal error
> > handling. I also extended the documentation of the contract for the
> > callback function.
> 
> > 
> > Some discussion points:
> > Should we restore the fifo, i.e. reset the wptr to its original value,
> > in case of an error from the callback function?
> 
> No, definitly not. It would loose an unknown amount of data.

Fair enough.

> [...]
> 
> > Index: libavutil/fifo.h
> > ===================================================================
> > --- libavutil/fifo.h.orig	2008-04-03 13:25:00.335085945 +0200
> > +++ libavutil/fifo.h	2008-04-03 15:19:28.010964623 +0200
> > @@ -79,6 +79,21 @@
> >  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.
> > + * 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.
> > + * @param *src data source
> 
> > + * @return the number of bytes written to the fifo, or an AVERROR() code in
> > + * case of problems.
> 
> Thats fine but ONLY if a AVERROR return implies ZERO bytes written. As soon as
> a single func() has returned a byte you no longer can return an error from
> av_fifo_generic_write() in that way.

Right. Return values adjusted and documentation updated.

> [...]
> 
> > +int av_fifo_generic_write(AVFifoBuffer *f, int size, int (*func)(void*, void*, int), void* src)
> > +{
> > +    int total = 0;
> >      do {
> > -        int len = FFMIN(f->end - f->wptr, size);
> > -        memcpy(f->wptr, buf, len);
> > +        int len = FFMIN(f->end - f->wptr, size-total);
> > +        if(func) {
> > +            len = func(src, f->wptr, len);
> > +            if(len < 0)
> > +                return len;
> > +            else if(len == 0)
> > +                return total;
> > +        } else {
> > +            memcpy(f->wptr, src, len);
> > +            src = (uint8_t*)src + len;
> > +        }
> > +        total += len;
> >          f->wptr += len;
> >          if (f->wptr >= f->end)
> >              f->wptr = f->buffer;
> > -        buf += len;
> > -        size -= len;
> > -    } while (size > 0);
> > +    } while (total < size);
> > +    return total;
> >  }
> 
> I do not think that so many changes are needed.

Code adjusted and three more lines salvaged from av_fifo_write().

The patch passes "make test".

-- 
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: 2356 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080404/7f605b2b/attachment.bin>



More information about the ffmpeg-devel mailing list