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

[Michael Niedermayer]

On Thu, Apr 24, 2008 at 08:24:25AM +0700, Vladimir Voroshilov wrote:
> 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.

So a reencoded version of that synthetic vector is not affected by the
existence of these checks?


> 
> > 
> > 
> > [...]
> > > +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 ?

no, if its used somewhere, leave it.


[...]
> > 
> > 
> > > +        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).

The double write is definitly useless after the first 10 iterations as
after that you can just work in the out buffer.

foobar_filter(filter_data+10, 10);
memcpy(out, filter_data+10, 10);
foobar_filter(out+10, N-10);

should work fine and will for large N (dunno how large it is, so maybe
this isnt worth it ...) be faster. Also it allows filter_data to be smaller.


> 
> > > +    }
> > > +
> > > +    // 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.

+        if(ff_acelp_lp_synthesis_filter(
+                &lp[i][0],
+                ctx->exc  + i * ctx->subframe_size,
+                out_frame + i * ctx->subframe_size,
+                ctx->syn_filter_data,
+                ctx->subframe_size,
+                0))
+            //Overflow occured, downscale excitation signal...
+            for(j=0; j<2*MAX_SUBFRAME_SIZE+PITCH_LAG_MAX+INTERPOL_LEN; j++)
+                ctx->exc_base[j] >>= 2;
+
+            ff_acelp_lp_synthesis_filter(
+                    &lp[i][0],
+                    ctx->exc  + i * ctx->subframe_size,
+                    out_frame + i * ctx->subframe_size,
+                    ctx->syn_filter_data,
+                    ctx->subframe_size,
+                    1);

Above unconditionally executes the code twice. If you skip the second
one (like the indention suggests) it will fail due to the missing 
update_filter_data based code IMHO

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/20080424/70be31bd/attachment.pgp>