[Speex-dev] Re: Minor fixed point scaling problem

[Jean-Marc Valin]

Hi Jerry,

> First, let me say that I think the speex code is incredible in the way it
> supports floating and fixed point code from one set of code.  The same is
> true for supporting multiple processors, etc...  I've used speex with the
> PC, TI 64xx and 55xx.  Please view the following comments not as an attack
> on speex (which I think is incredible!) but as my contribution to an open
> source project.

Don't worry, I'm quite open (and interested) to criticism like that.
Even more so when it's coming from people like you who know what they're
doing and have spent time actually looking at the code.

> I know this is a minor point, but speex/AEC fixed point could use a few
> improvements shifting 2's complement values.
> 
> I realize these things are "almost" correct and I doubt if fixing these
> problems will be audible.  But doing it correctly does help consistency when
> the code is optimized and it makes it easier to understand why limits are
> enforced within the code.

You're right that there's some strange things in the current Speex
codebase. They're all in areas where accuracy doesn't really matter for
the final quality, but I guess it could be cleaner.

The first issue is about saturation on the negative side. My original
idea for saturating at -16383 and -32767 instead of -16384 and -32768
was to make sure that no overflow happens if I try to use the NEG
operator or if I compute the square. I'm not sure if the issue is real,
but it's worth investigating because it would make the code nicer and
probably slightly easier to optimize on DSP hardware.

> I have made these changes (and more) to my code, but I still don't
> understand the process of getting these changes into and released from the
> speex SVN.  So here are a few changes, I think should be made to the code.

The simplest way is usually to send a patch or (in this case) a file to
be added somewhere.

> I found these problems when dropping in the _norm() instruction available on
> the TI DSP families. _norm() returns the number or redundant bits which lets
> you figure out how many bits to shift.  You don't need to worry about the
> sign with this function and the BlackFin has a corresponding instruction.
> Using this instruction saved me about 10% of my total encoder instructions.
> 
> It would probably make sense to have a NORM macro that maps to _norm() on TI
> platforms or whatever the processor specific instruction is available.

I think what you're suggesting is the same (or similar) to the
spx_ilog2() I recently added. Do you think spx_ilog2() is sufficient?

Cheers,

	Jean-Marc