[FFmpeg-devel] [PATCH] FLAC parser

[Michael Chinen]

Hi,

On Wed, Aug 18, 2010 at 7:29 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
[...]
>> Putting in MN's suggestion to concatenate sequential valid CRCs instead of checking them twice.
>
> elaborate on what you do here please

When a new header candidate (FLACHeaderMarker) is found and added as
the nth header a linked list, CRC is verified from the start of the
n-1 header to the nth.  If this check fails, the n-2 header to n CRC
is computed up to n-kth header, where k is the
FLAC_MAX_SEQUENTIAL_HEADERS constant.  If a check succeeds the (m-1)
bit of the N-mth FLACHeaderMarker's crc_valid is set.  Then all
headers that came before it and within the max distance are marked
valid to the nth header by having the appropriate bit of crc_valid set
if they are marked valid to the N-mth header.

This means that if CRCs verify no byte will be double checked.  If a
header's crc fails, then they will.   This could be removed at the
cost of tracking non zero CRC remainders for those that fail, but
since this does not happen often, I thought it was a not a good
optimization.

>
>
> [...]
>> +static const int sample_size_table[] = { 0, 8, 12, 0, 16, 20, 24, 0 };
>> +
>> +static int64_t get_utf8(GetBitContext *gb)
>> +{
>> + ? ?int64_t val;
>> + ? ?GET_UTF8(val, get_bits(gb, 8), return -1;)
>> + ? ?return val;
>> +}
>
>> -static const int sample_size_table[] =
>> -{ 0, 8, 12, 0, 16, 20, 24, 0 };
>> -
>> -static int64_t get_utf8(GetBitContext *gb)
>> -{
>> - ? ?int64_t val;
>> - ? ?GET_UTF8(val, get_bits(gb, 8), return -1;)
>> - ? ?return val;
>> -}
>> -
>
> code moving should be seperate from functional changes
> also the table would fit in uint8_t
moved to its own 0001 patch and tested build/run.
Reattaching all three because indexes changed.

