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

[George de Vries]

Hi all I have spent the last three days evaluating CELT on our supported platforms.

I found a bug in quant_bands.c, that due to processor/compilation differences did not cause an issue on x86 platforms, but is a problem on the MIPS processor embedded devices. When decoding on the MIPS devices, there was a lot of noise added during the decoding, the noise is mainly in the 15 khz to 21 khz range.  The amount of noise also caused the output to clip numerous times.

The problems where observed when encoding 48kHz at 60kbs using the following parameters:
<rate> 48000
 <channels> 1
<frame size> 1024
<bytes per packet> 160

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.

In case its is of use to anyone, I have included the contents of a patch file at the bottom of the email.  The patched version may be slower on some architectures, but has left the measurable speed of encoding a 24 second file on the MIPS unchanged.

Two more elaborate fixes that I can thick of involve the eMeans[] array being  allocated or reallocated on initialisation or including two loops in quant_coarse_energy() and unquant_coarse_energy().  One loop the first five bands 0 to 4 and the other for bans 5 and above.

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.

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)