[Speex-dev] Speex on TI C6x, Problem with TI C5x Patch

[Jean-Marc Valin]

Hi Stuart,

> 1) We need our own "fixed_xx.h" header file. I don't know why, and haven't 
> had time to investigate, but there is a definite improvement when I use the 
> attached fixed_c55x.h file which has turned all the maths into inline 
> functions. 

Did you try with fixed_generic.h or just with fixed_debug.h?
fixed_debug.h uses int and short directly, so I know it won't work with
the C5x. However, I think fixed_generic.h should work and has all the
operators defined as macros anyway, so inlining isn't a problem.

> Some optimisation or something is probably possible here to 
> reduce code size and inline the functions, as by default the C55x compiler 
> does not seem to inline them (perhaps due to debugging mode). This can be 
> enabled with a C55X_ASM definition following the ARM fixed point math 
> definition convention, and some it could be converted to assembler in the 
> future.

The assembly definitions for the operators are only useful if you have
gcc-like inline assembly. Otherwise, the explicit register loads will
make it worse.

> 2) Proper definitions for the speex types are required in the speex_types.h 
> file - I did this and you can enable it via a __C55X__ definition. File 
> attached. My definition follows the convention of other defines in this 
> file. It could be covered by the C55X_ASM define above, but the content of 
> this file is not going to have assembler in it. I leave it up to you if 
> think this is wrong or right - just tell me and I'll follow!

I didn't include the C5x in speex_types.h because I was under the
impression that autoconf would be used, but I can add it.

> 3) There seem to be further int/long on a C55X issues in nb_celp.c for the 
> nb_encoder_ctl and nb_decoder_ctl functions. I think that all the 
> (*(int*)ptr) or *((int*)ptr) should be (*(long*)ptr) for a C55X. I don't 
> know why. What I presume was happening was that the data passed to the 
> function in void *ptr was being lost in the upper or lower half of a 32-bit 
> word. So it didn't matter what you passed as a setting for SPEEX_SET_QUALITY 
> and SPEEX_SET_COMPLEXITY, the (*(int*)ptr) always = 0. For quality this 
> doesn't matter I don't think due to the default settings, but the 
> st->complexity always ended up as zero, and the decoded bit-stream ends up 
> sounding very ropy. 

Actually, I don't see why long/int would be a problem here since you're
also passing an (int*) and the values are never higher than 32767.

> I think this the issue Jim alluded to in his question 2 
> regarding 'artifacts'. I don't think it was artifacts but perhaps this issue 
> I have described. In nb_celp.c I changed all those effected (*(int*)ptr) to 
> long, but perhaps it would be better to have a more platform independant 
> resolution? Does ptr need to be a void *? Can't we use an int * or, better 
> still, one of the nice Speex types so we know what we are expecting to 
> arrive at the function? 

It has to be a (void*) because the type is not always int. Some calls
use floats, some even use a struct.

> I can make the changes and submit, I just want to 
> know how best to do a platform independant change. (I attach my nb_celp.c 
> for Jim and just so you can see what I did). Jim: Note that this will push 
> your MIPS up again as now the encode is actually doing a decent job on the 
> decoding - I think this was the reason for the huge performance increase.

I don't see what you're talking about. 

> 4) Jamie's CONFIG_TI_C55X definition doesn't seem to work for me. Whenever I 
> compile using this option, I get a "Buffer too small to apck bits" and then 
> "Could not resize input buffer: not packing" messages. I haven't had time to 
> figure out what that means or what causes it, but will if I get time.

I just fixed a but in bits.c (see Jim's previous email), maybe that was
the cause.

> 5) Re: stack memory allocations, yes, a compile-time option would be great 
> to reduce the stack sizes. Ideally what is needed is the minimum stack usage 
> so that us embedded developers can support streaming audio, for either 
> compression or decompression. For nb mode, for example, this would mean 
> receiving a 160 byte raw data frame, compressing and storing or sending, 
> then dealing with the next frame. This is my opinion anyway, and I think 
> this usage model is a good target model to keep in mind during development. 
> For an embedded system it is likely that developers would do only 
> compression, or only decompression in their application, so splitting the 
> encoder and decoder into seperate files would also be a good idea in the 
> future.

I might add an option to disable the decoder or the encoder, but a lot
of the common code will still be there. BTW, have you tried removing the
preprocessor, the echo canceller and the jitter buffer if you want to
save space?

> Whether you'll want all that stuff put 
> back into the main Speex release I don't know, but if you do that is fine 
> with me too.

Unless it's really ugly, I don't see why it couldn't go in the main
releases.

	Jean-Marc

-- 
Jean-Marc Valin <Jean-Marc.Valin at USherbrooke.ca>
Université de Sherbrooke