Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SCUMM: DiMUSE: New Digital iMUSE Engine for DIG and COMI - DO NOT MERGE #3368

Open
wants to merge 70 commits into
base: master
Choose a base branch
from

Conversation

@AndywinXp
Copy link
Contributor

@AndywinXp AndywinXp commented Sep 14, 2021

Phew, ok, this is a long one.

tl;dr: The engine features work as intended, but I am having trouble with a couple of bugs I think I need help with.

This (DiMUSE_v2) is a reimplementation from scratch of the Digital iMUSE audio engine, in the version used by the full versions of The Dig and The Curse of Monkey Island.

It uses (and sometimes adds new stuff to) some components of the old DiMUSE_v1, namely the states/sequences tables, and the sound/bundle/codec manager.

I frankly don't know where to start with this beast, so let's answer some possible questions:

Which games are supported?

The supported games are DIG and COMI, in their full version, with official supported languages. This means that no compressed/custom music and voice bundles are supported.
The demo versions are not supported because they have different interpreters which might probably present subtle (or enormous?) differences in the engine, and just I felt it wasn't worth the effort; and besides, I fixed audio for COMI demo last year on the current DiMUSE_v1 engine, and it works fine.
I didn't touch The Dig though. I hate The Dig. Ew.

What if I want to use demos, or compressed bundle files, or fan translations which use raw bundles?

Good point; the demos already fallback to DiMUSE_v1. For the rest, I plan to implement a fallback to DiMUSE_v1 at start-up time for these situations; the thing is, I don't quite know how to detect these situations, so please reach out if you know how, I need some guidance.

Hey, why did you leave out Full Throttle? That game also uses DiMUSE!

Full Throttle uses some kind of very primitive and stripped down version of DiMUSE, which in the original EXE is also quite buggy.
We currently have an implementation of that engine on ScummVM (for which I fixed some bugs last year) which I'd argue is better than the original, so while planning this me and sev- decided it just wasn't worth the effort.

Are DiMUSE_v1 savegames supported?

Yes! ...kind of!
You can load any old savegame, and it will probably not load with the desired audio status, but as new sound commands arrive from SCUMM, they will play just fine. I just did the bare minimum to ensure that it won't crash and that the current music file is started as soon as the savegame is loaded.

Why the hell are you outputting all the audio on a single ScummVM mixer channel?

The engine provides an internal mixer, which has the following features:

  • It handles internal volume and panning for sounds (of course);
  • It can take source audio with a bit depth of 8, 12 and 16 bits, and output them at a single target bit depth of 16 bits;
  • It provides custom amplitude quantization tables;
  • It provides pseudo-logarithmic volume scaling (click here for more information) as opposed to linear scaling, which is the one that the current ScummVM mixer uses.
  • It flushes and sums every audio source (in a non linear way, using the pseudo-logarithmic conversion mentioned above) to a single output buffer;

The last two points (and the annoying hacks needed to tell the mixer which type of audio we were feeding) are what ultimately convinced me to drop a custom three channel implementation, and keep the single channel buffer, which is then fed to a QueueingAudioStream as a kPlainSoundType.

But we can still separatedly change the volume of music, speech and SFX on ScummVM GUI, right?

Of course! It just has been implemented a little bit differently, by using the in-engine volume group handler.

Why is this PR in a DO NOT MERGE state?

The engine currently presents several issues, starting from the worst one down to the "meh" one:

  1. For the life of me I just can't figure out the correct way to handle SFX resource pointers, I really really need someone to lend me a hand for this, because after a week I'm just going in circles; this is the current code (in dimuse_v2_files.cpp):
uint8 *DiMUSEFilesHandler::getSoundAddrData(int soundId) {
	// This function is always used for SFX (tracks which do not
	// have a stream pointer), hence the use of the resource address
	if (soundId != 0 && soundId < 0xFFFFFFF0) {
		if (_vm->_res->isResourceLoaded(rtSound, soundId))
			return _vm->getResourceAddress(rtSound, soundId);
		else
			return NULL;
	}
	debug(5, "DiMUSEFilesHandler::getSoundAddrData(): soundId is 0 or out of range");
	return NULL;
}

Doing this I get lots and lots of NULL pointers for sounds which apparently aren't in the same room block as the one the player is currently at.
If I just do:

uint8 *DiMUSEFilesHandler::getSoundAddrData(int soundId) {
	// This function is always used for SFX (tracks which do not
	// have a stream pointer), hence the use of the resource address
	if (soundId != 0 && soundId < 0xFFFFFFF0) {
		return _vm->getResourceAddress(rtSound, soundId);
	}
	debug(5, "DiMUSEFilesHandler::getSoundAddrData(): soundId is 0 or out of range");
	return NULL;
}

