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

Vladimir Voroshilov voroshil
Tue Apr 22 04:12:16 CEST 2008


On Tue, Apr 22, 2008 at 6:05 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> 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.

Full math expression was put into comment.
I hope it clearly clarifies scaling.
Min/max values for argument and result also were added

>  > +int16_t ff_acelp_cos(int16_t arg)
>  > +{
>
>  > +    int16_t offset= arg & 0xff;
>  > +    int16_t ind = arg >> 8;
>
>  These are uint8_t

Fixed

>
>  [...]
>  > +/**
>  > + * \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?

Full math expression was put instead of this wrong comment
Also min/max was clarified.

>  > +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?

No, removed.

>
>
>  > +    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

Fixed

>  > + * \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.

ff_acelp_pow2_14_x renamed to ff_acelp_exp2.

Do you want to say that i have choosen wrong file to place them in (as
long as their prefix) ?
Please suggest proper place and prefix and i'll fix it immediately.

>  > +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

Fixed

>  > +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.

Removed. Corresponding code was inlined.

-- 
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: acelp_math_20.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080422/6b33f6e1/attachment.asc>



More information about the ffmpeg-devel mailing list