[CELT-dev] bug found in CELT 0.6.1, fix proposed.

[Jean-Marc Valin]

Hi,

George de Vries wrote:
> At these settings nbEBands = 25.  However the eMeans[] array as
> defined in the top of  quant_bands.c and used in
> quant_coarse_energy() and unquant_coarse_energy() has only 24
> members.  Using the settings above, the code illegally reads the 25th
> member from random memory.  If this memory is 0 as in the x86
> implementations tested then there is no problem, but on MIPS it is
> not zero.

Oops, you indeed found a bug. It turns out that it was introduced in
0.6.1 because I added support for frame sizes up to 1024 (512 and below
are *not* affected). I'll fix that ASAP (probably with the fix you propose).

Note that in general I would recommend using frames of 512 instead of
1024 as there are still some quality issues with 1024 (it's a bit on the
experimental side).

> I am very impressed with the quality of the audio that the codec has
> produced so far.  The CPU usage is a little higher that I would like
> on the MIPS devices, but we may be able to tweak some of our other
> code to allow its use within the CPU budget of the device.

Did you set the complexity to 1? Depending on the configuration, that
may speed things up a bit. Also, any particular function eating lots of
CPU in your configuration?

Cheers,

	Jean-Marc

> Regards George de Vries. Senior Software Engineer Open Access.
> 
> 
> 
> *** libcelt/quant_bands.c	Wed Dec  2 10:23:42 2009 ---
> libcelt/orig/quant_bands.c	Wed Oct 21 14:43:10 2009 *************** 
> *** 42,52 **** #include "mathops.h" #include "stack_alloc.h"
> 
> - #define E_MEANS_SIZE (5) #ifdef FIXED_POINT ! const celt_word16
> eMeans[E_MEANS_SIZE] = {1920, -341, -512, -107, 43}; #else ! const
> celt_word16 eMeans[E_MEANS_SIZE] = {7.5f, -1.33f, -2.f, -0.42f,
> 0.17f}; #endif
> 
> /* FIXME: Implement for stereo */ --- 42,51 ---- #include "mathops.h"
>  #include "stack_alloc.h"
> 
> #ifdef FIXED_POINT ! const celt_word16 eMeans[24] = {1920, -341,
> -512, -107, 43, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> 0}; #else ! const celt_word16 eMeans[24] = {7.5f, -1.33f, -2.f,
> -0.42f, 0.17f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f,
> 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f, 0.f}; #endif
> 
> /* FIXME: Implement for stereo */ *************** *** 113,119 **** 
> celt_word16 q;   /* dB */ celt_word16 x;   /* dB */ celt_word16 f;
> /* Q8 */ !          celt_word16 mean =  (i < E_MEANS_SIZE) ?
> MULT16_16_Q15(Q15ONE-coef,eMeans[i]) : 0; x =
> eBands[i+c*m->nbEBands]; #ifdef FIXED_POINT f = x-mean
> -MULT16_16_Q15(coef,oldEBands[i+c*m->nbEBands])-prev[c]; --- 112,118
> ---- celt_word16 q;   /* dB */ celt_word16 x;   /* dB */ celt_word16
> f;   /* Q8 */ !          celt_word16 mean =
> MULT16_16_Q15(Q15ONE-coef,eMeans[i]); x = eBands[i+c*m->nbEBands]; 
> #ifdef FIXED_POINT f = x-mean
> -MULT16_16_Q15(coef,oldEBands[i+c*m->nbEBands])-prev[c]; 
> *************** *** 242,248 **** do { int qi; celt_word16 q; !
> celt_word16 mean =  (i < E_MEANS_SIZE) ?
> MULT16_16_Q15(Q15ONE-coef,eMeans[i]) : 0; /* If we didn't have enough
> bits to encode all the energy, just assume something safe. We allow
> slightly busting the budget here */ if (ec_dec_tell(dec, 0) > budget)
>  --- 241,247 ---- do { int qi; celt_word16 q; !          celt_word16
> mean = MULT16_16_Q15(Q15ONE-coef,eMeans[i]); /* If we didn't have
> enough bits to encode all the energy, just assume something safe. We
> allow slightly busting the budget here */ if (ec_dec_tell(dec, 0) >
> budget) _______________________________________________ celt-dev
> mailing list celt-dev at xiph.org 
> http://lists.xiph.org/mailman/listinfo/celt-dev
>