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

Michael Niedermayer michaelni
Sun Apr 27 13:12:04 CEST 2008


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
2. The cliping does not sigificantly change the output (but in that case
   its not needed)
3. The float implementation is buggy and produces significantly different
   output for some valid files.
4. The integer implementation is buggy (this isnt possible per definition)


> 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

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



> 
> 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.
----

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

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/20080427/2b80ed35/attachment.pgp>



More information about the ffmpeg-devel mailing list