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

[Vladimir Voroshilov]

On Sun, Apr 27, 2008 at 8:52 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>
> On Sun, Apr 27, 2008 at 06:56:54PM +0700, Vladimir Voroshilov wrote:
>  > On Sun, Apr 27, 2008 at 6:12 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>  > >
>  > > On Sun, Apr 27, 2008 at 11:28:38AM +0700, Vladimir Voroshilov wrote:
>  > >  > 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,
>  > >
>  > >  No it is very relevant. Either:
>  > >  1. The cliping does never occur for valid input
>  >
>  > If "valid" = "produced from regular speech", parhaps. But when "valid"
>  > = "bitstream made according to spec" it does.
>  >
>  > >  2. The cliping does not sigificantly change the output (but in that case
>  > >    its not needed)
>  >
>  > It does due to int type overflow.
>
>  Where does an overflow occur?

I was wrong. There is no int overflow.
There is only [-0x40000000;0x3fffffff] range overflow.
In this case reference code reset accumulator variable to nearest bound.
This affects all remaining accumulations and thus result.
In other words reference code uses exactly clip(200+200)-200 version
instead of clip(200+200-200)

Since there are no int overflows output will be still valid (in
audible meaning).
What about removing those clipping but adding note about this situation?


>  > >  3. The float implementation is buggy and produces significantly different
>  > >    output for some valid files.
>  >
>  > It produces different (about 30 PSNR) result for every file (comparing
>  > with fixed-point).
>  >
>  > >  4. The integer implementation is buggy (this isnt possible per definition)
>  >
>  > dunno that
>
>  The spec says the integer implementation is to be the ultimate correct thing
>  in case there is a difference between spec and implementation IIRC.
>
>  Anyway, my point is one of the points must be true. Your apparent claim that
>  all are false is self contradictionary its like a==1 b==0 a==b it cant be both
>  are correct but different.

As i said above i was wrong.
Looks like (2) is aplicapable to our case.

Since i'm starting to loose the thread of discussion i
suggest stop it and implement it as you want.
But i still think that appropriate comment will be useful
for a case of future troubles (if so).

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