Speex dev - Apr 2006 - Major internal changes, TI DSP build change

> 1.  I got a compile error at line 410 in nb_celp.c, because this compiler 
> will not allow variable declarations in the midst of executable code.  I 
> fixed this by bracketing this code block:
Thanks for letting me know. I just fixed it in svn.
> 2:  The decoder is broken.  Running the Male.wav test file, I get all zeros
> out for the first 5 seconds, then 8 samples ranging from -2 to 2, and then 
> all 0001 samples to the end of the file.  I got the same result feeding the
> encoded bits from Speex version 1.12.  Before I look into this further, I 
> wanted to see if build 11147 was known to be broken.
Hmm, this isn't good. I wasn't aware of anything being broken, so it
must be something TI-specific that I accidently broke. Could you check
other revisions and see where it got broken? At this poing, I have no
clue what could be responsible. 1.1.12 was working fine, right?
> On a separate topic, I want to propose a change to the TI C54x, C55x
builds.
> Because these processors have 16 bit char size, there is some special code 
> in bits.c to handle the packing.  As part of this, there is code to swap
the
> byte order for each 16-bit char within speex_bits_write.  There is no such 
> byte swapping in speex_bits_read_from, speex_bits_read_whole_bytes, or 
> speex_bits_write_whole_bytes.  For consistency, the byte swapping should be
> done in all four places or in none of them.  In my present build, I have 
> byte swapping in all four places, but it seems cleaner to take this out and
> leave it to the calling function to do the swapping if it is needed.  The 
> demo builds are not affected, because the loopback application only uses 
> speex_bits_write.
> 
> Since this affects the Speex interface (just for these targets), I though I
> should ask for feedback before sending a patch.  Does anybody care?
Just so I understand, what you mean is that if you use
speex_bits_write() with speex_bits_read_from() the way things are, it's
just going to corrupt things? If that's the case, then you're more than
welcome to fix it. I think your judgement is much better than mine on
this, so you can do it the way you like as long as the patch is clean
enough.
> Note that speex_bits_read_from is broken in other ways (the length checking
> is wrong for the if BYTES_PER_CHAR=2), so bits.c needs to be updated either
> way.
Patch welcome :-)

	Jean-Marc
> Regards,
> 
> Jim Crichton
> 
> ----- Original Message ----- 
> From: "Jean-Marc Valin" <jean-marc.valin@usherbrooke.ca>
> To: <speex-dev@xiph.org>
> Sent: Tuesday, April 11, 2006 9:34 PM
> Subject: [Speex-dev] Major internal changes
> 
> 
> > Hi everyone,
> >
> > I've recently done some major internal changes in Speex aimed at
> > reducing RAM (by nearly a factor of 2!) and improving quality of the
> > fixed-point. In doing so, I might have accidently  broken a few
things.
> > I'd like to hear feedback on the current svn code to make sure I
fix any
> > regression before the next release. I'm already aware that
> > --enable-vorbis-psy is broken and I'm working on fixing it.
> >
> > Jean-Marc
> > _______________________________________________
> > Speex-dev mailing list
> > Speex-dev@xiph.org
> > http://lists.xiph.org/mailman/listinfo/speex-dev
> > 
> 
> 
>

>> 2:  The decoder is broken.  Running the Male.wav test file, I get all 
>> zeros
>> out for the first 5 seconds, then 8 samples ranging from -2 to 2, and 
>> then
>> all 0001 samples to the end of the file.  I got the same result feeding
>> the
>> encoded bits from Speex version 1.12.  Before I look into this further,
I
>> wanted to see if build 11147 was known to be broken.
>
> Hmm, this isn't good. I wasn't aware of anything being broken, so
it
> must be something TI-specific that I accidently broke. Could you check
> other revisions and see where it got broken? At this poing, I have no
> clue what could be responsible. 1.1.12 was working fine, right?
I was wrong, it is the encoder that is not working, and it stopped working 
in build 11103.  The log message for this build is "another 640 bytes 
removed from the encoder state (using the input data instead of copying it 
to st->frame/st->inBuf)".  Only nb_celp.c/h are changed in this
build.  What
I am seeing out of the decoder is an extremely low signal (sample values of 
+/-10, in this build, by build 11126 the output is almost all zeros).

It may be a compiler hiccup in scaling/shifting, or else somehow the pointer 
to the data has been trashed.  I tried scaling the input data up by a factor 
of 10, and this affected the output, but it is still just low amplitude 
noise.  I will look some more tomorrow.  Maye the first place that I should 
look is at line 782, "/*FIXME: This is a kludge that will break eventually 
*/.

- Jim

> I was wrong, it is the encoder that is not working, and it stopped working 
> in build 11103.  The log message for this build is "another 640 bytes 
> removed from the encoder state (using the input data instead of copying it 
> to st->frame/st->inBuf)".  Only nb_celp.c/h are changed in this
build.  What
> I am seeing out of the decoder is an extremely low signal (sample values of
> +/-10, in this build, by build 11126 the output is almost all zeros).
That's quite strange, but I think I figured it out. Could you try
changing the definition of SHL32 in fixed_generic.h from
#define SHR32(a,shift) ((a) >> (shift))
to
#define SHR32(a,shift) (((spx_word32_t)(a)) >> (shift))

This is not the correct fix, but if it does solve the problem, then I
know how to fix it properly (by using EXTEND32) in svn.
> It may be a compiler hiccup in scaling/shifting, or else somehow the
pointer
> to the data has been trashed.  I tried scaling the input data up by a
factor
> of 10, and this affected the output, but it is still just low amplitude 
> noise.  I will look some more tomorrow.  Maye the first place that I should
> look is at line 782, "/*FIXME: This is a kludge that will break
eventually
> */.
Yeah, that code sort of makes too many assumptions, but they happen to
be valid for now. I suspect the shift (described above) to scrap the top
16 bits when I attempt to shift a 16-bit value.

	Jean-Marc