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

[Vladimir Voroshilov]

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.



-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0002-G729-decoder.101.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080927/f82b6a82/attachment.txt>