[FFmpeg-devel] [PATCH] AMR-WB Decoder

[Diego Biurrun]

On Fri, Sep 17, 2010 at 05:15:26PM -0300, Marcelo Galv?o P?voa wrote:
> 
> The code has already been reviewed mostly by Vitor Sessak, discussed
> and tested at the ffmpeg-soc list.

Then why does this patch not even follow everything listed in the
New codecs or formats checklist:

http://www.ffmpeg.org/developer.html#SEC7

Vitor?

Does the Doxygen build without errors or warnings?

.. some nits below ..

> --- /dev/null
> +++ b/libavcodec/amrwbdec.c
> @@ -0,0 +1,1237 @@
> +/**
> + * Decodes quantized ISF vectors using 36-bit indices (6K60 mode only)

We write "indexes" in most other places.

> + * @param[in] ind                  Array of 5 indices
> + * @param[out] isf_q               Buffer for isf_q[LP_ORDER]
> + *
> + */
> +static void decode_isf_indices_36b(uint16_t *ind, float *isf_q)
> +{
> +    int i;
> +
> +    for (i = 0; i < 9; i++) {
> +        isf_q[i] = dico1_isf[ind[0]][i] * (1.0f / (1 << 15));
> +    }
> +    for (i = 0; i < 7; i++) {
> +        isf_q[i + 9] = dico2_isf[ind[1]][i] * (1.0f / (1 << 15));
> +    }
> +    for (i = 0; i < 5; i++) {
> +        isf_q[i] += dico21_isf_36b[ind[2]][i] * (1.0f / (1 << 15));
> +    }
> +    for (i = 0; i < 4; i++) {
> +        isf_q[i + 5] += dico22_isf_36b[ind[3]][i] * (1.0f / (1 << 15));
> +    }
> +    for (i = 0; i < 7; i++) {
> +        isf_q[i + 9] += dico23_isf_36b[ind[4]][i] * (1.0f / (1 << 15));
> +    }

You can drop the {} like you do in most other places I think.

Also, I think the following is more readable.

   for (i = 0; i < 9; i++)
       isf_q[i]      = dico1_isf[ind[0]][i]      * (1.0f / (1 << 15));
   for (i = 0; i < 7; i++)
       isf_q[i + 9]  = dico2_isf[ind[1]][i]      * (1.0f / (1 << 15));
   for (i = 0; i < 5; i++)
       isf_q[i]     += dico21_isf_36b[ind[2]][i] * (1.0f / (1 << 15));
   for (i = 0; i < 4; i++)
       isf_q[i + 5] += dico22_isf_36b[ind[3]][i] * (1.0f / (1 << 15));
   for (i = 0; i < 7; i++)
       isf_q[i + 9] += dico23_isf_36b[ind[4]][i] * (1.0f / (1 << 15));

same below

> +static void decode_pitch_lag_low(int *lag_int, int *lag_frac, int pitch_index,
> +                        uint8_t *base_lag_int, int subframe, enum Mode mode)

Indentation is off.

> +static void decode_4p_track(int *out, int code, int m, int off)
> +{   ///code: 4m bits

This looks weird, the { is usually left alone.

> +    switch (BIT_STR(code, 4*m - 2, 2)) /* case ID (2 bits) */
> +    {
> +        case 0: /* 0 pulses in A, 4 pulses in B or vice versa */

switch (  ) {
Don't indent the case, same below.

> +/**
> + * Decode the algebraic codebook index to pulse positions and signs,
> + * then construct the algebraic codebook vector
> + *
> + * @param[out] fixed_vector        Buffer for the fixed codebook excitation
> + * @param[in] pulse_hi             MSBs part of the pulse index array (higher modes only)
> + * @param[in] pulse_lo             LSBs part of the pulse index array
> + * @param[in] mode                 Mode of the current frame
> + */

Here and in other places, the variable names look slightly nicer aligned.

> +    double p_ener = (double) ff_dot_productf(p_vector, p_vector,
> +                             AMRWB_SFR_SIZE) * p_gain * p_gain;
> +    double f_ener = (double) ff_dot_productf(f_vector, f_vector,
> +                             AMRWB_SFR_SIZE) * f_gain * f_gain;

Indentation is off.

> +AVCodec amrwb_decoder =

AVCodec amrwb_decoder = {

Diego