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

Hi Jim,
> Build 11169 in SVN works correctly.  
Good. I'll try not to forget the EXTEND32 from now on.
> I have attached a zip file (renamed 
> .txt) with a patch to bits.c to make the byteswapping for TI DSPs 
> consistent.  
Seems like unzip can't read it. Either it's in an unknown format or the
file got corrupted. Could simply send as multiple (uncompressed)
attachments? BTW, broken down patches would be nice.
> I also added a switch so that byte swapping could be enabled or 
> disabled in config.h.  
Why would you want to turn it on/off?
> There are also updates to the .pjt files to add 
> window.c, and updates in the test main files in the TI directory to account
> for the reduction in delay from 10ms to 5ms.  There is also an added file: 
> the C6x build needs speex/speex_config_types.h, so I created a speex 
> subdirectory under TI with this file.  The subversion patch generation tool
> would not handle an added file.
Actually, speex_config_types.h is not necessary for the TI builds. It's
best to put the definition directly in speex_types.h.
> I first made a patch against build 11146, applied this against 11169, made
a
> couple of other changes, and made a new patch agains 11169.  The second 
> patch is larger because some line ends got changed in a couple of the files
> when the first patch was applied.  I am sorry for the clutter, but its my 
> first time with the patch tool.
> 
> I have one lingering concern.  The C6x encoder does not produce the same 
> results as the C55x and C54x.  The waveform reproduction is less accurate
as
> measured by a sample by sample comparison.  In the test programs, the SNR
is
> 11.10 in the C55x and C54x, and 10.79 in the C6x build.  I figured that the
> encoders might not produce bit-exact results because of differences in
their
> wordlengths, but at one time they were the same (1.1.8, I think).  The 
> decoders do produce the same results.  Do you think that this is anything
to
> be worried about?
Yes, it worries me a bit. Not the fact that the SNR are slightly
different (one file is probably not enough to say which is "better"),
but the fact that they differ at all for the same fixed-point version.
Could you check when the results started being different. There might be
the same kind of error as earlier, but on a less critical bit of data.
> Finally, in the simulator I measured the peak MIPs for the C55x as 29.4, 
> where it was 41.5 in Speex 1.1.8.  I was expecting some improvement from
the
> continuing work you have been doing on the fixed point build, but this is 
> really impressive, almost too good to be true.  This is all straight 
> compiled C, after all (though the compiler has been updated also).
I think this is probably due to some filters that got changed from
32-bit accuracy to 16-bit. There is an option to use *all* filters with
16-bit accuracy (just define PRECISION16) and I suspect the difference
wouldn't be very large. Note that the quality isn't be as good with that
option on, whereas the changes I made recently don't hurt quality.

	Jean-Marc

