#5598 COMI: Incorrect decoding of audio codec 13/15 = crackles

[Jimmi Thøgersen]

ScummVM 1.2.1, confirmed to be in latest revision.
ScummVM-tools 1.2 uses the same erroneous code (that's where I got the audio clip below - by taking the intermediary wav file before it's encoded to flac)
Game: The Curse of Monkey Island
Platform: Win32 (but quite certain the bug is platform independent, since it's a decoding bug)

There is (faint, but audible if you listen for it) crackle in the ScummVM audio decoded from codecs 13/15 (the IMA ADPCM codecs).

Can't attach large enough files, so:
Clip of proper decode of 1098-CREDITS.IMX: http://dl.dropbox.com/u/14034871/clip_proper.wav
Clilp of ScummVM decode of 1098-CREDITS.IMX: http://dl.dropbox.com/u/14034871/clip_scummvm.wav

The culprit would be this line:

int32 delta = imcTable[curTablePos] * (2 * data + 1) >> (curTableEntryBitCount - 1);

This seems to be an attempt at getting rid of the precalculated lookup tables that the original game uses. In pseudo-C, the condensed original lines would read:

int lookupTableIndex = (data << (7 - curTableEntryBitCount)) + (curTablePos << 6);
int delta = (imcTable[tablePos] >> (curTableEntryBitCount - 1)) + IMC_VALUE_TABLE[lookupTableIndex];

The lookup table in the original game is calculated like this (again, pseudo-C) - in the same place as the bit count lookup table:

static int IMC_VALUE_TABLE[0x40 * ARRAYSIZE(imcTable)]; // Number of different values for 'data' variable * number of delta values

for (int n = 0; n <= 0x3f; n++)
{
int destTablePos = n;
for (int srcTablePos = 0; srcTablePos < ARRAYSIZE(imcTable); srcTablePos++ )
{
int mask = 0x20;
int put = 0;
int tableValue = imcTable[srcTablePos];
do
{
if ((mask & n) != 0)
{
put += tableValue;
}
mask >>= 1;
tableValue >>= 1;
} while (count != 0);

IMC_VALUE_TABLE[destTablePos] = put;
destTablePos += 0x40;
}
}

This can very likely be reduced - and the lookup table written out. But not in the way ScummVM does it right now. I'd submit a patch - if I knew enough about the ScummVM code (and if I could code fluently in C/C++ ;-))

[Jimmi Thøgersen]

Other than the lack of indent (I never use SourceForge bug tracking), these lines:

mask >>= 1;
tableValue >>= 1;
} while (count != 0);

... should obviously read:

mask >>= 1;
tableValue >>= 1;
} while (mask != 0); // not "count"

[Torbjörn Andersson]

Fingolfin, do you know anything about this? I believe this particular part of the code was changed in 5cdf1bb621f51d47c0536004335481892641c509 (SVN r19770), "Simplified COMI IMA codec (resulting code needs less memory and should be faster on modern CPUs)".

(Granted, this was five years ago, but I'm sure you remember it like it was only four years ago. ;-)

[Max Horn]

I really don't recally what I did there... but after a brief glance, I really have no idea how may commit from back then could be correct, so I tend to think Jimmi is completely right. It still would be nice to be able to go on without that buffer, as it was 22kb (at least in the original code), which is an issue for small systems.

I wonder if we could just use the IMA ADPCM decoder we now have in audio/decoders/adpcm.cpp, possibly after some tweaking (adding yet another variant to it ;).

[Max Horn]

Fixed in commit 9c2ff87db77235fc3