[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