Jean-Marc,
>> Build 11169 in SVN works correctly.
>
> Good. I'll try not to forget the EXTEND32 from now on.
>
>> I have attached a zip file (renamed
>> .txt) with a patch to bits.c to make the byteswapping for TI DSPs
>> consistent.
>
> Seems like unzip can't read it. Either it's in an unknown format or
the
> file got corrupted. Could simply send as multiple (uncompressed)
> attachments? BTW, broken down patches would be nice.
I will redo the patches and resend them, probably over the weekend.
>> I also added a switch so that byte swapping could be enabled or
>> disabled in config.h.
>
> Why would you want to turn it on/off?
I mean a #define, and I would not want to turn it on and off, but it occurs
to me that someone else might want a different byte order than I do.
Remember that in SVN, the write routine swaps and the read routine doesn't.
Swapping makes the endian-ness of the data match the endian-ness of the
audio samples, so that is how the examples work with file I/O.  I suppose,
looking at it that way, that the switch is not necessary and the data should
always be swapped.  I will redo the patch that way.
>> There are also updates to the .pjt files to add
>> window.c, and updates in the test main files in the TI directory to
>> account
>> for the reduction in delay from 10ms to 5ms.  There is also an added
>> file:
>> the C6x build needs speex/speex_config_types.h, so I created a speex
>> subdirectory under TI with this file.  The subversion patch generation
>> tool
>> would not handle an added file.
>
> Actually, speex_config_types.h is not necessary for the TI builds. It's
> best to put the definition directly in speex_types.h.
When this was done in build 10313, the switch for the C6x was mistyped as 
CONFIG_TI_C5X instead of (CONFIG_TI_C6X), and I was not picking this up in 
my build.  I will include this in the patches.
>> I first made a patch against build 11146, applied this against 11169,
>> made a
>> couple of other changes, and made a new patch agains 11169.  The second
>> patch is larger because some line ends got changed in a couple of the
>> files
>> when the first patch was applied.  I am sorry for the clutter, but its
my
>> first time with the patch tool.
>>
>> I have one lingering concern.  The C6x encoder does not produce the
same
>> results as the C55x and C54x.  The waveform reproduction is less
accurate
>> as
>> measured by a sample by sample comparison.  In the test programs, the
SNR
>> is
>> 11.10 in the C55x and C54x, and 10.79 in the C6x build.  I figured that
>> the
>> encoders might not produce bit-exact results because of differences in
>> their
>> wordlengths, but at one time they were the same (1.1.8, I think).  The
>> decoders do produce the same results.  Do you think that this is
anything
>> to
>> be worried about?
>
> Yes, it worries me a bit. Not the fact that the SNR are slightly
> different (one file is probably not enough to say which is
"better"),
> but the fact that they differ at all for the same fixed-point version.
> Could you check when the results started being different. There might be
> the same kind of error as earlier, but on a less critical bit of data.
The C5x and C6x output diverges in build 10143, which has log message "lpc 
floor converted to fixed-point."  Also, the measured SNR changed from 11.05
in builds 9854-10141 to 9.22 and 9.24 in 10143.

There is just four lines in modes.c which declare the constant, and one line 
changed in nb_celp.c and sb_celp.c which use the constant.  Looking at 
nb_mode, QCONT16(.0002,15) evaluates to 0x3FF9 on the C55 and 0x4006 on the 
C6x.  When I patch the value 0x4006 into the C55 build, the output matches 
the C6x.  The problem is that 2^15 evaluates to -32768 on the C55 and 32768 
on the C6x.

Applying our friend EXTEND32 causes the constant to evaluate correctly.  In 
fixed_generic.h,
#define QCONST16(x,bits) 
((spx_word16_t)((x)*((EXTEND32(1))<<(bits))+((EXTEND32(1))<<((bits)-1))))

Later I will check if this change makes these two builds match in the latest 
SVN code.
>> Finally, in the simulator I measured the peak MIPs for the C55x as
29.4,
>> where it was 41.5 in Speex 1.1.8.  I was expecting some improvement
from
>> the
>> continuing work you have been doing on the fixed point build, but this
is
>> really impressive, almost too good to be true.  This is all straight
>> compiled C, after all (though the compiler has been updated also).
>
> I think this is probably due to some filters that got changed from
> 32-bit accuracy to 16-bit. There is an option to use *all* filters with
> 16-bit accuracy (just define PRECISION16) and I suspect the difference
> wouldn't be very large. Note that the quality isn't be as good with
that
> option on, whereas the changes I made recently don't hurt quality.
The MIPs are not a problem for me, and the C55 does very well on 32x16 
multiplies, so I have not played with PRECISION16 since last year.

- Jim

