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

[Vladimir Voroshilov]

2008/5/18 Michael Niedermayer <michaelni at gmx.at>:
> On Sun, May 18, 2008 at 01:39:42AM +0700, Vladimir Voroshilov wrote:
>> 2008/5/18 Michael Niedermayer <michaelni at gmx.at>:
>> > On Sat, May 17, 2008 at 10:25:05PM +0700, Vladimir Voroshilov wrote:
>> >> 2008/5/14 Michael Niedermayer <michaelni at gmx.at>:
>> >> > On Wed, May 14, 2008 at 12:08:01AM +0700, Vladimir Voroshilov wrote:
>> >> >> 2008/5/12 Michael Niedermayer <michaelni at gmx.at>:
>> >> >> > On Sun, May 11, 2008 at 09:46:04PM +0700, Vladimir Voroshilov wrote:
>> >> >> >  > 2008/5/9 Vladimir Voroshilov <voroshil at gmail.com>:
>> >>
>> >> [...]
>> >>
>> >> >> >  > > Hm. I hope, this version is better..
>> >> >> >
>> >> >> >  there are still 2 seperate functions and now there are even wraper functions
>> >> >> >  this is not better IMHO.
>> >> >> >  see my example above, is there a problem with it?
>> >> >>
>> >> >> Already implemented yours routine.
>> >> >> Now i should remove those wrappers,
>> >> >> make common routine non-static and
>> >> >> use it in main loop, right?
>> >> >
>> >> > yes
>> >> >
>> >> >
>> >> >>
>> >> >> >  > +/**
>> >> >> >  > + * low-pass FIR (Finite Impulse Response) filter coefficients
>> >> >> >  > + *
>> >> >> >  > + *   A similar filter is named b30 in G.729.
>> >> >> >  > + *
>> >>
>> >> [...]
>> >>
>> >> >> >  Maybe the generic interpolation should be in a seperate file.
>> >> >>
>> >> >> Move it back to acelp_filters together with corresponding lookup tables?
>> >> >
>> >> > yes, it seems o fit better there
>> >>
>> >> Attached patch is version with  removed interpolation filter.
>> > [...]
>> >
>> >> +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)
>> >> +}
>> >
>> > duplicate
>> >
>> > My suggestion for the third time:
>> >
>> >> >  > >>     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;
>> >
>> > Or is there some reason why you do not use this? So far you didnt say
>> > why not IIRC.
>>
>> Did you see that second one is using gray decoding when first is not?
>> Or you mean restoring "5*i" tables, one with gray coding and one
>> without (you asked
>> me to remove such tables some time ago)
>> and pass them (as long as fc_2pulses_9bits_track2 table) via parameters?
>
> yes, that was the idea, i think that the 50 bytes or so of tables extra
> are better than the near duplicated function.
> And sorry for the back and forward, it wasnt clear to me that the silly
> 5*i table would allow the code to be sigificantly simplified.

What about such uniform routine?

.h:
enum fc_codings(fcmode0, fcmode1,fcmode2,bla bla);
fc_acelp_fc_decode(int16_t* fc, int fc_index, int pulses, int coding);

.c:
fc_acelp_fc_decode(int16_t* fc, int fc_index, int pulses, int coding)
{
   uint8_t * tab;
   uint8_t * tab2;
   int gray,bits,pulse_count,i,mask, index;

   switch(coding)
  {
   case fcmode0:
       gray=0;
       bits=3;
       pulse_count=3;
       tab = fcmode1_tab1;
       tab2 = fcmode1_tab2;
       break;
   case fcmode1:
       gray=0;
       bits = 4;
       pulse_count=3;
       tab = fcmode1_tab1;
       tab2 = fcmode1_tab2;
       break;
   case fcmode2:
       gray=1;
       bits = 4;
       pulse_count=1;
       tab = fcmode2_tab1;
       tab2 = fcmode2_tab2;
       break;
  }
  mask = (1<<bits) - 1;
  for(i=0; i<pulse_count; i++){
        index = fc_index & mask;
        if(gray) index = gray_decode[index];
        index = i + tab[index];
        fc_v[ index ] += (pulses_signs & 1) ? 8191 : -8192;
         fc_index >>= bits;
         pulses_signs >>= 1;
  }

   index = fc_index;
   if(gray) index = gray_decode[index];
   index = tab2[fc_index];
    fc_v[ index ] += pulses_signs ? 8191 : -8192;
}

-- 
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719