[FFmpeg-devel] [PATCH] FLAC parser

[Justin Ruggles]

Hi,

Michael Chinen wrote:

>>>>> +/**
>>>>> + * Non-destructive fast fifo pointer fetching
>>>>> + * Returns a pointer from the specified offset.
>>>>> + * If possible the pointer points within the fifo buffer.
>>>>> + * Otherwise (if it would cause a wrap around,) a temporary
>>>>> + * buffer is used which is valid till the next call to
>>>>> + * flac_fifo_read
>>>>> + */
>>>>> +static uint8_t* flac_fifo_read(FLACParseContext *fpc, int offset, int len,
>>>>> +                               uint8_t** wrap_buf, int* allocated_size)
>>>>> +{
>>>>> +    AVFifoBuffer *f   = fpc->fifo_buf;
>>>>> +    uint8_t *start    = f->rptr + offset;
>>>>> +    uint8_t *tmp_buf;
>>>>> +
>>>>> +    if (start > f->end)
>>>>> +        start -= f->end - f->buffer;
>>>>> +    if(f->end - start >= len)
>>>>> +        return start;
>>>>> +
>>>>> +    tmp_buf = av_fast_realloc(*wrap_buf, allocated_size,
>>>>> +                              len + *allocated_size);
>>>>> +    if (!tmp_buf) {
>>>>> +        av_log(fpc->avctx, AV_LOG_ERROR,
>>>>> +               "couldn't reallocate wrap buffer of size addition %d"
>>>>> +               " to exisiting size %d\n", len, *allocated_size);
>>>>> +        return NULL;
>>>>> +    }
>>>>> +    *wrap_buf = tmp_buf;
>>>>> +    do {
>>>>> +        int seg_len = FFMIN(f->end - start, len);
>>>>> +        memcpy(tmp_buf, start, seg_len);
>>>>> +        tmp_buf = (uint8_t*)tmp_buf + seg_len;
>>>>> +// memory barrier needed for SMP here in theory
>>>>> +
>>>>> +        start += seg_len - (f->end - f->buffer);
>>>>> +        len -= seg_len;
>>>>> +    } while (len > 0);
>>>>> +
>>>>> +    return *wrap_buf;
>>>>> +}
>>>> Do you have any tests to show whether using the FIFO makes the parser
>>>> faster and/or use less memory?  It certainly seems more complicated.
>>>> Also, it uses a lot of pointer math.  Have you checked that it is safe
>>>> from integer overflow?
>>> I just guessed it is faster because the old one used memmove which
>>> would mean shifting of 20 frames every time a frame is returned.
>>> The fifo version has a few extra buffers sitting around, but uses at
>>> most a frame's worth more memory.
>>>
>>> But you're right, speed should be tested.  I ran tests of fifo vs. the
>>> old memmove sliding version.
>>>
>>> I used the START_TIMER/STOP_TIMER macros to profile flac_parse().
>>>
>>> 4 runs on a four minute flac file (with fifo):
>>> 236920 dezicycles in with fifo, 14541 runs, 1843 skips
>>> 237042 dezicycles in with fifo, 14544 runs, 1840 skips
>>> 237761 dezicycles in with fifo, 14537 runs, 1847 skips
>>> 241054 dezicycles in with fifo, 14543 runs, 1841 skips
>>>
>>> 4 runs on the same file (without fifo):
>>> 273416 dezicycles in without fifo, 14551 runs, 1833 skips
>>> 272648 dezicycles in without fifo, 14562 runs, 1822 skips
>>> 273739 dezicycles in without fifo, 14544 runs, 1840 skips
>>> 275199 dezicycles in without fifo, 14545 runs, 1839 skips
>>>
>>> There is about 15% faster overall in the function. I suspect the
>>> actual parsing to be the real bottleneck, but because the buffer
>>> access is scattered throughout the parsing code I didn't try to figure
>>> out exactly how much the actual buffer code speed differences is. I
>>> interpreted it to mean it is likely much better than 15%.
>> Great, thanks.
>>
>> You're doing a lot of tampering with the AVFifoBuffer fields.  Why not
>> add your wrapping FIFO functionality to fifo.c/h?  Nothing seems
>> specific to this parser, although I am still not quite clear on why you
>> have 2 temporary wrap buffers for the same fifo.
> 
> I added a comment about why two buffers are needed above flac_fifo_read.
> 
> I thought it was better not to add to fifo.c/h at first since these
> are pretty non-fifo-ish use cases (reading from an arbitrary point in
> the fifo.) But if it's desired then I could submit a patch.  Or
> perhaps a move afterwards would be good, since the FLAC parser needs
> it?

Ok, then the patches look good to me.  Anyone else want to comment on
this before it is committed?

-Justin