I get every sound I need, everywhere. Except when it starts spitting out memory access violations (and AKOS related errors?) after a couple minutes in LeChuck's Hold in COMI.
And what if I load the sound with the DiMUSESndMgr? Same story as the first case, except here the resourcePtr doesn't go NULL if the resource is not found, so I get plenty and plenty of memory access violations.
Disabling sound effects by always returning NULL on DiMUSEFilesHandler::getSoundAddrData() kills off any memory access violation crash. Of course the PR won't leave the "DO NOT MERGE" status until this is fixed.
Please, help.

  1. This is an annoying one, since I can't reproduce it consistently: every now and then, the whole engine just bails out and the game starts playing without audio. This usually happens after a SAN video finishes playing, or after watching some amount of a SAN video and then skipping it. The culprit seems to be _waveSlicingHalted, which at some point manages to get to a value which blocks tracksCallback(). Thing is, every time _waveSlicingHalted is incremented, it is also decremented in the same function, with no return instructions in between ever. This also needs to be fixed before the PR is ready. Apparently this was the result of a race condition, thanks to criezy for suggesting this, and to eriktorbjorn for actually fixing it!

  2. Sometimes the crossfades happening after a JUMP instruction are not correctly lined up. This happens inconsistently and each time with different delays; I don't know why this happens, it might because of some subtle difference in the audio data reading+decompressing routine which I just can't catch. But hey, it never crashes the engine and it's still nicer to hear than the old transitions, so this issue is not prioritary. If you want an example of this, just start Part 3 of COMI, get to the song, and just wait for the first part to loop. Everytime the fade is misaligned in a different way.

For now, these are all the questions that came into my mind, but please if you have more let me know and I'll gladly answer them.

AndywinXp added 30 commits Aug 24, 2021
@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Sep 18, 2021

Absolutely! That's kind of in the plans... sooner or later :)

Great! Though in the meantime... Sorry about going a bit off-topic, but you wouldn't happen to know anything about this Digital iMUSE crash bug: https://bugs.scummvm.org/ticket/12932

I'm a bit worried that the same problem may be lurking in the SCUMM engine, and that would be bad this close to a release...

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 19, 2021

Absolutely! That's kind of in the plans... sooner or later :)

Great! Though in the meantime... Sorry about going a bit off-topic, but you wouldn't happen to know anything about this Digital iMUSE crash bug: https://bugs.scummvm.org/ticket/12932

I'm a bit worried that the same problem may be lurking in the SCUMM engine, and that would be bad this close to a release...

(Answered on Discord but it probably got lost, so: in my DiMUSE_v1 changes I eliminated all the cloneToFadeOutTrack calls from the games I worked on, FT and COMI, so there shouldn't be any problems)

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 20, 2021

Hey everyone, I've just added SMUSH/DiMUSE_v2 interoperability for The Dig (yeah, after I said I would never do it alone :P).
COMI already works exactly as it should (extract raw sound data from *.SAN file, throw it right away into the output soundbuffer) so no changes there.

As for The Dig, it appears to be stable (as in "it never crashes the game and featurewise works exactly as it should")

The only small issue is that in the intro I've noticed some buffer overflow messages (two in the ENG version, one in the ITA) from the streamer module which just skip (or stop) the current sound (SAN audio is composed of separated and independent DiMUSE sounds - music, SFXs and speech - so sound for the relevant category is resumed soon after a new one is started).

I will investigate these cases, but overall it seems pretty sturdy.

Also, I disabled scriptRefresh commands during SMUSH movies, just like the original does (it actually shouldn't execute anything SCUMM related until the movie is finished, but I digress) and this appears to have solved issue 2 for now.

EDIT: turns out, the buffer overflow is expected to happen :) I fixed its handling though, as it was buggy, and now the behaviour matches the EXE 1:1, and this implementation of DiMUSE is feature complete for real, this time

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Sep 21, 2021

Something still needs investigating, though. I've been trying the Curse of Monkey Island pirate song (since it was mentioned as an example), using boot param 465, and it works great... for a while. But I still haven't been able to make it through the whole thing without the sound completely stopping. The animation continues, but the game doesn't respond, perhaps because it's waiting for Guybrush to finish speaking? (The subtitle text is still up on the screen.)

I'll see if I can figure anything out, but don't get your hopes up...

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 21, 2021

Something still needs investigating, though. I've been trying the Curse of Monkey Island pirate song (since it was mentioned as an example), using boot param 465, and it works great... for a while. But I still haven't been able to make it through the whole thing without the sound completely stopping. The animation continues, but the game doesn't respond, perhaps because it's waiting for Guybrush to finish speaking? (The subtitle text is still up on the screen.)

I'll see if I can figure anything out, but don't get your hopes up...

Just to sum it up for everyone who hasn't read the conversation on Discord, this is still an instance of Issue 2, unfortunately...
I'm happy you can (almost reliably) reproduce it constantly though, since I really can't on my machine.

criezy suggested to check if _waveSlicingHalted is being modified by both the audio thread and the main thread and, well, it appears it is: most modifications happen within tracksCallback(), while others unfortunately happen all over dimuse_v2_wave.cpp.
All those function do check for _waveSlicingHalted != 0 before decrementing it though, theoretically bringing it to a consistent state, but it seems it's not enough.

Anyway, I also pushed a commit which shows Issue 1 in action: just load up this savegame (ENG version - comicrash_savegame.zip), go to Blondebeard's restaurant, choose the third dialogue option (don't skip anything!), and wait for the room to change; at that point, the game should throw an exception which unfortunately is different everytime: most of the time it laments a wrong basetag for a resource, or it shows the _akos byte pointer (akos.h) actually pointing to a sound resource (I can see that because the first four bytes on the location pointed by _akos are iMUS).

