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

[Vitor Sessak]

Diego Biurrun wrote:
> 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..

Fixed in soc.

>> +/** 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..

They are there because the header is nicely separated in different 
sections. A possible solution would be moving the defines to the C file, 
but I'm undecided.

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

Michael, fine for you?

>> --- 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++)

Rest also done.

-Vitor