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

[Michael Niedermayer]

On Mon, Apr 14, 2008 at 06:56:34AM +0700, Vladimir Voroshilov wrote:
> On Mon, Apr 14, 2008 at 5:35 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Wed, Mar 26, 2008 at 11:23:04PM +0600, Vladimir Voroshilov wrote:
> 
> [...]
> 
> >  > 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.
> 
> Something like that for l_add:
> "Using 64bit arithmetic to avoid overflow in 32bit domain before
> clipping. This also can be done in 32bit arithmetic but with a bit
> more complex code."
> 
> Or you meant something else?

I meant

/* green has can be within 123-456 see section 1.2.3 this through the gray
   transform can cause blue+red to be larger than 32bit and need cliping
   we thus need to use a additon with cliping here.
   see test vector abcd.g729 as example where this occurs */
blah= l_add(blue, red);


> 
> >  >
> >  > 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.
> 
> Well... While waiting for review, i've started implementing G.729D
> (G.729 @6.4kHz)
> and now about 80% is finished (only huge long-term postfilter remaining).
> After a week or two work should be completed.

great.


> As an additional advantage there will be full G.729 decoder (instead
> of simplified G.729A).
> and (as separate patch) standard quality VOC demuxer/decoder (media
> format with "VCP162_VOC_File" tag in header).
> 
> Due to complexity, post filtering code will require several review roundup.
> Several routines were also changes to avoid code duplication.
> If you wish, i can post current code here to start review process as
> soon as possible and allow fixing issues in common code in parallel
> with finishing post filter.

Feel free to post, but please try tp split the file/code/patch, 100k is too
large (and adding more things will not make it smaller ...)
Review will be faster if its in smaller chunks.


[...]
> 
> >  > +#define SHARP_MAX 13017 //0.8 in Q14
> >
> >  typo, 0.8 is 13107 not 13017
> 
> Surprise! This is not typo (at least not mine). This constant (as long
> as comment) was got from reference code. Replacing 13017 with 13107
> breaks bit-exact result.

Well i suspect it is a typo in the spec nothing we can do i guess
the float reference uses 0.7945 as well ...


> 
> >  > +/**
> >  > + * \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
> 
> So i should replace this routine with appropriate  in-place checks
> (for shift sign and clipping), rigth? Note, that main goal of this
> routine is clipping. Having negative shift check just makes code a bit
> shorter.
> What about having splitting code into l_shl without shift sign check
> and l_shift (calling l_shl internally) with it ? And use l_shift only
> when necessary.

Do what is cleanest and fastest :)


> 
> >  > +/**
> >  > + * \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
> 
> Hm. bit-exact test passed. i'll remove both.

:)


> 
> > > +/**
> >  > + * \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
> 
> What about having separated #define Q15_MIN/MAX, Q12_MIN/MAX instead ?

ok

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

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- 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/6463e640/attachment.pgp>