[FFmpeg-devel] [PATCH] FLAC parser

Justin Ruggles justin.ruggles
Thu Aug 5 02:01:45 CEST 2010


Justin Ruggles wrote:

> Hi,
> 
> Michael Chinen wrote:
> 
>> I've included everything from yours and Diego's review, but I'll
>> respond to the questions below.  I feel more confident about it now,
>> thanks again to both of you.
>>
>> On Sun, Aug 1, 2010 at 10:29 PM, Justin Ruggles
>> <justin.ruggles at gmail.com> wrote:
>> [...]
>>>> +/* the maximum number of adjacent headers that will compare crcs against each other */
>>>> +#define FLAC_MAX_SEQUENTIAL_HEADERS 4
>>>> +/* the minimum number of headers that need to be buffered and checked before returning frames */
>>>> +#define FLAC_MIN_HEADERS 20
>>> Just out of curiosity, have you done testing with fewer headers, both
>>> buffered and sequential?
>> I tried before with 3 and 10 respectively, and things still seemed to
>> work okay, but arbitrarily went with 4 and 20.  Maybe it's overkill?
> 
> If 3 and 10 works well, then I think it should be used.  I'll test the
> FLAC files in my lossless music collection (about 450 songs).
> 
> Your newest patch is broken.  It took me a while to figure out what you
> had changed, so I don't have time this evening to write up a new review.
>  But you need to take a look at flac_parser.c line 238.
> 
> I'll send a new review tomorrow evening.  Maybe Diego will do another
> style/doc review before then?  That's where most of the remaining issues
> are.


Ok. Here is a quick review.

