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

[Vladimir Voroshilov]

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>