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

[Michael Niedermayer]

On Fri, Apr 04, 2008 at 11:11:32AM +0000, Bj?rn Axelsson wrote:
> 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?

2nd patch ...


[...]

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


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


>  
>  /**
>   * 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;




> +        } else {

> +            memcpy(f->wptr, src, len);
> -        memcpy(f->wptr, buf, len);

cosmetic


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Thouse who are best at talking, realize last or never when they are wrong.
-------------- 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/20080404/d0aa3f17/attachment.pgp>