[FFmpeg-devel] [PATCH] Common ACELP code & G.729 [4/7] - G.729 core

[Vladimir Voroshilov]

2008/9/17 Michael Niedermayer <michaelni at gmx.at>:
> On Sun, Sep 07, 2008 at 07:26:46PM +0700, Vladimir Voroshilov wrote:

>> +static const G729_format_description formats[] =
>> +{
>> +  {8000, 10, 160, 13, 0xf892c,},
>> +// Note: may not work
>> +  {4400, 11, 176, 17, 0xf966b,},
>> +};
>
> Is the last parameter really  best specified in hex?

No differences for me. fixed.

>> +/**
>> + * \brief Decode LSP coefficients from L0-L3 (3.2.4).
>> + * \param lsfq [out] (2.13) decoded LSP coefficients
>> + * \param lq_prev [in/out] (2.13) quantized LSF coefficients from previous frames
>> + * \param lsf_prev [in/out] (2.13) LSF coefficients from previous frame
>> + * \param ma_predictor switched MA predictor of LSP quantizer
>> + * \param vq_1st first stage vector of quantizer
>> + * \param vq_2nd_low second stage lower vector of LSP quantizer
>> + * \param vq_2nd_high second stage higher vector of LSP quantizer
>> + */
>> +static void g729_lsf_decode(
>> +        int16_t* lsfq,
>> +        int16_t lq_prev[MA_NP][10],
>> +        int16_t* lsf_prev,
>> +        int16_t ma_predictor,
>> +        int16_t vq_1st,
>> +        int16_t vq_2nd_low,
>> +        int16_t vq_2nd_high)
>> +{
>> +    int i,j,k;
>> +    static const uint8_t min_distance[2]={10, 5}; //(2.13)
>
>> +    int16_t lq[10];       //(2.13)
>
> what does "lq" stand for?

I've looked into spec more carefully.
In this routine variables are (as they called in spec):
lsfq[i] - quantized LSF coefficients for the current frame (??(m) in spec)
lq[i] - current quantizer output (l?(m) in spec)
lq_prev[i] - previous quantizer outputs (l?(m?k) in spec)

Hm.. looks like selected names for lq and lq_prev variables are wrong.
I sugges renaming (same for restore_from_previous routine):
 lsf_prev -> "lsfq_prev"
 lq -> "quantizer_output"
lq_prev -> "past_quantizer_outputs"

> [...]
>> +    g729_lq_rotate(lq_prev, lq);
>> +
>> +    ff_acelp_reorder_lsf(lsfq, LSFQ_DIFF_MIN, LSFQ_MIN, LSFQ_MAX, 10);
>> +}
>> +

[...]

>> +    for(i=0; i<10; i++)
>> +    {
>> +        tmp = lsfq[i] << 15;
>> +
>> +        for(k=0; k<MA_NP; k++)
>> +            tmp -= lq_prev[k][i] * cb_ma_predictor[ma_predictor_prev][k][i];
>> +
>> +        lq[i] = ((tmp >> 15) * cb_ma_predictor_sum_inv[ma_predictor_prev][i]) >> 12;
>
> the cb_ma_predictor_sum_inv table is unneeded division by cb_ma_predictor_sum
> can be used

cm_ma_predictor_sum_inv was added especially to avoid division.

> also the code looks a little odd, its subtracting here but adding above ...

You are talking about lsf_decode and lsf_restore_from previous routines, right?

In lsf_decode lsfq[i] is calculated from lq[i]:
lsfq[i] = lq[i] * ma_pred_sum[k] + sum[k=0..3] { lq_prev[i][k] *	ma_pred[i][k] }

lsf_restore_from_previous lq[i] is calculated from lsfq[i]:
lq[i] = [ lsfq[i] - sum[k=0..3]{ lq_prev[i][k] *	ma_pred[i][k] } ] /
ma_pred_sum[k]

For some reason this calculation is used instead of simple lq[i] = sum
[k=0..2]{lq_prev[i][k] } / 3

>> +    }
>> +
>> +    g729_lq_rotate(lq_prev, lq);
>> +}
>
> g729_lq_rotate() can be factorized out of these 2 functions that are only
> called by if/else

In this case lq[i] should be moved outside from g729_lsf_decode and
g729_lsf_restore_from_previous routines  (while it is used only there)
to upper level  g729_decode_frame routine

-- 
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719