[FFmpeg-devel] [PATCH] FLAC parser

[Justin Ruggles]

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.

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

>>> +        /* 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,
Justin