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

[Michael Niedermayer]

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.


> +
> +    //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?


[...]
> +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]);


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

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

There will always be a question for which you do not know the correct awnser.
-------------- 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/e84f99f6/attachment.pgp>