Speex dev - Apr 2006 - 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

I rebuilt the TI DSP build for the C55 this morning, in order to add the 
window.c file to the TI project.  I encountered two problems:

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:

line 410:
      {  /* Added */
      spx_word16_t *exc16_alias = (spx_word16_t*)st->exc;
--26 more lines, then--
         ol_gain = SHL32(EXTEND32(compute_rms16(exc16_alias, 
st->frameSize)),SIG_SHIFT);
#ifdef EPIC_48K
      }
#endif
      }  /* Added */

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.

This build is 8kbps, fixed point, no preprocessor/echo canceller.

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?

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.

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
>

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