> The C5x and C6x output diverges in build 10143, which has log message
"lpc
> floor converted to fixed-point."  Also, the measured SNR changed from
11.05
> in builds 9854-10141 to 9.22 and 9.24 in 10143.
Actually, build 10143 introduced another bug, that was the reason for
the 1.1.11.1 release. 
> There is just four lines in modes.c which declare the constant, and one
line
> changed in nb_celp.c and sb_celp.c which use the constant.  Looking at 
> nb_mode, QCONT16(.0002,15) evaluates to 0x3FF9 on the C55 and 0x4006 on the
> C6x.  When I patch the value 0x4006 into the C55 build, the output matches 
> the C6x.  The problem is that 2^15 evaluates to -32768 on the C55 and 32768
> on the C6x.
Right on!
> Applying our friend EXTEND32 causes the constant to evaluate correctly.  In
> fixed_generic.h,
> #define QCONST16(x,bits) 
>
((spx_word16_t)((x)*((EXTEND32(1))<<(bits))+((EXTEND32(1))<<((bits)-1))))
Actually, this is a case for a simple cast to (spx_word32_t) because
QCONST can be used in a static initialization and EXTEND32 *can* be
defined as a function (e.g. for fixed-point debug). 
> Later I will check if this change makes these two builds match in the
latest
> SVN code.
I fixed it in svn. Could you check that?
> The MIPs are not a problem for me, and the C55 does very well on 32x16 
> multiplies, so I have not played with PRECISION16 since last year.
Does the C55 have a 32x16 multiplier or do you mean it handles my
emulation of it well?

	Jean-Marc
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Ceci est une partie de message
	=?ISO-8859-1?Q?num=E9riquement?= =?ISO-8859-1?Q?_sign=E9e?Url :
http://lists.xiph.org/pipermail/speex-dev/attachments/20060422/067dce30/attachment.pgp

On 4/21/06, Jim Crichton <jim.crichton@comcast.net> wrote:
> >> Finally, in the simulator I measured the peak MIPs for the C55x as
29.4
> ,
> >> where it was 41.5 in Speex 1.1.8.  I was expecting some
improvement
> from
> >> the
> >> continuing work you have been doing on the fixed point build, but
this
> is
> >> really impressive, almost too good to be true.  This is all
straight
> >> compiled C, after all (though the compiler has been updated also).

Judging from these numbers it looks within the bare realms of feasibility to
get speex working on the PA168, which includes a ADSP2181. This is a popular
chip for voip applications  (see
http://www.voip-info.org/wiki/view/PA168for more details)

Has anyone explored this chip?




--
Mike Taht
PostCards From the Bleeding Edge
http://the-edge.blogspot.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
http://lists.xiph.org/pipermail/speex-dev/attachments/20060422/377f39a3/attachment.html

Jean-Marc,

Here are the patches, broken out one per file.  I did some additional work 
in the testenc files based on Peter Mlakar's post to make it easier (though 
still a bit klunky) to try different rates.

These changes also incorporate the file window.c into the builds, change the 
decoder startup delay to account for the change in the algorithm delay from 
10ms to 5ms, and change bits.c to make the byte swapping uniform for the TI 
C55 and C54 builds.

- Jim


----- Original Message ----- 
From: "Jean-Marc Valin" <Jean-Marc.Valin@USherbrooke.ca>
To: "Jim Crichton" <jim.crichton@comcast.net>
Cc: <speex-dev@xiph.org>
Sent: Thursday, April 20, 2006 7:13 PM
Subject: Re: [Speex-dev] Major internal changes, TI DSP build change

