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

[Vladimir Voroshilov]

On Sun, Apr 27, 2008 at 6:58 PM, Vladimir Voroshilov <voroshil at gmail.com> wrote:
>
> On Sun, Apr 27, 2008 at 6:56 PM, Vladimir Voroshilov <voroshil at gmail.com> 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.
>  >
>  >
>  >  >  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)"
>
>  damn. i meant:
>  "byte=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?
>

Here is latest version.

-- 
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
A non-text attachment was scrubbed...
Name: acelp_filt_38.diff
Type: text/x-diff
Size: 12296 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080427/a0978343/attachment.diff>