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

[Michael Niedermayer]

On Sun, Apr 27, 2008 at 10:18:14PM +0700, Vladimir Voroshilov wrote:
> 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?

ok

[...]
-- 
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/a7bc84a0/attachment.pgp>