> Hi Jim,
>
>> Build 11169 in SVN works correctly.
>
> Good. I'll try not to forget the EXTEND32 from now on.
>
>> I have attached a zip file (renamed
>> .txt) with a patch to bits.c to make the byteswapping for TI DSPs
>> consistent.
>
> Seems like unzip can't read it. Either it's in an unknown format or
the
> file got corrupted. Could simply send as multiple (uncompressed)
> attachments? BTW, broken down patches would be nice.
>
>> I also added a switch so that byte swapping could be enabled or
>> disabled in config.h.
>
> Why would you want to turn it on/off?
>
>> There are also updates to the .pjt files to add
>> window.c, and updates in the test main files in the TI directory to 
>> account
>> for the reduction in delay from 10ms to 5ms.  There is also an added 
>> file:
>> the C6x build needs speex/speex_config_types.h, so I created a speex
>> subdirectory under TI with this file.  The subversion patch generation 
>> tool
>> would not handle an added file.
>
> Actually, speex_config_types.h is not necessary for the TI builds. It's
> best to put the definition directly in speex_types.h.
>
>> I first made a patch against build 11146, applied this against 11169, 
>> made a
>> couple of other changes, and made a new patch agains 11169.  The second
>> patch is larger because some line ends got changed in a couple of the 
>> files
>> when the first patch was applied.  I am sorry for the clutter, but its
my
>> first time with the patch tool.
>>
>> I have one lingering concern.  The C6x encoder does not produce the
same
>> results as the C55x and C54x.  The waveform reproduction is less
accurate
>> as
>> measured by a sample by sample comparison.  In the test programs, the
SNR
>> is
>> 11.10 in the C55x and C54x, and 10.79 in the C6x build.  I figured that
>> the
>> encoders might not produce bit-exact results because of differences in 
>> their
>> wordlengths, but at one time they were the same (1.1.8, I think).  The
>> decoders do produce the same results.  Do you think that this is
anything
>> to
>> be worried about?
>
> Yes, it worries me a bit. Not the fact that the SNR are slightly
> different (one file is probably not enough to say which is
"better"),
> but the fact that they differ at all for the same fixed-point version.
> Could you check when the results started being different. There might be
> the same kind of error as earlier, but on a less critical bit of data.
>
>> Finally, in the simulator I measured the peak MIPs for the C55x as
29.4,
>> where it was 41.5 in Speex 1.1.8.  I was expecting some improvement
from
>> the
>> continuing work you have been doing on the fixed point build, but this
is
>> really impressive, almost too good to be true.  This is all straight
>> compiled C, after all (though the compiler has been updated also).
>
> I think this is probably due to some filters that got changed from
> 32-bit accuracy to 16-bit. There is an option to use *all* filters with
> 16-bit accuracy (just define PRECISION16) and I suspect the difference
> wouldn't be very large. Note that the quality isn't be as good with
that
> option on, whereas the changes I made recently don't hurt quality.
>
> Jean-Marc
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: testenc-TI-C64x.c.patch
Type: application/octet-stream
Size: 5143 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/speex-dev/attachments/20060424/82413d6a/testenc-TI-C64x.c-0001.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arch.h.patch
Type: application/octet-stream
Size: 301 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/speex-dev/attachments/20060424/82413d6a/arch.h-0001.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bits.c.patch
Type: application/octet-stream
Size: 2466 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/speex-dev/attachments/20060424/82413d6a/bits.c-0001.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: speex_C54_test.pjt.patch
Type: application/octet-stream
Size: 1464 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/speex-dev/attachments/20060424/82413d6a/speex_C54_test.pjt-0001.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: speex_C55_test.pjt.patch
Type: application/octet-stream
Size: 753 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/speex-dev/attachments/20060424/82413d6a/speex_C55_test.pjt-0001.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: speex_C64_test.pjt.patch
Type: application/octet-stream
Size: 1692 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/speex-dev/attachments/20060424/82413d6a/speex_C64_test.pjt-0001.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: speex_types.h.patch
Type: application/octet-stream
Size: 383 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/speex-dev/attachments/20060424/82413d6a/speex_types.h-0001.obj
-------------- next part --------------
A non-text attachment was scrubbed...
Name: testenc-TI-C5x.c.patch
Type: application/octet-stream
Size: 5746 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/speex-dev/attachments/20060424/82413d6a/testenc-TI-C5x.c-0001.obj

Jim,

The patches for bits.c, arch.h and speex_types.h applied fine, but the
ones for the files in ti/ (i.e. the project files and testenc-TI*)
didn't apply. Looks like a problem with the newlines (CRLF vs LF). Could
you send me an updated patch for that part? BTW, what I talked about
breaking down patches, I meant breaking down by functionality, not
necessarily by file. In this case, it could have been one patch for
bits.c, one patch with the stuff for arch.h and speex_types.h and one
patch for all the TI stuff in the ti/ directory.

	Jean-Marc

