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

[Marcelo Galvão Póvoa]

On 19 September 2010 17:54, Diego Biurrun <diego at biurrun.de> wrote:
> 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 = {
>

Two days ago, I sent a reply to this review attaching a new patch.
Since the message was too large (slightly over 200KB), it was sent to
moderation. It seems it was not approved yet, so I would appreciate if
someone does.

-- 
Marcelo