[FFmpeg-devel] [PATCH] FLAC parser

[Diego Biurrun]

On Sun, Aug 01, 2010 at 06:36:06AM +0200, Michael Chinen wrote:
> 
> On Fri, Jul 30, 2010 at 8:55 AM, Diego Biurrun <diego at biurrun.de> wrote:
> >> --- a/libavcodec/flacdec.c
> >> +++ b/libavcodec/flacdec.c
> >> @@ -26,7 +26,7 @@
> >> ? *
> >> ? * For more information on the FLAC format, visit:
> >> ? * ?http://flac.sourceforge.net/
> >> - *
> >> +v *
> >
> > oops..
> >
> > Never forget to compile the code you submit :)
> but I don't want to give the impression that I don't compile before
> submitting.  This was in a comment block :)

Right :)
Still you should double-check your diffs to make sure you don't have
such goofups in there.  It has saved me from embarassment countless
times...

> I looked over the code a few times to fix style, but I'm not confident
> that I'm not missing things.  Please bear with me on this and let me
> know what needs to be made nice.  Both my fingers and eyes are still
> getting used to this style.

Here's some more guidance in that regard ...

> --- a/libavcodec/flac.c
> +++ b/libavcodec/flac.c
> @@ -19,7 +19,97 @@
>  
> +static const int sample_size_table[] =
> +{ 0, 8, 12, 0, 16, 20, 24, 0 };

This line is below 80 characters, no need to break it - and if you break
it, it should be indented.

> +int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
> +                               FLACFrameInfo *fi)

You should align the arguments, not the arguments and the '('.

> +    if (fi->ch_mode < FLAC_MAX_CHANNELS) {
> +        fi->channels = fi->ch_mode + 1;
> +        fi->ch_mode = FLAC_CHMODE_INDEPENDENT;

Align the '='.

> +    /* blocksize */
> +    if (bs_code == 0) {
> +        return FLAC_PARSE_ERROR_SAMPLE_RATE;
> +    } else if (bs_code == 6) {
> +        fi->blocksize = get_bits(gb, 8) + 1;
> +    } else if (bs_code == 7) {
> +        fi->blocksize = get_bits(gb, 16) + 1;
> +    } else {
> +        fi->blocksize = ff_flac_blocksize_table[bs_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_PARSE_ERROR_SAMPLE_RATE;
> +    }
> +
> --- a/libavcodec/flac.h
> +++ b/libavcodec/flac.h
> +int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
> +                               FLACFrameInfo *fi);

ditto

> --- /dev/null
> +++ b/libavcodec/flac_parser.c
> @@ -0,0 +1,415 @@
> +static void update_sequences_header(FLACParseContext *fpc, FLACHeaderMarker *mid, FLACHeaderMarker *end, int distance)

long line, more below

> +        /* if mid is crc verified to a child that is verified to the end then mid */
> +        /* is also verified till the end.  Also if mid is not verified to a child */
> +        /* that is verified to the end then mid will fail the crc check           */

If you are writing real English sentences I suggest you follow proper
capitalization rules, i.e. start sentences with an uppercase letter
and end them in a period.  Also, these lines are unnecessarily long.

> +    /* set a bit showing the validity at this distance if crc is ok     */
> +    /* (distance variable is really one less than linked list distance) */
> +    if (!av_crc(av_crc_get_table(AV_CRC_16_ANSI), 0, fpc->buffer + mid->offset, end->offset - mid->offset) )

long line and stray space before the ')'.

> +        mid->crc_valid |= 1 << distance;
> +    else
> +        av_log(NULL, AV_LOG_DEBUG, "update_sequence crc failed at dist %i from %i to %i\n",
> +                     distance, mid->offset, end->offset);

weird indentation

> +            /* actual size of the linked list is now size + 1 */
> +            update_sequences(fpc, size - FLAC_MAX_SEQUENTIAL_HEADERS,
> +                                  FFMIN(size, FLAC_MAX_SEQUENTIAL_HEADERS), *ptr);

ditto

> +/**
> + * Score a header.
> + * Give FLAC_HEADER_BASE_SCORE points to a frame for existing.
> + * If it has children (subsequent frames of which the preceeding crc

CRC

> + * validates against this one,) then take the maximum score of the children,

misplaced ','

> + * with a penalty of FLAC_HEADER_CHANGED_PENALTY applied for each change to
> + * bps, sample rate, channels, but not decorellation mode, or blocksize,
> + * because it can change often.)

stray ')'

> +                if(child->fi.frame_or_samp_num != header->fi.frame_or_samp_num + 1)

if (

You should not fix such things manually but automatically, otherwise you
will always overlook an instance as was now apparently the case.

> +static int get_best_header(FLACParseContext* fpc, AVCodecContext *avctx,
> +                      const uint8_t **poutbuf, int *poutbuf_size, int buf_size)

indentation

Please be consistent, this is the most important thing and would have
avoided all the things I'm nitpicking about now.

> +    if (!child)
> +        *poutbuf_size = fpc->buffer_size - header->offset;
> +    else {

if () {
} else {

> +    /* fill the buffer */
> +    new_buffer = av_fast_realloc(fpc->buffer, &fpc->buffer_allocated_size, buf_size + fpc->buffer_size);
> +
> +    /* tag the headers and update sequences. */
> +    nb_headers = find_new_headers(fpc, FFMAX(0, fpc->buffer_size - (buf_size + (MAX_FRAME_HEADER_SIZE - 1))));
> +
> +    /* wait until we have enough headers to be confident enough to output a valid frame */

more examples of needlessly long lines

> +            /* output a junk frame */
> +            av_log(NULL, AV_LOG_DEBUG, "flac parser:junk frame till offset %i\n",
> +                         fpc->best_header->offset);

...

> --- a/libavcodec/flacdec.c
> +++ b/libavcodec/flacdec.c
> @@ -760,20 +613,15 @@ static int flac_decode_frame(AVCodecContext *avctx,
>      if (bytes_read > buf_size) {
>          av_log(s->avctx, AV_LOG_ERROR, "overread: %d\n", bytes_read - buf_size);
> -        s->bitstream_size=0;
> -        s->bitstream_index=0;
>          return -1;
>      }
> +    if(bytes_read < buf_size) {

if (

Respect the surrounding style.

> +        av_log(s->avctx, AV_LOG_DEBUG, "underread: %d orig size: %d\n", buf_size - bytes_read, buf_size);

long line

Diego