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

[Michael Niedermayer]

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

[...]

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


[...]
> >  > +    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)?


> 
> >  [...]
> >  > +    /*
> >  > +       First pass: searching the best T0 (pitch delay)
> >  > +       Second pass is not used in G.729A: fractional part is always zero
> >  > +    */
> >  > +    k=minT0;
> >  > +    corellation=0;
> >  > +    /* 4.2.1, Equation 80 */
> >
> > > +    for(n=0; n<ctx->subframe_size; n++)
> >  > +        corellation+=ctx->residual[n+PITCH_MAX]*ctx->residual[n+PITCH_MAX-k];
> >  > +
> >  > +    corr_max=corellation;
> >  > +    intT0=k;
> >  > +
> >  > +    for(; k<=maxT0; k++)
> >  > +    {
> >  > +        corellation=0;
> >  > +        /* 4.2.1, Equation 80 */
> >
> > > +        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;
> >  > +        }
> >  > +    }
> >
> >  the first loop does nothing, remove it
> 
> Hm. Not sure.
> This is not sum of squeres, thus can be negative.
> replacing "intT0=k" with "intT0=k++" before loop will avoid calculating
> the same value twice.

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;
    }
}



> 
> >  > +    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)


> 
> >  > +    /* 3.2.6, Equation 25*/
> >  > +    for(i=0;i<5;i++)
> >  > +    {
> >  > +        ff1[i]=f1[i+1]+f1[i];
> >  > +        ff2[i]=f2[i+1]-f2[i];
> >  > +    }
> >  > +
> >  > +    /* 3.2.6, Equation 26*/
> >  > +    for(i=0;i<5;i++)
> >  > +    {
> >  > +        a[i]=(ff1[i]+ff2[i])/2;
> >  > +        a[i+5]=(ff1[4-i]-ff2[4-i])/2;
> >  > +    }
> >
> >  /* 3.2.6, Equation 25 and 26 */
> >  for(i=0;i<5;i++){
> >     a[  i]=
> >     a[9-i]= (f1[i+1] + f1[i] - f2[i+1] + f2[i])*0.5;
> >  }
> 
> Wrong. You mislooked +/- in Equation 26

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;
}


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

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

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- 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/20080227/71a7cd94/attachment.pgp>