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

Vladimir Voroshilov voroshil
Thu Feb 28 20:36:01 CET 2008


Hi, Michael

Let's start second roundup of patch cleanup procedure.
I want to collect issues for fixing at weekend :)


On Thu, Feb 28, 2008 at 3:13 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Thu, Feb 28, 2008 at 01:10:46AM +0600, Vladimir Voroshilov wrote:
>  [...]
>
> > > > +/**
>  > >  > + * L1 codebook (10-dimensional, with 128 entries (3.2.4)
>  > >  > + */
>  > >  > +static const float cb_L1[128][10] = {
>  > > >
>  > > > [...]
>  > > >
>  > >
>  > >  *10000 and it fits in uint16_t
>  >
>  > This and tables below replaced with fixed-point tables from AnnexA
>  > reference code but this decreased PSNR significantly.
>  > Multiplying by 10000 did not decrease  PSNR, though
>
>  Hmm, then use the *10000 tables for now ...

Well... :) i've found error. Real PSNR decrease after changing tables
is about 0.8 (except OVERFLOW test which gives about 3.21) comparing
with AnnexC.

>  [...]
>
>
>  > >  > +
>  > >  > +/**
>  > >  > + * \brief Decoding of the adaptive-codebook vector delay for first subframe (4.1.3)
>  > >  > + * \param ctx private data structure
>  > >  > + * \param P1 Pitch delay first subframe
>  > >  > + * \param intT [out] integer part of delay
>  > >  > + * \param frac [out] fractional part of delay [-1, 0, 1]
>  > >  > + */
>  > >  > +static void g729_decode_ac_delay_subframe1(G729A_Context* ctx, int P1, int* intT, int* frac)
>  > >  > +{
>  > >  > +    /* if no parity error */
>  > >  > +    if(!ctx->bad_pitch)
>  > >  > +    {
>  > >  > +        if(P1<197)
>  > >  > +        {
>  > >  > +            *intT=1.0*(P1+2)/3+19;
>  > >  > +            *frac=P1-3*(*intT)+58;
>  > >
>  > >  remove the float (1.0) use proper / and %
>  [...]
>
> > But i don't see how return these two values via "return"
>  > (packing them into one "int" will be small overhead. imho)
>
>  You split P1 into int and frac in the code above. Just return P1
>  with proper offset and scale and do the /3 %3 split outside.

Done. Routines became obviously simpler.

>  [...]
>
> > >  > +    if(!gain_after)
>  > >  > +        return;
>  > >
>  > >  testing floats for equallity is risky ...
>  >
>  > How can i avoid division by zero in this case?
>
>  Well, what does the spec say (no i didnt check)?

Spec says nothing.
Reference code (both AnnexA and AnnexC uses the same check).

>  corr_max= INT_MIN;
>
>  for(k=minT0; k<=maxT0; k++){
>     float corellation=0;
>
>
>     for(n=0; n<ctx->subframe_size; n++)
>         corellation+=ctx->residual[n+PITCH_MAX]*ctx->residual[n+PITCH_MAX-k];
>     if(corellation>corr_max){
>         corr_max=corellation;
>         intT0=k;
>     }
>  }

done with:

corr_max=INT_MIN;
intT0=minT0;

to avoid compilation warning

> > >  > +    for(i=0; i<ctx->subframe_size; i++)
>  > >  > +    {
>  > >
>  > >  > +        tmp_speech[i] = FFMIN(tmp_speech[i],32767.0);
>  > >  > +        tmp_speech[i] = FFMAX(tmp_speech[i],-32768.0);
>  > >  > +        speech[i]=g729_round(tmp_speech[i]);
>  > >
>  > >  lrintf() or float_to_int16()
>  >
>  > even simple assignment will be enough.
>  > +/-1 on resulted PCM sample will not hurt signal, imho.
>
>  It is not because of accuracy but speed, simple assignment requires the
>  floating poit unit to be reconfigured to a different rounding mode and
>  back to the original. (for 387 based ones at least)

lrintf added

>  darn signs ;)
>  lets see how many typos the following contains ...
>
>  for(i=0;i<5;i++){
>     float ff1= f1[i+1] + f1[i];
>     float ff2= f2[i+1] - f2[i];
>     a[  i]=(ff1 + ff2)/2;
>     a[9-i]=(ff1 - ff2)/2;
>
> }

This is better :)

>  > >  [...]
>  > >  > +/**
>  > >  > + * \brief decode one G.729 frame into PCM samples
>  > >  > + * \param serial array if bits (0x81 - 1, 0x7F -0)
>  > >  > + * \param serial_size number of items in array
>  > >  > + * \param out_frame array for output PCM samples
>  > >  > + * \param out_frame_size maximum number of elements in output array
>  > >  > + */
>  > >  > +static int  g729a_decode_frame_internal(void* context, short* out_frame, int out_frame_size, int *parm)
>  > >  > +{
>  > >
>  > >  there is no need for g729a_ prefixes
>  >
>  > Is this related to *_decode_* *_init *close (i.e. non-g729 specific)
>  > routines only ?
>
>  no static identifer in g729a.c needs a g729a_ prefix

I would like to keep prefixes.
The goal was to use g729a_ prefix for AnnexA specific code and g729_
prefix for G.729
common code. Code not directly related to G.729 should not have any
special prefix.
This should help adding G.729 (not AnnexA) in future.


-- 
Regards,
Vladimir Voroshilov     mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: g729a_04.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080229/bf666a86/attachment.txt>



More information about the ffmpeg-devel mailing list