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

Vladimir Voroshilov voroshil
Sat Apr 26 22:35:08 CEST 2008


On Sun, Apr 27, 2008 at 3:10 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>
> On Sun, Apr 27, 2008 at 01:05:07AM +0700, Vladimir Voroshilov wrote:
>  > Hello, Diego
>  > Thank you for your review.
>  >
>  > On Sat, Apr 26, 2008 at 11:32 PM, Diego Biurrun <diego at biurrun.de> wrote:
>  > > On Sat, Apr 26, 2008 at 11:11:56PM +0700, Vladimir Voroshilov wrote:
>  > >  >
>  > >  > P.S. Please anybody check English spelling too.
>  > >  >
>  > >  > --- /dev/null
>  > >  > +++ b/libavcodec/acelp_filters.c
>  > >  > @@ -0,0 +1,265 @@
>  > >  > +/**
>  > >  > + *
>  > >  > + *   G.729 specification says:
>  > >  > + *     b30 is based on Hamming windowed sinc functions, truncated at +/-29 and
>  > >
>  > >  What's sinc?
>  >
>  > Math function: y=sinc(x)=sin(x)/x
>  > Widely used, imho
>  >
>  > >  > +    // TODO: clarify why used such expression (hint: -1/3 , 0 ,1/3 order in interpol_filter)
>  > >
>  > >  clarify why such an expression is used
>  >
>  > Is comment in attached patch enough clear?
>  >
>  > >  > +/**
>  > >  > + * \brief Circularly convolve fixed vector with a phase dispersion impulse response filter
>  > >
>  > >  "convolve" is not an English word.  I have no idea what you are trying
>  > >  to say here.
>  >
>  > "convolve" is a math term meaning something like "roll up" (not sure
>  > about synonym though)
>  > http://en.wikipedia.org/wiki/Convolve
>  >
>  > >
>  > >
>  > >  > + * \param filter impulse response of phase filter to apply
>  > >
>  > >  umm, ?
>  >
>  > Changed to "phase filter coefficients"
>  >
>  > [...]
>  >
>  > >  > + * \param speech [in/out] reconstructed speech signal for applying filter to
>  > >
>  > >  ?
>  >
>  > Changed to "speech data to proceed"
>  >
>  >
>  > All thing not mentioned are fixed too.
>  [...]
>
> > +void ff_acelp_interpolate_excitation(
>  > +        int16_t* ac_v,
>  > +        int pitch_delay_6x,
>  > +        int subframe_size)
>
> > +{
>  > +    int n, i;
>  > +    int v;
>  > +
>  > +    /* Lookup table contain values corresponding to -2/6 -1/6 0 1/6 2/6 3/6 fractions order.
>  > +       Filtering process uses negative pitch lag offset, but negative offset should
>  > +       not be used in table. To avoid negative offset in table dimension corresponding to
>  > +       fractional delay following conversion applies:
>  > +
>  > +       pitch_delay = 6*intT0 + frac + 2, thus
>  > +
>  > +       -(pitch_delay - 2) = -(6*intT0+frac) = -6*intT0 - frac =
>  > +
>  > +         / -6*(intT0) - frac,       frac <  0
>  > +       =<
>  > +         \ -6*(intT0+1) + (6-frac), frac >= 0
>  > +    */
>  > +
>
>  > +    // Compute negative value of fractional delay (-frac)
>  > +    int pitch_delay_frac = 2 - (pitch_delay_6x%6);
>  > +    // Compute integer part of pitch delay (intT0)
>  > +    int pitch_delay_int  = pitch_delay_6x / 6;
>
> > +
>  > +    //Make sure that pitch_delay_frac will be always positive
>  > +    if(pitch_delay_frac < 0)
>  > +    {
>  > +        pitch_delay_frac += 6;
>  > +        pitch_delay_int++;
>  > +    }
>
>  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 have not strong objections. Thus if you think putting the comment
outside routine
will be better, i'll put it at the end of corresponding doxygen comment.

>  > +    //pitch_delay_frac [0; 5]
>
> > +    //pitch_delay_int  [PITCH_LAG_MIN-1; PITCH_LAG_MAX]
>
> > +    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

>  > +void ff_acelp_high_pass_filter(
>  > +        int16_t* hpf_z,
>  > +        int* hpf_f,
>  > +        int16_t* speech,
>  > +        int length)
>  > +{
>  > +    int i;
>  > +
>  > +    for(i=0; i<length; i++)
>  > +    {
>  > +        memmove(hpf_z + 1, hpf_z, 2 * sizeof(hpf_z[0]));
>  > +        hpf_z[0] = speech[i];
>  > +
>  > +        hpf_f[0] =  MULL(hpf_f[1], 15836);
>  > +        hpf_f[0] += MULL(hpf_f[2], -7667);
>  > +
>
>  > +        hpf_f[0] += hpf_z[0] * 7699;
>  > +        hpf_f[0] += hpf_z[1] * -15398;
>  > +        hpf_f[0] += hpf_z[2] * 7699;
>
>  hpf_f[0] += 7699*(hpf_z[0] - 2*hpf_z[1] + hpf_z[2]);

ok.

>
>
>  > +
>  > +        // Clippin is required to pass G.729 OVERFLOW test
>  > +        if(hpf_f[0] >= 0xfffffff)
>  > +        {
>  > +            speech[i] = SHRT_MAX;
>  > +            hpf_f[0] = 0x3fffffff;
>  > +        }
>  > +        else if (hpf_f[0] <= -0x10000000)
>  > +        {
>  > +            speech[i] = SHRT_MIN;
>  > +            hpf_f[0] = -0x40000000;
>  > +        }
>  > +        else
>  > +        {
>
>  > +            hpf_f[0] <<= 2;
>
>  this is avoidable by changing a few other shifts

But can cause int overflow in hpf_f[0], isn't it?

-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719




More information about the ffmpeg-devel mailing list