[FFmpeg-devel] [PATCH] FLAC parser
Justin Ruggles
justin.ruggles
Sun Aug 1 22:29:47 CEST 2010
Hi,
Michael Chinen wrote:
> Thanks Justin and Diego, these were good suggestions, and I've put
> them all in, attached here (don't forget to apply the original second
> patch if you are testing on ffplay.)
>
> On Fri, Jul 30, 2010 at 8:55 AM, Diego Biurrun <diego at biurrun.de> wrote:
>>> --- a/libavcodec/flacdec.c
>>> +++ b/libavcodec/flacdec.c
>>> @@ -26,7 +26,7 @@
>>> *
>>> * For more information on the FLAC format, visit:
>>> * http://flac.sourceforge.net/
>>> - *
>>> +v *
>> oops..
>>
>> Never forget to compile the code you submit :)
> but I don't want to give the impression that I don't compile before
> submitting. This was in a comment block :)
>
> I looked over the code a few times to fix style, but I'm not confident
> that I'm not missing things. Please bear with me on this and let me
> know what needs to be made nice. Both my fingers and eyes are still
> getting used to this style.
Here is a review of your patch, modified as indicated in my previous email.
> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> index 545c355..860b817 100644
> --- libavcodec/Makefile
> +++ libavcodec/Makefile
> @@ -560,6 +560,7 @@ OBJS-$(CONFIG_DIRAC_PARSER) += dirac_parser.o
> OBJS-$(CONFIG_DNXHD_PARSER) += dnxhd_parser.o
> OBJS-$(CONFIG_DVBSUB_PARSER) += dvbsub_parser.o
> OBJS-$(CONFIG_DVDSUB_PARSER) += dvdsub_parser.o
> +OBJS-$(CONFIG_FLAC_PARSER) += flac_parser.o flacdata.o flac.o
> OBJS-$(CONFIG_H261_PARSER) += h261_parser.o
> OBJS-$(CONFIG_H263_PARSER) += h263_parser.o
> OBJS-$(CONFIG_H264_PARSER) += h264_parser.o h264.o \
> diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
> index f17d03a..18df3cc 100644
> --- libavcodec/allcodecs.c
> +++ libavcodec/allcodecs.c
> @@ -366,6 +366,7 @@ void avcodec_register_all(void)
> REGISTER_PARSER (DNXHD, dnxhd);
> REGISTER_PARSER (DVBSUB, dvbsub);
> REGISTER_PARSER (DVDSUB, dvdsub);
> + REGISTER_PARSER (FLAC, flac);
> REGISTER_PARSER (H261, h261);
> REGISTER_PARSER (H263, h263);
> REGISTER_PARSER (H264, h264);
ok.
> diff --git a/libavcodec/flac.c b/libavcodec/flac.c
> index a649e08..354960d 100644
> --- libavcodec/flac.c
> +++ libavcodec/flac.c
> @@ -19,7 +19,97 @@
> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> */
>
> +#include "libavutil/crc.h"
> #include "flac.h"
> +#include "flacdata.h"
> +
> +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;
> +}
ok.
> +
> +int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
> + FLACFrameInfo *fi)
vertical alignment. also, avctx is not needed.
> +{
> + int bs_code, sr_code, bps_code;
> +
> + /* frame sync code */
> + if ((get_bits(gb, 15) & 0x7FFF) != 0x7FFC)
> + return FLAC_PARSE_ERROR_SYNC;
> +
> + /* uses fixed size stream code */
> + fi->is_var_size = get_bits1(gb);
> +
> + /* block size and sample rate codes */
> + bs_code = get_bits(gb, 4);
> + sr_code = get_bits(gb, 4);
> +
> + /* channels and decorrelation */
> + 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;
> + } else if (fi->ch_mode <= FLAC_CHMODE_MID_SIDE) {
> + fi->channels = 2;
> + } else {
> + return FLAC_PARSE_ERROR_CHANNEL_CFG;
> + }
> +
> + /* bits per sample */
> + bps_code = get_bits(gb, 3);
> + if (bps_code == 3 || bps_code == 7) {
> + fi->bps = bps_code;
> + return FLAC_PARSE_ERROR_BPS;
> + }
> + fi->bps = sample_size_table[bps_code];
> +
> + /* reserved bit */
> + if (get_bits1(gb))
> + return FLAC_PARSE_ERROR_PADDING;
> +
> + /* sample or frame count */
> + fi->frame_or_samp_num = get_utf8(gb);
> + if (fi->frame_or_samp_num < 0)
> + return FLAC_PARSE_ERROR_SAMPLE_NUM;
> +
> + /* blocksize */
> + if (bs_code == 0) {
> + return FLAC_PARSE_ERROR_SAMPLE_RATE;
> + } else if (bs_code == 6) {
> + fi->blocksize = get_bits(gb, 8) + 1;
> + } else if (bs_code == 7) {
> + fi->blocksize = get_bits(gb, 16) + 1;
> + } else {
> + fi->blocksize = ff_flac_blocksize_table[bs_code];
> + }
> +
> + /* sample rate */
> + if (sr_code < 12) {
> + fi->samplerate = ff_flac_sample_rate_table[sr_code];
> + } else if (sr_code == 12) {
> + fi->samplerate = get_bits(gb, 8) * 1000;
> + } else if (sr_code == 13) {
> + fi->samplerate = get_bits(gb, 16);
> + } else if (sr_code == 14) {
> + fi->samplerate = get_bits(gb, 16) * 10;
> + } else {
> + fi->samplerate = sr_code;
> + return FLAC_PARSE_ERROR_SAMPLE_RATE;
The only invalid sr_code is 15, so we don't need to set fi->samplerate
when returning the error.
> + }
> +
> + /* 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))
> + return FLAC_PARSE_ERROR_CRC;
> +
> + return 0;
> +}
ok. personally I prefer to add braces even for 1 line if there is more
than one line in the if(), but that's up to you.
>
> int ff_flac_get_max_frame_size(int blocksize, int ch, int bps)
> {
> diff --git a/libavcodec/flac.h b/libavcodec/flac.h
> index 1b11463..043a0a5 100644
> --- libavcodec/flac.h
> +++ libavcodec/flac.h
> @@ -28,11 +28,13 @@
> #define AVCODEC_FLAC_H
>
> #include "avcodec.h"
> +#include "get_bits.h"
>
> #define FLAC_STREAMINFO_SIZE 34
> #define FLAC_MAX_CHANNELS 8
> #define FLAC_MIN_BLOCKSIZE 16
> #define FLAC_MAX_BLOCKSIZE 65535
> +#define FLAC_MIN_FRAME_SIZE 11
>
> enum {
> FLAC_CHMODE_INDEPENDENT = 0,
ok.
> @@ -57,6 +59,17 @@ enum FLACExtradataFormat {
> FLAC_EXTRADATA_FORMAT_FULL_HEADER = 1
> };
>
> +enum {
> + FLAC_PARSE_ERROR_SYNC = -1,
> + FLAC_PARSE_ERROR_CHANNEL_CFG = -2,
> + FLAC_PARSE_ERROR_BPS = -3,
> + FLAC_PARSE_ERROR_PADDING = -4,
> + FLAC_PARSE_ERROR_SAMPLE_NUM = -5,
> + FLAC_PARSE_ERROR_BLOCK_SIZE = -6,
> + FLAC_PARSE_ERROR_SAMPLE_RATE = -7,
> + FLAC_PARSE_ERROR_CRC = -8,
> +};
> +
> #define FLACCOMMONINFO \
> int samplerate; /**< sample rate */\
> int channels; /**< number of channels */\
since the error codes are used by ff_flac_decode_frame_header(), maybe
the enum values should be:
FLAC_HEADER_ERROR_* or FLAC_FRAME_HEADER_ERROR_*
> @@ -78,10 +91,28 @@ typedef struct FLACStreaminfo {
>
> typedef struct FLACFrameInfo {
> FLACCOMMONINFO
> - int blocksize; /**< block size of the frame */
> - int ch_mode; /**< channel decorrelation mode */
> + int blocksize; ///< block size of the frame
> + int ch_mode; ///< channel decorrelation mode
cosmetic. I don't mind making the comment styles match, but it should be
done separately.
> + int64_t frame_or_samp_num; ///< frame number or sample number
why not just add 2 more letters and make it frame_or_sample_num?
> + int is_var_size; ///< also determines if the above variable is frames or samples
> } FLACFrameInfo;
>
> +typedef struct FLACContext {
> + FLACSTREAMINFO
> +
> + AVCodecContext *avctx; ///< parent AVCodecContext
> + GetBitContext gb; ///< GetBitContext initialized to start at the current frame
> +
> + int blocksize; ///< number of samples in the current frame
> + int curr_bps; ///< bps for current subframe, adjusted for channel correlation and wasted bits
> + int sample_shift; ///< shift required to make output samples 16-bit or 32-bit
> + int is32; ///< flag to indicate if output should be 32-bit instead of 16-bit
> + int ch_mode; ///< channel decorrelation type in the current frame
> + int got_streaminfo; ///< indicates if the STREAMINFO has been read
> +
> + int32_t *decoded[FLAC_MAX_CHANNELS]; ///< decoded samples
> +} FLACContext;
> +
Why are you moving FLACContext to flac.c? The parser doesn't use it.
> /**
> * Parse the Streaminfo metadata block
> * @param[out] avctx codec context to set basic stream parameters
> @@ -120,4 +151,14 @@ void ff_flac_parse_block_header(const uint8_t *block_header,
> */
> 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
mention the enum with the error values.
> + */
> +int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
> + FLACFrameInfo *fi);
same as above, avctx not needed and vertical alignment.
> +
> #endif /* AVCODEC_FLAC_H */
> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> new file mode 100644
> index 0000000..70d2212
> --- /dev/null
> +++ libavcodec/flac_parser.c
> @@ -0,0 +1,415 @@
> +/*
> + * FLAC parser
> + * Copyright (c) 2010 Michael Chinen
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include "libavutil/crc.h"
> +#include "bytestream.h"
> +#include "parser.h"
> +#include "flac.h"
> +
> +/* the maximum number of adjacent headers that will compare crcs against each other */
> +#define FLAC_MAX_SEQUENTIAL_HEADERS 4
> +/* the minimum number of headers that need to be buffered and checked before returning frames */
> +#define FLAC_MIN_HEADERS 20
Just out of curiosity, have you done testing with fewer headers, both
buffered and sequential?
> +
> +/* scoring settings for score_header */
> +#define FLAC_HEADER_BASE_SCORE 10
> +#define FLAC_HEADER_CHANGED_PENALTY 7
> +#define FLAC_HEADER_NOT_SCORED_YET -100000
Have you tested any files where the header change is valid to see if the
penalty value works ok? You would probably have to craft your own file
since I don't know of any encoders that will make such files.
> +
> +/* largest possible size of flac header. do not change. */
> +#define MAX_FRAME_HEADER_SIZE 16
is the "do not change" really necessary? the rest of the comment says
what it is.
> +
> +typedef struct FLACHeaderMarker {
> + int offset;
> + int crc_valid;
> + int max_score;
> + FLACFrameInfo fi;
> + struct FLACHeaderMarker *next;
> + struct FLACHeaderMarker *best_child;
> +} FLACHeaderMarker;
> +
> +typedef struct FLACParseContext {
> + uint8_t *buffer;
> + int buffer_size;
> + int buffer_allocated_size;
> + FLACHeaderMarker *headers;
> + FLACHeaderMarker *best_header;
> + int nb_headers_found;
> + int best_header_valid;
> +} FLACParseContext;
how about some documentation? for the structs, and alsy maybe some
general documentation about how the parser works.
> +
> +static int frame_header_is_valid(const uint8_t *buf, FLACFrameInfo *fi)
> +{
> + GetBitContext gb;
> + init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE*8);
> + if (ff_flac_decode_frame_header(NULL, &gb, fi))
> + return 0;
> + return 1;
> +}
> +
> +static void update_sequences_header(FLACParseContext *fpc, FLACHeaderMarker *mid, FLACHeaderMarker *end, int distance)
long line.
> +{
> + FLACHeaderMarker *child = mid;
> + int dist_from_start = 0;
> +
> + /* do the children first to find out where they connect to */
> + if (!mid->next)
> + return;
> + update_sequences_header(fpc, mid->next, end, distance - 1);
do what to the children?
> +
> + child = mid->next;
> + while (end != child) {
> + /* if mid is crc verified to a child that is verified to the end then mid */
> + /* is also verified till the 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 << dist_from_start) {
> + if (mid->crc_valid & 1 << (distance - dist_from_start) )
remove the extra space before the end parenthesis
> + mid->crc_valid |= 1 << distance;
> + return;
> + }
> + child = child->next;
> + 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) */
> + if (!av_crc(av_crc_get_table(AV_CRC_16_ANSI), 0, fpc->buffer + mid->offset, end->offset - mid->offset) )
long line
> + mid->crc_valid |= 1 << distance;
> + else
> + av_log(NULL, AV_LOG_DEBUG, "update_sequence crc failed at dist %i from %i to %i\n",
> + distance, mid->offset, end->offset);
vertical alignment
> +}
> +
> +static void update_sequences(FLACParseContext *fpc, int start_index, int start_distance, FLACHeaderMarker *end)
long line
> +{
> + int distance = start_distance - 1;
> + FLACHeaderMarker* mid = fpc->headers;
FLACHeaderMarker *mid
and you could align the =
> +
> + while (start_index-- > 0)
> + mid = mid->next;
> +
> + update_sequences_header(fpc, mid, end, distance);
> +}
> +
> +static int find_new_headers(FLACParseContext *fpc, int search_start)
> +{
> + FLACFrameInfo fi;
> + FLACHeaderMarker *end;
> + int i, end_offset = -1, size = 0;
> +
> + fpc->nb_headers_found = 0;
> +
> + /* search for a new header of at least 16 bytes */
> + for (i = search_start; i < fpc->buffer_size - (MAX_FRAME_HEADER_SIZE - 1); i++) {
> + /* FIXME: if we read into the previous buffer make sure the header returned */
> + /* is long engough to be into the current buffer. */
FIXMEs should be avoided if possible. this could possibly be solved by
having ff_flac_decode_frame_header() give the frame header size in bytes
(add to FLACFrameInfo).
> + if ((AV_RB16(fpc->buffer + i) & 0xFFFE) == 0xFFF8 &&
> + frame_header_is_valid(fpc->buffer + i, &fi)) {
> + FLACHeaderMarker **ptr= &fpc->headers;
could you use a variable name that has some relationship to its meaning
instead of ptr?
> +
> + size = 0;
> + while (*ptr) {
> + end_offset = (*ptr)->offset;
> + ptr = &(*ptr)->next;
> + size++;
> + }
> +
> + /* a header may have been found in the last search at or after search_start. */
> + if (end_offset >= i) {
> + av_log(NULL, AV_LOG_DEBUG, "found header a second time, skipping\n");
> + continue;
> + }
> +
> + *ptr= av_mallocz( sizeof(FLACHeaderMarker));
> + if (!*ptr)
> + return -1;
return AVERROR(ENOMEM)
> +
> + (*ptr)->fi = fi;
> + (*ptr)->offset = i;
> + /* actual size of the linked list is now size + 1 */
> + update_sequences(fpc, size - FLAC_MAX_SEQUENTIAL_HEADERS,
> + FFMIN(size, FLAC_MAX_SEQUENTIAL_HEADERS), *ptr);
vertical alignment
> + fpc->nb_headers_found++;
> + size++;
> + }
> + }
> + /* we need to return the size */
> + if (!size && fpc->headers) {
> + end = fpc->headers;
> + while (size++)
> + end = end->next;
> + }
this doesn't seem right. if size == 0 then execution will never enter
the while loop.
> + return size;
> +}
> +
> +/**
> + * Score a header.
> + * Give FLAC_HEADER_BASE_SCORE points to a frame for existing.
> + * If it has children (subsequent frames of which the preceeding crc
s/preceeding/preceding
> + * validates against this one,) then take the maximum score of the children,
is this parenthesis a typo or is the one below wrong?
> + * with a penalty of FLAC_HEADER_CHANGED_PENALTY applied for each change to
> + * bps, sample rate, channels, but not decorellation mode, or blocksize,
s/decorellation/decorrelation
> + * 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 our children's scores */
"our children" just sounds funny to me. but then again I don't have any
children. ;)
> + child = header->next;
> + while (1 << dist <= header->crc_valid) {
> + if (1 << dist++ & header->crc_valid) {
> + /* look at the child's frame header info to see if we need to penalize changes */
> + child_score = score_header(child);
> +
> + if (child->fi.samplerate != header->fi.samplerate)
> + child_score -= FLAC_HEADER_CHANGED_PENALTY;
> + if (child->fi.bps != header->fi.bps)
> + child_score -= FLAC_HEADER_CHANGED_PENALTY;
> + if (child->fi.is_var_size != header->fi.is_var_size)
> + child_score -= FLAC_HEADER_CHANGED_PENALTY;
Changing blocking strategy should disqualify a header. The FLAC format
specification clearly states that this cannot change:
Under the NOTES section of FRAME_HEADER:
"The 'blocking strategy' bit must be the same throughout the entire stream"
> + if (child->fi.channels != header->fi.channels)
> + child_score -= FLAC_HEADER_CHANGED_PENALTY;
> + /* check sample numbers */
> + if (header->fi.is_var_size) {
> + if (child->fi.frame_or_samp_num - header->fi.frame_or_samp_num != child->fi.blocksize)
> + child_score -= FLAC_HEADER_CHANGED_PENALTY;
> + } else {
> + if(child->fi.frame_or_samp_num != header->fi.frame_or_samp_num + 1)
> + child_score -= FLAC_HEADER_CHANGED_PENALTY;
> + }
This could use some comments telling when you're checking the frame
number and when you're checking the sample number.
> +
> + if (child_score + header->max_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;
> + }
> + /* second pass to score them all */
> + curr = fpc->headers;
> + while (curr) {
> + if (score_header(curr) > best_score) {
> + fpc->best_header = curr;
> + best_score = curr->max_score;
> + }
> + curr = curr->next;
> + }
> +}
> +
> +static int get_best_header(FLACParseContext* fpc, AVCodecContext *avctx,
> + const uint8_t **poutbuf, int *poutbuf_size, int buf_size)
> +{
> + FLACHeaderMarker *header = fpc->best_header, *child = header->best_child;
> + if (!child)
> + *poutbuf_size = fpc->buffer_size - header->offset;
> + else {
> + *poutbuf_size = child->offset - header->offset;
> +
> + if (child->fi.samplerate != header->fi.samplerate)
> + av_log(avctx, AV_LOG_DEBUG,
> + "selecting header whose best child has changed sample rate\n");
> + if (child->fi.bps != header->fi.bps)
> + av_log(avctx, AV_LOG_DEBUG,
> + "selecting header whose best child has bps change\n");
> + if (child->fi.is_var_size != header->fi.is_var_size)
> + av_log(avctx, AV_LOG_DEBUG,
> + "selecting header whose best child has variable blocksize change\n");
> + if (child->fi.channels != header->fi.channels)
> + av_log(avctx, AV_LOG_DEBUG,
> + "selecting header whose best child has num channels change\n");
> + if (header->fi.is_var_size) {
> + if (child->fi.frame_or_samp_num - header->fi.frame_or_samp_num != child->fi.blocksize)
> + av_log(avctx, AV_LOG_DEBUG,
> + "selecting header whose best child's sample number does not match\n");
> + } else {
> + if (child->fi.frame_or_samp_num != header->fi.frame_or_samp_num + 1)
> + av_log(avctx, AV_LOG_DEBUG,
> + "selecting header whose frame number not consecutive with best child\n");
> + }
I would change these to AV_LOG_WARNING.
> + }
> + avctx->sample_rate = header->fi.samplerate;
> + avctx->channels = header->fi.channels;
> + avctx->frame_size = header->fi.blocksize;
> + avctx->bit_rate = header->fi.bps * header->fi.samplerate;
avctx->bit_rate should not be set here. FLAC always has a variable bit
rate. It is currently set in the demuxer as the average bit rate if the
file size and number of total samples is known.
> + *poutbuf = fpc->buffer + header->offset;
> +
> + if (fpc->best_header_valid) {
> + fpc->best_header_valid = 0;
> + return 0;
> + }
> + return buf_size;
> +}
> +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;
> + int nb_headers;
> + void* new_buffer;
> +
> + if (s->flags & PARSER_FLAG_COMPLETE_FRAMES) {
> + *poutbuf = buf;
> + *poutbuf_size = buf_size;
> + return buf_size;
It might be useful to parse the header here to set avctx->frame_size.
> + }
> +
> + if (fpc->best_header_valid)
> + return get_best_header(fpc, avctx, poutbuf, poutbuf_size, buf_size);
> +
> + /* if we found a best_header last call it needs to be removed along with the buffer data */
> + if (fpc->best_header && fpc->best_header->best_child) {
> + FLACHeaderMarker *curr = fpc->headers, *temp, *best_child = fpc->best_header->best_child;
This would be easier to read if they were on separate lines.
> + /* remove all references in our list and all headers that came up to the end of the best_header */
> + while (curr != best_child) {
> + if (curr != fpc->best_header)
> + av_log(NULL,AV_LOG_DEBUG,"dropping low score %i frame header from offset %i to %i\n",
> + curr->max_score, curr->offset, curr->next->offset);
This should be a warning.
> +
> + temp = curr->next;
> + av_free(curr);
> + curr = temp;
> + }
> + /* TODO: make into ring buffer or something cleaner. for now slide the data stream down. */
> + fpc->buffer_size = fpc->buffer_size - best_child->offset;
> + memmove(fpc->buffer, fpc->buffer + best_child->offset, fpc->buffer_size);
> +
> + /* now 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) {
> + /* if there is no end frame we don't need to delete the buffer - this probably eof */
> + FLACHeaderMarker *curr = fpc->headers, *temp;
> + while (curr != fpc->best_header->next) {
> + temp = curr->next;
> + av_free(curr);
> + curr = temp;
> + }
> + fpc->headers = curr;
> + fpc->best_header = NULL;
> + }
> +
> + /* if EOF just return buffered frames by priority */
> + if (!buf_size)
> + goto select_best;
> +
> + /* fill the buffer */
> + new_buffer = av_fast_realloc(fpc->buffer, &fpc->buffer_allocated_size, buf_size + fpc->buffer_size);
> + if (!new_buffer)
> + goto handle_error;
> + fpc->buffer = new_buffer;
> + memcpy(fpc->buffer + fpc->buffer_size, buf, buf_size);
> + fpc->buffer_size += buf_size;
> +
> + /* tag the headers and update sequences. */
> + nb_headers = find_new_headers(fpc, FFMAX(0, fpc->buffer_size - (buf_size + (MAX_FRAME_HEADER_SIZE - 1))));
> +
> + /* wait until we have enough headers to be confident enough to output a valid frame */
> + if (nb_headers < FLAC_MIN_HEADERS)
> + goto handle_error;
> +
> + /* if we found headers update the scores since we have longer sequences. */
> + if (fpc->nb_headers_found)
> + score_sequences(fpc);
> +
> +select_best:
how about:
if (buf_size) {
/* fill the buffer */
etc...
}
and remove select_best
> + if (!fpc->best_header) {
> + FLACHeaderMarker *curr;
> + int best_score = FLAC_HEADER_NOT_SCORED_YET;
> + curr = fpc->headers;
> + while (curr) {
> + if (curr->max_score > best_score) {
> + fpc->best_header = curr;
> + best_score = curr->max_score;
> + }
> + curr = curr->next;
> + }
> + }
> + if (fpc->best_header) {
> + if (fpc->best_header->offset > 0) {
> + /* output a junk frame */
> + av_log(NULL, AV_LOG_DEBUG, "flac parser:junk frame till offset %i\n",
> + fpc->best_header->offset);
> +
> + /* TODO: make sure this makes compute_pkt_fieds/add_index_entry do the correct thing */
> + avctx->frame_size = 0;
> + *poutbuf = fpc->buffer;
> + *poutbuf_size = fpc->best_header->offset;
> + fpc->best_header_valid = 1;
> + return buf_size;
> + }
> + return get_best_header(fpc, avctx, poutbuf, poutbuf_size, buf_size);
> + }
> +
in what cases will this point be reached? is the 2nd check for
fpc->best_header needed?
> +handle_error:
> + *poutbuf = NULL;
> + *poutbuf_size = 0;
> + return buf_size;
> +}
> +
> +static void flac_parse_close(AVCodecParserContext *c)
> +{
> + FLACParseContext *fpc = c->priv_data;
> + FLACHeaderMarker *curr = fpc->headers, *temp;
> +
> + while (curr) {
> + temp = curr->next;
> + av_free(curr);
> + curr = temp;
> + }
> + av_freep(&fpc->buffer);
> +}
> +
> +AVCodecParser flac_parser = {
> + { CODEC_ID_FLAC },
> + sizeof(FLACParseContext),
> + NULL,
> + flac_parse,
> + flac_parse_close,
> +};
ok. but you should probably go through flac_parser.c again to check for
long lines.
> diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c
> index 07acf6e..86837b3 100644
> --- libavcodec/flacdec.c
> +++ libavcodec/flacdec.c
> @@ -47,36 +47,6 @@
> #undef NDEBUG
> #include <assert.h>
>
> -typedef struct FLACContext {
> - FLACSTREAMINFO
> -
> - AVCodecContext *avctx; ///< parent AVCodecContext
> - GetBitContext gb; ///< GetBitContext initialized to start at the current frame
> -
> - int blocksize; ///< number of samples in the current frame
> - int curr_bps; ///< bps for current subframe, adjusted for channel correlation and wasted bits
> - int sample_shift; ///< shift required to make output samples 16-bit or 32-bit
> - int is32; ///< flag to indicate if output should be 32-bit instead of 16-bit
> - int ch_mode; ///< channel decorrelation type in the current frame
> - int got_streaminfo; ///< indicates if the STREAMINFO has been read
> -
> - int32_t *decoded[FLAC_MAX_CHANNELS]; ///< decoded samples
> - uint8_t *bitstream;
> - unsigned int bitstream_size;
> - unsigned int bitstream_index;
> - unsigned int allocated_bitstream_size;
> -} FLACContext;
again, keep this here.
> -
> -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 void allocate_buffers(FLACContext *s);
>
> int ff_flac_is_extradata_valid(AVCodecContext *avctx,
ok
> @@ -150,20 +120,10 @@ static void allocate_buffers(FLACContext *s)
>
> assert(s->max_blocksize);
>
> - if (s->max_framesize == 0 && s->max_blocksize) {
> - s->max_framesize = ff_flac_get_max_frame_size(s->max_blocksize,
> - s->channels, s->bps);
> - }
> -
Since this is being removed, ff_flac_get_max_frame_size() can be moved
to from flac.c to flacenc.c. But that can be done later.
> for (i = 0; i < s->channels; i++) {
> s->decoded[i] = av_realloc(s->decoded[i],
> sizeof(int32_t)*s->max_blocksize);
> }
> -
> - if (s->allocated_bitstream_size < s->max_framesize)
> - s->bitstream= av_fast_realloc(s->bitstream,
> - &s->allocated_bitstream_size,
> - s->max_framesize);
> }
>
> void ff_flac_parse_streaminfo(AVCodecContext *avctx, struct FLACStreaminfo *s,
ok.
> @@ -480,104 +440,41 @@ static inline int decode_subframe(FLACContext *s, int channel)
> return 0;
> }
>
> -/**
> - * 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
> - */
> -static int decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
> - FLACFrameInfo *fi)
> -{
> - int bs_code, sr_code, bps_code;
> -
> - /* frame sync code */
> - skip_bits(gb, 16);
> -
> - /* block size and sample rate codes */
> - bs_code = get_bits(gb, 4);
> - sr_code = get_bits(gb, 4);
> -
> - /* channels and decorrelation */
> - 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;
> - } else if (fi->ch_mode <= FLAC_CHMODE_MID_SIDE) {
> - fi->channels = 2;
> - } else {
> - av_log(avctx, AV_LOG_ERROR, "invalid channel mode: %d\n", fi->ch_mode);
> - return -1;
> - }
> -
> - /* 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 = sample_size_table[bps_code];
> -
> - /* reserved bit */
> - if (get_bits1(gb)) {
> - av_log(avctx, AV_LOG_ERROR, "broken stream, invalid padding\n");
> - return -1;
> - }
> -
> - /* sample or frame count */
> - if (get_utf8(gb) < 0) {
> - av_log(avctx, AV_LOG_ERROR, "utf8 fscked\n");
> - return -1;
> - }
> -
> - /* blocksize */
> - if (bs_code == 0) {
> - av_log(avctx, AV_LOG_ERROR, "reserved blocksize code: 0\n");
> - return -1;
> - } else if (bs_code == 6) {
> - fi->blocksize = get_bits(gb, 8) + 1;
> - } else if (bs_code == 7) {
> - fi->blocksize = get_bits(gb, 16) + 1;
> - } else {
> - fi->blocksize = ff_flac_blocksize_table[bs_code];
> - }
> -
> - /* sample rate */
> - if (sr_code < 12) {
> - fi->samplerate = ff_flac_sample_rate_table[sr_code];
> - } else if (sr_code == 12) {
> - fi->samplerate = get_bits(gb, 8) * 1000;
> - } else if (sr_code == 13) {
> - fi->samplerate = get_bits(gb, 16);
> - } 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;
> - }
> -
> - /* 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 0;
> -}
> -
ok
> static int decode_frame(FLACContext *s)
> {
> - int i;
> + int i, ret;
> GetBitContext *gb = &s->gb;
> FLACFrameInfo fi;
>
> - if (decode_frame_header(s->avctx, gb, &fi)) {
> - av_log(s->avctx, AV_LOG_ERROR, "invalid frame header\n");
> + ret = ff_flac_decode_frame_header(s->avctx, gb, &fi);
> + if (ret) {
> + av_log(s->avctx, AV_LOG_ERROR, "invalid frame header: ");
> + switch (ret) {
> + case FLAC_PARSE_ERROR_SYNC:
> + av_log(s->avctx, AV_LOG_ERROR, "frame sync error\n");
> + break;
> + case FLAC_PARSE_ERROR_CHANNEL_CFG:
> + av_log(s->avctx, AV_LOG_ERROR, "invalid channel mode: %d\n", fi.ch_mode);
> + break;
> + case FLAC_PARSE_ERROR_BPS:
> + av_log(s->avctx, AV_LOG_ERROR, "invalid sample size code (%d)\n", fi.bps);
> + break;
> + case FLAC_PARSE_ERROR_PADDING:
> + av_log(s->avctx, AV_LOG_ERROR, "broken stream, invalid padding\n");
> + break;
> + case FLAC_PARSE_ERROR_SAMPLE_NUM:
> + av_log(s->avctx, AV_LOG_ERROR, "sample/frame number invalid; utf8 fscked\n");
> + break;
> + case FLAC_PARSE_ERROR_BLOCK_SIZE:
> + av_log(s->avctx, AV_LOG_ERROR, "reserved blocksize code: 0\n");
> + break;
> + case FLAC_PARSE_ERROR_SAMPLE_RATE:
> + av_log(s->avctx, AV_LOG_ERROR, "illegal sample rate code %d\n", fi.samplerate);
> + break;
> + case FLAC_PARSE_ERROR_CRC:
> + av_log(s->avctx, AV_LOG_ERROR, "header crc mismatch\n");
> + break;
> + }
> return -1;
> }
>
> @@ -641,7 +538,7 @@ static int flac_decode_frame(AVCodecContext *avctx,
> const uint8_t *buf = avpkt->data;
> int buf_size = avpkt->size;
> FLACContext *s = avctx->priv_data;
> - int i, j = 0, input_buf_size = 0, bytes_read = 0;
> + int i, j = 0, bytes_read = 0;
> int16_t *samples_16 = data;
> int32_t *samples_32 = data;
> int alloc_data_size= *data_size;
> @@ -649,43 +546,12 @@ static int flac_decode_frame(AVCodecContext *avctx,
>
> *data_size=0;
>
> - if (s->max_framesize == 0) {
> - s->max_framesize= FFMAX(4, buf_size); // should hopefully be enough for the first header
> - s->bitstream= av_fast_realloc(s->bitstream, &s->allocated_bitstream_size, s->max_framesize);
> - }
> -
> - if (1 && s->max_framesize) { //FIXME truncated
> - if (s->bitstream_size < 4 || AV_RL32(s->bitstream) != MKTAG('f','L','a','C'))
> - buf_size= FFMIN(buf_size, s->max_framesize - FFMIN(s->bitstream_size, s->max_framesize));
> - input_buf_size= buf_size;
> -
> - if (s->bitstream_size + buf_size < buf_size || s->bitstream_index + s->bitstream_size + buf_size < s->bitstream_index)
> - return -1;
> -
> - if (s->allocated_bitstream_size < s->bitstream_size + buf_size)
> - s->bitstream= av_fast_realloc(s->bitstream, &s->allocated_bitstream_size, s->bitstream_size + buf_size);
> -
> - if (s->bitstream_index + s->bitstream_size + buf_size > s->allocated_bitstream_size) {
> - memmove(s->bitstream, &s->bitstream[s->bitstream_index],
> - s->bitstream_size);
> - s->bitstream_index=0;
> - }
> - memcpy(&s->bitstream[s->bitstream_index + s->bitstream_size],
> - buf, buf_size);
> - buf= &s->bitstream[s->bitstream_index];
> - buf_size += s->bitstream_size;
> - s->bitstream_size= buf_size;
> -
> - if (buf_size < s->max_framesize && input_buf_size) {
> - return input_buf_size;
> - }
> - }
> -
> /* check that there is at least the smallest decodable amount of data.
> this amount corresponds to the smallest valid FLAC frame possible.
> FF F8 69 02 00 00 9A 00 00 34 46 */
> - if (buf_size < 11)
> - goto end;
> + if (buf_size < FLAC_MIN_FRAME_SIZE) {
> + return buf_size;
> + }
unneeded braces
>
> /* check for inline header */
> if (AV_RB32(buf) == MKBETAG('f','L','a','C')) {
> @@ -693,26 +559,13 @@ static int flac_decode_frame(AVCodecContext *avctx,
> av_log(s->avctx, AV_LOG_ERROR, "invalid header\n");
> return -1;
> }
> - bytes_read = get_metadata_size(buf, buf_size);
> - goto end;
> - }
> -
> - /* check for frame sync code and resync stream if necessary */
> - if ((AV_RB16(buf) & 0xFFFE) != 0xFFF8) {
> - const uint8_t *buf_end = buf + buf_size;
> - av_log(s->avctx, AV_LOG_ERROR, "FRAME HEADER not here\n");
> - while (buf+2 < buf_end && (AV_RB16(buf) & 0xFFFE) != 0xFFF8)
> - buf++;
> - bytes_read = buf_size - (buf_end - buf);
> - goto end; // we may not have enough bits left to decode a frame, so try next time
> + return get_metadata_size(buf, buf_size);
> }
>
> /* decode frame */
> init_get_bits(&s->gb, buf, buf_size*8);
> if (decode_frame(s) < 0) {
> av_log(s->avctx, AV_LOG_ERROR, "decode_frame() failed\n");
> - s->bitstream_size=0;
> - s->bitstream_index=0;
> return -1;
> }
> bytes_read = (get_bits_count(&s->gb)+7)/8;
> @@ -722,7 +575,7 @@ static int flac_decode_frame(AVCodecContext *avctx,
> if (output_size > alloc_data_size) {
> av_log(s->avctx, AV_LOG_ERROR, "output data size is larger than "
> "allocated data size\n");
> - goto end;
> + return -1;
> }
> *data_size = output_size;
>
ok.
> @@ -760,20 +613,15 @@ static int flac_decode_frame(AVCodecContext *avctx,
> DECORRELATE( (a-=b>>1) + b, a)
> }
>
> -end:
> if (bytes_read > buf_size) {
> av_log(s->avctx, AV_LOG_ERROR, "overread: %d\n", bytes_read - buf_size);
> - s->bitstream_size=0;
> - s->bitstream_index=0;
> return -1;
> }
> + if(bytes_read < buf_size) {
> + av_log(s->avctx, AV_LOG_DEBUG, "underread: %d orig size: %d\n", buf_size - bytes_read, buf_size);
> + }
unneeded braces. also put a space after the if.
>
> - if (s->bitstream_size) {
> - s->bitstream_index += bytes_read;
> - s->bitstream_size -= bytes_read;
> - return input_buf_size;
> - } else
> - return bytes_read;
> + return bytes_read;
> }
>
> static av_cold int flac_decode_close(AVCodecContext *avctx)
ok
> @@ -784,19 +632,10 @@ static av_cold int flac_decode_close(AVCodecContext *avctx)
> for (i = 0; i < s->channels; i++) {
> av_freep(&s->decoded[i]);
> }
> - av_freep(&s->bitstream);
>
> return 0;
> }
>
> -static void flac_flush(AVCodecContext *avctx)
> -{
> - FLACContext *s = avctx->priv_data;
> -
> - s->bitstream_size=
> - s->bitstream_index= 0;
> -}
> -
> AVCodec flac_decoder = {
> "flac",
> AVMEDIA_TYPE_AUDIO,
> @@ -806,9 +645,5 @@ AVCodec flac_decoder = {
> NULL,
> flac_decode_close,
> flac_decode_frame,
> - CODEC_CAP_DELAY | CODEC_CAP_SUBFRAMES, /* FIXME: add a FLAC parser so that
> - we will not need to use either
> - of these capabilities */
> - .flush= flac_flush,
> .long_name= NULL_IF_CONFIG_SMALL("FLAC (Free Lossless Audio Codec)"),
> };
ok.
Great job so far. I'm looking forward to seeing it fixed and committed
soon. :)
-Justin
More information about the ffmpeg-devel
mailing list