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

Michael Niedermayer michaelni
Mon Apr 28 11:31:05 CEST 2008


On Mon, Apr 28, 2008 at 10:41:03AM +0700, Vladimir Voroshilov wrote:
> 
> 
> On Mon, Apr 28, 2008 at 1:35 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > 
> > On Mon, Apr 28, 2008 at 01:02:32AM +0700, Vladimir Voroshilov wrote:
> 
> [...]
> 
> >  > >  > +       -pitch_delay = -(6*pitch_delay_int+pitch_delay_frac) =
> >  > >  > +
> >  > >  > +       =  -6*pitch_delay_int - pitch_delay_frac =
> >  > >  > +
> >  > >  > +         / -6*(pitch_delay_int)   - pitch_delay_frac,     pitch_delay_frac <  0
> >  > >  > +       =<
> >  > >  > +         \ -6*(pitch_delay_int+1) + (6-pitch_delay_frac), pitch_delay_frac >= 0
> >  > >  > +    */
> >  > >  > +
> >  > >  > +    /* Compute negative value of fractional delay */
> >  > >  > +    pitch_delay_frac = - pitch_delay_frac;
> >  > >  > +
> >  > >
> >  > >  > +    /* Now make sure that pitch_delay_frac will always be positive */
> >  > >
> >  > > > +    if(pitch_delay_frac < 0)
> >  > >  > +    {
> >  > >  > +        pitch_delay_frac += 6;
> >  > >  > +        pitch_delay_int++;
> >  > >  > +    }
> >  > >
> >  > >  Can it really be >= 0 and <0 ?
> >  >
> >  > Didn't understand you.
> >  > According to current pitch delay decode routine pitch_delay_frac
> >  > belongs to [-2; 3]
> >  > -pitch_delay_frac belongs to [-3; 2]
> >  
> >  In the latest code i have it is:
> >  
> >  +                pitch_delay_3x = 3 * ctx->pitch_delay_int_prev + 1;
> >  +                if(!i)
> >  +                    pitch_delay_1st_int = pitch_delay_3x / 3;
> >  +        }
> >  +        else if (!i)
> >  +        {
> >  +            // Decoding of the adaptive-codebook vector delay for first subframe (4.1.3)
> >  +            pitch_delay_3x = parm->ac_index[i] + 59;
> >  +            if(pitch_delay_3x > 255)
> >  +                pitch_delay_3x = 3 * pitch_delay_3x - 512;
> >  +            pitch_delay_1st_int = pitch_delay_3x / 3;
> >  +        }
> >  +        else
> >  +        {
> >  +            // Decoding of the adaptive-codebook vector delay for second subframe (4.1.3)
> >  +            int pitch_delay_min_3k = 3 * av_clip(
> >  +                    ctx->pitch_delay_int_prev-5,
> >  +                    PITCH_LAG_MIN,
> >  +                    PITCH_LAG_MAX-9);
> >  +
> >  +                pitch_delay_3x = parm->ac_index[i] + pitch_delay_min_3k - 1;
> >  +        }
> >  ...
> >  +        ff_acelp_interpolate_excitation(pitch_delay_3x, ctx->exc + i*ctx->subframe_size, ctx->subframe_size);
> >  
> >  In this code pitch_delay_3x should be >0 thus the if will always be true and
> >  can be simplified out! The same applies to the *(-1) before it.
> 
> Michael, you messed up two different versions.
> "if" in quoted code applies to "1-pitch_delay_3x%3", not to "pitch_delay_3x"
> thus argument of "if"  can be negative obviously.
> Here are two versions.

I didnt mess up anything, i clearly said "latest code i have" you never
posted the uptodate code.
Anyway, let me say it more generically
The code is a mess it is more complex than needed and should be simplified.
The int/frac code might now be split over 3 patches, and each of them
is far worse and messy than the orignal combined patch was.
The routine here should receive 2 variable and use them unchanged.
no *-1 no if() no + no - !


> 
> One (packed int/frac):
> 
> decode_lag()
> {
>   return int*3+frac+1 (frac = [-1; 1])
> }
> 
> interp(pitch_delay_3_x)
> {
>   frac = 1-pitch_delay_3x%3
>   if(frac<0)
>   {
>     frac+=3;
>     int++;
>   }
> }
> 
> Second (not packed, used in my latest patches, G.729
> and AMR reference codes):
> 
> decode_lag(*int, *frac)
> {
>   *int=
>   *frac=  (frac = [-1; 1])
> }
> 
> interp(int, frac)
> {
>   frac = -frac
>   if(frac<0)
>   {
>     frac+=3;
>     int++;
>   }
> }
> 
> I cant see how those "if" can be eliminated without
> complicating this and decode_lag code.

The original received a always positive single variable and
did completely unneeded calculations on it
This could have been changed to a /3 %3 outside aka
func(blah/3, blah%3) with no other changes outside and especially
no if(frac<0) or frac-=frac ANYWHERE.
why that would become impossible after some other changes is unclear
to me.


> 
> [...]
> 
> >  > >  > +void ff_acelp_high_pass_filter(
> >  > >  > +        int16_t* out,
> >  > >
> >  > > > +        int16_t* hpf_z,
> >  > >  > +        int* hpf_f,
> >  > >  > +        const int16_t* in,
> >  > >
> >  > > > +        int length)
> >  > >  > +{
> >  > >  > +    int i;
> >  > >  > +
> >  > >  > +    for(i=0; i<length; i++)
> >  > >  > +    {
> >  > >  > +        memmove(hpf_z + 1, hpf_z, 2 * sizeof(hpf_z[0]));
> >  > >  > +        hpf_z[0] = in[i];
> >  > >
> >  > > > +
> >  > >  > +        hpf_f[0] =  MULL(hpf_f[1], 15836);
> >  > >  > +        hpf_f[0] += MULL(hpf_f[2], -7667);
> >  > >  > +
> >  > >  > +        hpf_f[0] += 7699 * (hpf_z[0] - 2*hpf_z[1] + hpf_z[2]);
> >  > >  > +
> >  > >  > +        /* Clippin is required to pass G.729 OVERFLOW test */
> >  > >
> >  > > > +        if(hpf_f[0] >= 0xfffffff)
> >  > >  > +        {
> >  > >  > +            out[i] = SHRT_MAX;
> >  > >
> >  > > > +            hpf_f[0] = 0x3fffffff;
> >  > >  > +        }
> >  > >  > +        else if (hpf_f[0] <= -0x10000000)
> >  > >  > +        {
> >  > >  > +            out[i] = SHRT_MIN;
> >  > >
> >  > > > +            hpf_f[0] = -0x40000000;
> >  > >  > +        }
> >  > >  > +        else
> >  > >  > +        {
> >  > >
> >  > >  > +            hpf_f[0] <<= 2;
> >  > >
> >  > >  The shift is avoidable i think as already said
> >  >
> >  > Changed and added comment about bitexactness.
> >  
> >  I do think the bitexactness can be maintained.
> 
> I DO, or you do NOT ?
> Assuming you DO, added #ifdef G729_BITEXACT :)

ideg
it can be bitexact with no extra instructions, especially no
& masking LSBs or <<2 you just need a change a few of the shifts
and one mutiply constant IIRC.


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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/20080428/bd7b218e/attachment.pgp>



More information about the ffmpeg-devel mailing list