[FFmpeg-devel] [PATCH] Move pitch vector interpolation filter to acelp_filters.
Vladimir Voroshilov
voroshil
Sat May 24 08:02:54 CEST 2008
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.
--
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01_acelp_filt_58.diff
Type: text/x-diff
Size: 5044 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080524/2309c893/attachment.diff>
More information about the ffmpeg-devel
mailing list