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

[Michael Niedermayer]

On Sun, May 18, 2008 at 01:23:20PM +0700, Vladimir Voroshilov wrote:
> 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?

I think its a mess.

tab, tab2, pulse_count, bits should be arguments to the function not some
enum which sets them in a switch()

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/20080518/f63edc10/attachment.pgp>