[FFmpeg-devel] [PATCH] IFF demuxer and 8SVX decoder

[Vitor Sessak]

Hi

Jai Menon wrote:
> Changes made.

Some nitpicking...

> Index: 8svx.c
> ===================================================================
> --- 8svx.c	(revision 0)
> +++ 8svx.c	(revision 0)
> @@ -0,0 +1,122 @@
> +/*
> + * 8SVX Audio Decoder
> + * Copyright (C) 2008 Jaikrishnan Menon
> + *
> + * 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
> + */
> +
> +/**
> + * @file 8svx.c
> + * 8svx audio decoder
> + * @author Jaikrishnan Menon
> + * supports: fibonacci delta encoding
> + *         : exponential encoding
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>

Are those two includes really needed?

> +#include "avcodec.h"
> +
> +/**
> + * decoder context
> + */
> +typedef struct EightSvxContext {
> +        int16_t fib_acc;
> +        int first_frame;
> +        int8_t *table;
> +} EightSvxContext;
> +
> +
> +static int8_t fibonacci[16]   = { -34, -21, -13,  -8, -5, -3, -2, -1, 0, 1, 2, 3, 5,  8, 13, 21 };
> +static int8_t exponential[16] = {-128, -64, -32, -16, -8, -4, -2, -1, 0, 1, 2, 4, 8, 16, 32, 64 };

could be

static const int8_t (...etc..)

(...)

> +    *data_size = consumed << 2;
> +
> +    return consumed;
> +}
> +
> +
> +/**
> + *
> + * initialize 8svx decoder
> + *
> + */

Could be done in three lines.

(...)

> +/** 8svx specific chunk IDs */
> +#define ID_8SVX    MKTAG('8','S','V','X')
> +#define ID_VHDR    MKTAG('V','H','D','R')

Doxygen will interpret this comment as applying just to ID_8SVX. Maybe 
doing a non-doxy comment instead?

> +/** generic chunk IDs we may encounter */
> +#define ID_FORM       MKTAG('F','O','R','M')
> +#define ID_ANNO       MKTAG('A','N','N','O')

Same thing.

> +/** 8svx channel specifications */
> +#define LEFT    2
> +#define RIGHT   4

Ditto.

-Vitor