[FFmpeg-devel] [PATCH] FLAC parser

Michael Chinen mchinen
Sat Oct 16 22:52:47 CEST 2010


Hi,

On Fri, Oct 15, 2010 at 1:43 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>[...]
>> >
>> >> +static int find_new_headers(FLACParseContext *fpc, int search_start)
>> >> +{
>> >> + ? ?FLACFrameInfo fi;
>> >> + ? ?FLACHeaderMarker *end;
>> >> + ? ?int i, search_end, end_offset = -1, size = 0;
>> >> + ? ?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);
>> >> + ? ?buf = flac_fifo_read(fpc, search_start,
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? search_end + (MAX_FRAME_HEADER_SIZE - 1) - search_start,
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? &fpc->wrap_buf, &fpc->wrap_buf_allocated_size);
>> >> + ? ?for (i = 0; i < search_end - search_start; i++) {
>> >> + ? ? ? ?if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8 &&
>> >
>> > This code does not need a flat buffer.
>> > only frame_header_is_valid needs a max of 16 bytes flat buffer
>>
>> I tried using a 16 bytes max wrap buffer here that is read into inside
>> the for loop, but this was much slower. ?This for loop gets iterated
>> over each byte that goes in so having things inside the for loop has
>> penalties. ?(This is therefore not included in the patches but is easy to add.)
>
> you should search for a valid start code and then load the 16 byte not load 16
> byte for every byte.

I redid this function.

> [...]
>> @@ -140,12 +138,36 @@ static uint8_t* flac_fifo_read(FLACParseContext *fpc, int offset, int len,
>> ? ? ?return *wrap_buf;
>> ?}
>>
>> +/**
>> + * 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.
>> + **/
>> +static uint8_t* flac_fifo_read(FLACParseContext *fpc, int offset, int *len)
>> +{
>> + ? ?AVFifoBuffer *f ? = fpc->fifo_buf;
>> + ? ?uint8_t *start ? ?= f->rptr + offset;
>> +
>> + ? ?if (start > f->end)
>> + ? ? ? ?start -= f->end - f->buffer;
>
>> + ? ?if (start > f->end) {
>> + ? ? ? ?av_log(fpc->avctx, AV_LOG_ERROR, "fifo read request size larger than fifo\n");
>> + ? ? ? ?*len = 0;
>> + ? ? ? ?return NULL;
>> + ? ?}
>
> how can this occur?

It's not needed.  Removed.

>[...]
>> + ? ?*len = FFMIN(*len, f->end - start);
>> + ? ?return start;
>> +}
>> +
>> ?static void update_sequences_header(FLACParseContext *fpc,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?FLACHeaderMarker *mid,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?FLACHeaderMarker *end, int distance)
>> ?{
>> ? ? ?FLACHeaderMarker *child = mid;
>> - ? ?int dist_from_start = 0;
>> + ? ?int dist_from_start = 0, read_len;
>> + ? ?uint32_t crc;
>> ? ? ?uint8_t *buf;
>> ? ? ?/* CRC verify the children first to find out where they connect to */
>> ? ? ?if (!mid->next)
>> @@ -166,10 +188,17 @@ static void update_sequences_header(FLACParseContext *fpc,
>>
>> ? ? ?/* Set a bit showing the validity at this distance if CRC is ok.
>> ? ? ? ? (distance variable is really one less than linked list distance) */
>> - ? ?buf = flac_fifo_read(fpc, mid->offset, end->offset - mid->offset,
>> - ? ? ? ? ? ? ? ? ? ? ? ? &fpc->crc_buf, &fpc->crc_buf_allocated_size);
>> - ? ?if (!av_crc(av_crc_get_table(AV_CRC_16_ANSI), 0,
>> - ? ? ? ? ? ? ? ?buf, end->offset - mid->offset)) {
>> + ? ?read_len = end->offset - mid->offset;
>> + ? ?buf = flac_fifo_read(fpc, mid->offset, &read_len);
>> + ? ?crc = av_crc(av_crc_get_table(AV_CRC_16_ANSI), 0, buf, read_len);
>> + ? ?read_len = (end->offset - mid->offset) - read_len;
>> +
>> + ? ?if (read_len) {
>> + ? ? ? ?buf = flac_fifo_read(fpc, end->offset - read_len, &read_len);
>> + ? ? ? ?crc = av_crc(av_crc_get_table(AV_CRC_16_ANSI), 0, buf, read_len);
>> + ? ?}
>
> id say, this code doesn work

Oops, thanks. It was wrong, but crc was still 0 because due to a bug
in the new flac_fifo_read it was returning size 0 for the post-wrap
half.  So it would have been harder to catch without you mentioning
it.  Fixed.

I did profiling again and it turns out I missed one exit point for the
function the last time.  The non-flat wrap buffer version is about
2-4% faster overall.  I've squashed it into the 0003.

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/20101016/2d6a59bb/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-error-codes-for-FLAC-header-parsing-and-move-log.patch
Type: application/octet-stream
Size: 7589 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101016/2d6a59bb/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-FLAC-Parser.patch
Type: application/octet-stream
Size: 37104 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101016/2d6a59bb/attachment-0002.obj>



More information about the ffmpeg-devel mailing list