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

Michael Niedermayer michaelni
Mon Apr 14 00:35:43 CEST 2008


On Wed, Mar 26, 2008 at 11:23:04PM +0600, Vladimir Voroshilov wrote:
> 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 ?

no, but we need a proof for each individual use of them that the cliping
is needed. If you do not have a file which needs it and cant provide valid
input values that need the cliping then it has to be removed or replaced
by assert()
Also i would like to see some comments for each int64 and clip which
explains under what circumstances they are needed and which of the test
vectors needs it.


> 
> 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.

Review below is just partial, i thought its better than letting you wait
until GSOC qualifications end. Ill look at your code again after the
clip/int64 stuff has been cleaned up.


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

Hmm, have you read the development policy and patch checklist for ffmpeg?
Code submitted must be optimal, i would not hesitate to reject the whole
patch because it does a unneeded copy or memset somewhere.
Thats also why iam a little annoyed by the clip() and int64_t. They are
slow unles the compiler is smart (= not gcc) and they smell avoidable.


[...]
> diff --git a/libavutil/common.h b/libavutil/common.h
> index ce017b0..7c9f1de 100644
> --- a/libavutil/common.h
> +++ b/libavutil/common.h
> @@ -87,6 +87,7 @@
>  #define FFSIGN(a) ((a) > 0 ? 1 : -1)
>  
>  #define FFMAX(a,b) ((a) > (b) ? (a) : (b))
> +#define FFMAX3(a,b,c) FFMAX(FFMAX(a,b),c)
>  #define FFMIN(a,b) ((a) > (b) ? (b) : (a))
>  
>  #define FFSWAP(type,a,b) do{type SWAP_tmp= b; b= a; a= SWAP_tmp;}while(0)

this hunk is ok and can be commited


[...]
> +//Stability constants (3.2.4)
> +#define LSFQ_MIN    40 //0.005 in Q13
> +#define LSFQ_MAX 25681 //3.135 in Q13
> +
> +#define LSFQ_DIFF_MIN 321 //0.0391 in Q13
> +
> +/* Gain pitch maximum and minimum (3.8) */
> +#define SHARP_MIN  3277 //0.2 in Q14

doxygen incompatible comments ...


> +#define SHARP_MAX 13017 //0.8 in Q14

typo, 0.8 is 13107 not 13017

[...]
> +/**
> + * \brief shift to left with check for overflow
> + * \param value shifted value
> + * \param shift size of shifting (can be negative)
> + *
> + * \return value shifted to left by given shift value and clipped to [INT_MIN; INT_MAX]
> + *
> + * \remark if shift is negative, routine will do right
> + *         shift by absolute value of given parameter
> + */
> +static int l_shl(int value, int shift)
> +{
> +    if(shift < 0)
> +        return value >> -shift;
> +
> +    if(value > (INT_MAX >> shift))
> +        return INT_MAX;
> +
> +    if(value < (INT_MIN >> shift))
> +        return INT_MIN;
> +
> +    return value << shift;
> +}

many uses of this always have value>=0 many can only have shift being >=0
thus this is not optimal


> +
> +/**
> + * \brief divides one 16-bit integer by another
> + * \param num numenator
> + * \param denom denominator
> + *
> + * \return result of num/denom in Q15
> + *
> + * \note num and denom has to be positive and
> + *       denom must be greater or equal to num
> + *
> + * \note if num == denom reoutine returns SHRT_MAX
> + */
> +static int16_t div_int16(int16_t num, int16_t denom)
> +{
> +    assert(denom>=num && num>=0);
> +
> +    if (!num)
> +        return 0;
> +
> +    if (num == denom)
> +        return SHRT_MAX;
> +
> +    return (num << 15)/denom;
> +}

SHRT_MAX totally is wrong here
also the if(!num) test is redundant


[...]
> +/**
> + * \brief Calculates sum of array elements multiplications
> + * \param speech array with input data
> + * \param cycles number elements to proceed
> + * \param offset offset for calculation sum of s[i]*s[i+offset]
> + * \param shift right shift by this value will be done before multiplication
> + *
> + * \return sum of multiplications
> + *
> + * \note array must be at least length+offset long!
> + */
> +static int sum_of_squares(const int16_t* speech, int cycles, int offset, int shift)
> +{
> +    int n;
> +    int sum = 0;
> +
> +    for(n=0; n<cycles; n++)
> +       sum = l_mac(sum, speech[n] >> shift, speech[n + offset] >> shift);
> +
> +    return av_clip(sum, INT_MIN >> 1, INT_MAX >> 1);
> +}

all uses of INT_MAX/MIN are likely wrong in your code there is no reason why
INT_MAX couldnt be 1<<60 which is not what your code expects


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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/20080414/3a32ddef/attachment.pgp>



More information about the ffmpeg-devel mailing list