[FFmpeg-devel] [PATCH] Common ACELP routines (2/3) - filters

[Vladimir Voroshilov]

On Sun, Apr 27, 2008 at 4:02 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>
> On Sun, Apr 27, 2008 at 03:35:08AM +0700, Vladimir Voroshilov wrote:
>  > On Sun, Apr 27, 2008 at 3:10 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>  > >

[...]

>  > >  I wonder if it would be cleanerto do this outside of this function.
>  >
>  > But it describe calculating of two local variables.
>  > I'm afraid this comment will confuse peoples if will be placed outside.
>
>  I mean the spliting of the pitch_delay variable not the comment.
>  IIRC its split outside already as its needed splited for something else.

Fixed locally.

[...]

>  > > > +    for(n=0; n<subframe_size; n++)
>  > >  > +    {
>  > >  > +        /* 3.7.1 of G.729, Equation 40 */
>  > >
>  > > > +        v=0;
>  > >  > +        for(i=0; i<10; i++)
>  > >  > +        {
>  > >  > +            /*  R(x):=ac_v[-k+x] */
>  > >  > +            v += ac_v[n - pitch_delay_int - i    ] * ff_acelp_interp_filter[i][    pitch_delay_frac];
>  > >  > +            v = av_clip(v, -0x40000000, 0x3fffffff); //v += R(n-i)*ff_acelp_interp_filter(t+3i)
>  > >  > +            v += ac_v[n - pitch_delay_int + i + 1] * ff_acelp_interp_filter[i][6 - pitch_delay_frac];
>  > >  > +            v = av_clip(v, -0x40000000, 0x3fffffff); //v += R(n+i+1)*ff_acelp_interp_filter(3-t+3i)
>  > >
>  > >  does amr and the others also clip at such illogical place?
>  >
>  > In this loop int overflow occurs in synthetic.
>  > AMR fixed point reference code does checks here (via the L_mac routine).
>  > Moreover reference code checks for overflow in 95% of math operations through
>  > calls to L_mac, L_mult, L_add, etc everywhere.
>  >
>  > If you wish i can put this clipping under #ifdef G729_BITEXACT
>
>  I do not see this cliping in the float reference of g729. I also do not
>  see it in soc/amr. It cant be that the cliping is correct in one implementation
>  but not the other.

I'm afraid soc/amr was not checked for overflows.
Floating point code irrelevant here, imho,
because the reason of int overflow is using fixed-point.
And all reference fixed-point code has this check (both AMR and G.729).

As i already said this check affects only synthetic overflow test.
Reencoded file does not trigger overflow.
But for easy checking for introduced errors i want to keep this check
at least under #ifdef.

> > >  > +        // Clippin is required to pass G.729 OVERFLOW test
>  > >  > +        if(hpf_f[0] >= 0xfffffff)
>  > >  > +        {
>  > >  > +            speech[i] = SHRT_MAX;
>  > >  > +            hpf_f[0] = 0x3fffffff;
>  > >  > +        }
>  > >  > +        else if (hpf_f[0] <= -0x10000000)
>  > >  > +        {
>  > >  > +            speech[i] = SHRT_MIN;
>  > >  > +            hpf_f[0] = -0x40000000;
>  > >  > +        }
>  > >  > +        else
>  > >  > +        {
>  > >
>  > >  > +            hpf_f[0] <<= 2;
>  > >
>  > >  this is avoidable by changing a few other shifts
>  >
>  > But can cause int overflow in hpf_f[0], isn't it?
>
>  this is a << not a >>

Hm (deep night?) :-/
I thought that exactly "<<" can cause overflow, not ">>".
Thus kept as is.

P.S. patch will be attached in reply to another your mail.

-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719