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

[Michael Niedermayer]

On Thu, Apr 24, 2008 at 09:32:53PM +0700, Vladimir Voroshilov wrote:
> On Thu, Apr 24, 2008 at 3:33 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Thu, Apr 24, 2008 at 02:41:54AM +0700, Vladimir Voroshilov wrote:
> >  [...]
> >  > > > +/**
> >  > > > + * \brief Calculates sum of array elements absolute values
> >  > > > + * \param speech array with input data
> >  > > > + * \param cycles number elements to proceed
> >  > > > + * \param shift right shift by this value will be done before addition
> >  > > > + *
> >  > > > + * \return sum of absolute values
> >  > > > + */
> >  > > > +static int sum_of_absolute(const int16_t* speech, int cycles, int shift)
> >  > > > +{
> >  > > > +    int n;
> >  > > > +    int sum = 0;
> >  > > > +
> >  > > > +    for(n=0; n<cycles; n++)
> >  > > > +       sum += FFABS(speech[n] >> shift);
> >  > > > +
> >  > > > +    return sum;
> >  > > > +}
> >  > >
> >  > > The place where the shift is done is not optimal for precission.
> >  >
> >  > I've just realized that i'm not using shift at all.
> >  > Should i keep routine as is, remove shift or remove all routine?
> >  > Currently moved outside loop.
> >
> >  if shift is always 0 remove shift.
> >  and maybe yes, remove the whole routine its so simple that iam not sure
> >  if this makes sense here, maybe if its speed critical it could be put in
> >  dsputil but i guess it could as well stay in g729.c for now
> 
> I've inlined that code.

[...]

> +int ff_log2(int value)

value should be unsigned.


> +{
> +    uint32_t result;
> +    uint8_t  power_int;
> +    uint8_t  frac_x0;
> +    uint16_t frac_dx;
> +
> +    assert(value > 0);
> +
> +    // Stripping zeros from beginning
> +    power_int = av_log2(value);
> +    result = value << (31 - power_int);

value could be used here, which would mean 1 variable less


[...]
> +/**
> + * \brief multiplies 32-bit integer by another 16-bit and divides result by 2^15
> + * \param var_q24 32-bit integer
> + * \param var_15 16-bit integer
> + *
> + * \return result of (var_q24 * var_q15 >> 15) with clipping to [INT_MIN; INT_MAX] range
> + */
> +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 < -0x7fffffff)
> +        return -0x7fffffff;
> +    else if (tmp > 0x7fffffff)
> +        return 0x7fffffff;
> +    else
> +        return tmp;
> +}
> +

does amr/qcelp also require such cliped multiply?
Also the word clip should occur in the name, maybe cliped_mul_24_15()



> +/**
> + * \brief Calculates sum of array elements multiplications
> + * \param speech array with input data
> + * \param cycles number elements to proceed
> + * \param offset offset for calculation sum of s[i]*s[i+offset]
> + * \param shift right shift by this value will be done before multiplication
> + *
> + * \return sum of multiplications
> + *
> + * \note array must be at least length+offset long!
> + */
> +static int sum_of_squares(const int16_t* speech, int cycles, int offset, int shift)
> +{
> +    int n;
> +    int sum = 0;
> +
> +    for(n=0; n<cycles; n++)
> +       sum += (speech[n] >> shift) * (speech[n + offset] >> shift);

 >> shift should be done after the *
and the n can be moved out of the inner loop:

for(speech_end= speech + cycles; speech < speech_end; speech++)
    sum += (speech[0] * speech[offset]) >> shift;


> +
> +    return av_clip(sum, -0x40000000, 0x3fffffff);
> +}

if such clip is needed it should be done outside the common code

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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20080424/5f723cb9/attachment.pgp>