[FFmpeg-devel] [PATCH] Common ACELP routines (2/3) - filters
Michael Niedermayer
michaelni
Sun Apr 27 17:57:57 CEST 2008
On Sun, Apr 27, 2008 at 08:39:15PM +0700, Vladimir Voroshilov wrote:
> On Sun, Apr 27, 2008 at 6:58 PM, Vladimir Voroshilov <voroshil at gmail.com> wrote:
> >
> > On Sun, Apr 27, 2008 at 6:56 PM, Vladimir Voroshilov <voroshil at gmail.com> wrote:
> > >
> > > On Sun, Apr 27, 2008 at 6:12 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > >
> > > > On Sun, Apr 27, 2008 at 11:28:38AM +0700, Vladimir Voroshilov wrote:
> > > > > On Sun, Apr 27, 2008 at 4:02 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > > > >
> > > > > > On Sun, Apr 27, 2008 at 03:35:08AM +0700, Vladimir Voroshilov wrote:
> > > > > > > On Sun, Apr 27, 2008 at 3:10 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > > > > > >
> > > > >
> > > > > [...]
> > > > >
> > > > > > > > I wonder if it would be cleanerto do this outside of this function.
> > > > > > >
> > > > > > > But it describe calculating of two local variables.
> > > > > > > I'm afraid this comment will confuse peoples if will be placed outside.
> > > > > >
> > > > > > I mean the spliting of the pitch_delay variable not the comment.
> > > > > > IIRC its split outside already as its needed splited for something else.
> > > > >
> > > > > Fixed locally.
> > > > >
> > > > > [...]
> > > > >
> > > > > > > > > + for(n=0; n<subframe_size; n++)
> > > > > > > > > + {
> > > > > > > > > + /* 3.7.1 of G.729, Equation 40 */
> > > > > > > >
> > > > > > > > > + v=0;
> > > > > > > > > + for(i=0; i<10; i++)
> > > > > > > > > + {
> > > > > > > > > + /* R(x):=ac_v[-k+x] */
> > > > > > > > > + v += ac_v[n - pitch_delay_int - i ] * ff_acelp_interp_filter[i][ pitch_delay_frac];
> > > > > > > > > + v = av_clip(v, -0x40000000, 0x3fffffff); //v += R(n-i)*ff_acelp_interp_filter(t+3i)
> > > > > > > > > + v += ac_v[n - pitch_delay_int + i + 1] * ff_acelp_interp_filter[i][6 - pitch_delay_frac];
> > > > > > > > > + v = av_clip(v, -0x40000000, 0x3fffffff); //v += R(n+i+1)*ff_acelp_interp_filter(3-t+3i)
> > > > > > > >
> > > > > > > > does amr and the others also clip at such illogical place?
> > > > > > >
> > > > > > > In this loop int overflow occurs in synthetic.
> > > > > > > AMR fixed point reference code does checks here (via the L_mac routine).
> > > > > > > Moreover reference code checks for overflow in 95% of math operations through
> > > > > > > calls to L_mac, L_mult, L_add, etc everywhere.
> > > > > > >
> > > > > > > If you wish i can put this clipping under #ifdef G729_BITEXACT
> > > > > >
> > > > > > I do not see this cliping in the float reference of g729. I also do not
> > > > > > see it in soc/amr. It cant be that the cliping is correct in one implementation
> > > > > > but not the other.
> > > > >
> > > > > I'm afraid soc/amr was not checked for overflows.
> > > >
> > > > > Floating point code irrelevant here, imho,
> > > >
> > > > No it is very relevant. Either:
> > > > 1. The cliping does never occur for valid input
> > >
> > > If "valid" = "produced from regular speech", parhaps. But when "valid"
> > > = "bitstream made according to spec" it does.
> > >
> > >
> > > > 2. The cliping does not sigificantly change the output (but in that case
> > > > its not needed)
> > >
> > > It does due to int type overflow.
> > >
> > >
> > > > 3. The float implementation is buggy and produces significantly different
> > > > output for some valid files.
> > >
> > > It produces different (about 30 PSNR) result for every file (comparing
> > > with fixed-point).
> > >
> > >
> > > > 4. The integer implementation is buggy (this isnt possible per definition)
> > >
> > > dunno that
> > >
> > >
> > > > > because the reason of int overflow is using fixed-point.
> > > > > And all reference fixed-point code has this check (both AMR and G.729).
> > > >
> > > > overflow != cliping
> > > > Explain how the code above can overflow with a single cliping at the very end!
> > > > Cliping after each addition gives a _WRONG_ value
> > >
> > > of course
> > >
> > >
> > > > an example would be
> > > > clip_uint8(200+200)-200 != clip_uint8(200+200-200)
> > >
> > > and absent clipping will produce wrong result too
> > > an example would be
> > > "byte=clip_uint8(byte)+200" != "byte=clip_uint8(byte+200)"
> >
> > damn. i meant:
> > "byte=byte+200" != "byte=clip_uint8(byte+200)"
> >
> >
> >
> > > Which of them is correct?
> > >
> > >
> > > > > As i already said this check affects only synthetic overflow test.
> > > >
> > > > So why do we care about this test at all?
> > > >
> > > > The readme file clearly says:
> > > > ----
> > > > This directory contains testvectors to validate the correct execution
> > > > of the G.729 ANSI-C software (version 3.3). NOTE that these vectors
> > > > are not part of a validation procedure.
> > >
> > > To make sure that we will not introduce overflow somewhere else in future.
> > >
> > > I'm afraid we can flaming for a long time.
> > > Thus i'll remove checks in next version and keep it in local tree only.
> > >
> > > Are the rest (except spelling mentioned by Diego) ok?
> >
>
> Here is latest version.
[...]
> +void ff_acelp_interpolate_excitation(
> + int16_t* ac_v,
> + int pitch_delay_int,
> + int pitch_delay_frac,
> + int subframe_size)
> +{
> + int n, i;
> + int v;
> +
> + /* The lookup table contain values corresponding
> + to -2/6 -1/6 0 1/6 2/6 3/6 fraction order.
> + The filtering process uses a negative pitch lag offset, but
> + a negative offset should not be used in then table. To avoid
> + a negative offset in the table dimension corresponding to
> + fractional delay the following conversion applies:
> +
trailing whitespace
> + -pitch_delay = -(6*pitch_delay_int+pitch_delay_frac) =
> +
> + = -6*pitch_delay_int - pitch_delay_frac =
> +
> + / -6*(pitch_delay_int) - pitch_delay_frac, pitch_delay_frac < 0
> + =<
> + \ -6*(pitch_delay_int+1) + (6-pitch_delay_frac), pitch_delay_frac >= 0
> + */
> +
> + /* Compute negative value of fractional delay */
> + pitch_delay_frac = - pitch_delay_frac;
> +
> + /* Now make sure that pitch_delay_frac will always be positive */
> + if(pitch_delay_frac < 0)
> + {
> + pitch_delay_frac += 6;
> + pitch_delay_int++;
> + }
Can it really be >= 0 and <0 ?
[...]
> +void ff_acelp_weighted_filter(
> + int16_t *out,
> + const int16_t* in,
> + int16_t weight,
> + int filter_length)
> +{
> + int weight_pow = 1 << 15;
> + int n;
> +
> + for(n=0; n<filter_length; n++)
> + {
> + /* (0.15) * (3.12) and (3.27) -> (3.12) with rounding */
> + out[n] = (in[n] * weight_pow + 0x4000) >> 15;
> + /* (0.15) * (3.12) and (3.27) -> (3.12) with rounding */
> + weight_pow = (weight_pow * weight + 0x4000) >> 15;
> + }
> +}
as weight is a constant weight_pow is as well and half of the computations
are unneeded
> +
> +void ff_acelp_high_pass_filter(
> + int16_t* out,
> + int16_t* hpf_z,
> + int* hpf_f,
> + const int16_t* in,
> + int length)
> +{
> + int i;
> +
> + for(i=0; i<length; i++)
> + {
> + memmove(hpf_z + 1, hpf_z, 2 * sizeof(hpf_z[0]));
> + hpf_z[0] = in[i];
> +
> + hpf_f[0] = MULL(hpf_f[1], 15836);
> + hpf_f[0] += MULL(hpf_f[2], -7667);
> +
> + hpf_f[0] += 7699 * (hpf_z[0] - 2*hpf_z[1] + hpf_z[2]);
> +
> + /* Clippin is required to pass G.729 OVERFLOW test */
> + if(hpf_f[0] >= 0xfffffff)
> + {
> + out[i] = SHRT_MAX;
> + hpf_f[0] = 0x3fffffff;
> + }
> + else if (hpf_f[0] <= -0x10000000)
> + {
> + out[i] = SHRT_MIN;
> + hpf_f[0] = -0x40000000;
> + }
> + else
> + {
> + hpf_f[0] <<= 2;
The shift is avoidable i think as already said
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I wish the Xiph folks would stop pretending they've got something they
do not. Somehow I fear this will remain a wish. -- M?ns Rullg?rd
-------------- 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/20080427/465c08d4/attachment.pgp>
More information about the ffmpeg-devel
mailing list