[FFmpeg-devel] [PATCH] FLAC parser

[Michael Chinen]

Hi,

On Thu, Nov 11, 2010 at 1:05 AM, Justin Ruggles
<justin.ruggles at gmail.com> wrote:
> 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.

I can't find a reference to AV_LOG_MAX with grep and AV_LOG_DEBUG is
the largest value in log.h.

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

changed to "Each time it finds and verifies a CRC-8 header it sees which..."
because I wanted to explain how the 8 bit and 16 bit CRCs are treated.

>
>> +/** 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"

ok

>
>> + ? ? ? ?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.

thanks for seeing this.

>
>> + ? ?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.

ok, made one space.

>
>> +
>> + ? ?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;

done.

>
>
>> + ? ?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.

I don't know much about matroska.  Is this desired?

I'm waiting on the AV_LOG_MAX bit before attaching.

Michael


>
> 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
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
>