[FFmpeg-devel] [PATCH] FLAC parser

[Michael Chinen]

Hi,

On Tue, Oct 12, 2010 at 12:02 AM, Justin Ruggles
<justin.ruggles at gmail.com> wrote:
> Hi,
>
> Michael Chinen wrote:
>
>> On Sat, Oct 9, 2010 at 10:14 AM, Justin Ruggles
>> <justin.ruggles at gmail.com> wrote:
>>> [...]
>>>> From b5890854908d6032819cf77aec0524da3ba352fb Mon Sep 17 00:00:00 2001
>>>> From: Michael Chinen <mchinen at gmail.com>
>>>> Date: Thu, 7 Oct 2010 17:47:52 +0200
>>>> Subject: [PATCH 2/3] Add error codes for FLAC header parsing and move log errors to flacdec.c
>>>>
>>>> ---
>>>> ?libavcodec/flac.c ? ?| ? 45 +++++++++++++++++++--------------------------
>>>> ?libavcodec/flac.h ? ?| ? 20 ++++++++++++++++----
>>>> ?libavcodec/flacdec.c | ? 33 ++++++++++++++++++++++++++++++---
>>>> ?3 files changed, 65 insertions(+), 33 deletions(-)
>>>>
>>>> From ecd062d8bf5f00da470bde387c641c50004239d9 Mon Sep 17 00:00:00 2001
>>>> From: Michael Chinen <mchinen at gmail.com>
>>>> Date: Thu, 7 Oct 2010 17:49:22 +0200
>>>> Subject: [PATCH 3/3] Add FLAC Parser
>>>> ?flac_parser.c verifies all possible chains within a certain header radius and scores them based on changes in sample rate, bit depth, channels.
>>>> ?Update new flac seek test reference file.
>>>> [...]
>>>
>>>> + ? ?int max_score; ? ?/**< maximum score found after checking each child that
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?has a valid CRC ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? */
>>>
>>> vertical alignment
>
> still there.

Sorry, missed this.  Done now.

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

>
>>>> + ? ? ? ?/* Remove headers in list until the end of the best_header. */
>>>> + ? ? ? ?for (curr = fpc->headers; curr != best_child; curr = temp) {
>>>> + ? ? ? ? ? ?if (curr != fpc->best_header) {
>>>> + ? ? ? ? ? ? ? ?av_log(avctx, AV_LOG_DEBUG,
>>>> + ? ? ? ? ? ? ? ? ? ? ? "dropping low score %i frame header from offset %i "
>>>> + ? ? ? ? ? ? ? ? ? ? ? "to %i\n",
>>>> + ? ? ? ? ? ? ? ? ? ? ? curr->max_score, curr->offset, curr->next->offset);
>>> weird alignment/wrapping
>>
>> Please clarify what's weird if I didn't get it.
>> The string wrapping is to adhere to the under 80 rule.
>> The alignment is to the first parameter for av_log.
>> I took a guess and made each parameter on it's own line.
>
> 80 chars is more of a soft rule. ?If you only have a little bit past 80
> and it would be more readable that way, probably nobody will complain.
> If you do want to keep it strictly under 80 chars, you can still put the
> remaining parameters on the same line as the string parameter.
> Bottom-line, readability is most important.
>
>
>>>> + ? ? ? ? ? ?/* Set frame_size to 0. It is unknown or invalid in a junk frame. */
>>>> + ? ? ? ? ? ?avctx->frame_size = 0;
>>>> + ? ? ? ? ? ?*poutbuf_size ? ? = fpc->best_header->offset;
>>>> + ? ? ? ? ? ?*poutbuf ? ? ? ? ?= flac_fifo_read(fpc, 0,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?*poutbuf_size,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&fpc->crc_buf,
>>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?&fpc->crc_buf_allocated_size);
>>>
>>> weird alignment/wrapping
>>
>> fixed alignment. ?Also put the 0 on its own line (is this desired?)
>
>
> See above. ?Having each parameter on a separate line is not really
> desired. ?If all the parameters are squeezed against the 80 char
> boundary, I would personally bend the rules and go past 80 a little to
> keep the function call from spanning too many lines.

Thanks for the clarification.

Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-move-decode_frame_header-from-flacdec.c-to-flac.c-h.patch
Type: application/octet-stream
Size: 8119 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101013/9df85885/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-error-codes-for-FLAC-header-parsing-and-move-log.patch
Type: application/octet-stream
Size: 7589 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101013/9df85885/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-FLAC-Parser.patch
Type: application/octet-stream
Size: 35025 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101013/9df85885/attachment-0002.obj>