[FFmpeg-devel] [PATCH] Add AMR-NB decoder, next try

[Diego Biurrun]

On Sun, Jan 03, 2010 at 06:01:49PM +0100, Vitor Sessak wrote:
> Hi, the SoC AMR decoder is in a pretty nice shape and have already gone  
> through a few review rounds, so I'll give my try in getting it committed.
>
> --- libavcodec/amrnbdata.h	(revision 0)
> +++ libavcodec/amrnbdata.h	(revision 0)
> @@ -0,0 +1,1801 @@
> +
> +#define AMR_BLOCK_SIZE              160   ///< Samples per frame
> +#define AMR_SUBFRAME_SIZE            40   ///< Samples per subframe
> +#define AMR_SAMPLE_BOUND        32768.0   ///< Threshold for synthesis overflow

Lowercase all these comments.

> +static const int16_t lsf_3_1_MODE_7k95[512][3] = {
> +{ -890,-1550,-2541},{ -819, -970,  175},{ -826,-1234, -762},

Spaces after the commas would IMO help readability here and in
other tables.

> +#define LSF_R_FAC          (8000.0/32768.0) ///< LSF residual tables to Hertz

I'd place spaces around the /.

> +static const uint16_t gains_MODE_4k75[512][2] = {
> +{  812,  128}, {  542,  140}, { 2873, 1135}, { 2266, 3402}, { 2067,  563},

Oh, here we have spaces in a similar table..

> +/** Number of impulse response coefficients used for tilt factor */
> +#define AMR_TILT_RESPONSE   22
> +/** Tilt factor = 1st reflection coefficient * gamma_t */
> +#define AMR_TILT_GAMMA_T   0.8
> +/** Adaptive gain control factor used in post-filter */
> +#define AMR_AGC_ALPHA      0.9

The bottom of a *huge* header file with tables does not seem like
a good place for these..

Maybe you can commit the tables right away, they are so big that
they make the patch unwieldy.

> --- libavcodec/Makefile	(revision 20995)
> +++ libavcodec/Makefile	(working copy)
> @@ -49,6 +49,7 @@
>  OBJS-$(CONFIG_ALS_DECODER)             += alsdec.o
> +OBJS-$(CONFIG_AMRNB_DECODER)           += amrnbdec.o celp_filters.o celp_math.o acelp_filters.o acelp_vectors.o lsp.o acelp_pitch_delay.o

Break this long line please.

Also, I wonder if some of these CELP dependencies could be refactored.

> --- libavcodec/amrnbdec.c	(revision 0)
> +++ libavcodec/amrnbdec.c	(revision 0)
> @@ -0,0 +1,1036 @@
> +
> +/** Double version of ff_weighted_vector_sumf() */
> +void weighted_vector_sumd(double *out, const double *in_a, const double *in_b,
> +                          double weight_coeff_a, double weight_coeff_b,
> +                          int length)

Shouldn't this be static?

> +    for(i=0; i<length; i++)

for (i = 0; i < length; i++)

Diego