[FFmpeg-devel] [PATCH] Common ACELP code & G.729 [3/7] - vectors operations
Vladimir Voroshilov
voroshil
Fri May 9 08:28:58 CEST 2008
Michael Niedermayer wrote:
> On Sat, May 03, 2008 at 02:53:40PM +0700, Vladimir Voroshilov wrote:
> > Michael Niedermayer wrote:
> > > On Fri, May 02, 2008 at 06:43:18PM +0700, Vladimir Voroshilov wrote:
[...]
> > > fc_v[ index ] += (pulses_signs & 1) ? 8191 : -8192;
> > > stuff is duplicated all over the place
> >
> > This line adds pulses to fixed vector.
> > Lines are the same just because sign bits are packed together each other (one per
> > index) and separately from index bits.
> > There are also different packing methods (sipr @16k, amr @10.2k)
> > I don't think implementing this 1 line of code via macro or inline
> > routine is a good idea.
>
> no, i meant
>
> for(i=0; i<2 or 4; i++)
> fc_v[ index[i] ] += ((pulses_signs>>i) & 1) ? 8191 : -8192;
>
> But i do not know yet if this is a good idea, it might turn out that its
> worse after some other simplifications are done ...
>
>
I don't see advantage yet.
[...]
> > + /* Reference G.729 and AMR fixed point code performs clipping after
> > + each of the two following accumulations.
> > + Since clipping affects only synthetic OVERFLOW test and still not
> > + cause int type oveflow, it was moved outside loop. */
> > +
> > + /* R(x):=ac_v[-k+x] */
> > + /* v += R(n-i)*ff_acelp_interp_filter(t+6i) */
> > + v += ac_v[n - pitch_delay_int - i] * ff_acelp_interp_filter[i][2-pitch_delay_frac];
> > + /* v += R(n+i+1)*ff_acelp_interp_filter(6-t+6i) */
> > + v += ac_v[n - pitch_delay_int + i] * ff_acelp_interp_filter[i][pitch_delay_frac-2];
>
> Like with the other patches id like to have the comments seperated somehow
> the code is quite hard to read with them intermingled like this.
Three one line comments are merged together.
> > + }
> > + ac_v[n] = av_clip_int16((v + 0x4000) >> 15);
> > + }
> > +}
> > +
>
> > +/**
> > + * \brief common routines for 4pulses_13bits and 4pulses_21bits
> > + *
> > + * Tables differs only by width, so can be handled via common routine.
> > + */
> > +static void ff_acelp_4pulses_13_21_bits(
> > + int16_t* fc_v,
> > + int fc_index,
> > + int pulses_signs,
> > + int bits)
> > +{
> > + int mask = (1 << bits) - 1;
> > + int i, index;
> > +
> > + for(i=0; i<3; i++)
> > + {
> > + index = i + 5 * (fc_index & mask);
> > + fc_v[ index ] += (pulses_signs & 1) ? 8191 : -8192; // +/-1 in (2.13)
> > +
> > + fc_index >>= bits;
> > + pulses_signs >>= 1;
> > + }
> > +
> > + index = 3 + (fc_index & 1) + 5 * ((fc_index>>1) & mask);
> > + fc_v[ index ] += (pulses_signs & 1) ? 8191 : -8192;
> > +}
> > +
> [...]
> > +
> > +void ff_acelp_fc_2pulses_9bits_g729d(
> > + int16_t* fc_v,
> > + int fc_index,
> > + int pulses_signs)
> > +{
> > + int index;
> > +
> > + index = ((5 * gray_decode[fc_index & 15]) >> 1) + 1;
> > + fc_v[ index ] += (pulses_signs & 1) ? 8191 : -8192; // +/-1 in (2.13)
> > +
> > + pulses_signs >>= 1;
> > + fc_index >>= 4;
> > +
> > + index = fc_2pulses_9bits_track2[gray_decode[fc_index & 31]];
> > + fc_v[ index ] += (pulses_signs & 1) ? 8191 : -8192; // +/-1 in (2.13)
> > +}
>
> The only thing that seperates these is the different tables
>
> for(i=0; i<pulse_count; i++){
> index = i + tab[fc_index & mask];
> fc_v[ index ] += (pulses_signs & 1) ? 8191 : -8192;
>
> fc_index >>= bits;
> pulses_signs >>= 1;
> }
>
> index = tab2[fc_index];
> fc_v[ index ] += pulses_signs ? 8191 : -8192;
>
> This also means you no longer need the function pointers.
>
Hm. I hope, this version is better..
> > +
> > +void ff_acelp_fc_enchance_harmonics(
> > + int16_t* fc_v,
> > + int pitch_delay,
> > + int16_t gain_pitch,
> > + int length)
> > +{
> > + int i;
> > +
> > + for(i=pitch_delay; i<length;i++)
> > + fc_v[i] += (fc_v[i - pitch_delay] * gain_pitch) >> 14;
> > +}
> > +
> > +void ff_acelp_build_excitation(
> > + int16_t* exc,
> > + const int16_t *ac_v,
> > + const int16_t *fc_v,
> > + int16_t gp,
> > + int16_t gc,
> > + int subframe_size)
> > +{
> > + int i;
> > +
> > + //clipping required here. breaks OVERFLOW test
> > + for(i=0; i<subframe_size; i++)
> > + exc[i] = av_clip_int16((ac_v[i] * gp + fc_v[i] * gc + 0x2000) >> 14);
> > +}
>
> Does it really makes sense to put 1 line loops in functions?
0. This is not only one line loop. This is more or less separate part of
algorithm - building excitation signal from innovation and algebraic
codevectors.
1. This peace of code is used in all acelp-based codes.
2. It makes code simpler in main loop.
3. Converting it to macro/static will turn into duplication of such routine in
each codec.
--
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
Omsk State University
JID: voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 03_acelp_vectors43.diff
Type: text/x-diff
Size: 14421 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080509/901ed057/attachment.diff>
More information about the ffmpeg-devel
mailing list