[FFmpeg-devel] [PATCH] Common ACELP code & G.729 [2/7] - pitch lag decoding
Vladimir Voroshilov
voroshil
Fri Jun 13 13:41:40 CEST 2008
2008/6/12 Michael Niedermayer <michaelni at gmx.at>:
> On Thu, Jun 12, 2008 at 12:20:14AM +0700, Vladimir Voroshilov wrote:
[...]
>> +void ff_acelp_update_past_gain_erasure(int16_t *quant_energy, int log2_ma_pred_order)
>> +{
>> + int avg_gain=quant_energy[(1 << log2_ma_pred_order) - 1]; // (5.10)
>> + int i;
>> +
>> + /* 4.4.3 of G.729, Equation 95 */
>
> This comment could be outside the function body, which would make reading
> the code easier.
Moved to doxygen comment.
>> + for(i=(1 << log2_ma_pred_order) - 1; i>0; i--)
>> + {
>> + avg_gain += quant_energy[i-1];
>> + quant_energy[i] = quant_energy[i-1];
>> + }
>> + quant_energy[0] = FFMAX(avg_gain >> log2_ma_pred_order, -10240) - 4096; // -10 and -4 in (5.10)
>> +}
>> +
>
>> +void ff_acelp_update_past_gain(int16_t* quant_energy, int gain_corr_factor, int ma_pred_order)
>> +{
>> + int i;
>> +
>> + for(i=ma_pred_order-1; i>0; i--)
>> + quant_energy[i] = quant_energy[i-1];
>> +
>> + /* 3.9.1 of G.729, Equation 72
>> + quant_energy[0] = 20*log10(gain_corr_factor) in (3.12)
>> + 24660 = 10 / log2(10) in (2.13) */
>
> same
> listing the equations a function implements before the function would
> make the code alot more readable.
> The function has just 3 lines of code, it doesnt need a 3 line comment in
> the middle. Deciphering what the comment is supposed to mean takes at least
> as long as reading the code itself
Note about equation moved to doxygen comment, the rest is removed.
>> + quant_energy[0] = (24660 * ((ff_log2(gain_corr_factor) >> 2) - (13 << 13))) >> 15;
>> +}
>> +
>> +int16_t ff_acelp_decode_gain_code(
>> + int gain_corr_factor,
>> + const int16_t* fc_v,
>> + int mr_energy,
>> + const int16_t* quant_energy,
>> + const int16_t* ma_prediction_coeff,
>> + int subframe_size,
>> + int ma_pred_order)
>> +{
>> + int i;
>> + int energy;
>> + int innov_energy;
>> +#ifdef G729_BITEXACT
>> + int exp;
>> +#else
>
>> + static const float c_exp = M_LN10 / (20 << 23);
>
> you dont need that temporary variable
Fixed.
>> +#endif
>> +
>> + energy = mr_energy << 10;
>> +
>> + /* 3.9.1 of G.729, Equation 69 */
>> + for(i=0; i<ma_pred_order; i++)
>> + energy += quant_energy[i] * ma_prediction_coeff[i];
>> +
>> + /* 3.9.1 of G.729, Equations 66 and 71 */
>> + innov_energy = sum_of_squares(fc_v, subframe_size, 0, 0);
>
> Please move the comments outside the function body or to the right
> of the code. They do not help reability, if one is cross checking the
> code against the spec, having the reference before the function is as
> good as before each line of c code. If one does not crosscheck the
> code against the spec the comments are completely useless.
Moved to header.
>> +/// minimum pitch lag
>> +#define PITCH_LAG_MIN 20
>> +
>> +/// maximum pitch lag
>> +#define PITCH_LAG_MAX 143
>
> The comments are useless they just repeat the names of the constants.
Removed.
> Please recheck your comments
> * code should be self explanatory, use good and clear variable names
> not use comments to explain what bad variable names mean.
> * comments should not be after each 1-2 lines of C code. The kernel
> style guide has some more verbose explanations about why
> * comments should explain what is not obvious from the code never repeat
> what is completely obvious already.
Not sure that i've fixed all, but will keep in mind.
>> +
>> +/**
>> + * \brief Decode pitch lag of the first subframe encoded by 8 bits with 1/3
>> + * resolution.
>
> Ive read this a dozen times but somehow it sounds very odd.
Agree, but don't have anything better yet.
The best for me is "Decode pitch lag of the first subframe", but
such comment will be repeated over about three routines.
>> + * \param ac_index adaptive codebook index (8 bits)
>> + *
>
>> + * \return 3 * pitch_lag
>
> @returns pitch lag in 1/3 units
>
>
>> + *
>> + * Pitch delay is coded:
>> + *
>> + * 1 2
>> + * [19 ---; 84 ---] - with 1/3 resolution
>> + * 3 3
>> + *
>> + * [85; 143] - integers only
>> + *
>
>> + * Sample code from spec (first subframe):
>> + *
>> + * if(P1<197)
>> + * int(T1) = (P1 + 2)/3 + 19
>> + * frac = P1 - 3*int(T1) + 58
>> + * else
>> + * int(T1) = P1 - 112
>> + * frac = 0
>> + * end
>
> This repeats what the c code already says clearer
> What is this good for?
Removed.
>> + */
>> +int ff_acelp_decode_lag3_1st_8_bits(int ac_index);
>> +
>> +/**
>> + * \brief Decode pitch lag of the second subframe encoded by 5 or 6 bits
>> + * with 1/3 precision.
>> + * \param ac_index adaptive codebook index (5 or 6 bits)
>
>> + * \param pitch_lag_min lower bound (integer) of pitch delay interval
>> + * for second subframe
>> + *
>> + * \return 3 * pitch_delay
>> + *
>
>> + * From G.729 specification:
>
> From THE ...
Fixed here and in several other places.
>> + *
>> + * Pitch delay is coded:
>
> is it pitch delay or pitch lag? you mix the terms randomly ...
Hm. i thought "lag" is synonym for "delay" (as my dictionary says).
Specification uses "delay" in most of text, while reference
code uses "lag" everywhere.
Tell me if i should replace all instances with "lag" or "delay" only
(if so, tell also what is better and why, please)
>> + * - second subframe
>> + *
>> + * 2 2
>> + * [int(T1) - 5 --- ; int(T1) + 4 ---] - with 1/3 resolution
>> + * 3 3
>
> What in this equation is the pitch delay? how is it coded, what is in
> 1/3 resolution? what does the ; mean in the middle?
> After writing this i realize this likely is a range of something,
> possibly the return value. But that is guessing, pick something
> * remove this
> * use english text or normal mathematical notation
>
> "pitch delay is between 123 and 987" or
> "123 < pitch_delay < 987"
These are allowed ranges (of resulted value) written via algebraic
fractions with
small ascii art.
Replaced with 123 2/3 <= pitch_delay <= 456 1/3
> Also if one considers how "many" lines the function has these comments
> seem very serious overkill.
> Id suggest that all comments for a function should have fewer lines
> than the function body.
>
> Also writing ranges cleanly can be done with assert() like
> assert(123<X && X<567);
> this also has the sideeffect of being usefull for debuging
> but for trivial 1 line functions this is overkill as well IMHO
>
>
>> + *
>> + * where T1 is the pitch lag from previous subframe.
>
> NO, no single letter variables
replaced with prev_pitch_delay (T1 was using only in comment to reduce
expression length, though)
> rest of the patch not reviewed, please cleanup the comments or remove them.
> I can live with undocumented code, but code documented by comments more
> obfuscated than the code itself is not ok.
Number of comment (at least in .c reduced).
Here is result.
--
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02_acelp_lag62.diff
Type: text/x-diff
Size: 12799 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080613/600bee88/attachment.diff>
More information about the ffmpeg-devel
mailing list