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

[Michael Niedermayer]

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?


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

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

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato
-------------- 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/3e844c15/attachment.pgp>