similar to: Compiling for TI cl6x

Hi, Apologies if this is a known issue, but running make on revision e3187444692195957eb66989622c7b1ad8448b06 fails one of the tests when using fixed point configuration (floating point is ok) on my linux x86. Note that libopus1.1, as extracted from the tar ball, is OK. Specifically, the tests that fail are in celt/tests/test_unit_mdct: nfft=32 inverse=0,snr = 85.341197 nfft=32 inverse=1,snr =

Hi Jean Marc, please find attached the audio file (mono 16khz). I shortened it to about 10 seconds. I also add 2 patches that worked for me. Further info that might help: - The problem seems to be related to silk_burg_modified not reaching the maximum gain, so the actual filter order is 16 rather than 2 (which is what would be expected with a sine wave). - The problem seems to happen when

I tracked down the bug to an incorrect use of restrict. I would not consider this a compiler bug: we are lying to the optimizer by telling it that a pointer is restrict when in fact it isn't. This can be fixed like so: diff --git a/celt/mdct.c b/celt/mdct.c index 1634e8e..fa5098c 100644 --- a/celt/mdct.c +++ b/celt/mdct.c @@ -276,8 +276,8 @@ void clt_mdct_backward(const mdct_lookup *l,

Hi Jean-Marc, well spotted! The patch provided fixes the issue for me. Nevertheless, in my code (and I would suggest doing the same in libopus) I am going to replace the function with one that accumulates on 64 bits and then calculates the shift, for at least 4 reasons: - It is less and simpler code - The result is likely to be slightly more accurate in case big numbers come early in the

After a few experiments, I found that both alternatives are very similar, and 2~5% slower compared to the following: diff --git a/celt/mdct.c b/celt/mdct.c index 1634e8e..e490c3b 100644 --- a/celt/mdct.c +++ b/celt/mdct.c @@ -277,7 +277,7 @@ void clt_mdct_backward(const mdct_lookup *l, kiss_fft_scalar *in, kiss_fft_scala it in-place. */ { kiss_fft_scalar * OPUS_RESTRICT yp0 =

Yes those instructions exist, although they're a bit slower than the basic 16x16->32 with 32-bit accumulation (SMLABB). So I'd be surprised if the function with 64 bit accumulation would run as fast as the current code. Don't know how much we care about 16-bit platforms. And accuracy should not matter. On the other hand, a 64-bit implementation is much cleaner/shorter, which is

Hi, Apologies if this is a known issues, but I have found what I believe is a bug in the fixed point implementation of the Silk codec and could not find any mention on this in the archives. The bug can be easily reproduced with the fixed point demo program (./configure ?enable-fixed-point ?disable-float-api && make) using the following command: ./opus_demo voip 16000 1 23000

Hi Marcello, Actually, we were careful to avoid the undefined behaviour here. In fact, we are specifically running a clang test detecting undefined behaviour. If you look at the silk_SMLABB_ovflw() macro, you will see it is based on silk_ADD32_ovflw(), which is defined as: #define silk_ADD32_ovflw(a, b) ((opus_int32)((opus_uint32)(a) + (opus_uint32)(b))) By casting to unsigned, all the cases

Hi Marcello, Thanks for the info and the proposed fixes. I'm currently investigating what's going on here before deciding on the best way to fix the problem. Have you been able to figure out why it doesn't work for rshifts >= 3? Cheers, Jean-Marc On 13/06/14 12:28 PM, Marcello Caramma (mcaramma) wrote: > Hi Jean Marc, > > please find attached the audio file (mono

Hi Marcello, It turns out that the problem has a much simpler explanation. As far as I can tell, it's actually a bug in silk_sum_sqr_shift() and this trivial patch appears to fix the problem: http://jmvalin.ca/misc_stuff/sum_sqr_shift_fix.patch It would still require some testing to check that the fix doesn't have any bad side effect. Let me know how well the fix works for you. Again,

Good catch! I think gcc is just unable to make use of restrict for anything useful. Apparently your compiler does, so could you benchmark both patches below and see if it makes a difference in terms of speed. If it's worth it, I can keep the restrict and add the odd case handling, but if not I'll just remove the restrict. Thanks, Jean-Marc On 21/02/14 05:54 PM, Marcello Caramma

Right, there shouldn't be a problem with undefined behavior. That said, a 64 bit implementation will work very well - in fact that's how it was done originally. The reason for the current implementation is to minimize 64-bit operations in order to improve performance on limited-width architectures. This functions gets used extensively, and I think the current implementation is faster on

> Nice call. The culprit is SHRL32. This is not used in many places, and in > most of those, the operand comes from EXTEND32. The only really suspicious > instances is in lsp.c (lsp_interpolate): > > spx_word16_t tmp = DIV32_16(SHL32(1 + subframe,14),nb_subframes); > (subframe is an int) > ... > I see that your changes include adding an EXTEND32 to the line above.

> For the persistent storage, the only change that I have made is to > MAX_CHARS_PER_FRAME, which is set to 2000 in bits.c. I changed bits.c to > set this value only if it was not already defined, and then put my own, much > smaller value in config.h. Yeah, I think I'll add an option like that. > For the scratch stack, I replace the fixed values in nb_encoder_init and

> I incorporated Stuarts fixed_c55x.h file, and that eliminated the artifacts, > at the expense of a ~30% increase in MIPs. Now the male.wav file through > encoder/decoder produces a bit-exact match with the C64x test that I did > earlier. I will do some more testing to isolate the, but it may be a few > days before I get to this task. As Jean-Marc says, fixed_generic should

>> Nice call. The culprit is SHRL32. This is not used in many places, and >> in >> most of those, the operand comes from EXTEND32. The only really >> suspicious >> instances is in lsp.c (lsp_interpolate): >> >> spx_word16_t tmp = DIV32_16(SHL32(1 + subframe,14),nb_subframes); >> (subframe is an int) >> ... >> I see that your

Jim Crichton a ?crit : > Itay, > > I tested on C6x in May with build 11343 and 11398, and it worked > fine. I just ran the same test with build 11768, and there was no > cancellation, just as you saw. I will try to track this down in the > next day or two. In the meantime, I suggest that you try testing > with 11398. Hmm, that's odd because the fixed-point seems to

For me, this was a memory allocation problem. I am using a private heap, and somewhere between build 11522 and 11700, the allocation for two large buffers was changed from calloc to speex_alloc (I am sure that this was a cleanup change, and I have not had a chance to locate it yet). This was overrunning my heap, and enlarging the heap solved the problem. I suspect that Itay is having a

Jean-Marc, I followed your advice on running the trivial case. The float version started cancelling sounds out within a second. The fixed point version also worked, but took a little longer before the effect was noticeable. Since I now realized the fixed point version might need a little more tweaking than the float version, I started modifying some things and ended up with the following changes:

Jim, You're right, the memset really is there, how could I miss that? :) I also spoke too soon, as my change doesn't solve anything. At each run, something random causes the algorithm to either work or not work at all. There is a correspondence between whether the echo cancel works and whether the st->PHI and st->W arrays contain data or zeroes. (the arrays contain zeroes when alg