[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