[FFmpeg-devel] [PATCH] G.729A (now fixed-point) decoder

[Vladimir Voroshilov]

Well. Here is updated (bit-exact) patch as well as suggested changes
in common.h (can be commited separately)

I'm not sure about one-line routines like l_add, l_mac, l_msu, etc
Should i convert them to macro ?

There were also made many changes in misc parts of code to get
bit-exact result. Thus, i'm afraid, complete review from zero point is
required.

On Wed, Mar 26, 2008 at 5:45 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
>
> On Tue, Mar 25, 2008 at 06:54:58PM +0600, Vladimir Voroshilov wrote:

>  >
>  > What about GAMMA_P ;) ?
>
>  i hate all the gammas in there ...

Renamed to LT_FILT_FACTOR
No more gamma's in code  :)

>  > cb_GA->cb_gain_1st
>  > cb->GB->cb_gain_2nd
>
>  if you prefer ...
>  it could also be simply gain_cb[2]

It could not.
cb_gain_1st and cb_gain_2nd dimensions does not match.

>  > >  This predicts the gain and then "adds" the correction from the bitstream.
>  > >  maybe that should be split in 2 functions
>  >
>  > Do you mean code under after "shift prediction error vector" ?
>  > this is hard to move that 3 lines out of this routine.
>  > There is no other "adds" in this routine.
>
>  IIRC i meant what the reference did, but i dont remember exactly ATM
>  I somehow felt that spliting this would allow maybe some code in the
>  middle to be factored out as that was similar in the error concealment case
>  and that the splited code could be half used in an encoder ...
>
>  just leave it if what i said makes no sense ...
>  ill look at it again in the next iteration of the review

Got it (after renaming variables properly ;) )
These one line of code can't be simply moved outside.
Prrecision of the routine result is not enough
(local variable can be shifted to right before return) to apply
correction exactly with reference code.

>  > >  > +/**
>  > >  > + * \brief compensates the tilt in the short-term postfilter (4.2.3)
>  > >
>  > > > + * \param ctx private data structure
>  > >  > + * \param lp_gn (Q12) coefficients of A(z/GAMMA_N) filter
>  > >  > + * \param lp_gd (Q12) coefficients of A(z/GAMMA_D) filter
>  > >  > + * \param res_pst [in/out] (Q0) residual signal (partially filtered)
>  > >  > +*/
>  > >
>  > > > +static void g729a_tilt_compensation(G729A_Context *ctx, const int16_t *lp_gn, const int16_t *lp_gd, int16_t* res_pst)
>  > >  > +{
>  > >  > +    int tmp;
>  > >  > +    int gt;      // Q12
>  > >  > +    int rh1,rh0; // Q12
>  > >  > +    int16_t hf_buf[11+22]; // Q12 A(Z/GAMMA_N)/A(z/GAMMA_D) filter impulse response
>  > >  > +    int sum;
>  > >  > +    int i, n;
>  > >  > +
>  > >  > +    memset(hf_buf, 0, 33 * sizeof(int16_t));
>  > >  > +
>  > >
>  > >  > +    hf_buf[10] = 4096; //1.0 in Q12
>  > >
>  > > > +    for(i=0; i<10; i++)
>  > >  > +        hf_buf[i+11] = lp_gn[i];
>  > >  > +
>  > >  > +    /* Apply 1/A(z/GAMMA_D) filter to hf */
>  > >  > +    for(n=0; n<22; n++)
>  > >  > +    {
>  > >  > +        sum=hf_buf[n+10];
>  > >
>  > > > +        for(i=0; i<10; i++)
>  > >  > +            sum -= (lp_gd[i] * hf_buf[n+10-i-1]) >> 12;
>  > >  > +        hf_buf[n+10] = sum;
>  > >  > +    }
>  > >
>  > >  for(n=0; n<22; n++){
>  > >     sum= lp_gn[n];
>  > >
>  > >     for(i=0; i<10; i++)
>  > >         sum -= (lp_gd[i] * hf_buf[n+10-i-1]) >> 12;
>  > >     hf_buf[n+10] = sum;
>  > >  }
>  >
>  > Wrong. Out of bounds in lp_gn
>  > Or you mean extend lp_gn to 22 items ?
>  > I don't see here any advantage.
>
>  less memsetting and copying IIRC

with increased complexity of code and much worse readability, IMHO
Keeping as is, until i implement clean solution (if so)

>  > >  also pass the arrays not the context as parameters, this is cleaner IMHO
>  > >  and makes it more clear what is changed
>  >
>  > when i pass lq_prev parameter as "int16**" program segfaults.
>  > Perhaps int[][] array can not be passed as int**.
>
>  learn C or learn how to use cundecl :)
>  a 2 dimensional array is not the same as a array of pointers

:) Done.
Now context is used only in decode_frame, decode_frame_internal and
decoder_init routines.

>  > >  besides the very ugly misalignment of the parameters
>  > >  it would be MUCH cleaner to look up all values outside, not look up the first 2
>  > >  outside and the next 2 inside the function
>  >
>  > 1. I used 8-spaces shift, what would be better ?
>
>  bad:
>
> ctx->gain_code = g729_get_gain_code(parm->ga_cb_index[i],
>         parm->gb_cb_index[i],
>
>  good:
>
> ctx->gain_code = g729_get_gain_code(parm->ga_cb_index[i],
>                                     parm->gb_cb_index[i],
>
>  also good:
>
> ctx->gain_code = g729_get_gain_code(
>         parm->ga_cb_index[i],
>         parm->gb_cb_index[i],
>

I like this one (with 8 spaces shift to differ it from {} blocks shift)

> > 2. What "first 2" and "next 2" values do you mean ?
>
>  IIRC
>  first  2: cb_GA[parm->ga_cb_index[i]][0], cb_GB[parm->gb_cb_index[i]][0]
>  second 2: cb_GA[parm->ga_cb_index[i]][1], cb_GB[parm->gb_cb_index[i]][1]
>
>  its weirdly split into part before and part in the function.

Fixed. Moved second one outside the routine.
And gain correction became obvious ;)

-- 
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
A non-text attachment was scrubbed...
Name: common_12.diff
Type: text/x-diff
Size: 874 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080326/26aa85d3/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: g729a_12.diff
Type: text/x-diff
Size: 74305 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080326/26aa85d3/attachment-0001.diff>