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

[Michael Niedermayer]

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.


> 
> > > 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.


[...]

> 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.


[...]

> +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.


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

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- 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/20080403/cc9ab63d/attachment.pgp>