[FFmpeg-devel] [PATCH] FLAC parser
Justin Ruggles
justin.ruggles
Thu Oct 14 01:17:42 CEST 2010
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
More information about the ffmpeg-devel
mailing list