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

[Vladimir Voroshilov]

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.

>  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

>  > because the reason of int overflow is using fixed-point.
>  > And all reference fixed-point code has this check (both AMR and G.729).
>
>  overflow != cliping
>  Explain how the code above can overflow with a single cliping at the very end!
>  Cliping after each addition gives a _WRONG_ value

of course

>  an example would be
>  clip_uint8(200+200)-200 != clip_uint8(200+200-200)

and absent clipping will produce wrong result too
an example would be
"byte=clip_uint8(byte)+200" != "byte=clip_uint8(byte+200)"

Which of them is correct?

>  > As i already said this check affects only synthetic overflow test.
>
>  So why do we care about this test at all?
>
>  The readme file clearly says:
>  ----
>  This directory contains testvectors to validate the correct execution
>  of the G.729 ANSI-C software (version 3.3). NOTE that these vectors
>  are not part of a validation procedure.

To make sure that we will not introduce overflow somewhere else in future.

I'm afraid we can flaming for a long time.
Thus i'll remove checks in next version and keep it in local tree only.

Are the rest (except spelling mentioned by Diego) ok?

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