[FFmpeg-devel] [PATCH] FLAC parser
Michael Niedermayer
michaelni
Tue Oct 19 14:48:16 CEST 2010
On Sat, Oct 16, 2010 at 10:52:47PM +0200, Michael Chinen wrote:
> 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.
what is the speed difference between current svn and after this patch ?
[...]
> 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
> 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
> 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
[...]
> +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
[...]
> +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
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20101019/7c3da532/attachment.pgp>
More information about the ffmpeg-devel
mailing list