[FFmpeg-devel] [PATCH] FLAC parser

[Justin Ruggles]

Michael Chinen wrote:

> Hi,
> 
> On Tue, Oct 19, 2010 at 2:48 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> [...]
>>> I did profiling again and it turns out I missed one exit point for the
>>> function the last time.  The non-flat wrap buffer version is about
>>> 2-4% faster overall.  I've squashed it into the 0003.
>> what is the speed difference between current svn and after this patch ?
> 
> I used the -benchmark flag for 'ffmpeg -i fourminsong.flac a.wav' and
> five runs and got
> without patch: utime = 2.044-2.042s
> with patch:    utime = 2.363-2.379s
> 
> So flac demuxing with the parser is slower.

As expected.  It does more work.


>>> diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c
>>> index d9e1c80..133adbb 100644
>>> --- a/libavcodec/flacdec.c
>>> +++ b/libavcodec/flacdec.c
>>> @@ -472,12 +472,39 @@ static inline int decode_subframe(FLACContext *s, int channel)
>>>
>>>  static int decode_frame(FLACContext *s)
>>>  {
>>> -    int i;
>>> +    int i, ret;
>>>      GetBitContext *gb = &s->gb;
>>>      FLACFrameInfo fi;
>>>
>>> -    if (ff_flac_decode_frame_header(s->avctx, gb, &fi)) {
>>> -        av_log(s->avctx, AV_LOG_ERROR, "invalid frame header\n");
>>> +    ret = ff_flac_decode_frame_header(gb, &fi);
>>> +    if (ret) {
>>> +        av_log(s->avctx, AV_LOG_ERROR, "invalid frame header: ");
>>> +        switch (ret) {
>>> +        case FLAC_FRAME_HEADER_ERROR_SYNC:
>>> +            av_log(s->avctx, AV_LOG_ERROR, "frame sync error\n");
>>> +            break;
>>> +        case FLAC_FRAME_HEADER_ERROR_CHANNEL_CFG:
>>> +            av_log(s->avctx, AV_LOG_ERROR, "invalid channel mode: %d\n", fi.ch_mode);
>>> +            break;
>>> +        case FLAC_FRAME_HEADER_ERROR_BPS:
>>> +            av_log(s->avctx, AV_LOG_ERROR, "invalid sample size code (%d)\n", fi.bps);
>>> +            break;
>>> +        case FLAC_FRAME_HEADER_ERROR_PADDING:
>>> +            av_log(s->avctx, AV_LOG_ERROR, "broken stream, invalid padding\n");
>>> +            break;
>>> +        case FLAC_FRAME_HEADER_ERROR_SAMPLE_NUM:
>>> +            av_log(s->avctx, AV_LOG_ERROR, "sample/frame number invalid; utf8 fscked\n");
>>> +            break;
>>> +        case FLAC_FRAME_HEADER_ERROR_BLOCK_SIZE:
>>> +            av_log(s->avctx, AV_LOG_ERROR, "reserved blocksize code: 0\n");
>>> +            break;
>>> +        case FLAC_FRAME_HEADER_ERROR_SAMPLE_RATE:
>>> +            av_log(s->avctx, AV_LOG_ERROR, "illegal sample rate code: 15\n");
>>> +            break;
>>> +        case FLAC_FRAME_HEADER_ERROR_CRC:
>>> +            av_log(s->avctx, AV_LOG_ERROR, "header crc mismatch\n");
>>> +            break;
>>> +        }
>>>          return -1;
>>>      }
>>>
>>> --
>>> 1.7.1
>>>
>> also its a alot simpler to pass a offset to adjust the log level instead of
>> returning error codes and then translating them to log messages
> 
> It was originally more like what you suggest, but it was changed to be
> similar to the ac3 parser (see discussion in this thread on july 30.)


I suggested the error codes because it seemed better than the previous
solution of checking if the log context was non-NULL.  In this case the
error codes aren't used for other purposes like they are in the AC3, so
adjusting the log level does sound like a more appropriate solution.

-Justin