> [...]
> +    /* blocksize */
> +    if (bs_code == 0) {
> +        return FLAC_FRAME_HEADER_ERROR_SAMPLE_RATE;


wrong error code.

> [...]
> +    /* sample rate */
> +    if (sr_code < 12) {
> +        fi->samplerate = ff_flac_sample_rate_table[sr_code];
> +    } else if (sr_code == 12) {
> +        fi->samplerate = get_bits(gb, 8) * 1000;
> +    } else if (sr_code == 13) {
> +        fi->samplerate = get_bits(gb, 16);
> +    } else if (sr_code == 14) {
> +        fi->samplerate = get_bits(gb, 16) * 10;
> +    } else {
> +        fi->samplerate = sr_code;
> +        return FLAC_FRAME_HEADER_ERROR_SAMPLE_RATE;


don't set fi->samplerate to the invalid sr_code.


> [...]
> -    int blocksize;          /**< block size of the frame                 */
> -    int ch_mode;            /**< channel decorrelation mode              */
> +    int blocksize;                /**< block size of the frame                               */
> +    int ch_mode;                  /**< channel decorrelation mode                            */


cosmetic. should be in a separate patch (which you don't necessarily
have to provide)

> [...]
> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> new file mode 100644
> index 0000000..843904d
> --- /dev/null
> +++ b/libavcodec/flac_parser.c
> @@ -0,0 +1,460 @@
> +/*
> + * FLAC parser
> + * Copyright (c) 2010 Michael Chinen
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "libavutil/crc.h"
> +#include "bytestream.h"
> +#include "parser.h"
> +#include "flac.h"
> +
> +/* the maximum number of adjacent headers that will compare crcs against each other */
> +#define FLAC_MAX_SEQUENTIAL_HEADERS 4
> +/* the minimum number of headers that need to be buffered and checked before returning frames */
> +#define FLAC_MIN_HEADERS 20
> +
> +/* scoring settings for score_header */
> +#define FLAC_HEADER_BASE_SCORE        10
> +#define FLAC_HEADER_CHANGED_PENALTY   7
> +#define FLAC_HEADER_NOT_SCORED_YET    -100000
> +
> +/* largest possible size of flac header. */
> +#define MAX_FRAME_HEADER_SIZE 16


should be doxygen style

> +
> +/**
> + * The FLAC parser builds up an internal buffer until FLAC_MIN_HEADERS has been found.
> + * Each time it finds a CRC-8 verified header see which of the FLAC_MAX_SEQUENTIAL_HEADERS
> + * that came before it have a valid CRC-16 footer that ends at the newly found header.
> + * Headers are scored by FLAC_HEADER_BASE_SCORE plus the max of it's crc-verified children,
> + * penalized by changes in sample rate, frame number, etc.
> + * The parser returns the header with the highest score.
> + **/


should not be doxygen style, unless you put it inside an @file comment
block, which might not be a bad idea.

> +
> +typedef struct FLACHeaderMarker {
> +    /* offset in bytes from the start of FLACParseContext's buffer                        */
> +    int offset;
> +    /* each bit of crc_valid is a flag which is set if the 16 bit CRC is valid            */
> +    /* from this header to the header at a distance equal to the bit position             */
> +    int crc_valid;
> +    /* maximum score found after checking each child that has a valid CRC                 */
> +    int max_score;
> +    /* decoded frame header info                                                          */
> +    FLACFrameInfo fi;
> +    /* the next CRC-8 verified header that immediately follows this one in the bytestream */
> +    struct FLACHeaderMarker *next;
> +    /* the following frame header with which this frame has the best score with           */
> +    struct FLACHeaderMarker *best_child;
> +} FLACHeaderMarker;
> +
> +typedef struct FLACParseContext {
> +    /* buffer to store all data until headers can be verified                             */
> +    uint8_t *buffer;
> +    int buffer_size;
> +    int buffer_allocated_size;
> +    /* a linked-list that starts at the first CRC-8 verified header within the buffer     */
> +    FLACHeaderMarker *headers;
> +    /* highest scoring header within the buffer                                           */
> +    FLACHeaderMarker *best_header;
> +    /* number of headers found in the last flac_parse() call                              */
> +    int nb_headers_found;
> +    /* flag set when the parser returns junk and should return the best_header next time  */
> +    int best_header_valid;
> +} FLACParseContext;


all above parameter documentation should be doxygen style (and probably
inline instead of above for better readability.

> [...]
> +        /* If mid is CRC verified to a child verified to end then mid    */
> +        /* is verified till end.  Also if mid is not verified to a child */
> +        /* that is verified to the end then mid will fail the CRC check. */
> +        if (child->crc_valid & 1 << dist_from_start) {


I don't know if we have a policy or guideline on multi-line comment
blocks, but you don't need to have each line in a separate comment
block.  If you prefer it I guess it's not a big deal...

> +            if (mid->crc_valid & 1 << (distance - dist_from_start) )


remove the extra space before the last parenthesis

> [...]
> +        av_log(NULL, AV_LOG_WARNING, "update_sequence CRC failed at dist %i from %i to %i\n",
> +               distance, mid->offset, end->offset);


should be AV_LOG_DEBUG

> [...]

> +                end_offset = (*end_handle)->offset;
> +                end_handle = &(*end_handle)->next;


vertically align "end_handle"

> [...]
> +            *end_handle = av_mallocz( sizeof(FLACHeaderMarker));


remove the extra space after "av_mallocz("

> [...]
> +/**
> + * Score a header.
> + * Give FLAC_HEADER_BASE_SCORE points to a frame for existing.
> + * If it has children, (subsequent frames of which the preceding CRC footer
> + * validates against this one,) then take the maximum score of the children,
> + * with a penalty of FLAC_HEADER_CHANGED_PENALTY applied for each change to
> + * bps, sample rate, channels, but not decorellation mode, or blocksize,


decorrelation is misspelled

>[...]
> +            if (header->fi.is_var_size) {
> +                /* check sample numbers */
> +                if (child->fi.frame_or_sample_num - header->fi.frame_or_sample_num 
> +                    != child->fi.blocksize) {
> +                    child_score -= FLAC_HEADER_CHANGED_PENALTY;
> +                }
> +            } else {
> +                /* check frame numbers  */
> +                if (child->fi.frame_or_sample_num != header->fi.frame_or_sample_num + 1)
> +                    child_score -= FLAC_HEADER_CHANGED_PENALTY;
> +            }

Sorry I didn't catch this before, but this check needs to be made a
little less strict.  The "blocking strategy bit" in the header only
became part of the FLAC specification in 2007 in version 1.2.0.  Before
that the incremental number was still either frame or sample number, but
there were other ways of indicating variable frame size.  My suggestion
is to check for either +1 or +blocksize, independent of is_var_size.

> +            if (child_score + header->max_score > header->max_score) {
> +                /* keep the child because the frame scoring is dynamic */
> +                header->best_child = child;
> +                header->max_score  = FLAC_HEADER_BASE_SCORE + child_score;
> +            }


This is the part that you changed and is now incorrect.  Previously, it was:
if (child_score + FLAC_HEADER_BASE_SCORE > header->max_score)

> [...]
> +        if (header->fi.is_var_size) {
> +            if (child->fi.frame_or_sample_num - header->fi.frame_or_sample_num 
> +                != child->fi.blocksize) {
> +                av_log(avctx, AV_LOG_WARNING,
> +                       "selecting header whose best child's sample number does not match\n");
> +            }
> +        } else {
> +            if (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 frame number not consecutive\n");
> +            }
> +        }


see above

> +                av_log(NULL,AV_LOG_WARNING,
> +                       "dropping low score %i frame header from offset %i to %i\n",
> +                       curr->max_score, curr->offset, curr->next->offset);

should be AV_LOG_DEBUG.
and add a space after the comma.

> [...]
> +        best_child->offset = 0;
> +        fpc->headers     = best_child;
> +        fpc->best_header = NULL;


vertical alignment

> [...]
> +    /* If EOF just return buffered frames by priority. */
> +    if (buf_size) {
> +        /* fill buffer */


The comment no longer describes the code after the change from
if(!buf_size) to if(buf_size).

> [...]
> +            /* TODO: does frame_size = 0 make compute_pkt_fieds do the correct thing? */
> +            avctx->frame_size      = 0;

AFAIK it should be fine.  Try it out (damage a stream) and see how it
affects packet durations and timestamps before, during, and after the
junk frame packet.  Also try stream copy to make sure it still works.


> diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c

All the flacdec.c changes look good to me.

-Justin




More information about the ffmpeg-devel mailing list