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

[Vladimir Voroshilov]

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.

>  [...]
>  > +#define PITCH_MIN 20
>  > +#define PITCH_MAX 143
>  > +#define INTERPOL_LEN 11
>
> > +
>  > +#define L0_BITS 1             ///< Switched MA predictor of LSP quantizer (size in bits)
>  > +#define L1_BITS 7             ///< First stage vector of quantizer (size in bits)
>  > +#define L2_BITS 5             ///< First stage lowervector of quantizer (size in bits)
>
> > +#define L3_BITS 5             ///< First stage hihjer vector of quantizer (size in bits)
>  > +#define P1_BITS 8             ///< Adaptive codebook index for first subframe (size in bits)
>  > +#define P2_BITS 5             ///< Adaptive codebook index for second subframe (size in bits)
>  > +#define P0_BITS 1             ///< Parity bit for pitch delay (size in bits)
>  > +#define GA_BITS 3             ///< GA codebook index (size in bits)
>  > +#define GB_BITS 4             ///< GB codebook index (size in bits)
>  > +#define FC_PULSE_COUNT 4      ///< Number of pulses in fixed-codebook vector
>
>  > +#define FC_BITS(ctx) (formats[ctx->format].fc_index_bits*FC_PULSE_COUNT+1) ///< Fixed codebook index (size in bits)
>
>  I do not like hiding code behind macros when this isnt obvious from the name
>  of the macro.

Macro is named to comply with previous set and used to make code abit
more readable.
Perhaps FC_BITS(ctx->format) will be better than FC_BITS(ctx)


>  [...]
>  > +    int16_t pred_energ_q[4];    ///< (Q10) past quantized energies
>
>  energY

Was undestood and fixed as "(Q10) past quantized energy"

>  [...]
>  > +/**
>
> > + * \brief multiplies 32-bit integer by abother 16-bit and divides result by 2^15
>  > + * \param var_q24 (Q24) 32-bit integer
>  > + * \param var_15 (Q15) 16-bit integer
>  > + * \return (Q24) result of mupliplication
>  > + *
>  > + * \note this code is bit-equal to reference's Mpy_32_16
>  > + */
>  > +static inline int mul_24_15(int var_q24, int16_t var_q15)
>  > +{
>  > +    int hi = var_q24 >> 15;
>  > +    int lo = var_q24 & 0x7fff;
>  > +    return var_q15 * hi + ((var_q15 * lo) >> 15);
>  > +}
>
>  if the following is faster (and equal), please use it:
>  ((int64_t)var_q24 * var_q15) >> 15

Didn't make speed tests, but it gives the same result so fixed as suggested.

>  > +}
>  > +
>  > +/**
>  > + * \brief Computes 1/sqrt(x)
>  > + * \param arg (Q0) positive integer
>  > + *
>  > + * \return (Q29) 0 < 1/sqrt(arg) <= 1
>  > + */
>  > +static int l_inv_sqrt(int arg)
>  > +{
>  > +    uint32_t result;
>  > +    uint16_t frac_x0;
>  > +    uint16_t frac_dx;
>  > +    int8_t power_int;
>  > +
>  > +    assert(arg > 0);
>  > +
>
>  > +    result=arg;
>  > +    for(power_int=16; power_int>=0 && !(result & 0xc0000000); power_int--)
>  > +        result <<= 2;
>
>  av_log2()>>1

Not so simple. But similar.
Fixed.

>  > +/**
>  > + * \brief divide two positive fixed point numbers
>  > + * \param num numenator
>  > + * \param denom denumenator
>  > + * \param base base to scale result to
>  > + *
>  > + * \return result of division scaled to given base
>  > + *
>  > + * \remark numbers should in same base, result will be scaled to given base
>  > + *
>  > + * \todo Better implementation requred
>  > + */
>  > +int l_div(int num, int denom, int base)
>  > +{
>  > +    int diff = 0;
>  > +    int sig = 0;
>  > +
>  > +    assert(denom);
>  > +
>  > +    if(!num)
>  > +        return 0;
>
> > +
>  > +    if(num < 0)
>  > +    {
>  > +        num = -num;
>  > +        sig = !sig;
>  > +    }
>  > +
>  > +    if(denom < 0)
>  > +    {
>  > +        denom = -denom;
>  > +        sig = !sig;
>  > +    }
>
>  "\brief divide two positive fixed point numbers"
>  remove the negative checks please if it divides 2 positive
>  but if you do add a assert(num >= 0) !
>  iam not sure if it really is always positive, iam quite sure denom is though

Wrong (not updated) comment.
Routine was initially written for two positive values and later was
entended to support negative values too.
Code fixed with help of FFABS and av_log2

>  > +
>  > +    // FIXME: Compensation. Makes result bit-equal with reference code
>  > +    energy -= 2;
>
>  hmmmmmmm

Heh. This is result of differences in fixed-point arithmetic.

>  > +
>  > +        lsp[i] = base_cos[ind] + ((slope_cos[ind] * offset) >> 12);
>
>  lsp[i] =      base_cos[freq>>8]
>          + ((slope_cos[freq>>8] * (freq&0xFF)) >> 12);
>
>  also the reference does >> 13 ?

Reference code does one extra "<<1" in L_mac.

All other issues, not mentioned above, were fixed as suggested by you.
Updated version is attached.

-- 
Regards,
Vladimir Voroshilov mailto:voroshil at gmail.com
JID: voroshil at gmail.com, voroshil at jabber.ru
ICQ: 95587719
-------------- next part --------------
A non-text attachment was scrubbed...
Name: g729a_06.diff
Type: application/octet-stream
Size: 68848 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080320/3c6c109a/attachment.obj>