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)
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 >