[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