[FFmpeg-devel] [PATCH] G.729A (now fixed-point) decoder

[Reimar Döffinger]

Hello,
On Sun, Mar 16, 2008 at 12:24:58AM +0600, Vladimir Voroshilov wrote:
> +/**
> + * \brief shift to right with rounding
> + * \param var1 32-bit integer to shift
> + * \param var2 16-bit shift
> + */
> +static int l_shr_r(int var1, int16_t var2)
> +{
> +    if(var1 && (var1 & (1 << (var2 - 1))))
> +        return (var1 >> var2) + 1;
> +    else
> +        return (var1 >> var2);
> +}

Same as
return ((var1 >> (var2 - 1)) + 1) >> 1;
?

> +    if(num < 0)
> +    {
> +        num = -num;
> +        sig = !sig;
> +    }
> +
> +    if(denom < 0)
> +    {
> +        denom = -denom;
> +        sig = !sig;
> +    }

Hm,
sig = (num ^ denom) >> 31;
num = FFABS(num);
denom = FFABS(denom);
might be better.

> +    for(; num < 0x4000000; diff++)
> +        num <<= 1;
> +
> +    for(; denom < 0x4000000; diff--)
> +        denom <<= 1;
> +
> +    if(diff > base)
> +        num >>= diff-base;
> +    else
> +        denom >>= base-diff;

I'd expect this can be optimized using some av_log... function.

> +static int sum_of_squares(const int16_t* speech, int cycles, int offset, int shift)
> +{
> +    int n;
> +    int sum = 0;
> +
> +    if(offset < 0)
> +        return 0;
> +
> +    for(n=0; n<cycles; n++)
> +       sum += (speech[n] >> shift) * (speech[n + offset] >> shift);

Since "speech" is 16 bit, why not
(speech[n] * speech[n + offset]) >> shift,
or does the specification not want that?
Probably not good since it won't match the specification,
but making sum 64 bit and shifting only at the very end
probably is faster even on 32 bit systems.

> +        Azg[n] = (Az[n] * gamma_pow) >> 15;
> +        gamma_pow = (gamma_pow * gamma) >> 15;

Btw. I think you should probably try using MULL from mathops.h, it might
be slower though since it is for 32*32 bit multiplication.

> +    //Downscaling corellaions to fit on 16-bit
> +    tmp = FFMAX(corr_0, FFMAX(corr_t0, corr_max));
> +    for(n=0; n<32 && tmp > SHRT_MAX; n++)
> +    {
> +        tmp      >>= 1;
> +        corr_t0  >>= 1;
> +        corr_0   >>= 1;
> +        corr_max >>= 1;
> +    }

I think you can calculate the shift count via av_log2 and thus avoid the
loop.

> +static void g729a_tilt_compensation(G729A_Context *ctx, const int16_t *lp_gn, const int16_t *lp_gd, int16_t* res_pst)
> +{
> +    int tmp;
> +    int gt;      // Q12
> +    int rh1,rh0; // Q12
> +    int16_t hf_buf[11+22]; // Q12 A(Z/GAMMA_N)/A(z/GAMMA_D) filter impulse response
> +    int sum;
> +    int i, n;
> +    
> +    memset(hf_buf, 0, 33 * sizeof(int16_t));

I don't think it is necessary to zero all of these.

> +    for(n=0; n<subframe_size; n++)
> +    {
> +        sum = tmp_speech[n] << 12;
> +        for(i=0; i<10; i++)
> +            sum += lp[i] * tmp_speech[n-i-1];
> +        sum = FFMAX(FFMIN(sum, SHRT_MAX << 12), SHRT_MIN << 12);

This and similar places probably should use av_clip

> +
> +        f_0 = mul_24_15(ctx->hpf_f1, 15836)
> +            + mul_24_15(ctx->hpf_f2, -7667)
> +            +  7699 * ctx->hpf_z0
> +            - 15398 * ctx->hpf_z1
> +            +  7699 * z_2;
> +	f_0 <<= 2; // Q13 -> Q15

Very ugly indentation.

Well, that is all I could find at a very quick glance.

Greetings,
Reimar D?ffinger