[FFmpeg-devel] [PATCH] FLAC parser

[Justin Ruggles]

Hi,

Michael Chinen wrote:

> On Sun, Oct 24, 2010 at 3:27 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> On Sat, Oct 23, 2010 at 04:08:59AM +0200, Michael Chinen wrote:
>> [...]
>>> +static int find_new_headers(FLACParseContext *fpc, int search_start)
>>> +{
>>> +    FLACHeaderMarker *end;
>>> +    int search_end, size = 0, read_len, temp;
>>> +    uint8_t *buf;
>>> +    fpc->nb_headers_found = 0;
>>> +
>>> +    /* Search for a new header of at most 16 bytes. */
>>> +    search_end = av_fifo_size(fpc->fifo_buf) - (MAX_FRAME_HEADER_SIZE - 1);
>>> +    read_len   = search_end - search_start + 1;
>>> +    buf        = flac_fifo_read(fpc, search_start, &read_len);
>>> +    size       = find_headers_search(fpc, buf, read_len, search_start);
>>> +    search_start += read_len - 1;
>>> +
>>> +    /* if we hit the fifo end we need a wrap buffer for the two byte wrap around */
>>> +    if (search_start != search_end) {
>>> +        buf  = flac_fifo_read_wrap(fpc, search_start, 2,
>>> +                                  &fpc->wrap_buf, &fpc->wrap_buf_allocated_size);
>>> +        temp = find_headers_search(fpc, buf, 2, search_start);
>>> +        size = FFMAX(size, temp);
>>> +        search_start += 1;
>>> +    }
>> the 2 bytes can be checked directly and find_headers_search_validate() be called
> 
> ok, I added this.
> 
>> also i leave further review to the flac maintainer
>>
>> a bit of further simplification and cleanup would be nice though
> 
> Also removed the error codes and used offsets for logs where applicable.

I think maybe AV_LOG_MAX should be added, which would match the highest
log level in case more levels above AV_LOG_DEBUG are added.  Otherwise
your code would stop working as intended.

> From 6e7b3caeef85cd2143df7bbe61a2bd80f273882f 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.
> 
> ---
>  configure                |    1 +
>  libavcodec/Makefile      |    1 +
>  libavcodec/allcodecs.c   |    1 +
>  libavcodec/flac.h        |    1 +
>  libavcodec/flac_parser.c |  644 ++++++++++++++++++++++++++++++++++++++++++++++
>  libavcodec/flacdec.c     |   92 +------
>  tests/ref/seek/flac_flac |   68 ++++--
>  7 files changed, 703 insertions(+), 105 deletions(-)
>  create mode 100644 libavcodec/flac_parser.c
> [...]
> + * 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 frame with the highest score.
> + **/

nit-pick: CRC-verified

> +/** maximum number of adjacent headers that compare CRCs against each other   */
> +#define FLAC_MAX_SEQUENTIAL_HEADERS 3
> +/** minimum number of headers buffered and checked before returning frames    */
> +#define FLAC_MIN_HEADERS 10
> +
> +/** scoring settings for score_header */
> +#define FLAC_HEADER_BASE_SCORE        10
> +#define FLAC_HEADER_CHANGED_PENALTY   7
> +#define FLAC_HEADER_CRC_FAIL_PENALTY  50
> +#define FLAC_HEADER_NOT_PENALIZED_YET 100000
> +#define FLAC_HEADER_NOT_SCORED_YET    -100000

You don't really need to prefix these with FLAC_ since they're only used
within the FLAC parser.  But if you prefer that's ok with me.

> +/**
> + * return a pointer in the fifo buffer where the offset starts at until
> + * the wrap point or end of request.
> + * len will contain the valid length of the returned buffer.
> + * A second call to flac_fifo_read (with new offset and len) should be called
> + * to get the post-wrap buf if the returned len is less than the requested.
> + **/

nit-pick: capitalize "return"

> +        if (!crc ^ !inverted_test) {
> +            deduction += FLAC_HEADER_CRC_FAIL_PENALTY;
> +            av_log(fpc->avctx, AV_LOG_WARNING + log_level_offset,
> +                   "crc check failed from offset %i (frame %llu) to %i (frame %llu)\n",
> +                   header->offset, header_fi->frame_or_sample_num,
> +                   child->offset, child_fi->frame_or_sample_num);


frame_or_sample_num is int64_t, so you should use %"PRId64" instead of %llu.

> +    fpc->avctx->sample_rate = header->fi.samplerate;
> +    fpc->avctx->channels    = header->fi.channels;
> +    fpc->avctx->frame_size  = header->fi.blocksize;
> +    *poutbuf           = flac_fifo_read_wrap(fpc, header->offset, *poutbuf_size,
> +                                        &fpc->wrap_buf,
> +                                        &fpc->wrap_buf_allocated_size);


weird indentation at *poutbuf.  I'd say either line it up with the other
= above or just put one space and don't worry about the vertical
alignment of the = in this case.

> +
> +    fpc->best_header_valid = 0;
> +    /* Return the negative overread index so the client can compute pos.
> +       This should be the amount overread to the beginning of the child */
> +    return child ? child->offset - av_fifo_size(fpc->fifo_buf) : 0;


Your way is more compact, but I think it would be more readable as:
if (child)
    return child->offset - av_fifo_size(fpc->fifo_buf);
return 0;


> +    if (s->flags & PARSER_FLAG_COMPLETE_FRAMES) {
> +        FLACFrameInfo fi;
> +        if (frame_header_is_valid(avctx, buf, &fi))
> +            avctx->frame_size = fi.blocksize;
> +        *poutbuf      = buf;
> +        *poutbuf_size = buf_size;
> +        return buf_size;
> +    }


Right now the matroska demuxer sets
st->need_parsing = AVSTREAM_PARSE_HEADERS;
So this part will be executed for matroska packets if the FLAC parser is
enabled.

Maybe we should consider adding that to other demuxers that can handle
FLAC.  This could be done later though.  I just thought I would bring it up.


The rest looks ok.

-Justin