[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