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

[Vladimir Voroshilov]

2008/9/27 Vladimir Voroshilov <voroshil at gmail.com>:
> 2008/9/17 Vladimir Voroshilov <voroshil at gmail.com>:
>> 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
>
> Since i didn't receive any aswer, here is updated patch
> with fixed hex constants and renamed variables.
> g729_lq_rotate placement is not changed.
>

Just a reminder



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