[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