[FFmpeg-devel] [PATCH] FLAC parser
Diego Biurrun
diego
Sun Aug 1 22:48:00 CEST 2010
On Sun, Aug 01, 2010 at 06:36:06AM +0200, Michael Chinen wrote:
>
> 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 :)
Right :)
Still you should double-check your diffs to make sure you don't have
such goofups in there. It has saved me from embarassment countless
times...
> 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's some more guidance in that regard ...
> --- a/libavcodec/flac.c
> +++ b/libavcodec/flac.c
> @@ -19,7 +19,97 @@
>
> +static const int sample_size_table[] =
> +{ 0, 8, 12, 0, 16, 20, 24, 0 };
This line is below 80 characters, no need to break it - and if you break
it, it should be indented.
> +int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
> + FLACFrameInfo *fi)
You should align the arguments, not the arguments and the '('.
> + if (fi->ch_mode < FLAC_MAX_CHANNELS) {
> + fi->channels = fi->ch_mode + 1;
> + fi->ch_mode = FLAC_CHMODE_INDEPENDENT;
Align the '='.
> + /* 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;
> + }
> +
> --- a/libavcodec/flac.h
> +++ b/libavcodec/flac.h
> +int ff_flac_decode_frame_header(AVCodecContext *avctx, GetBitContext *gb,
> + FLACFrameInfo *fi);
ditto
> --- /dev/null
> +++ b/libavcodec/flac_parser.c
> @@ -0,0 +1,415 @@
> +static void update_sequences_header(FLACParseContext *fpc, FLACHeaderMarker *mid, FLACHeaderMarker *end, int distance)
long line, more below
> + /* 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 you are writing real English sentences I suggest you follow proper
capitalization rules, i.e. start sentences with an uppercase letter
and end them in a period. Also, these lines are unnecessarily long.
> + /* 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 and stray space before the ')'.
> + 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);
weird indentation
> + /* 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);
ditto
> +/**
> + * 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
CRC
> + * validates against this one,) then take the maximum score of the children,
misplaced ','
> + * with a penalty of FLAC_HEADER_CHANGED_PENALTY applied for each change to
> + * bps, sample rate, channels, but not decorellation mode, or blocksize,
> + * because it can change often.)
stray ')'
> + if(child->fi.frame_or_samp_num != header->fi.frame_or_samp_num + 1)
if (
You should not fix such things manually but automatically, otherwise you
will always overlook an instance as was now apparently the case.
> +static int get_best_header(FLACParseContext* fpc, AVCodecContext *avctx,
> + const uint8_t **poutbuf, int *poutbuf_size, int buf_size)
indentation
Please be consistent, this is the most important thing and would have
avoided all the things I'm nitpicking about now.
> + if (!child)
> + *poutbuf_size = fpc->buffer_size - header->offset;
> + else {
if () {
} else {
> + /* fill the buffer */
> + new_buffer = av_fast_realloc(fpc->buffer, &fpc->buffer_allocated_size, buf_size + fpc->buffer_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 */
more examples of needlessly long lines
> + /* output a junk frame */
> + av_log(NULL, AV_LOG_DEBUG, "flac parser:junk frame till offset %i\n",
> + fpc->best_header->offset);
...
> --- a/libavcodec/flacdec.c
> +++ b/libavcodec/flacdec.c
> @@ -760,20 +613,15 @@ static int flac_decode_frame(AVCodecContext *avctx,
> 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) {
if (
Respect the surrounding style.
> + av_log(s->avctx, AV_LOG_DEBUG, "underread: %d orig size: %d\n", buf_size - bytes_read, buf_size);
long line
Diego
More information about the ffmpeg-devel
mailing list