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

Michael Niedermayer michaelni
Sat May 24 18:42:19 CEST 2008


On Sat, May 24, 2008 at 01:02:54PM +0700, Vladimir Voroshilov wrote:
> 2008/5/19 Michael Niedermayer <michaelni at gmx.at>:
> > 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,
> > };
> >
> 
> Fixed.
> 
> >
> >> +
> >> +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.
> 
> Removed
> 
> >
> >
> > [...]
> >> +/**
> >> + * \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 ...
> 
> Fixed.
> 
> >
> >
> >> + * \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
> 
> See patch
> 
> >
> >
> >> + * \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"
> 
> Fixed.
> 
> >
> >
> >> + *
> >> + * 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
> >
> 
> Perhaps attached version will be better.


patch looks ok

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

Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- 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/20080524/e56f86f4/attachment.pgp>



More information about the ffmpeg-devel mailing list