[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