[FFmpeg-devel] [PATCH] FLAC parser
Michael Chinen
mchinen
Wed Oct 20 15:38:14 CEST 2010
Hi,
On Tue, Oct 19, 2010 at 2:48 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>[...]
>> 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.
>
>what is the speed difference between current svn and after this patch ?
I used the -benchmark flag for 'ffmpeg -i fourminsong.flac a.wav' and
five runs and got
without patch: utime = 2.044-2.042s
with patch: utime = 2.363-2.379s
So flac demuxing with the parser is slower.
>> ?flac.c ? ?| ? 39 +++++++++++++++++----------------------
>> ?flac.h ? ?| ? 23 +++++++++++++++++++----
>> ?flacdec.c | ? 33 ++++++++++++++++++++++++++++++---
>> ?3 files changed, 66 insertions(+), 29 deletions(-)
>> 04d2d10d0789a24002a5d5e12503ecfee92ec446 ?0002-Add-error-codes-for-FLAC-header-parsing-and-move-log.patch
>> From 6598289f49b9bc5636c35c548891a43a4b92109e Mon Sep 17 00:00:00 2001
>> From: Michael Chinen <mchinen at gmail.com>
>> Date: Thu, 7 Oct 2010 17:47:52 +0200
>> Subject: [PATCH 2/3] Add error codes for FLAC header parsing and move log errors to flacdec.c
>>
>> ---
>> ?libavcodec/flac.c ? ?| ? 39 +++++++++++++++++----------------------
>> ?libavcodec/flac.h ? ?| ? 23 +++++++++++++++++++----
>> ?libavcodec/flacdec.c | ? 33 ++++++++++++++++++++++++++++++---
>> ?3 files changed, 66 insertions(+), 29 deletions(-)
>>
>> diff --git a/libavcodec/flac.c b/libavcodec/flac.c
>> index f6b65ce..318119d 100644
>> --- a/libavcodec/flac.c
>> +++ b/libavcodec/flac.c
>> @@ -32,13 +32,16 @@ static int64_t get_utf8(GetBitContext *gb)
>> ? ? ?return val;
>> ?}
>>
>> -int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?FLACFrameInfo *fi)
>> +int ff_flac_decode_frame_header(GetBitContext *gb, FLACFrameInfo *fi)
>> ?{
>> ? ? ?int bs_code, sr_code, bps_code;
>>
>> ? ? ?/* frame sync code */
>> - ? ?skip_bits(gb, 16);
>> + ? ?if ((get_bits(gb, 15) & 0x7FFF) != 0x7FFC)
>> + ? ? ? ?return FLAC_FRAME_HEADER_ERROR_SYNC;
>> +
>> + ? ?/* variable block size stream code */
>> + ? ?fi->is_var_size = get_bits1(gb);
>>
>> ? ? ?/* block size and sample rate codes */
>> ? ? ?bs_code = get_bits(gb, 4);
>> @@ -48,39 +51,34 @@ int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
>> ? ? ?fi->ch_mode = get_bits(gb, 4);
>> ? ? ?if (fi->ch_mode < FLAC_MAX_CHANNELS) {
>> ? ? ? ? ?fi->channels = fi->ch_mode + 1;
>> - ? ? ? ?fi->ch_mode = FLAC_CHMODE_INDEPENDENT;
>> + ? ? ? ?fi->ch_mode ?= FLAC_CHMODE_INDEPENDENT;
>> ? ? ?} else if (fi->ch_mode <= FLAC_CHMODE_MID_SIDE) {
>
> cosmetic
reverted.
>
>
>> ? ? ? ? ?fi->channels = 2;
>> ? ? ?} else {
>> - ? ? ? ?av_log(avctx, AV_LOG_ERROR, "invalid channel mode: %d\n", fi->ch_mode);
>> - ? ? ? ?return -1;
>> + ? ? ? ?return FLAC_FRAME_HEADER_ERROR_CHANNEL_CFG;
>> ? ? ?}
>>
>> ? ? ?/* bits per sample */
>> ? ? ?bps_code = get_bits(gb, 3);
>> ? ? ?if (bps_code == 3 || bps_code == 7) {
>> - ? ? ? ?av_log(avctx, AV_LOG_ERROR, "invalid sample size code (%d)\n",
>> - ? ? ? ? ? ? ? bps_code);
>> - ? ? ? ?return -1;
>> + ? ? ? ?fi->bps = bps_code;
>> + ? ? ? ?return FLAC_FRAME_HEADER_ERROR_BPS;
>> ? ? ?}
>
> this sets the struct to an invalid value
Should this be a valid value? The error is returned so the client can
do something with it, but this might be moot per your next point on
error codes.
>
>
>> ? ? ?fi->bps = sample_size_table[bps_code];
>>
>> ? ? ?/* reserved bit */
>> ? ? ?if (get_bits1(gb)) {
>> - ? ? ? ?av_log(avctx, AV_LOG_ERROR, "broken stream, invalid padding\n");
>> - ? ? ? ?return -1;
>> + ? ? ? ?return FLAC_FRAME_HEADER_ERROR_PADDING;
>> ? ? ?}
>>
>> ? ? ?/* sample or frame count */
>> - ? ?if (get_utf8(gb) < 0) {
>> - ? ? ? ?av_log(avctx, AV_LOG_ERROR, "utf8 fscked\n");
>> - ? ? ? ?return -1;
>> - ? ?}
>> + ? ?fi->frame_or_sample_num = get_utf8(gb);
>> + ? ?if (fi->frame_or_sample_num < 0)
>> + ? ? ? ?return FLAC_FRAME_HEADER_ERROR_SAMPLE_NUM;
>>
>> ? ? ?/* blocksize */
>> ? ? ?if (bs_code == 0) {
>> - ? ? ? ?av_log(avctx, AV_LOG_ERROR, "reserved blocksize code: 0\n");
>> - ? ? ? ?return -1;
>> + ? ? ? ?return FLAC_FRAME_HEADER_ERROR_BLOCK_SIZE;
>> ? ? ?} else if (bs_code == 6) {
>> ? ? ? ? ?fi->blocksize = get_bits(gb, 8) + 1;
>> ? ? ?} else if (bs_code == 7) {
>> @@ -99,17 +97,14 @@ int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
>> ? ? ?} else if (sr_code == 14) {
>> ? ? ? ? ?fi->samplerate = get_bits(gb, 16) * 10;
>> ? ? ?} else {
>> - ? ? ? ?av_log(avctx, AV_LOG_ERROR, "illegal sample rate code %d\n",
>> - ? ? ? ? ? ? ? sr_code);
>> - ? ? ? ?return -1;
>> + ? ? ? ?return FLAC_FRAME_HEADER_ERROR_SAMPLE_RATE;
>> ? ? ?}
>>
>> ? ? ?/* header CRC-8 check */
>> ? ? ?skip_bits(gb, 8);
>> ? ? ?if (av_crc(av_crc_get_table(AV_CRC_8_ATM), 0, gb->buffer,
>> ? ? ? ? ? ? ? ? get_bits_count(gb)/8)) {
>> - ? ? ? ?av_log(avctx, AV_LOG_ERROR, "header crc mismatch\n");
>> - ? ? ? ?return -1;
>> + ? ? ? ?return FLAC_FRAME_HEADER_ERROR_CRC;
>> ? ? ?}
>>
>> ? ? ?return 0;
>> diff --git a/libavcodec/flac.h b/libavcodec/flac.h
>> index fe38463..15c1760 100644
>> --- a/libavcodec/flac.h
>> +++ b/libavcodec/flac.h
>> @@ -58,6 +58,17 @@ enum FLACExtradataFormat {
>> ? ? ?FLAC_EXTRADATA_FORMAT_FULL_HEADER = 1
>> ?};
>>
>> +enum {
>> + ? ?FLAC_FRAME_HEADER_ERROR_SYNC ? ? ? ? = -1,
>> + ? ?FLAC_FRAME_HEADER_ERROR_CHANNEL_CFG ?= -2,
>> + ? ?FLAC_FRAME_HEADER_ERROR_BPS ? ? ? ? ?= -3,
>> + ? ?FLAC_FRAME_HEADER_ERROR_PADDING ? ? ?= -4,
>> + ? ?FLAC_FRAME_HEADER_ERROR_SAMPLE_NUM ? = -5,
>> + ? ?FLAC_FRAME_HEADER_ERROR_BLOCK_SIZE ? = -6,
>> + ? ?FLAC_FRAME_HEADER_ERROR_SAMPLE_RATE ?= -7,
>> + ? ?FLAC_FRAME_HEADER_ERROR_CRC ? ? ? ? ?= -8,
>> +};
>> +
>> ?#define FLACCOMMONINFO \
>> ? ? ?int samplerate; ? ? ? ? /**< sample rate ? ? ? ? ? ? ? ? ? ? ? ? ? ? */\
>> ? ? ?int channels; ? ? ? ? ? /**< number of channels ? ? ? ? ? ? ? ? ? ? ?*/\
>> @@ -81,6 +92,11 @@ typedef struct FLACFrameInfo {
>> ? ? ?FLACCOMMONINFO
>> ? ? ?int blocksize; ? ? ? ? ?/**< block size of the frame ? ? ? ? ? ? ? ? */
>> ? ? ?int ch_mode; ? ? ? ? ? ?/**< channel decorrelation mode ? ? ? ? ? ? ?*/
>> + ? ?int64_t frame_or_sample_num; ?/**< frame number or sample number ? ? ? ? */
>> + ? ?int is_var_size; ? ? ? ? ? ? ?/**< specifies if the stream uses variable
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? block sizes or a fixed block size;
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? also determines the meaning of
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? frame_or_sample_num ? ? ? ? ? ? ? ? ? */
>> ?} FLACFrameInfo;
>>
>> ?/**
>> @@ -123,11 +139,10 @@ int ff_flac_get_max_frame_size(int blocksize, int ch, int bps);
>>
>> ?/**
>> ? * Validate and decode a frame header.
>> - * @param ? ? ?avctx AVCodecContext to use as av_log() context
>> ? * @param ? ? ?gb ? ?GetBitContext from which to read frame header
>> ? * @param[out] fi ? ?frame information
>> - * @return non-zero on error, 0 if ok
>> + * @return non-zero on error - see FLAC_FRAME_HEADER_ERROR_* enum, 0 if ok
>> ? */
>> -int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
>> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?FLACFrameInfo *fi);
>> +int ff_flac_decode_frame_header(GetBitContext *gb, FLACFrameInfo *fi);
>> +
>> ?#endif /* AVCODEC_FLAC_H */
>> diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c
>> index d9e1c80..133adbb 100644
>> --- a/libavcodec/flacdec.c
>> +++ b/libavcodec/flacdec.c
>> @@ -472,12 +472,39 @@ static inline int decode_subframe(FLACContext *s, int channel)
>>
>> ?static int decode_frame(FLACContext *s)
>> ?{
>> - ? ?int i;
>> + ? ?int i, ret;
>> ? ? ?GetBitContext *gb = &s->gb;
>> ? ? ?FLACFrameInfo fi;
>>
>> - ? ?if (ff_flac_decode_frame_header(s->avctx, gb, &fi)) {
>> - ? ? ? ?av_log(s->avctx, AV_LOG_ERROR, "invalid frame header\n");
>> + ? ?ret = ff_flac_decode_frame_header(gb, &fi);
>> + ? ?if (ret) {
>> + ? ? ? ?av_log(s->avctx, AV_LOG_ERROR, "invalid frame header: ");
>> + ? ? ? ?switch (ret) {
>> + ? ? ? ?case FLAC_FRAME_HEADER_ERROR_SYNC:
>> + ? ? ? ? ? ?av_log(s->avctx, AV_LOG_ERROR, "frame sync error\n");
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?case FLAC_FRAME_HEADER_ERROR_CHANNEL_CFG:
>> + ? ? ? ? ? ?av_log(s->avctx, AV_LOG_ERROR, "invalid channel mode: %d\n", fi.ch_mode);
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?case FLAC_FRAME_HEADER_ERROR_BPS:
>> + ? ? ? ? ? ?av_log(s->avctx, AV_LOG_ERROR, "invalid sample size code (%d)\n", fi.bps);
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?case FLAC_FRAME_HEADER_ERROR_PADDING:
>> + ? ? ? ? ? ?av_log(s->avctx, AV_LOG_ERROR, "broken stream, invalid padding\n");
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?case FLAC_FRAME_HEADER_ERROR_SAMPLE_NUM:
>> + ? ? ? ? ? ?av_log(s->avctx, AV_LOG_ERROR, "sample/frame number invalid; utf8 fscked\n");
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?case FLAC_FRAME_HEADER_ERROR_BLOCK_SIZE:
>> + ? ? ? ? ? ?av_log(s->avctx, AV_LOG_ERROR, "reserved blocksize code: 0\n");
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?case FLAC_FRAME_HEADER_ERROR_SAMPLE_RATE:
>> + ? ? ? ? ? ?av_log(s->avctx, AV_LOG_ERROR, "illegal sample rate code: 15\n");
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?case FLAC_FRAME_HEADER_ERROR_CRC:
>> + ? ? ? ? ? ?av_log(s->avctx, AV_LOG_ERROR, "header crc mismatch\n");
>> + ? ? ? ? ? ?break;
>> + ? ? ? ?}
>> ? ? ? ? ?return -1;
>> ? ? ?}
>>
>> --
>> 1.7.1
>>
>
> also its a alot simpler to pass a offset to adjust the log level instead of
> returning error codes and then translating them to log messages
It was originally more like what you suggest, but it was changed to be
similar to the ac3 parser (see discussion in this thread on july 30.)
>
> [...]
>> +static uint8_t* flac_fifo_read_wrap(FLACParseContext *fpc, int offset, int len,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? uint8_t** wrap_buf, int* allocated_size)
>> +{
>> + ? ?AVFifoBuffer *f ? = fpc->fifo_buf;
>> + ? ?uint8_t *start ? ?= f->rptr + offset;
>> + ? ?uint8_t *tmp_buf;
>> +
>> + ? ?if (start >= f->end)
>> + ? ? ? ?start -= f->end - f->buffer;
>> + ? ?if(f->end - start >= len)
>> + ? ? ? ?return start;
>> +
>> + ? ?tmp_buf = av_fast_realloc(*wrap_buf, allocated_size, len);
>> +
>> + ? ?if (!tmp_buf) {
>> + ? ? ? ?av_log(fpc->avctx, AV_LOG_ERROR,
>> + ? ? ? ? ? ? ? "couldn't reallocate wrap buffer of size %d", len);
>> + ? ? ? ?return NULL;
>> + ? ?}
>> + ? ?*wrap_buf = tmp_buf;
>> + ? ?do {
>> + ? ? ? ?int seg_len = FFMIN(f->end - start, len);
>> + ? ? ? ?memcpy(tmp_buf, start, seg_len);
>> + ? ? ? ?tmp_buf = (uint8_t*)tmp_buf + seg_len;
>
>> +// memory barrier needed for SMP here in theory
>
> elaborate
This wrapping code was modified from fifo.c. I left the comment in,
not knowing much about SMP is.
>
>
> [...]
>> +static int find_headers_search(FLACParseContext *fpc, uint8_t *buf, int buf_size,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int search_start)
>> +
>> +{
>> + ? ?FLACFrameInfo fi;
>> + ? ?int size = 0, i;
>> + ? ?uint8_t *header_buf;
>> +
>> + ? ?for (i = 0; i < buf_size - 1; i++) {
>> + ? ? ? ?if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8) {
>
> something based on testing several positions at once is likely faster
> like
> x= AV_RB32()
> (x & ~(x+0x01010101))&0x80808080
> that will detect 0xFF bytes and only after that testing the 4 positions for
> FFF8
Hmm. Since in both cases (header there/header not there) this will
require more masks on a 2 byte int how will it be faster?
Also since it is 15 bits that we are looking for is the 32 bit
handling a mistake?
Anyway I tried profiling find_new_headers() and with the bit position
testing the fastest with the bit position testing I got was
fastest:
399319 dezicycles in with more pos testing, 256 runs, 0 skips
slowest:
425047 dezicycles in with more pos testing, 255 runs, 1 skips
vs the existing method:
fastest:
357554 dezicycles in with more pos testing, 16353 runs, 31 skips
slowest:
373757 dezicycles in with more pos testing, 256 runs, 0 skips
so if I am doing it correctly then the original appears to be faster
Attaching these debug profiling patches if you want to inspect.
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/20101020/21c35cdb/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: 7369 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101020/21c35cdb/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/20101020/21c35cdb/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-DEBUG-add-bit-position-testing-for-header-search.patch
Type: application/octet-stream
Size: 1043 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101020/21c35cdb/attachment-0003.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-DEBUG-add-timer-profiling-for-find_new_headers.patch
Type: application/octet-stream
Size: 1735 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101020/21c35cdb/attachment-0004.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-DEBUG-restore-original-method-for-header-search.patch
Type: application/octet-stream
Size: 1121 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101020/21c35cdb/attachment-0005.obj>
More information about the ffmpeg-devel
mailing list