[FFmpeg-devel] [PATCH] Common ACELP routines (2/3) - filters
Michael Niedermayer
michaelni
Sat Apr 26 23:02:25 CEST 2008
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:
> >
> > On Sun, Apr 27, 2008 at 01:05:07AM +0700, Vladimir Voroshilov wrote:
> > > Hello, Diego
> > > Thank you for your review.
> > >
> > > On Sat, Apr 26, 2008 at 11:32 PM, Diego Biurrun <diego at biurrun.de> wrote:
> > > > On Sat, Apr 26, 2008 at 11:11:56PM +0700, Vladimir Voroshilov wrote:
> > > > >
> > > > > P.S. Please anybody check English spelling too.
> > > > >
> > > > > --- /dev/null
> > > > > +++ b/libavcodec/acelp_filters.c
> > > > > @@ -0,0 +1,265 @@
> > > > > +/**
> > > > > + *
> > > > > + * G.729 specification says:
> > > > > + * b30 is based on Hamming windowed sinc functions, truncated at +/-29 and
> > > >
> > > > What's sinc?
> > >
> > > Math function: y=sinc(x)=sin(x)/x
> > > Widely used, imho
> > >
> > > > > + // TODO: clarify why used such expression (hint: -1/3 , 0 ,1/3 order in interpol_filter)
> > > >
> > > > clarify why such an expression is used
> > >
> > > Is comment in attached patch enough clear?
> > >
> > > > > +/**
> > > > > + * \brief Circularly convolve fixed vector with a phase dispersion impulse response filter
> > > >
> > > > "convolve" is not an English word. I have no idea what you are trying
> > > > to say here.
> > >
> > > "convolve" is a math term meaning something like "roll up" (not sure
> > > about synonym though)
> > > http://en.wikipedia.org/wiki/Convolve
> > >
> > > >
> > > >
> > > > > + * \param filter impulse response of phase filter to apply
> > > >
> > > > umm, ?
> > >
> > > Changed to "phase filter coefficients"
> > >
> > > [...]
> > >
> > > > > + * \param speech [in/out] reconstructed speech signal for applying filter to
> > > >
> > > > ?
> > >
> > > Changed to "speech data to proceed"
> > >
> > >
> > > All thing not mentioned are fixed too.
> > [...]
> >
> > > +void ff_acelp_interpolate_excitation(
> > > + int16_t* ac_v,
> > > + int pitch_delay_6x,
> > > + int subframe_size)
> >
> > > +{
> > > + int n, i;
> > > + int v;
> > > +
> > > + /* Lookup table contain values corresponding to -2/6 -1/6 0 1/6 2/6 3/6 fractions order.
> > > + Filtering process uses negative pitch lag offset, but negative offset should
> > > + not be used in table. To avoid negative offset in table dimension corresponding to
> > > + fractional delay following conversion applies:
> > > +
> > > + pitch_delay = 6*intT0 + frac + 2, thus
> > > +
> > > + -(pitch_delay - 2) = -(6*intT0+frac) = -6*intT0 - frac =
> > > +
> > > + / -6*(intT0) - frac, frac < 0
> > > + =<
> > > + \ -6*(intT0+1) + (6-frac), frac >= 0
> > > + */
> > > +
> >
> > > + // Compute negative value of fractional delay (-frac)
> > > + int pitch_delay_frac = 2 - (pitch_delay_6x%6);
> > > + // Compute integer part of pitch delay (intT0)
> > > + int pitch_delay_int = pitch_delay_6x / 6;
> >
> > > +
> > > + //Make sure that pitch_delay_frac will be always positive
> > > + if(pitch_delay_frac < 0)
> > > + {
> > > + pitch_delay_frac += 6;
> > > + pitch_delay_int++;
> > > + }
> >
> > 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.
>
> I have not strong objections. Thus if you think putting the comment
> outside routine
> will be better, i'll put it at the end of corresponding doxygen comment.
>
> > > + //pitch_delay_frac [0; 5]
> >
> > > + //pitch_delay_int [PITCH_LAG_MIN-1; PITCH_LAG_MAX]
> >
> > > + 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.
[...]
> > > + // Clippin is required to pass G.729 OVERFLOW test
> > > + if(hpf_f[0] >= 0xfffffff)
> > > + {
> > > + speech[i] = SHRT_MAX;
> > > + hpf_f[0] = 0x3fffffff;
> > > + }
> > > + else if (hpf_f[0] <= -0x10000000)
> > > + {
> > > + speech[i] = SHRT_MIN;
> > > + hpf_f[0] = -0x40000000;
> > > + }
> > > + else
> > > + {
> >
> > > + hpf_f[0] <<= 2;
> >
> > this is avoidable by changing a few other shifts
>
> But can cause int overflow in hpf_f[0], isn't it?
this is a << not a >>
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- 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/20080426/506df8fb/attachment.pgp>
More information about the ffmpeg-devel
mailing list