Le lundi 24 avril 2006 ? 08:59 -0400, Jim Crichton a ?crit
:
> Jean-Marc,
> 
> Here are the patches, broken out one per file.  I did some additional work 
> in the testenc files based on Peter Mlakar's post to make it easier
(though
> still a bit klunky) to try different rates.
> 
> These changes also incorporate the file window.c into the builds, change
the
> decoder startup delay to account for the change in the algorithm delay from
> 10ms to 5ms, and change bits.c to make the byte swapping uniform for the TI
> C55 and C54 builds.
> 
> - Jim
> 
> 
> ----- Original Message ----- 
> From: "Jean-Marc Valin" <Jean-Marc.Valin@USherbrooke.ca>
> To: "Jim Crichton" <jim.crichton@comcast.net>
> Cc: <speex-dev@xiph.org>
> Sent: Thursday, April 20, 2006 7:13 PM
> Subject: Re: [Speex-dev] Major internal changes, TI DSP build change
> 
> 
> > Hi Jim,
> >
> >> Build 11169 in SVN works correctly.
> >
> > Good. I'll try not to forget the EXTEND32 from now on.
> >
> >> I have attached a zip file (renamed
> >> .txt) with a patch to bits.c to make the byteswapping for TI DSPs
> >> consistent.
> >
> > Seems like unzip can't read it. Either it's in an unknown
format or the
> > file got corrupted. Could simply send as multiple (uncompressed)
> > attachments? BTW, broken down patches would be nice.
> >
> >> I also added a switch so that byte swapping could be enabled or
> >> disabled in config.h.
> >
> > Why would you want to turn it on/off?
> >
> >> There are also updates to the .pjt files to add
> >> window.c, and updates in the test main files in the TI directory
to
> >> account
> >> for the reduction in delay from 10ms to 5ms.  There is also an
added
> >> file:
> >> the C6x build needs speex/speex_config_types.h, so I created a
speex
> >> subdirectory under TI with this file.  The subversion patch
generation
> >> tool
> >> would not handle an added file.
> >
> > Actually, speex_config_types.h is not necessary for the TI builds.
It's
> > best to put the definition directly in speex_types.h.
> >
> >> I first made a patch against build 11146, applied this against
11169,
> >> made a
> >> couple of other changes, and made a new patch agains 11169.  The
second
> >> patch is larger because some line ends got changed in a couple of
the
> >> files
> >> when the first patch was applied.  I am sorry for the clutter, but
its my
> >> first time with the patch tool.
> >>
> >> I have one lingering concern.  The C6x encoder does not produce
the same
> >> results as the C55x and C54x.  The waveform reproduction is less
accurate
> >> as
> >> measured by a sample by sample comparison.  In the test programs,
the SNR
> >> is
> >> 11.10 in the C55x and C54x, and 10.79 in the C6x build.  I figured
that
> >> the
> >> encoders might not produce bit-exact results because of
differences in
> >> their
> >> wordlengths, but at one time they were the same (1.1.8, I think). 
The
> >> decoders do produce the same results.  Do you think that this is
anything
> >> to
> >> be worried about?
> >
> > Yes, it worries me a bit. Not the fact that the SNR are slightly
> > different (one file is probably not enough to say which is
"better"),
> > but the fact that they differ at all for the same fixed-point version.
> > Could you check when the results started being different. There might
be
> > the same kind of error as earlier, but on a less critical bit of data.
> >
> >> Finally, in the simulator I measured the peak MIPs for the C55x as
29.4,
> >> where it was 41.5 in Speex 1.1.8.  I was expecting some
improvement from
> >> the
> >> continuing work you have been doing on the fixed point build, but
this is
> >> really impressive, almost too good to be true.  This is all
straight
> >> compiled C, after all (though the compiler has been updated also).
> >
> > I think this is probably due to some filters that got changed from
> > 32-bit accuracy to 16-bit. There is an option to use *all* filters
with
> > 16-bit accuracy (just define PRECISION16) and I suspect the difference
> > wouldn't be very large. Note that the quality isn't be as good
with that
> > option on, whereas the changes I made recently don't hurt quality.
> >
> > Jean-Marc
> >