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

[Måns Rullgård]

Roman Shaposhnik said:
> 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.

I agree with Roman here.  File renames and code cleanup should be done
separately.

>> > +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 form is easier to read.

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

If branches are expensive a conditional move should do the trick there.

-- 
M?ns Rullg?rd
mru at inprovide.com