[FFmpeg-devel] [PATCH] FLAC parser

Justin Ruggles justin.ruggles
Sat Oct 9 22:14:26 CEST 2010


Michael Chinen wrote:

> Hi,
> 
> On Wed, Oct 6, 2010 at 6:50 PM, Justin Ruggles <justin.ruggles at gmail.com> wrote:
>> Patch 0001 looks good.
>>
>> The part of patch 0002 that changes ff_flac_decode_frame_header() to
>> return various error codes instead of logging error messages should be a
>> separate commit, to be applied after patch 0001.
> 
> done.
> 
>> Unless I'm overlooking something, the actual parser seems missing from
>> your patches.  Did you forget to git add flac_parser.c?
> 
> Thanks for catching my mistake!
> 
> 
>> Patch 0003 seems to be related to more than just your FLAC parser.  I
>> would suggest submitting it for approval in its own thread, along with a
>> reiteration of why it is needed.
> 
> Yes, I will move that one to its own thread.
> 
>> Patch 0004 should be included with whatever patch causes those changes.
>>  We shouldn't break seek tests if we can easily avoid it, even if it is
>> fixed shortly after.
> 
> You're right; it's done.


> From b5890854908d6032819cf77aec0524da3ba352fb Mon Sep 17 00:00:00 2001
> From: Michael Chinen <mchinen at gmail.com>
> Date: Thu, 7 Oct 2010 17:47:52 +0200
> Subject: [PATCH 2/3] Add error codes for FLAC header parsing and move log errors to flacdec.c
> 
> ---
>  libavcodec/flac.c    |   45 +++++++++++++++++++--------------------------
>  libavcodec/flac.h    |   20 ++++++++++++++++----
>  libavcodec/flacdec.c |   33 ++++++++++++++++++++++++++++++---
>  3 files changed, 65 insertions(+), 33 deletions(-)
> 
> diff --git a/libavcodec/flac.c b/libavcodec/flac.c
> index f6b65ce..5e97564 100644
> --- a/libavcodec/flac.c
> +++ b/libavcodec/flac.c
> @@ -32,13 +32,16 @@ static int64_t get_utf8(GetBitContext *gb)
>      return val;
> [...] 
> +    /* uses fixed size stream code */
> +    fi->is_var_size = get_bits1(gb);

/* variable block size stream code */

>      /* reserved bit */
> -    if (get_bits1(gb)) {
> -        av_log(avctx, AV_LOG_ERROR, "broken stream, invalid padding\n");
> -        return -1;
> -    }
> +    if (get_bits1(gb))
> +        return FLAC_FRAME_HEADER_ERROR_PADDING;
> [...]
>      /* header CRC-8 check */
>      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");
> -        return -1;
> -    }
> +               get_bits_count(gb)/8))
> +        return FLAC_FRAME_HEADER_ERROR_CRC;


This is just a nit-pick, but changing from using braces to not using
braces in these 2 places is a cosmetic change.

