[FFmpeg-devel] [PATCH] G.729A (floating-point) decoder and ACT demuxer
Vladimir Voroshilov
voroshil
Sun Feb 24 16:39:21 CET 2008
Hi, Reimar
this is reply on g729a decoder related issues.
On Sun, Feb 24, 2008 at 12:39 AM, Reimar D?ffinger
<Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
> [...]
> > +/**
> > + * L1 codebook (10-dimensional, with 128 entries (3.2.4)
> > + */
> > +static const float cb_L1[128][10] = {
>
> These constants btw. look very much like they were designed with a
> 16-bit fixed-point implementation in mind...
L1 L2 L3 - are three different codebooks mentioned in specification.
Exact values are not described anywhere (or i was not searching carefully),
current values are got from floating-point reference code.
So if they are designed with 16-bit fixed-point implementation in mind
this is not
my fault :)
> > +/**
> > + * \brief rouding function from reference code
>
> rouNding
This...
> > + * FIXME: found system replacement for it
>
> You mean "find" instead of "found"?
...this...
> > +static inline int g729_round(float f)
> > +{
> > + int i=ceil(f*2);
> > + return i>>1;
> > +}
>
> Uh, is that just ordinary "round to nearest", which probably is the
> default for most FPUs anyway?
...and this were fixed by removing rounding routine.
Was used in early code for maximum comply with fixed-point code's result.
In current code rounding are beeing done only at final stage. And +/-1
in resulted PCM signal is not issue, imfo.
> > +/**
> > + * \brief pseudo random number generator
> > + */
> > +static inline uint16_t g729_random(G729A_Context* ctx)
> > +{
> > + return ctx->rand_seed = (uint16_t)(31821 * (uint32_t)ctx->rand_seed + 13849 + ctx->rand_seed);
>
> AFAICT rand_seed can not be negative, so why not just make rand_seed
> unsigned instead of int and:
> return ctx->rand_seed = 31822*ctx->rand_seed + 13849;
Fixed as suggested, moreover previous implementation was wrong.
> > +/**
> > + * \brief Check parity bit (3.7.2)
> > + * \param P1 Pitch delay first subframe
> > + * \param P0 Parity bit for Pitch delay
> > + *
> > + * \return 1 if parity check is ok, 0 - otherwise
> > + */
> > +int g729_parity_check(int P1, int P0)
> > +{
> > + int P=P1>>2;
> > + int S=P0&1;
> > + int i;
> > +
> > + for(i=0; i<6; i++)
> > + {
> > + S ^= P&1;
> > + P >>= 1;
> > + }
> > + S ^= 1;
> > + return (!S);
> > +}
>
> I am fairly certain that we already have a function for that, since I
> have seen a much less naive implementation...
> That may have been in MPlayer though, so the idea is:
> P1 >>= 1;
> P1 = (P1 & ~1) | (P0 & 1);
> P1 ^= P1 >> 4;
> P1 ^= P1 >> 2;
> P1 ^= P1 >> 1;
> return P1 & 1;
I already seen above code somewhere in m/l too.
Fixed as suggested.
> > +/**
> > + * \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;
> > + }
> > + else
> > + {
> > + *intT=P1-112;
> > + *frac=0;
> > + }
> > + }
> > + else{
> > + *intT=ctx->intT2_prev;
> > + *frac=0;
> > + }
> > + ctx->intT1=*intT;
>
> Why returning the value both via ctx and intT?
>
> > + //overflow can occure in 4.4k case
>
> "occur", without "e"
Fixed.
> > +/**
> > + * \brief Decoding of the adaptive and fixed codebook gains from previous subframe (4.4.2)
> > + * \param ctx private data structure
> > + * \param gp pointer to variable receiving quantized fixed-codebook gain (gain pitch)
> > + * \param gc pointer to variable receiving quantized adaptive-codebook gain (gain code)
> > + */
> > +static void g729_get_gain_from_previous(G729A_Context *ctx, float* gp, float* gc)
> > +{
> > + /* 4.4.2, Equation 93 */
> > + *gc=0.98*ctx->gain_code;
> > + ctx->gain_code=*gc;
> > +
> > + /* 4.4.2, Equation 94 */
> > + *gp=FFMIN(0.9*ctx->gain_pitch, 0.9);
> > + ctx->gain_pitch = *gp;
> > +}
>
> Again, why returning the value both ways? I performance is an issue I
> have the feeling there are better optimization-opportunities (not to
> mention that the compiler should inline this anyway).
Will be fixed.
> > +static void g729_update_gain(G729A_Context *ctx)
> > +{
> > + float avg_gain=0;
> > + int i;
> > +
> > + /* 4.4.3. Equation 95 */
> > + for(i=0; i<4; i++)
> > + avg_gain+=ctx->pred_vect_q[i];
> > +
> > + avg_gain = FFMAX(avg_gain * 0.25 - 4.0, -14);
> > +
> > + for(i=3; i>0; i--)
> > + ctx->pred_vect_q[i]=ctx->pred_vect_q[i-1];
>
> Maybe use memmove? Not so sure if it's a good idea.
>
> > +static void g729_lp_synthesis_filter(G729A_Context *ctx, float* lp, float *in, float *out, float *filter_data)
> > +{
> > + float* tmp_buf=av_mallocz((10+ctx->subframe_size)*sizeof(float));
>
> Not exactly an inner loop, but malloc here does not look too nice to me,
> maybe a fixed-sized buffer in the context would do (or does it even fit
> on the stack)?
Replaced with "float tmp_buf[SUBFRAME_MAX_SIZE+10] "
> > +static float g729_get_signal_gain(G729A_Context *ctx, float *speech)
> > +{
> > + int n;
> > + float gain;
> > +
> > + gain=0;
> > + for(n=0; n<ctx->subframe_size; n++)
> > + gain+=speech[n]*speech[n];
>
> > float gain = speech[0] * speech[0];
> > for (n = 1;...
>
> unless ctx->subframe_size can be 0.
subframe_size never can be zero.
Fixed as suggested.
Moreover corr_t0 and corr_0 were using the same 100% exact code.
All duplications were replaced to get_signal_gain call.
> > +static void g729a_weighted_filter(G729A_Context *ctx, float* Az, float gamma, float *Azg)
> > +{
> > + float gamma_tmp;
> > + int n;
> > +
> > + gamma_tmp=gamma;
> > + for(n=0; n<10; n++)
> > + {
> > + Azg[n]=Az[n]*gamma_tmp;
> > + gamma_tmp*=gamma;
> > + }
>
> gamma_pow or so would be more descriptive than gamma_tmp.
Fixed.
> > + float corellation, corr_max;
>
> correlation, 2r, 1l.
Fixed.
All fixes are done in local tree.
Updated patch will be sent later as reply to Michael's mail.
--
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
More information about the ffmpeg-devel
mailing list