[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