[FFmpeg-devel] [PATCH] Common fixed-point ACELP routines (1/3) - math

[Michael Niedermayer]

On Tue, Apr 22, 2008 at 02:11:48AM +0700, Vladimir Voroshilov wrote:
> On Tue, Apr 22, 2008 at 2:06 AM, Diego Biurrun <diego at biurrun.de> wrote:
> > On Tue, Apr 22, 2008 at 01:21:04AM +0700, Vladimir Voroshilov wrote:
> >  >
> >  > --- /dev/null
> >  > +++ b/libavcodec/acelp_math.c
> >  > @@ -0,0 +1,149 @@
> >  > +#include <inttypes.h>
> >  > +#include <limits.h>
> >
> >  missing license header
> >
> 
> Oops. Fixed.
> 
[...]
> +/**
> + * \brief fixed-point implementation of cosine
> + * \param arg Q13 cosine argument
> + *
> + * \return Q(15) values of cos(arg)
> + */

For a generic cos() this is too vague. First the Q* notation should be
replaced by something clearer, the max/min should be mentioned and the
scaling of the argument should be mentioned. That is which argument is
equivalent to PI.


> +int16_t ff_acelp_cos(int16_t arg)
> +{

> +    int16_t offset= arg & 0xff;
> +    int16_t ind = arg >> 8;

These are uint8_t

[...]
> +/**
> + * \brief Calculates 2^(14+x)
> + * \param arg (Q15) fractional part of power (>=0)
> + *
> + * \return (Q0) result of pow(2, (14<<15)+power)
> + */

Are you sure this is what the function does?


> +int ff_acelp_pow2_14_x(int16_t power)
> +{
> +    uint16_t frac_x0;
> +    uint16_t frac_dx;
> +    int result;
> +
> +    assert(power>=0);
> +
> +    /*
> +      power in Q15, thus
> +      b31-b15 - integer part
> +      b00-b14 - fractional part
> +
> +      When fractional part is treated as Q10,
> +      bits 10-14 are integer part, 00-09 - fractional
> +
> +    */

> +    frac_x0 = (power & 0x7c00) >> 10; // b10-b14 and Q10 -> Q0

is the & needed at all?


> +    frac_dx = (power & 0x03ff) << 5;  // b00-b09 and Q10 -> Q15
> +
> +    result = ff_g729_tab_pow2[frac_x0] << 15; // Q14 -> Q29;
> +    result += frac_dx * (ff_g729_tab_pow2[frac_x0+1] - ff_g729_tab_pow2[frac_x0]); // Q15*Q14;
> +

> +    // multiply by 2^14 and Q29 -> Q0 with rouding

Could you remove all these confusing comments please


> +    return (result + 0x4000) >> 15;
> +}
> +
> +/**
> + * \brief Calculates log2(x)
> + * \param value function argument (>0)
> + *
> + * \return (Q15) result of log2(value)
> + */

> +int ff_acelp_log2(int value)

These functions have nothing to do with acelp. log,cos,
exp2 (yes please name it exp2 not pow2) have been known before acelp.


[...]
> +static inline int mul_24_15(int var_q24, int16_t var_q15)
> +{
> +    int64_t tmp = (((int64_t)var_q24 * (int64_t)var_q15) >> 15);
> +
> +    if(tmp < INT_MIN)
> +        return INT_MIN;
> +    else if (tmp > INT_MAX)
> +        return INT_MAX;
> +    else
> +       return tmp;

indention


[...]
> +static inline int16_t div_int16(int16_t num, int16_t denom)
> +{
> +    assert(FFABS(denom)>=FFABS(num));
> +
> +    return (num << 15)/denom;
> +}

I do not think combining << and / needs its own function.

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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/20080422/54a87ca2/attachment.pgp>