[FFmpeg-devel] [libav-devel] [PATCH] fifo: add av_fifo_peek2()

[Michael Niedermayer]

On Thu, Jun 30, 2011 at 12:41:55PM +0200, Stefano Sabatini wrote:
> On date Thursday 2011-06-30 10:59:12 +0200, Diego Biurrun encoded:
> > On Thu, Jun 30, 2011 at 10:11:04AM +0200, Stefano Sabatini wrote:
> > > The new functions provides a more generic interface than
> > 
> > s/functions/function/
> > 
> > > av_fifo_peek2() for peeking data out of a fifo buffer.
> > 
> > s/fifo/FIFO/
> > 
> > > --- a/libavutil/fifo.h
> > > +++ b/libavutil/fifo.h
> > > @@ -113,4 +113,17 @@ static inline uint8_t av_fifo_peek(AVFifoBuffer *f, int offs)
> > > +
> > > +/**
> > > + * Return a pointer to the data offset by offs stored in the fifo,
> > > + * without modifying the fifo itself.
> > > + */
> > 
> > FIFO
> 
> Fixed.
>  
> > Neither the parameters, nor the return value have (Doxygen) documentation.
> 
> The return value is documented in the brief.
> 
> > But there is interesting stuff that you could document there, namely
> > that a NULL pointer will make this function crash.
> 
> Yes fixed.
> 

> >
> > > +static inline uint8_t *av_fifo_peek2(const AVFifoBuffer *f, int offs)
> > 
> > const AVFifoBuffer * const f, const int offs would be even more
> > const-correct.
> 
> Don't know, offs should not necessarily be const as it is passed per
> value, * const f looks a bit overkill, unless you have reasons for
> keeping that (e.g. avoiding potential warnings).

+1


> 
> > > +    uint8_t *ptr = f->rptr + offs;
> > > +    if (ptr >= f->end)
> > > +        ptr -= f->end - f->buffer;
> > > +    return ptr;
> > 
> > To me this looks like you can still end up outside the FIFO if the
> > offset is larger than two times the FIFO size.

yes, if you request byte 2000 from a 1000 byte sized buffer you
wont get that value because it cant be in there.
returning a random value from within the buffer does not seem a
good idea here. Either its an error condition (return NULL) or we
could just document that the offset must be within the available data.


> 
> Indeed, fixed the logic (which was copypasted from av_fifo_peek()).
> -- 
> Neglect of duty does not cease, by repetition, to be neglect of duty.
> 		-- Napoleon

>  fifo.h |   24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 18713c5dbe8b3aa107e58aab73c828e5664cb307  0004-fifo-add-av_fifo_peek2.patch
> From 8388c50d94084ca3a795b8f7e1cc364f47789010 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> Date: Wed, 29 Jun 2011 17:30:23 +0200
> Subject: [PATCH] fifo: add av_fifo_peek2()
> 
> The new function provides a more generic interface than the one of by
> av_fifo_peek() for peeking at a FIFO buffer data.
> ---
>  libavutil/fifo.h |   24 +++++++++++++++++++++---
>  1 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/libavutil/fifo.h b/libavutil/fifo.h
> index 999d0bf..ed09e4e 100644
> --- a/libavutil/fifo.h
> +++ b/libavutil/fifo.h
> @@ -106,11 +106,29 @@ int av_fifo_realloc2(AVFifoBuffer *f, unsigned int size);
>   */
>  void av_fifo_drain(AVFifoBuffer *f, int size);
>  
> -static inline uint8_t av_fifo_peek(AVFifoBuffer *f, int offs)
> +/**
> + * Return a pointer to the data offset by offs stored in a FIFO
> + * buffer, without modifying the FIFO buffer itself.
> + *
> + * @param pointer to the the FIFO buffer to peek at, must be non-NULL
> + * @param offs an offset in bytes
> + */
> +static inline uint8_t *av_fifo_peek2(const AVFifoBuffer *f, int offs)
>  {
>      uint8_t *ptr = f->rptr + offs;
>      if (ptr >= f->end)
> -        ptr -= f->end - f->buffer;
> -    return *ptr;
> +        ptr = f->buffer + (ptr - f->end) % (f->end - f->buffer);
> +    else if (ptr < f->buffer)
> +        ptr = f->end - (f->buffer - ptr) % (f->end - f->buffer);
> +    return ptr;
>  }
> +
> +/**
> + * @see av_fifo_peek2()
> + */
> +static inline uint8_t av_fifo_peek(AVFifoBuffer *f, int offs)
> +{
> +    return *(uint8_t *)av_fifo_peek2(f, offs);

this function is speed relevant (see its use in dvenc)
and the % make it probably quite slow

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110630/edc58e23/attachment.asc>