I feel stupid.
I think I have solved the M/S + intensity joint stereo problem.
I had thought the issue had to do with an ambiguous standard since my first attempt produced output quite different from two other references (mpg123 and Xaudio). After tweaking the algorithm (OPT_ISKLUGE) I ended up with something that sounded like Xaudio. However, it was inferior to the sound from mpg123.
While I still think ISO/IEC 11172-3 is ambiguous if not outright wrong -- it was clarified somewhat in ISO/IEC 13818-3 -- I discovered the cause of the inferior sound as a bug in my code.
I feel stupid because the fix is trivial:
--- layer3.old.c Mon Nov 6 01:56:35 2000 +++ layer3.c Mon Nov 6 01:57:57 2000 @@ -1112,7 +1112,7 @@ lsf_scale = lsf_is_table[right_ch->scalefac_compress & 0x1];
for (i = bound; i < 576; ++i) { - unsigned int is_pos = 7; + unsigned int is_pos;
if (f-- == 0) { if (w-- == 0) {
This is actually what I had written originally, but I had changed it due to a compiler warning. :-/
With this fix, the sound is now more like mpg123, much improved, and hopefully, correct in conjunction with OPT_ISKLUGE -- which may not really be a kluge any more.
Ironically, Xaudio must have a similar bug, even though I can't look at their code! :-)
Cheers, -rob
Rob Leslie wrote:
--- layer3.old.c Mon Nov 6 01:56:35 2000 +++ layer3.c Mon Nov 6 01:57:57 2000 @@ -1112,7 +1112,7 @@ lsf_scale = lsf_is_table[right_ch->scalefac_compress & 0x1];
for (i = bound; i < 576; ++i) {
unsigned int is_pos = 7;
unsigned int is_pos; if (f-- == 0) { if (w-- == 0) {This is actually what I had written originally, but I had changed it due to a compiler warning. :-/
Shouldn't is_pos at least be initialised to 0? From the looks of things, there's code paths to the first check of is_pos at line 1136 where is_pos isn't explicitly set, and an automatic variable's default value is undefined (whatever was left on the stack or in the register at the time).
Note that I haven't done an audio check on this...
Simon. -- Simon Burge simonb@wasabisystems.com NetBSD Sales, Support and Service: http://www.wasabisystems.com/
Shouldn't is_pos at least be initialised to 0? From the looks of things, there's code paths to the first check of is_pos at line 1136 where is_pos isn't explicitly set, and an automatic variable's default value is undefined (whatever was left on the stack or in the register at the time).
The way the code is structured, is_pos is always set the first time through the loop before it is checked at line 1136. GCC doesn't see this, but you can follow the logic.
The problem with initializing the value inside the for block is that it is reset *each time* through the loop, whereas it should retain its previous value until certain conditions are met (namely, a window or scalefactor band boundary is crossed.)
Another way to fix this is to move the declaration with initial value up into the containing block.
Cheers, -rob
Hello!
Rob Leslie schrieb am Mon, 06 Nov 2000: :: > Shouldn't is_pos at least be initialised to 0? From the looks of :: > things, there's code paths to the first check of is_pos at line 1136 :: > where is_pos isn't explicitly set, and an automatic variable's default :: > value is undefined (whatever was left on the stack or in the register at :: > the time). :: :: The way the code is structured, is_pos is always set the first time through :: the loop before it is checked at line 1136. GCC doesn't see this, but you can :: follow the logic. :: :: The problem with initializing the value inside the for block is that it is :: reset *each time* through the loop, whereas it should retain its previous :: value until certain conditions are met (namely, a window or scalefactor band :: boundary is crossed.) :: :: Another way to fix this is to move the declaration with initial value up into :: the containing block. :: :: Cheers, :: -rob
Sounds like it would be better a static variable? for (i = bound; i < 576; ++i) { static unsigned int is_pos = 7;
This way it gets it initial value once and keeps then the value from the previous iteration.
Ciao Robert
Robert Hegemann Robert.Hegemann@gmx.de wrote:
Sounds like it would be better a static variable?
for (i = bound; i < 576; ++i) { static unsigned int is_pos = 7;
This way it gets it initial value once and keeps then the value from the previous iteration.
Nah, there's no reason to keep the value across function calls, and this would force the compiler to generate code to store the value in permanently allocated memory before leaving the function (two drawbacks), whereas a register or the stack work just fine.
The code would also then become non-reentrant... not that this matters at the moment, but it's nice to leave open the possibility for multi-threading and SMP.
-rob
-
Rob Leslie -
Robert Hegemann -
Simon Burge