[FFmpeg-devel] [PATCH] FLAC parser

[Justin Ruggles]

Michael Chinen wrote:

> On Tue, Aug 10, 2010 at 4:03 AM, Michael Chinen <mchinen at gmail.com> wrote:
>> Hi,
>>
>> Here is the new patch with these changes and one note:
> 
> Sorry, I forgot to add the avmalloc fail log message.
> 
> Here's the updated patch.
> 

> [...]
> +            if (child->fi.samplerate != header->fi.samplerate)
> +                child_score -= FLAC_HEADER_CHANGED_PENALTY;
> +            if (child->fi.bps != header->fi.bps)
> +                child_score -= FLAC_HEADER_CHANGED_PENALTY;
> +            /* Changing blocking strategy not allowed per the spec */
> +            if (child->fi.is_var_size != header->fi.is_var_size)
> +                child_score -= FLAC_HEADER_BASE_SCORE;
> +            if (child->fi.channels != header->fi.channels)
> +                child_score -= FLAC_HEADER_CHANGED_PENALTY;
> +
> +            /* Check sample and frame numbers. */
> +            if ((child->fi.frame_or_sample_num - header->fi.frame_or_sample_num
> +                 != header->fi.blocksize) &&
> +                (child->fi.frame_or_sample_num
> +                 != header->fi.frame_or_sample_num + 1)) {
> +                    child_score -= FLAC_HEADER_CHANGED_PENALTY;
> +            }
> [...]
> +        if (child->fi.samplerate != header->fi.samplerate) {
> +            av_log(avctx, AV_LOG_WARNING,
> +                   "selecting header whose best child has changed sample "
> +                   "rate\n");
> +        }
> +        if (child->fi.bps != header->fi.bps) {
> +            av_log(avctx, AV_LOG_WARNING,
> +                   "selecting header whose best child has bps change\n");
> +        }
> +        if (child->fi.is_var_size != header->fi.is_var_size) {
> +            av_log(avctx, AV_LOG_WARNING,
> +                   "selecting header whose best child has variable blocksize "
> +                   "change\n");
> +        }
> +        if (child->fi.channels != header->fi.channels) {
> +            av_log(avctx, AV_LOG_WARNING,
> +                   "selecting header whose best child has num channels "
> +                   "change\n");
> +        }
> +        if ((child->fi.frame_or_sample_num - header->fi.frame_or_sample_num
> +             != header->fi.blocksize) &&
> +            (child->fi.frame_or_sample_num
> +             != header->fi.frame_or_sample_num + 1)) {
> +            av_log(avctx, AV_LOG_WARNING,
> +                   "selecting header whose best child's sample/frame number "
> +                   "not in order\n");
> +        }

This is the only thing remaining that I'm hesitant about.  These 5
checks are duplicated exactly.  Code duplication should be avoided as
much as possible.  One reason is that it is easy to miss changing both
places if a change is made to either part.

Also, the warning messages are kind of cryptic to someone not familiar
with how the FLAC parser works.  I think that since they're not
debugging messages they should be simpler.  For example, "sample rate
change detected in adjacent frames" or "sample/frame number mismatch in
adjacent frames".

As for the code duplication, maybe you could use something like this:

static int check_header_mismatch(AVCodecContext *avctx,
                                 FLACFrameInfo *header,
                                 FLACFrameInfo *child,
                                 int log_warning)
{
    int deduction = 0;
    if (child->samplerate != header->samplerate) {
        deduction += FLAC_HEADER_CHANGED_PENALTY;
        if (log_warning) {
            av_log(avctx, AV_LOG_WARNING,
                   "sample rate change detected in adjacent frames\n");
        }
    }
    etc...
    return deduction;
}
...
score_header():
/* deduct from score for mismatched header params */
child_score -= check_header_mismatch(NULL, &header->fi, &child->fi, 0);
...
get_best_header():
/* print warning messages for mismatched header params */
check_header_mismatch(avctx, &header->fi, &child->fi, 1);


What do you think?

-Justin