MOHAWK: Static in Myst Sounds (GOG ME Release)

[squigley]

I have just compiled the latest git code (ScummVM 2.3.0git15967-g6378f6d675 (May 3 2021 23:17:34)
Features compiled in: Vorbis FLAC MP3 ALSA SEQ sndio TiMidity RGB zLib MPEG2 FluidSynth Theora FreeType2 JPEG PNG TinyGL OpenGL). I also tried with ScummVM 2.0.0 (Mar 11 2018 21:56:27) from dpkg.

I am using Linux Mint 19, kernel 4.15.0-140-generic #144-Ubuntu SMP Fri Mar 19 14:12:35 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

I used to wine to install Myst ME from GOG.com (setup_myst_masterpiece_edition_1.0_svm_update_4_(22598)-1.bin).

It detects the game, it loads and runs fine, from a graphics perspective, however when moving around between screens, there are horrible loud white noise/static blasts for a few seconds when moving to a new screen or new page of a book etc.

[ctoroman]

I Confirm It. The game is interrupted. Log file in the attachment.

[ctoroman]

Workaround: Try to replace wave.cpp file in audio \ decoders folder and rebuild ScummVM

[squigley]

Sorry for the delay, I just checked this yesterday and saw it was updated (not sure if I should have received an email).

I rebuilt with your attached file, and I tested for 15 minutes or so, and I didn't see (hear) the issue at all, so it seems like your version attached above resolves this issue.

[digitall]

squigley: I think ctorman's workaround is basically to apply the recent fixes to WAVE decoder in the latest git master. Please try with the latest nightly build as I think this is fixed in the current development master, and confirm.

[ctoroman]

digitall, I tried on ScummVM 2.3.0git17408-g8e4369cde3 (May 19 2021 05:27:06).
The bug remained, nothing was fixed.

[digitall]

@squigley: OK. Thanks for confirming.

I have had a look at the modified wave.cpp provided by ctoroman and it is basically the state of wave.cpp prior to the following commit (but with a few spurious white space changes and minor bits leftover as well):
​https://github.com/scummvm/scummvm/commit/fed5608e439e9e60a454e06fa27246ea5844bf9d

That commit migrated the wave decoder to using substream rather than locally manually allocating memory buffers so probably is the cause of the issue i.e. if it deallocates
before it should, invalid memory could be played i.e. static.

It is possible that the later ​https://github.com/scummvm/scummvm/commit/4ea4627b1187b343ed95da8fcaa9d5292e9d0217 change is also causing this.

Since you can compile your own git version, I would suggest trying bisecting for this using git-bisect to confirm... or just manually trying the commit prior to fed5608 and fed5608 to see if this is the cause.

[digitall]

Ah, it appears it is as this bug had been previously reported as: https://bugs.scummvm.org/ticket/12523 by ctorman who had determined that commit as the cause. Have notified the author of that commit so hopefully this should be fixed.

[eriktorbjorn]

As an experiment, I made the following change to my local copy:

diff --git a/audio/decoders/wave.cpp b/audio/decoders/wave.cpp
index 67e0b31d83..c34cc3f64d 100644
--- a/audio/decoders/wave.cpp
+++ b/audio/decoders/wave.cpp
@@ -205,7 +205,7 @@ SeekableAudioStream *makeWAVStream(Common::SeekableReadStream *stream, DisposeAf
                        size &= ~(sampleSize - 1);
                }
        }
-       Common::SeekableReadStream *dataStream = new Common::SeekableSubReadStream(stream, stream->pos(), stream->pos() + size, disposeAfterUse);
+       Common::SeekableReadStream *dataStream = new Common::SafeSeekableSubReadStream(stream, stream->pos(), stream->pos() + size, disposeAfterUse);
 
        switch (type) {
        case kWaveFormatMSIMAADPCM:

This seems to fix the problem. My only guess is that the different sound resources point to the same file, and using the plain SeekableSubReadStream you can't be sure if the stream is still pointing to the correct position when it resumes playing a sound.

But I'm not at all sure.

[eriktorbjorn]

Changing the Mohawk engine so that getResource() creates a SafeSeekableSubReadStream() instead of a SeekableSubReadStream() seems to work too.

[ctoroman]

The sound was fixed, but other bugs appeared:

[eriktorbjorn]

As far as I know, nothing has been changed to fix the sound yet. But come to think of it, if it can leave the audio stream at the wrong position it can probably leave other resource streams in the wrong position as well, which I guess could lead to the errors you describe.

If so, it probably won't help to use SafeSeekableSubReadStream, because unless I'm mistaken audio runs in a separate thread, and this class is explicitly documented as "*not* threading safe".

I wonder if this is a problem only with Myst, or if other engines are impacted as well?

[eriktorbjorn]

Bud Tucker in Double Trouble seems to be affect too, but I don't yet understand why. That one seems to just play a looping WAV directly from a file.

But that's what bisecting pointed to:

Bisecting: 0 revisions left to test after this (roughly 0 steps)
[fed5608e439e9e60a454e06fa27246ea5844bf9d] AUDIO: Wrap raw streams in SeekableSubReadStream instead of allocating them

[SupSuper]

Fixed in ​https://github.com/scummvm/scummvm/commit/b263ca4cf3bd545599306cfcfd1566ce9348bc8c