Opened 13 years ago
Closed 13 years ago
#5598 closed defect (fixed)
COMI: Incorrect decoding of audio codec 13/15 = crackles
| Reported by: | SF/the_serge | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: | Monkey Island 3 |
Description
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++ ;-))
Ticket imported from: #3187622. Ticket imported from: bugs/5598.
Change History (7)
comment:1 by , 13 years ago
comment:2 by , 13 years ago
| Summary: | Incorrect decoding of audio codec 13/15 causes crackles → COMI: Incorrect decoding of audio codec 13/15 = crackles |
|---|
comment:3 by , 13 years ago
| Owner: | set to |
|---|
comment:4 by , 13 years ago
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. ;-)
comment:5 by , 13 years ago
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 ;).
comment:6 by , 13 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
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"