[Ffmpeg-devel] privatizing FifoBuffer into libavutil -- take II

Roman Shaposhnik rvs
Wed Sep 20 04:36:24 CEST 2006


Hi

On Tue, Sep 19, 2006 at 12:37:08PM +0200, Michael Niedermayer wrote:
> int size = f->wptr - f->rptr;
> if(size<0)
>     size += f->end - f->buffer;
> 
> is simpler, same for the other such pieces of code

  Sure. But I have a question for existing code -- when I move something
from one place to the other do I have to also clean up the code in the
same patch (the way you suggested) or do these have to be separate
patches ? The approach I took was to minimize any changes in this
patch to make it easier to review, but I'd be happy to polish
infrastructure changes as well.

> > +void av_fifo_drain(AVFifoBuffer *f, int size)
> >  {
> > -    char buf[1024];
> > -    return filename && (av_get_frame_filename(buf, sizeof(buf), 
> > +    f->rptr += size;
> > +    if (f->rptr >= f->end)
> > +        f->rptr = f->buffer + (f->rptr - f->end);
> 
> f->rptr -= f->end - f->buffer;


  Good catch (although, every sensible compiler would generate same code
for both ;-)). As a side note -- I'd be curious to find out which
version is considered to be more human readable:

   f->rptr = f->buffer + (f->rptr - f->end);
   f->rptr -= f->end - f->buffer;

To me, personally, the first one seems easier to grok. But what do the
rest of ffmpeg'ers think ?

> [...]
> > +static inline uint8_t av_fifo_peek(AVFifoBuffer *f, int offs)
> >  {
> >      return f->buffer[(f->rptr - f->buffer + offs) % (f->end - 
> 
> i bet that the following is faster
> 
> ptr= f->rptr + offs;
> if(ptr >= s->end)
>     ptr -= f->end - f->buffer;
> return *ptr;

  Amazingly enough IT IS! In fact almost 2x times faster. Michael, how
did you know ? Your version even contains the dreaded 'if' but it is
still faster. 'idiv' is pretty inexpensive on Xeon so the only possible
explanation I could come up with would be -- your code reduces register
pressure as well. 

  But anyway, I'd be happy to change it and PLEASE let me know the 
answer to the above question.


Thanks,
Roman.





More information about the ffmpeg-devel mailing list