[FFmpeg-devel] [PATCH] Common ACELP routines (2/3) - filters

[Vladimir Voroshilov]

Michael Niedermayer wrote: 
> On Thu, Apr 24, 2008 at 02:15:51AM +0700, Vladimir Voroshilov wrote:
> > Michael Niedermayer wrote: 
> [...]
> > > > +        v=0;
> > > > +        for(i=0; i<10; i++)
> > > > +        {
> > > 
> > > > +            /*  R(x):=ac_v[-k+x] */
> > > > +            v += ac_v[n - pitch_delay_int - i    ] * ff_g729_interp_filter[i][    pitch_delay_frac];
> > > > +            v = av_clip(v, -0x40000000, 0x3fffffff); //v += R(n-i)*ff_g729_interp_filter(t+3i)
> > > > +            v += ac_v[n - pitch_delay_int + i + 1] * ff_g729_interp_filter[i][3 - pitch_delay_frac];
> > > > +            v = av_clip(v, -0x40000000, 0x3fffffff); //v += R(n+i+1)*ff_g729_interp_filter(3-t+3i)
> > > 
> > > The cliping is incorrect for generic code. Also i doubt g729 really needs
> > > it. What happens without that cliping or at least with it at the end, just
> > > before storing in ac_v?
> > 
> > Removing those line breaks OVERFLOW test (regardless of clipping outside loop),
> > significantly reduces PSNR from bitexact's 99,99 to 18,54 without outside
> > clipping and to 26,01 with it.
> 
> What is the OVERFLOW test anyway? Is this a normal valid bitstream generated
> from a pcm wave? Or some sythetic overflow excercise which cannot be generated
> by any input wave?
> (If you dont know you can test it by encoding the wave from overflow to see if
> there are still any overflows happening from the resulting bitstream)

Completely synthetic test, imho. ITU does not provide any PCM stream related to
it. I don't know is it possible to generate such stream from regular PCM or not.

> 
> 
> [...]
> > +void ff_acelp_convolve_circ(const int16_t* fc_in, const int16_t* filter, int16_t* fc_out, int subframe_size)
> > +{
> > +    int i, k;
> > +
> > +    memset(fc_out, 0, subframe_size * sizeof(int16_t));
> > +
> > +    for(i=0; i<subframe_size; i++)
> > +    {
> > +        if(fc_in[i])
> > +        {
> > +            for(k=0; k<i; k++)
> > +                fc_out[k] += (fc_in[i] * filter[subframe_size + k - i]) >> 15;
> > +
> > +            for(k=i; k<subframe_size; k++)
> > +                fc_out[k] += (fc_in[i] * filter[k - i]) >> 15;
> > +        }
> > +    }
> > +}
> 
> where is this used? i cant find it in g729 and neither this file

Hm. In AMR and  G.729D. Should i remove it now ?

> 
> 
> > +
> > +int ff_acelp_lp_synthesis_filter(
> > +        const int16_t* filter_coeffs,
> > +        const int16_t *in,
> > +        int16_t *out,
> > +        int16_t *filter_data,
> > +        int subframe_size,
> > +        int update_filter_data)
> > +{
> > +    int i,n;
> > +    int sum;
> > +
> > +    for(n=0; n<subframe_size; n++)
> > +    {
> > +        int overflow=0;
> > +
> > +        sum = in[n] << 12;
> > +        for(i=0; i<10; i++)
> > +            sum -= filter_coeffs[i+1] * filter_data[10+n-i-1];
> > +
> > +        sum = (sum + 0x800) >> 12;
> > +
> 
> > +        if(sum > 0x7fff)
> > +        {
> > +            sum = 0x7fff;
> > +            overflow = 1;
> > +        }
> > +        else if (sum < -0x8000)
> > +        {
> > +            sum =-0x8000;
> > +            overflow = 1;
> > +        }
> > +
> > +        if(overflow && !update_filter_data)
> > +            return 1;
> 
> if(sum + 0x8000 > 0xFFFFU){
>     if(!update_filter_data)
>         return 1;
>     sum= (sum>>31) ^ 32767;
> }

ok

> 
> 
> > +        filter_data[10+n] = out[n] = sum;
> 
> This duplicated storeage is unacceptable.

First for all assigned to filter data values will be used in loop later.
Thus filter_data can not be eliminated.
I can't use "out" instead of it due to necessary 10 items
with data from previous subframe at top).
Extending out with 10 items at top will require another temporary buffer
one memcpy somewhere later (because i will not be able to use output buffer
directly).

> > +    }
> > +
> > +    // Saving data for using in next subframe
> > +    if(update_filter_data)
> > +        memcpy(filter_data, filter_data + subframe_size, 10 * sizeof(int16_t));
> 
> This design requires the memcpy to be duplicated.

Don't see. Please explain.
I can put this memcpy outside, but this will look like cosmetic
change since the overall count of memcpy will be the same.


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