[FFmpeg-devel] [PATCH] Common ACELP code & G.729 [3/7] - vectors operations

[Vladimir Voroshilov]

2008/5/9 Vladimir Voroshilov <voroshil at gmail.com>:
> 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.
>

Modifications of ff_acelp_interpolate_pitch_vector,
necessary to reuse code from G.729 postfilter.

-- 
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: 03_acelp_vectors49.diff
Type: text/x-diff
Size: 14892 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080511/b995dab8/attachment.diff>