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

[Michael Niedermayer]

Hi

On Tue, Sep 19, 2006 at 07:36:24PM -0700, Roman Shaposhnik wrote:
> 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.

well strictly speaking seperate patches would be correcter yes ...


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

i think the second is more readable, at least if i think that the whole
is a  % buf_size operation its -= buf_size


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

% is slow, if() is only slow if the "truthness" is unpredictable which
i guessed isnt the case here
a if(1) or a if((i++)&1) or such are very fast on modern cpus

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

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is