[FFmpeg-devel] [PATCH] Move pitch vector interpolation filter to acelp_filters.

[Michael Niedermayer]

On Mon, May 19, 2008 at 06:20:20PM +0700, Vladimir Voroshilov wrote:
> 
> 
> 2008/5/19 Vladimir Voroshilov <voroshil at gmail.com>:
> > 2008/5/19 Vladimir Voroshilov <voroshil at gmail.com>:
> >  
> > 
> > > 2008/5/18 Michael Niedermayer <michaelni at gmx.at>:
> >  >
> >  >
> >  > > On Sun, May 18, 2008 at 12:23:32AM +0700, Vladimir Voroshilov wrote:
> >  >  >
> 
> [...]
> 
> >  >  >  This code is written for a filter with an odd number of coeffs (1,3,5,...) but
> >  >  >  all but the first filter for which it is used have an even number thus they
> >  >  >  end up needing these 0 coeffs.
> >  >  >  The code should be changed so it expects even number of coeff filters and the
> >  >  >  unneeded 0 elements should be removed from the tables
> >  >  >
> >  >  >  anyway, the obvious implementation is:
> >  >  >
> >  >  >  for(n=0; n<subframe_size; n++){
> >  >  >     int idx = precision>>1;
> >  >  >     int v = 0x4000;
> >  >  >     for(i=1; i<filter_length+1; i++){
> >  >  >         v += in[n - pitch_delay_int - i] * filter_coeffs[idx - frac];
> >  >  >         v += in[n - pitch_delay_int + i] * filter_coeffs[idx + frac];
> >  >  >         idx += precision;
> >  >  >     }
> >  >  >     out[n] = av_clip_int16(v >> 15);
> >  >
> >  >  First, I can't see here equivalent  (due to i>0) for
> >  >
> >  >     int v = in[n - pitch_delay_int] * filter_coeffs[FFABS(pitch_delay_frac)];
> >  >
> >  >  Second, your code 'as is' gives me PSNR 10 and i'm afraid it is totally wrong:
> >  >  you have shifted filter represented in filter_coeffs by precision/2
> >  >  as respect signal. This will obviously produce different result.
> >  
> >  Sorry, i overreacted with 'totally wrong'.
> >  I'm still investingating problem, but already found several
> >  inconsistences between doxygen comment to the filter and it's usage
> >  (like passing [-5; 0] value
> >  in pitch_frac_delay).
> >  
> >  Code is not looks like yours yet, but is going to that direction.
> 
> finished.
> Here is result.
> It looks a bit hackish but i don't see yet  how to make it cleaner.
[...]
>  
> +const int16_t ff_acelp_interp_filter[66] =
> +{ /* (0.15) */
> +  29443, 28346, 25207, 20449, 14701,  8693,
> +   3143, -1352, -4402, -5865, -5850, -4673,
> +  -2783,  -672,  1211,  2536,  3130,  2991,
> +   2259,  1170,     0, -1001, -1652, -1868,
> +  -1666, -1147,  -464,   218,   756,  1060,
> +   1099,   904,   550,   135,  -245,  -514,
> +   -634,  -602,  -451,  -231,     0,   191,
> +    308,   340,   296,   198,    78,   -36,
> +   -120,  -163,  -165,  -132,   -79,   -19,
> +     34,    73,    91,    89,    70,    38,
> +};

you removed one 0 too much

const int16_t ff_acelp_interp_filter[66] =
{ /* (0.15) */
  29443, 28346, 25207, 20449, 14701,  8693,
   3143, -1352, -4402, -5865, -5850, -4673,
  -2783,  -672,  1211,  2536,  3130,  2991,
   2259,  1170,     0, -1001, -1652, -1868,
  -1666, -1147,  -464,   218,   756,  1060,
   1099,   904,   550,   135,  -245,  -514,
   -634,  -602,  -451,  -231,     0,   191,
    308,   340,   296,   198,    78,   -36,
   -120,  -163,  -165,  -132,   -79,   -19,
     34,    73,    91,    89,    70,    38,
      0,
};


> +
> +void ff_acelp_interpolate_pitch_vector(
> +        int16_t* out,
> +        const int16_t* in,
> +        const int16_t* filter_coeffs,
> +        int precision,
> +        int pitch_delay_int,
> +        int pitch_delay_frac,
> +        int filter_length,
> +        int subframe_size)
> +{

As this really is a generic interpolation function it should be

void ff_acelp_interpolate(
        int16_t* out,
        const int16_t* in,
        const int16_t* filter_coeffs,
        int precision,
        int frac,
        int filter_length,
        int out_len)
{

note especially the removed pitch_delay_int it served no purpose anyway.


[...]
> +/**
> + * \brief Decode the adaptive-codebook (pitch) vector (4.1.3 of G.729).

Its a generic interpolation function ...


> + * \param out [out] buffer to store decoded vector

Its a generic interpolation function ...


> + * \param in input data
> + * \param filter_coeffs interpolation filter coefficients (0.15)

> + * \param precision precision of passed interpolation filter

unclear, precission could be as well the number of bits in each coeff


> + * \param pitch_delay_int pitch delay, integer part
> + * \param pitch_delay_frac pitch delay, fractional part [0..5]
> + * \param filter_length filter length

> + * \param subframe_length subframe length

Its a generic interpolation function, theres no "frame"


> + *
> + * The routine assumes the following order of fractions (X - integer delay):
> + *
> + * 1/3 precision: X     1/3      2/3      X     1/3      2/3      X
> + * 1/6 precision: X 1/6 2/6 3/6  4/6  5/6 X 1/6 2/6 3/6  4/6  5/6 X
> + *
> + * The routine can be used for 1/3 precision, too, by
> + * passing 2*pitch_delay_frac as third parameter.
> + */

This is at least missing a clear specification of the contents of filter_coeffs


[...]
-- 
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/20080519/845ae3b9/attachment.pgp>