[FFmpeg-devel] [PATCH] Common ACELP code & G.729 [2/7] - pitch lag decoding
Michael Niedermayer
michaelni
Thu Jun 12 04:46:53 CEST 2008
On Thu, Jun 12, 2008 at 12:20:14AM +0700, Vladimir Voroshilov wrote:
> 2008/5/29 Michael Niedermayer <michaelni at gmx.at>:
> > On Wed, May 28, 2008 at 02:25:19PM +0700, Vladimir Voroshilov wrote:
>
> [...]
>
> > I meant just the last line ...
> >
> > energy= mr_energy;
> > for(i=0; i<ma_pred_order; i++)
> > energy += quant_energy[i] * ma_prediction_coeff[i];
> >
> > above can be in integers, while below
> >
> > return exp(energy * some_const) * gain_corr_factor
> > / sqrt(sum_of_squares(fc_v, subframe_size, 0, 0));
> >
> > would use floats, this would avoid all the mess with exp/log in fixed point
> >
> > anyway iam happy with integers as well but the code we seem to need for
> > the exp/log there is very ugly.
>
> Here is semi-float version.
> Hopefully better...
yes the code looks nicer now
>
> > Also the interleaved comments make the code unreadable. The comments should
> > be seperated either outside the function or to the right ignoring the 80 col
> > limit. Maybe that would already help enough, but as is its a 6 line function
> > bloated to 27 lines.
>
> I've merged several lines.
> I've kept two comments interleaved since they are
> belongs to multiple lines of code.
I think you will have to remove 50-70% of the comments to get this approved.
And rewrite the remaining. :)
[...]
> +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.
> + 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
> + 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
> +#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.
[...]
> +/// 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.
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.
> +
> +/**
> + * \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.
> + * \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?
> + */
> +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 ...
> + *
> + * Pitch delay is coded:
is it pitch delay or pitch lag? you mix the terms randomly ...
> + *
> + * - 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"
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
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.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080612/69d11de6/attachment.pgp>
More information about the ffmpeg-devel
mailing list