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

Michael Niedermayer michaelni
Sun Apr 27 17:57:57 CEST 2008


On Sun, Apr 27, 2008 at 08:39:15PM +0700, Vladimir Voroshilov wrote:
> 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.
[...]
> +void ff_acelp_interpolate_excitation(
> +        int16_t* ac_v,
> +        int pitch_delay_int,
> +        int pitch_delay_frac,
> +        int subframe_size)
> +{
> +    int n, i;
> +    int v;
> +
> +    /* The lookup table contain values corresponding
> +       to -2/6 -1/6 0 1/6 2/6 3/6 fraction order.
> +       The filtering process uses a negative pitch lag offset, but
> +       a negative offset should not be used in then table. To avoid
> +       a negative offset in the table dimension corresponding to
> +       fractional delay the following conversion applies:
> +       

trailing whitespace


> +       -pitch_delay = -(6*pitch_delay_int+pitch_delay_frac) =
> +
> +       =  -6*pitch_delay_int - pitch_delay_frac =
> +
> +         / -6*(pitch_delay_int)   - pitch_delay_frac,     pitch_delay_frac <  0
> +       =< 
> +         \ -6*(pitch_delay_int+1) + (6-pitch_delay_frac), pitch_delay_frac >= 0
> +    */
> +
> +    /* Compute negative value of fractional delay */
> +    pitch_delay_frac = - pitch_delay_frac;
> +

> +    /* Now make sure that pitch_delay_frac will always be positive */
> +    if(pitch_delay_frac < 0)
> +    {
> +        pitch_delay_frac += 6;
> +        pitch_delay_int++;
> +    }

Can it really be >= 0 and <0 ?


[...]
> +void ff_acelp_weighted_filter(
> +        int16_t *out, 
> +        const int16_t* in,
> +        int16_t weight,
> +        int filter_length)
> +{
> +    int weight_pow = 1 << 15;
> +    int n;
> +
> +    for(n=0; n<filter_length; n++)
> +    {
> +        /* (0.15) * (3.12) and (3.27) -> (3.12) with rounding */
> +        out[n] = (in[n] * weight_pow + 0x4000) >> 15;
> +        /* (0.15) * (3.12) and (3.27) -> (3.12) with rounding */
> +        weight_pow = (weight_pow * weight + 0x4000) >> 15;
> +    }
> +}

as weight is a constant weight_pow is as well and half of the computations
are unneeded


> +
> +void ff_acelp_high_pass_filter(
> +        int16_t* out,
> +        int16_t* hpf_z,
> +        int* hpf_f,
> +        const int16_t* in,
> +        int length)
> +{
> +    int i;
> +
> +    for(i=0; i<length; i++)
> +    {
> +        memmove(hpf_z + 1, hpf_z, 2 * sizeof(hpf_z[0]));
> +        hpf_z[0] = in[i];
> +
> +        hpf_f[0] =  MULL(hpf_f[1], 15836);
> +        hpf_f[0] += MULL(hpf_f[2], -7667);
> +
> +        hpf_f[0] += 7699 * (hpf_z[0] - 2*hpf_z[1] + hpf_z[2]);
> +
> +        /* Clippin is required to pass G.729 OVERFLOW test */
> +        if(hpf_f[0] >= 0xfffffff)
> +        {
> +            out[i] = SHRT_MAX;
> +            hpf_f[0] = 0x3fffffff;
> +        }
> +        else if (hpf_f[0] <= -0x10000000)
> +        {
> +            out[i] = SHRT_MIN;
> +            hpf_f[0] = -0x40000000;
> +        }
> +        else
> +        {

> +            hpf_f[0] <<= 2;

The shift is avoidable i think as already said


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

I wish the Xiph folks would stop pretending they've got something they
do not.  Somehow I fear this will remain a wish. -- M?ns Rullg?rd
-------------- 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/465c08d4/attachment.pgp>



More information about the ffmpeg-devel mailing list