[FFmpeg-devel] [PATCH] G.729A (now fixed-point) decoder
Michael Niedermayer
michaelni
Wed Mar 26 00:45:36 CET 2008
On Tue, Mar 25, 2008 at 06:54:58PM +0600, Vladimir Voroshilov wrote:
> Hi, Michael
>
> On Mon, Mar 24, 2008 at 1:00 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > On Thu, Mar 20, 2008 at 10:12:54PM +0600, Vladimir Voroshilov wrote:
> > > Hi, Michael.
> > >
> > > On Thu, Mar 20, 2008 at 8:29 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > > On Sun, Mar 16, 2008 at 12:24:58AM +0600, Vladimir Voroshilov wrote:
> > > >
> > > > > +Per-vector results (PASS means no hearable differences, FAILED means hearable artefacts):
> > > > > +algthm : PASS
> > > > > +erasure : PASS
> > > > > +fixed : PASS
> > > > > +lsp : PASS
> > > >
> > > > > +overflow: FAIL
> > > >
> > > > fix it!
> > >
> > > "FAIL" here means 39 PSNR vs 65-88 in other tests.
> > > Searching for error still in progress, though.
> >
> > just keep in mind this has to be fixed before it will hit svn!
> > Also i like to have binary identical output or an explanation why precissely
> > it is not.
>
> Current code in my local tree is bit-exact :)
> All overflows were found and fixed.
:))
>
>
> > > + int subframe_idx; ///< subframe index (for debugging)
> >
> > unused remove it!
>
> Will be removed in next patch. I'll use md5 sums from now.
>
> > > +/* 4.2.2 */
> > > +#define GAMMA_N 18022 //0.55 in Q15
> > > +#define GAMMA_D 22938 //0.70 in Q15
> >
> > s/N/NUM/
> > s/D/DEN/
> > s/GAMMA/FORMANT_PP_FACTOR/
>
> FORMANT_PP_FACTOR_NUM
> FORMANT_PP_FACTOR_DEN
> ?
yes
>
> What about GAMMA_P ;) ?
i hate all the gammas in there ...
>
> > [...]
> > > +/**
> > > + * GA codebook (3.9.2)
> > > + */
> >
> > Do you know what GA stands for? hint: gain
> > Write it in the comment!
>
> "GA codebook "->"Gain codebook (first stage)"
> "GB codebook "->"Gain codebook (second stage)"
ok
>
> and
>
> cb_GA->cb_gain_1st
> cb->GB->cb_gain_2nd
if you prefer ...
it could also be simply gain_cb[2]
[...]
>
> > [...]
> > > +/**
> > > + * \brief divide two fixed point numbers
> >
> > > + * \param num numenator
> > > + * \param denom denumenator
> > > + * \param base base to scale result to
> > > + *
> > > + * \return result of division scaled to given base
> > > + */
> > > +int l_div(int num, int denom, uint8_t base)
> > > +{
>
> > > [...]
> >
> > you can also get rid of the >> 31
> > and use if(sig<0)
> >
> > and the assert(denom) is useless, it will kill the prog with a div by
> > zero anyway, this isnt float stuff with infinity.
> >
>
> l_div is gone away and was replaced with "int
> div_int16(int16_t,int16_t)" similar to
> {
> return ((int32)num)<<15)/denom;
> }
> which uses two positive numbers (num<denom) as input.
you dont need the cast to int32
[...]
> > [...]
> > > +
> > > +/**
> > > + * \brief Decoding of the adaptive codebook gain (4.1.5 and 3.9.1)
> >
> > > + * \param ctx private data structure
> > > + * \param ga_cb_index GA gain codebook index (stage 2)
> > > + * \param gb_cb_index GB gain codebook (stage 2)
> >
> > > + * \param fc_v (Q13) fixed-codebook vector
> > > + * \param pred_energ_q [in/out] (Q10) past quantized energies
> >
> > > + * \param subframe_size length of subframe
> > > + *
> > > + * \return (Q1) quantized adaptive-codebook gain (gain code)
> > > + */
> > > +static int16_t g729_get_gain_code(int ga_cb_index, int gb_cb_index, const int16_t* fc_v, int16_t* pred_energ_q, int subframe_size)
> > > +{
> >
> > This predicts the gain and then "adds" the correction from the bitstream.
> > maybe that should be split in 2 functions
>
> Do you mean code under after "shift prediction error vector" ?
> this is hard to move that 3 lines out of this routine.
> There is no other "adds" in this routine.
IIRC i meant what the reference did, but i dont remember exactly ATM
I somehow felt that spliting this would allow maybe some code in the
middle to be factored out as that was similar in the error concealment case
and that the splited code could be half used in an encoder ...
just leave it if what i said makes no sense ...
ill look at it again in the next iteration of the review
[...]
>
> > > +/**
> > > + * \brief Calculates coefficients of weighted A(z/GAMMA) filter
> > > + * \param Az (Q12) source filter
> > > + * \param gamma (Q15) weight coefficients
> > > + * \param Azg [out] (Q12) resulted weighted A(z/GAMMA) filter
> > > + *
> > > + * Azg[i]=GAMMA^i*Az[i] , i=0..subframe_size
> > > + */
> > > +static void g729a_weighted_filter(const int16_t* Az, int16_t gamma, int16_t *Azg)
> >
> > What is gamma? This is not a greek book, gamma identifies a letter. But this
> > clearly is not about that letter.
>
> factor and "weight factor of postfiltering". Ok?
hmm
s/Azg/out/
s/Az/in/
s/gamma/weight/
not sure if this is perfect but it is better ...
>
> > > +/**
> > > + * \brief compensates the tilt in the short-term postfilter (4.2.3)
> >
> > > + * \param ctx private data structure
> > > + * \param lp_gn (Q12) coefficients of A(z/GAMMA_N) filter
> > > + * \param lp_gd (Q12) coefficients of A(z/GAMMA_D) filter
> > > + * \param res_pst [in/out] (Q0) residual signal (partially filtered)
> > > +*/
> >
> > > +static void g729a_tilt_compensation(G729A_Context *ctx, const int16_t *lp_gn, const int16_t *lp_gd, int16_t* res_pst)
> > > +{
> > > + int tmp;
> > > + int gt; // Q12
> > > + int rh1,rh0; // Q12
> > > + int16_t hf_buf[11+22]; // Q12 A(Z/GAMMA_N)/A(z/GAMMA_D) filter impulse response
> > > + int sum;
> > > + int i, n;
> > > +
> > > + memset(hf_buf, 0, 33 * sizeof(int16_t));
> > > +
> >
> > > + hf_buf[10] = 4096; //1.0 in Q12
> >
> > > + for(i=0; i<10; i++)
> > > + hf_buf[i+11] = lp_gn[i];
> > > +
> > > + /* Apply 1/A(z/GAMMA_D) filter to hf */
> > > + for(n=0; n<22; n++)
> > > + {
> > > + sum=hf_buf[n+10];
> >
> > > + for(i=0; i<10; i++)
> > > + sum -= (lp_gd[i] * hf_buf[n+10-i-1]) >> 12;
> > > + hf_buf[n+10] = sum;
> > > + }
> >
> > for(n=0; n<22; n++){
> > sum= lp_gn[n];
> >
> > for(i=0; i<10; i++)
> > sum -= (lp_gd[i] * hf_buf[n+10-i-1]) >> 12;
> > hf_buf[n+10] = sum;
> > }
>
> Wrong. Out of bounds in lp_gn
> Or you mean extend lp_gn to 22 items ?
> I don't see here any advantage.
less memsetting and copying IIRC
>
> > [...]
> > > +/**
> > > + * \brief save quantized LSF coefficients for using in next frame
> > > + * \param ctx context structure
> > > + * \param lq quantized LSF coefficients
> > > + */
> > > +static void g729_lq_rotate(G729A_Context *ctx, int* lq)
> > > +{
> > > + int i, k;
> >
> > > +
> >
> > > + /* Rotate lq_prev */
> > > + for(i=0; i<10; i++)
> > > + {
> > > + for(k=MA_NP-1; k>0; k--)
> > > + ctx->lq_prev[k][i] = ctx->lq_prev[k-1][i];
> > > +
> > > + ctx->lq_prev[0][i] = lq[i];
> > > + }
> >
> > reorder and use memcpy()
>
> done
>
> > also pass the arrays not the context as parameters, this is cleaner IMHO
> > and makes it more clear what is changed
>
> when i pass lq_prev parameter as "int16**" program segfaults.
> Perhaps int[][] array can not be passed as int**.
learn C or learn how to use cundecl :)
a 2 dimensional array is not the same as a array of pointers
>
> > > + if(!i)
> > > + {
> > > + // Decoding of the adaptive-codebook vector delay for first subframe (4.1.3)
> > > + if(ctx->bad_pitch || ctx->data_error)
> > > + pitch_delay_3x = 3 * ctx->pitch_delay_int_prev + 1;
> > > + else
> > > + {
> >
> > > + if(parm->ac_index[i] >= 197)
> > > + pitch_delay_3x = 3 * parm->ac_index[i] - 335;
> > > + else
> > > + pitch_delay_3x = parm->ac_index[i] + 59;
> >
> > not sure but somehow the follow looks nicer to me
> >
> > pitch_delay_3x = parm->ac_index[i] + 59;
> > if(pitch_delay_3x > 255)
> > pitch_delay_3x = 3 * pitch_delay_3x - 512;
>
> Cool!
>
> > [...]
> > > + g729_update_gain_erasure(ctx->pred_energ_q);
> > > + }
> > > + else
> > > + {
> > > + // Decoding of the fixed codebook gain (4.1.5 and 3.9.1)
> > > + ctx->gain_pitch = cb_GA[parm->ga_cb_index[i]][0] + cb_GB[parm->gb_cb_index[i]][0];
> > > +
> > > + ctx->gain_code = g729_get_gain_code(parm->ga_cb_index[i],
> > > + parm->gb_cb_index[i],
> >
> > besides the very ugly misalignment of the parameters
> > it woudl be MUCH cleaer to look up all values outside, not look up the first 2
> > outside and the next 2 inside the function
>
> 1. I used 8-spaces shift, what would be better ?
bad:
ctx->gain_code = g729_get_gain_code(parm->ga_cb_index[i],
parm->gb_cb_index[i],
good:
ctx->gain_code = g729_get_gain_code(parm->ga_cb_index[i],
parm->gb_cb_index[i],
also good:
ctx->gain_code = g729_get_gain_code(
parm->ga_cb_index[i],
parm->gb_cb_index[i],
> 2. What "first 2" and "next 2" values do you mean ?
IIRC
first 2: cb_GA[parm->ga_cb_index[i]][0], cb_GB[parm->gb_cb_index[i]][0]
second 2: cb_GA[parm->ga_cb_index[i]][1], cb_GB[parm->gb_cb_index[i]][1]
its weirdly split into part before and part in the function.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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/20080326/ca1394af/attachment.pgp>
More information about the ffmpeg-devel
mailing list