[FFmpeg-devel] [PATCH] Common ACELP code & G.729 [4/7] - G.729 core
Vladimir Voroshilov
voroshil
Fri Oct 10 19:36:32 CEST 2008
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
More information about the ffmpeg-devel
mailing list