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

Michael Niedermayer michaelni
Sat Apr 26 23:02:25 CEST 2008

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:
> >
> > 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 mean the spliting of the pitch_delay variable not the comment.
IIRC its split outside already as its needed splited for something else.

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

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.

[...]
> >  > +        // 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?

this is a << not a >>

[...]
--

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- 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/20080426/506df8fb/attachment.pgp>