[FFmpeg-devel] [PATCH] FLAC parser

[Diego Biurrun]

On Fri, Jul 23, 2010 at 05:26:02AM +0200, Michael Chinen wrote:
> 
> Here is a new patch with the discussed changes.
> Also style and log cleanups (although I'm sure there's more that I'm
> not catching.)

You asked for it, you got it :)

Try to vertically align operators where it aids readability, most
people around here are fond of it.

> --- a/libavcodec/Makefile
> +++ b/libavcodec/Makefile
> @@ -560,6 +560,7 @@ OBJS-$(CONFIG_DIRAC_PARSER)            += dirac_parser.o
>  OBJS-$(CONFIG_DVBSUB_PARSER)           += dvbsub_parser.o
>  OBJS-$(CONFIG_DVDSUB_PARSER)           += dvdsub_parser.o
> +OBJS-$(CONFIG_FLAC_PARSER)             += flac_parser.o flacdec.o flacdata.o flac.o

If the parser depends on the decoder, then the common code should likely
be extracted from the decoder, i.e. moved to flac.c.

> --- /dev/null
> +++ b/libavcodec/flac_parser.c
> @@ -0,0 +1,401 @@
> +
> +static void update_sequences_header(FLACParseContext *fpc, FLACHeaderMarker *mid, FLACHeaderMarker *end, int distance)
> +{
> +        /* if mid is crc verified to a child that is verified to the end then mid is also verified till the end */
> +        /* if mid is not verified to a child that is verified to the end then mid will fail the crc check */
> +
> +    /* 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) )
> +        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);
> +}
> +
> +static void update_sequences(FLACParseContext *fpc, int start_index, int start_distance, FLACHeaderMarker *end)

Where easily possible, like in the above cases and many more in other
places, try to keep line length below 80 characters.

> +    while (1 << dist <= header->crc_valid) {
> +        if (1<<dist++ & header->crc_valid) {

IMO more readable: (1 << dist++ & header->crc_valid)

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

if (, more in other places

> +                    //av_log(NULL, AV_LOG_DEBUG, "flac header sample number out of order\n");

debug cruft, more in other places

> +static void score_sequences(FLACParseContext *fpc, FLACContext* fc)

K&R style and consistency with the rest: *fc

> --- 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 :)

> @@ -554,6 +530,7 @@ static int decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
>      } else {
> +        if (avctx)
>          av_log(avctx, AV_LOG_ERROR, "illegal sample rate code %d\n",
>                 sr_code);
> @@ -563,7 +540,8 @@ static int decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
>      if (av_crc(av_crc_get_table(AV_CRC_8_ATM), 0, gb->buffer,
>                 get_bits_count(gb)/8)) {
> -        av_log(avctx, AV_LOG_ERROR, "header crc mismatch\n");
> +        if (avctx)
> +            av_log(avctx, AV_LOG_ERROR, "header crc mismatch\n");
>          return -1;

To indent or not to indent, that is the question..

Diego