>  
>      return 0;
>  }
> diff --git a/libavcodec/flac.h b/libavcodec/flac.h
> index fe38463..bb29676 100644
> --- a/libavcodec/flac.h
> +++ b/libavcodec/flac.h
> @@ -81,6 +92,8 @@ typedef struct FLACFrameInfo {
>      FLACCOMMONINFO
>      int blocksize;          /**< block size of the frame                 */
>      int ch_mode;            /**< channel decorrelation mode              */
> +    int64_t frame_or_sample_num;  /**< frame number or sample number                         */
> +    int is_var_size;              /**< determines if the above variable is frames or samples */


I think the documentation of is_var_size could be better.  It specifies
if the stream uses variable block sizes or a fixed block size, and it
determines the meaning of frame_or_sample_num.


> From ecd062d8bf5f00da470bde387c641c50004239d9 Mon Sep 17 00:00:00 2001
> From: Michael Chinen <mchinen at gmail.com>
> Date: Thu, 7 Oct 2010 17:49:22 +0200
> Subject: [PATCH 3/3] Add FLAC Parser
>  flac_parser.c verifies all possible chains within a certain header radius and scores them based on changes in sample rate, bit depth, channels.
>  Update new flac seek test reference file.
> [...]
> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> new file mode 100644
> index 0000000..5c2d806
> --- /dev/null
> +++ b/libavcodec/flac_parser.c
> @@ -0,0 +1,545 @@
> +/**
> + * @file
> + * FLAC parser
> + *
> + * The FLAC parser buffers input until FLAC_MIN_HEADERS has been found.
> + * Each time it finds a CRC-8 verified header it sees 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.
> + **/


The parser returns the *frame* whose header has the highest score.


> +    int max_score;    /**< maximum score found after checking each child that
> +                            has a valid CRC                                   */


vertical alignment

> +typedef struct FLACParseContext {
> +    AVCodecContext *avctx;         /**< codec context pointer for logging     */
> +    AVFifoBuffer *fifo_buf;        /**< buffer to store all data until headers
> +                                        can be verified                       */
> +    uint8_t *wrap_buf;             /**< general fifo read buffer when wrapped */
> +    int wrap_buf_allocated_size;   /**< actual allocated size of the buffer   */
> +    uint8_t *crc_buf;              /**< update_sequences_header's fifo read
> +                                        buffer when the request wraps         */
> +    int crc_buf_allocated_size;    /**< actual allocated size of the buffer   */
> +    FLACHeaderMarker *headers;     /**< linked-list that starts at the first
> +                                        CRC-8 verified header within buffer   */
> +    FLACHeaderMarker *best_header; /**< highest scoring header within buffer  */
> +    int nb_headers_found;          /**< number of headers found in the last
> +                                        flac_parse() call                     */
> +    int best_header_valid;         /**< flag set when the parser returns junk;
> +                                        if set return best_header next time   */
> +    int end_padded;                /**< if 1 then buffer end was padded       */
> +} FLACParseContext;

Can you separate the buffer-specific stuff from the parser-specific
stuff?  That would make it easier to read I think.

> +/**
> + * Non-destructive fast fifo pointer fetching
> + * Returns a pointer from the specified offset.
> + * If possible the pointer points within the fifo buffer.
> + * Otherwise (if it would cause a wrap around,) a temporary
> + * buffer is used which is valid till the next call to
> + * flac_fifo_read
> + */
> +static uint8_t* flac_fifo_read(FLACParseContext *fpc, int offset, int len,
> +                               uint8_t** wrap_buf, int* allocated_size)
> +{
> +    AVFifoBuffer *f   = fpc->fifo_buf;
> +    uint8_t *start    = f->rptr + offset;
> +    uint8_t *tmp_buf;
> +
> +    if (start > f->end)
> +        start -= f->end - f->buffer;
> +    if(f->end - start >= len)
> +        return start;
> +
> +    tmp_buf = av_fast_realloc(*wrap_buf, allocated_size,
> +                              len + *allocated_size);
> +    if (!tmp_buf) {
> +        av_log(fpc->avctx, AV_LOG_ERROR,
> +               "couldn't reallocate wrap buffer of size addition %d"
> +               " to exisiting size %d\n", len, *allocated_size);
> +        return NULL;
> +    }
> +    *wrap_buf = tmp_buf;
> +    do {
> +        int seg_len = FFMIN(f->end - start, len);
> +        memcpy(tmp_buf, start, seg_len);
> +        tmp_buf = (uint8_t*)tmp_buf + seg_len;
> +// memory barrier needed for SMP here in theory
> +
> +        start += seg_len - (f->end - f->buffer);
> +        len -= seg_len;
> +    } while (len > 0);
> +
> +    return *wrap_buf;
> +}


Do you have any tests to show whether using the FIFO makes the parser
faster and/or use less memory?  It certainly seems more complicated.
Also, it uses a lot of pointer math.  Have you checked that it is safe
from integer overflow?

> +        /* Remove headers in list until the end of the best_header. */
> +        for (curr = fpc->headers; curr != best_child; curr = temp) {
> +            if (curr != fpc->best_header) {
> +                av_log(avctx, AV_LOG_DEBUG,
> +                       "dropping low score %i frame header from offset %i "
> +                       "to %i\n",
> +                       curr->max_score, curr->offset, curr->next->offset);

weird alignment/wrapping

> +            /* Set frame_size to 0. It is unknown or invalid in a junk frame. */
> +            avctx->frame_size = 0;
> +            *poutbuf_size     = fpc->best_header->offset;
> +            *poutbuf          = flac_fifo_read(fpc, 0,
> +                                        *poutbuf_size,
> +                                        &fpc->crc_buf,
> +                                        &fpc->crc_buf_allocated_size);


weird alignment/wrapping

> +static int flac_parse_init(AVCodecParserContext *c)
> +{
> +    FLACParseContext *fpc = c->priv_data;
> +    fpc->fifo_buf = av_fifo_alloc(1024);

Why 1024?  A nearby power of 2 for one average 16-bit stereo FLAC frame
would be closer to 8192.

> diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c
> index 133adbb..77b548e 100644
> --- a/libavcodec/flacdec.c
> +++ b/libavcodec/flacdec.c
>[...]  
>      /* check that there is at least the smallest decodable amount of data.
>         this amount corresponds to the smallest valid FLAC frame possible.
>         FF F8 69 02 00 00 9A 00 00 34 46 */
> -    if (buf_size < 11)
> -        goto end;
> +     if (buf_size < FLAC_MIN_FRAME_SIZE)
> +         return buf_size;

extra space?


Cheers,
Justin



More information about the ffmpeg-devel mailing list