[FFmpeg-devel] [PATCH] FLAC parser

[Benoit Fouet]

Hi,

(my 2 cents review follows)

On 03/27/2009 06:05 AM, Justin Ruggles wrote:
> Hi,
>
> I finally got a working FLAC parser without resorting to buffering
> max_frame_size bytes like the FLAC decoder does.  It requires a slight
> change to ff_combine_frame() since the header can be up to 16 bytes long
> and ff_combine_frame() currently only supports up to 8 bytes of overread
> data (FF_INPUT_BUFFER_PADDING_SIZE).
>
> This works with all samples I've tested, but it would be great to have
> more tested as well.  There are quite a few corner cases, and while I've
> tried to think of everything I can, I might have missed something.
>
>   

> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> new file mode 100644
> index 0000000..fb33680
> --- /dev/null
> +++ b/libavcodec/flac_parser.c
> @@ -0,0 +1,226 @@
>
> [...]
>
> +static int frame_header_is_valid(const uint8_t *buf)
> +{
> +    GetBitContext gb;
> +    FLACFrameInfo fi;
> +
> +    init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE*8);
> +    if (ff_flac_decode_frame_header(NULL, &gb, &fi))
> +        return 0;
> +
> +    return 1;
> +}
>

maybe a return !ff_flac_decode_frame_header(NULL, &gb, &fi); could do

> +static int find_next_header(FLACParseContext *fpc, const uint8_t *buf,
> +                            int buf_size, int start, int save_state)
> +{
> +    int i;
> +    for (i = start; i <= buf_size-16; i++) {
> +        if (frame_header_is_valid(buf+i))
> +            return i;
> +    }
> +    if (save_state) {
> +        for (; i < buf_size-1; i++) {
> +            if ((AV_RB16(buf+i) & 0xFFFE) == 0xFFF8) {
> +                fpc->state_size = buf_size - i;
> +                memcpy(fpc->state, &buf[i], fpc->state_size);
> +                return END_NOT_FOUND;
> +            }
> +        }
> +        for (; i < buf_size; i++) {
> +            if (buf[i] == 0xFF) {
> +                fpc->state[0] = 0xFF;
> +                fpc->state_size = 1;
> +            }
> +        }
>

the last 'for' loop is unneeded

> diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c
> index 04e81c6..9efa42b 100644
> --- a/libavcodec/flacdec.c
> +++ b/libavcodec/flacdec.c
>
> [...]
>
> @@ -480,20 +466,17 @@ static inline int decode_subframe(FLACContext
*s, int channel)
>      return 0;
>  }
> 
> -/**
> - * Validate and decode a frame header.
> - * @param      avctx AVCodecContext to use as av_log() context
> - * @param      gb    GetBitContext from which to read frame header
> - * @param[out] fi    frame information
> - * @return non-zero on error, 0 if ok
> - */
> -static int decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
> +int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
>                                 FLACFrameInfo *fi)
>  {
>      int bs_code, sr_code, bps_code;
> 
>      /* frame sync code */
> -    skip_bits(gb, 16);
> +    if ((get_bits(gb, 16) & 0xFFFE) != 0xFFF8) {
> +        if (avctx)
> +        av_log(avctx, AV_LOG_ERROR, "frame sync error\n");
> +        return -1;
> +    }
>

indentation (copy paste I guess ;)

> @@ -563,7 +552,8 @@ static int decode_frame_header(AVCodecContext
*avctx, GetBitContext *gb,
>      skip_bits(gb, 8);
>      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;
>

the other way round

Ben