[Speex-dev] Major internal changes, TI DSP build change

[Jim Crichton]

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