[FFmpeg-devel] [PATCH] FLAC parser

[Michael Chinen]

Hi,

On Thu, Oct 14, 2010 at 10:25 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Wed, Oct 13, 2010 at 02:26:13AM +0200, Michael Chinen wrote:
> [...]
>> +static void update_sequences_header(FLACParseContext *fpc,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?FLACHeaderMarker *mid,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?FLACHeaderMarker *end, int distance)
>> +{
>> + ? ?FLACHeaderMarker *child = mid;
>> + ? ?int dist_from_start = 0;
>> + ? ?uint8_t *buf;
>> + ? ?/* CRC verify the children first to find out where they connect to */
>> + ? ?if (!mid->next)
>> + ? ? ? ?return;
>> + ? ?update_sequences_header(fpc, mid->next, end, distance - 1);
>> +
>> + ? ?for (child = mid->next; end != child; child = child->next) {
>> + ? ? ? ?/* If mid is CRC verified to a child verified to end then mid is
>> + ? ? ? ? ? verified till end. ?Also if mid is not verified to a child
>> + ? ? ? ? ? that is verified to the end then mid will fail the CRC check. */
>> + ? ? ? ?if (child->crc_valid & 1 << (distance - dist_from_start)) {
>> + ? ? ? ? ? ?if (mid->crc_valid & 1 << dist_from_start)
>> + ? ? ? ? ? ? ? ?mid->crc_valid |= 1 << distance;
>> + ? ? ? ? ? ?return;
>> + ? ? ? ?}
>> + ? ? ? ?dist_from_start++;
>> + ? ?}
>> +
>
>> + ? ?/* 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)) {
>
> crc calculation does not need a "flat" buffer, the memcpy this includes and
> code complexity is unneeded

I tried it without the flat buffer.  This doesn't seem to
change running time according to the profiling (maybe even a miniscule
slowdown,) and does add about 12 extra lines of code for the cases and
new non-wrap function.

>
>
>
>> + ? ? ? ?mid->crc_valid |= 1 << distance;
>> + ? ?} else {
>> + ? ? ? ?av_log(fpc->avctx, AV_LOG_DEBUG,
>> + ? ? ? ? ? ? ? "update_sequence CRC failed at dist %i from %i to %i\n",
>> + ? ? ? ? ? ? ? distance, mid->offset, end->offset);
>> + ? ?}
>> +}
>> +
>> +static void update_sequences(FLACParseContext *fpc, int start_index,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? int start_distance, FLACHeaderMarker *end)
>> +{
>> + ? ?FLACHeaderMarker *mid = fpc->headers;
>> +
>> + ? ?while (start_index-- > 0)
>> + ? ? ? ?mid = mid->next;
>> +
>> + ? ?update_sequences_header(fpc, mid, end, start_distance - 1);
>> +}
>> +
>
>> +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.)

These results are not so surprising because it the wrap around case is
not very common, but it was good to test.

I am including the patches (0004 for profiling timer and 0005 for the
non-wrap buffer functions and use), but I recommend not to add 0005
since it seems to be without much merit for the extra code that is
added.

I also improved the old flac_fifo_read function because I was doing
something dumb with reallocations.  This has been incorporated in
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/20101015/dcac39ff/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/20101015/dcac39ff/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-FLAC-Parser-flac_parser.c-verifies-all-possible-.patch
Type: application/octet-stream
Size: 35035 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101015/dcac39ff/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-DEBUGtimer-for-flac-parser-benchmarks.patch
Type: application/octet-stream
Size: 1572 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101015/dcac39ff/attachment-0003.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-DEBUGadd-flac_fifo_read_wrap-and-flac_fifo_read.patch
Type: application/octet-stream
Size: 7980 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101015/dcac39ff/attachment-0004.obj>