>
>
> [...]
>> +/**
>> + * Score a header.
>> + *
>> + * Give FLAC_HEADER_BASE_SCORE points to a frame for existing.
>> + * If it has children, (subsequent frames of which the preceding CRC footer
>> + * validates against this one,) then take the maximum score of the children,
>> + * with a penalty of FLAC_HEADER_CHANGED_PENALTY applied for each change to
>> + * bps, sample rate, channels, but not decorrelation mode, or blocksize,
>> + * because it can change often.
>> + **/
>> +static int score_header(FLACHeaderMarker *header)
>> +{
>> + ? ?FLACHeaderMarker *child;
>> + ? ?int dist = 0;
>> + ? ?int child_score;
>> +
>> + ? ?if (header->max_score != FLAC_HEADER_NOT_SCORED_YET)
>> + ? ? ? ?return header->max_score;
>> +
>> + ? ?header->max_score = FLAC_HEADER_BASE_SCORE;
>> +
>> + ? ?/* Check and compute the children's scores. */
>> + ? ?child = header->next;
>> + ? ?while (1 << dist <= header->crc_valid) {
>> + ? ? ? ?if (1 << dist++ & header->crc_valid) {
>
> thats the same as
> 1 << dist++ == header->crc_valid
>
> and this somehow doesnt look correct


Doesn't look the same to me.  header->crc_valid will likely have one
bit set for every header that follows it within the max distance which
CRC is valid so it will fail with an int equality test.

>
>
>> + ? ? ? ? ? ?/* Look at the child's frame header info and penalize suspicious
>> + ? ? ? ? ? ? ? changes between the headers. */
>> + ? ? ? ? ? ?child_score = score_header(child) -
>> + ? ? ? ? ? ? ? ?check_header_mismatch(NULL, &header->fi, &child->fi);
>> +
>> + ? ? ? ? ? ?if (FLAC_HEADER_BASE_SCORE + child_score > header->max_score) {
>> + ? ? ? ? ? ? ? ?/* Keep the child because the frame scoring is dynamic. */
>> + ? ? ? ? ? ? ? ?header->best_child = child;
>> + ? ? ? ? ? ? ? ?header->max_score ?= FLAC_HEADER_BASE_SCORE + child_score;
>> + ? ? ? ? ? ?}
>> + ? ? ? ?}
>> + ? ? ? ?child = child->next;
>> + ? ?}
>> + ? ?return header->max_score;
>> +}
>> +
>> +static void score_sequences(FLACParseContext *fpc)
>> +{
>> + ? ?FLACHeaderMarker *curr;
>> + ? ?int best_score = FLAC_HEADER_NOT_SCORED_YET;
>> + ? ?/* First pass to clear all old scores. */
>> + ? ?curr = fpc->headers;
>> + ? ?while (curr) {
>> + ? ? ? ?curr->max_score = FLAC_HEADER_NOT_SCORED_YET;
>> + ? ? ? ?curr ? ? ? ? ? ?= curr->next;
>> + ? ?}
>
>> + ? ?/* Do a second pass to score them all. */
>> + ? ?curr = fpc->headers;
>> + ? ?while (curr) {
>
> for(curr = fpc->headers; curr; curr = curr->next)

Thanks, this saves a lot of space.
Done and likewise for rest of applicable while loops.

>
>
> [...]
>> +static int flac_parse(AVCodecParserContext *s, AVCodecContext *avctx,
>> + ? ? ? ? ? ? ? ? ? ? ?const uint8_t **poutbuf, int *poutbuf_size,
>> + ? ? ? ? ? ? ? ? ? ? ?const uint8_t *buf, int buf_size)
>> +{
>> + ? ?FLACParseContext *fpc = s->priv_data;
>> + ? ?FLACHeaderMarker *curr;
>> + ? ?int nb_headers;
>> + ? ?void* new_buffer;
>> +
>> + ? ?if (s->flags & PARSER_FLAG_COMPLETE_FRAMES) {
>> + ? ? ? ?FLACFrameInfo fi;
>> + ? ? ? ?if (frame_header_is_valid(buf, &fi))
>> + ? ? ? ? ? ?avctx->frame_size = fi.blocksize;
>> + ? ? ? ?*poutbuf ? ? ?= buf;
>> + ? ? ? ?*poutbuf_size = buf_size;
>> + ? ? ? ?return buf_size;
>> + ? ?}
>> +
>> + ? ?if (fpc->best_header_valid)
>> + ? ? ? ?return get_best_header(fpc, avctx, poutbuf, poutbuf_size);
>> +
>> + ? ?/* If a best_header was found last call remove it with the buffer data. */
>> + ? ?if (fpc->best_header && fpc->best_header->best_child) {
>> + ? ? ? ?FLACHeaderMarker *temp;
>> + ? ? ? ?FLACHeaderMarker *best_child = fpc->best_header->best_child;
>> +
>> + ? ? ? ?curr = fpc->headers;
>> + ? ? ? ?/* Remove headers in list until the end of the best_header. */
>> + ? ? ? ?while (curr != best_child) {
>> + ? ? ? ? ? ?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);
>> + ? ? ? ? ? ?}
>> + ? ? ? ? ? ?temp = curr->next;
>> + ? ? ? ? ? ?av_free(curr);
>> + ? ? ? ? ? ?curr = temp;
>> + ? ? ? ?}
>
>> + ? ? ? ?/* Slide the data down the buffer.
>> + ? ? ? ? ? TODO: Make into ring buffer or something cleaner. */
>> + ? ? ? ?fpc->buffer_size = fpc->buffer_size - best_child->offset;
>> + ? ? ? ?memmove(fpc->buffer, fpc->buffer + best_child->offset,
>> + ? ? ? ? ? ? ? ?fpc->buffer_size);
>
> this doesnt look very fast

Changed to use fifo.
Using the existing fifo functions would be just as slow (because of
the memcpy) and impractical (because of the destructive read) so I
modified the read to have faster pointer access middle portions.
If this should go as a separate patch to fifo.h/c let me know (I
thought it would complicate the otherwise standard and simple fifo.)
If someone needs it we can always move later.

>
>
>> +
>> + ? ? ? ?/* Fix the offset for the headers remaining to match the new buffer. */
>> + ? ? ? ?curr = best_child->next;
>> + ? ? ? ?while (curr) {
>> + ? ? ? ? ? ?curr->offset -= best_child->offset;
>> + ? ? ? ? ? ?curr = curr->next;
>> + ? ? ? ?}
>> + ? ? ? ?best_child->offset = 0;
>> + ? ? ? ?fpc->headers ? ? ? = best_child;
>> + ? ? ? ?fpc->best_header = NULL;
>> + ? ?} else if (fpc->best_header) {
>> + ? ? ? ?/* No end frame no need to delete the buffer; probably eof */
>> + ? ? ? ?FLACHeaderMarker *temp;
>> + ? ? ? ?curr = fpc->headers;
>
>> + ? ? ? ?while (curr != fpc->best_header) {
>> + ? ? ? ? ? ?temp = curr->next;
>> + ? ? ? ? ? ?av_free(curr);
>> + ? ? ? ? ? ?curr = temp;
>> + ? ? ? ?}
>
> duplicate

Reduced to for loop as above.

Thanks,

Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-move-decode_frame_header-from-flacdec.c-to-flac.c-h.patch
Type: application/octet-stream
Size: 8119 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100820/ee214433/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-FLAC-Parser.patch
Type: application/octet-stream
Size: 39577 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100820/ee214433/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-fix-ffplay-to-keep-calling-av_read_frame-even-if-EOF.patch
Type: application/octet-stream
Size: 709 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100820/ee214433/attachment-0002.obj>