[FFmpeg-devel] [PATCH] Common ACELP code & G.729 [2/7] - pitch lag decoding

[Michael Niedermayer]

On Wed, May 28, 2008 at 02:25:19PM +0700, Vladimir Voroshilov wrote:
> 2008/5/24 Michael Niedermayer <michaelni at gmx.at>:
> > On Sat, May 24, 2008 at 12:11:16AM +0700, Vladimir Voroshilov wrote:
> 
> [...]
> 
> >> +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 exp;
> >> +
> >> +    /* 3.9.1 of G.729, Equation 66 */
> >> +    energy = sum_of_squares(fc_v, subframe_size, 0, 0);
> >> +
> >> +    /* 24660 = 10/log2(10) in (2.13) */
> >> +    energy = -ff_log2(energy);
> >> +    /* mr_energy = Em + 10log(N) + 10log(2^26) */
> >> +    energy += mr_energy;
> >> +
> >> +    energy <<= 10; // (7.13) -> (7.23)
> >> +    /* 3.9.1 of G.729, Equation 69 */
> >> +    for(i=0; i<ma_pred_order; i++)
> >> +        energy += quant_energy[i] * ma_prediction_coeff[i];
> >> +
> >> +    energy >>= 11;
> >> +
> >> +    /* The following code will calculate energy*2^14 instead of energy*2^exp
> >> +       due to the recent change of the integer part of energy_int.
> >> +       This is done to avoid overflow. Result fits into 16 bit. */
> >> +    exp = (energy >> 15);             // integer part (exponent)
> >> +    /* Only fraction part of (0.15) and rounding */
> >> +    energy = ((ff_exp2(energy & 0x7fff) + 16) >> 5);
> >> +
> >> +    /* apply correction */
> >> +    energy *= gain_corr_factor >> 1; // energy*2^14 in (3.12)
> >> +
> >> +    /* energy*2^14 in (3.12) -> energy*2^exp in (14.1) */
> >> +    return bidir_sal(energy, exp - 25);
> >
> > if id ignore all scaling and overflows then this is just:
> >
> > energy= mr_energy;
> > for(i=0; i<ma_pred_order; i++)
> >    energy += quant_energy[i] * ma_prediction_coeff[i];
> > return exp(energy) * gain_corr_factor / sum_of_squares(fc_v, subframe_size, 0, 0);
> 
> 
> 
> > This makes me quite unhappy as there is a huge difference in complexity.
> > If we cant find a clean way with integers then i would suggest that this last
> > line is done using floats.
> >
> 
> I got  this floating-point (partially) implementation:
> 
> f_energy = 0;
>  for(i=0; i<ma_pred_order; i++)
>     f_energy += quant_energy[i] * ma_prediction_coeff[i];
> f_energy = mean_energy + f_energy / (1<<23)
>  return 2 * exp(M_LN10 * f_energy / 20) * gain_corr_factor /
> sqrt(sum_of_squares(fc_v, subframe_size, 0, 0) / subframe_size);

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.

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.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 185 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080528/aace051c/attachment.pgp>