Is there a SCUMM v7-8 resource handling expert here who can please help me with that?

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Sep 22, 2021

So _waveSlicingHalted is pretty much a mutex? Threaded programming scares me, but the advice I've heard for implementing your own mutexes basically boiled down to "Don't. Because things that look atomic to you probably won't be, and then it will just fail unpredictably".

So as an experiment, without any pretense of understanding, I just mechanically replaced all the _waveSlicingHalted stuff with the mutex already present in the class, and was able to get through the song (listening to every verse) five times in a row without it hanging.

Then I reverted my patch, and it hung on my first attempt through the song. (Then it unhung/hung a few times while I was writing this. I don't remember if I ever waited long enough for that to happen before.) I'm attaching my experimental patch for reference.

dimuse-naive-mutex-patch.txt

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 22, 2021

So _waveSlicingHalted is pretty much a mutex? Threaded programming scares me, but the advice I've heard for implementing your own mutexes basically boiled down to "Don't. Because things that look atomic to you probably won't be, and then it will just fail unpredictably".

So as an experiment, without any pretense of understanding, I just mechanically replaced all the _waveSlicingHalted stuff with the mutex already present in the class, and was able to get through the song (listening to every verse) five times in a row without it hanging.

Then I reverted my patch, and it hung on my first attempt through the song. (Then it unhung/hung a few times while I was writing this. I don't remember if I ever waited long enough for that to happen before.) I'm attaching my experimental patch for reference.

dimuse-naive-mutex-patch.txt

Thanks for this! It appears to work fine on my end. For now I have commented everything _waveSlicingHalted-related, just in case. If this solution effectively fixes the problem reliably, I will delete all the commented out lines and clean-up everything.

Please check if it works as expected, and don't mind Issue 1 related crashes while doing that :)

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Sep 22, 2021

I played through the pirate song another two times with the latest change, and it still works for me. (Not bad considering that I never made it through the whole thing even once before!)

I was going to make myself a savegame near the treasure hold, since you said that was a source of crashes. On my way, there was some pretty bad audio distortion when Guybrush said "Hey, I'm getting pretty good at this." (or something to that effect) after using the cannon. I haven't checked if that's reproducible yet, though.

Edit: Or did you mean the first room, not the treasure hold?
Edit: The distortion didn't happen the second time I tried. (What it was sounding like was when you accidentally play signed audio data as unsigned, or the other way around. I think. I.e. you could hear the line clearly, just distorted.)

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 22, 2021

Sorry, I meant the first room, the one with Wally and the cannon.

Hmm, distortion apparently is one of the side-effects of that bug with SFXs: sounds like on the first try it manages to load a sound resource with the iMUS header, and then it appears to get whatever random data it finds and tries to play it as audio.

I did get distortion just before some crashes, so I guess this is correlated.

What's worse is that I can't reproduce the bug in the savegame above anymore after fixing the mutex bug, but it still randomly happens in the first act of the game :(

EDIT: I just love that the error is different every time
image

I'm sure this is a bad case of me mishandling the sound resource, but after all this time trying I really don't know how to do it otherwise; I am following what the SndMgr does when opening a sound like this, and it's driving me mad that DiMUSE_v1 has no problems with it and I'm, I don't know, probably corrupting memory in random places

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 22, 2021

While the previous savegame doesn't replicate the crash anymore, this does! comi.zip (Select "Qué?" as the dialogue option and it should crash in the same point as before, which is when changing the room).

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Sep 22, 2021

The first time I tried that savegame it did crash, but not since. I haven't managed to capture it in a debugger, and Valgrind reports nothing. Some sort of threading issue again, perhaps?

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 22, 2021

Heh, beats me; I literally know nothing about SCUMM resource handling, I just did what DiMUSE_v1 did in a stripped down way (I can't allocate a sound for each time getSoundAddr gets called), and about 5 variations of that, and still no dice.

EDIT: I should mention something though, more often than not I catched _room and _akos byte pointers having a "SOUN" (sound) header or an "iMUS" header.

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Sep 22, 2021

I get the impression that getSoundAddr() gets called from both the main and the audio thread. (By "impression" I mean I added a debug message that printed the return value of gettid().) That's probably bad, since there is no mutex protecting the resource manager's data structures?

I tried adding the debug message to loadResource(), and when the scene changed there were two calls where it appeared to be loading an rtSound resource from a different thread.

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 22, 2021

I get the impression that getSoundAddr() gets called from both the main and the audio thread. (By "impression" I mean I added a debug message that printed the return value of gettid().) That's probably bad, since there is no mutex protecting the resource manager's data structures?

I tried adding the debug message to loadResource(), and when the scene changed there were two calls where it appeared to be loading an rtSound resource from a different thread.

Uh! That's some very good info, thanks! I'll investigate in a few hours and see if I can solve this!

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 22, 2021

Hmm, nope, putting a mutex at the start of getSoundAddr() doesn't change anything, unfortunately.

image
(just saving this as another example of these crashes)

EDIT: This happens in The Dig too by the way, just much less often; got a similar crash about 1 hour into the game

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Sep 23, 2021

Hmm, nope, putting a mutex at the start of getSoundAddr() doesn't change anything, unfortunately.

That's to be expected. At most that would prevent the Digital iMUSE from calling the resource manager from the main and audio threads simultaneously. It wouldn't prevent the rest of the SCUMM engine from calling it, though. It's the resource manager itself that would have to be made thread safe. I don't know off-hand which functions that would affect, though.

Unless it's possible to move the call to it out of the audio thread...?

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 23, 2021

Unless it's possible to move the call to it out of the audio thread...?

Unfortunately not, because of the nature of how SFXs are handled the audio thread needs the resource pointer to be immediately available when there's need for it, so moving the call out of the audio thread is practically impossible.

It's the resource manager itself that would have to be made thread safe. I don't know off-hand which functions that would affect, though.

While that makes sense to me, I can't help but think that it would be a risky move (because it shouldn't break anything, but with a great emphasis on it shouldn't, after all, that code has been virtually untouched since 2011). I'm going to try it anyway though and see how it goes.

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Sep 23, 2021

Note that while some resource management goes through functions in ScummEngine, e.g. getResourceAddress(), in other places we call ResourceManager directly. I guess that should be changed.

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 23, 2021

I don't want to speak too soon, but I couldn't reproduce the crashes anymore (I stress-tested the first room in COMI and the usual savegame with Blondebeard)! Whether this actually fixes the problem or not, thanks a lot @eriktorbjorn, I really appreciate your help here!

Does this fix the crashes on your end too?

@sev- do you think the commit above might create problems somewhere else?

Note that while some resource management goes through functions in ScummEngine, e.g. getResourceAddress(), in other places we call ResourceManager directly. I guess that should be changed.

Just out of curiosity (I don't want to dig into and fix 10-years old code unless I have to, for now), any examples of that?

@eriktorbjorn
Copy link
Member

@eriktorbjorn eriktorbjorn commented Sep 23, 2021

Just out of curiosity (I don't want to dig into 10-years old code unless I have to, for now), any examples of that?

A quick search for "_res-" gets 300+ hits. Something that for all I know could turn up in The Dig would be the ScummEngine_v6 defineArray() / nukeArray() functions, that call _res->createResource() and _res->nukeResource() respectively.

It's quite possible that the mutex you added is all that's needed for Digital iMUSE (and it would seem to confirm the theory that the basic problem is the thread-unsafeness of the resource manager), but it seems very hackish to add a mutex that makes the resource manager thread safe in some cases, but not others.

Did I mention that threads scare me? ;-)

@AndywinXp
Copy link
Contributor Author

@AndywinXp AndywinXp commented Sep 23, 2021

A quick search for "_res-" gets 300+ hits. Something that for all I know could turn up in The Dig would be the ScummEngine_v6 defineArray() / nukeArray() functions, that call _res->createResource() and _res->nukeResource() respectively.

Hmm, we DO have some calls to _res->lock() and _res->unlock() in DiMUSE_v2; I can probably fix that after this has been merged, in a separate PR. Thanks!

It's quite possible that the mutex you added is all that's needed for Digital iMUSE (and it would seem to confirm the theory that the basic problem is the thread-unsafeness of the resource manager), but it seems very hackish to add a mutex that makes the resource manager thread safe in some cases, but not others.

I see your point. Though I wouldn't actually know which parts of the resource manager are being called by a thread which is not the main thread (if any! Unless I'm mistaken, audio seems the only thing which is ever being threaded in SCUMM) and therefore are in need of a mutex.

Did I mention that threads scare me? ;-)

Haha! Get in line, you're not alone :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants