Commit 54b0b4ac4cd4a42de660ea426edb27840ddfd4f0 breaks Myst ME

[naatje80]

I recently migrated to Centos8. I manually generate rpms for the software that is not supplied within the default repositories of the OS. I also compiled an rpm for scummvm using the current master branch (pre-2.3.0 release at the moment). Until know, most of my supported games seem to be working correctly. However, Myst seems to crash at versions points in the game with the error:
scummvm: backends/graphics/surfacesdl/surfacesdl-graphics.cpp:1489: virtual void SurfaceSdlGraphicsManager::copyRectToScreen(const void*, int, int, int, int, int): Assertion `h > 0 && y + h <= _videoMode.screenHeight' failed.
Aborted (core dumped)

For example, I'm already receiving this error if I go to the projector room located on the left just at the first screen of the beginning of Myst.

I've debugged scummvm using gdb, and noticed that the game crashed if the value of height and width is lower than zero. Two examples are (first is when entering the project room. Second is when entering the forest towards the clock tower)
hread 1 "scummvm" hit Breakpoint 1, SurfaceSdlGraphicsManager::copyRectToScreen (this=0x55555a171f90, buf=0x55555d1f9fa0, pitch=176, x=0, y=0, w=-1492, h=-5096)

Thread 1 "scummvm" hit Breakpoint 1, SurfaceSdlGraphicsManager::copyRectToScreen (this=0x55555a123c80, buf=0x55555d0044c0, pitch=1104, x=0, y=0, w=-6108, h=-6020)

In both cases it seems to be at a point a video should be embedded inside the screen. I first assumed that a certain dependency was missing (because it looked like the size of the video could not be determined), however the intro videos are working correctly. Also, I could not find a dependency that seems to be missing for this game.

At first I was still using SDL1. However, switching to SDL2 did not make any difference.

My scummvm build currently supports:
ScummVM 2.3.0git (Nov 12 2020 13:32:27)
Features compiled in: TAINTED Vorbis FLAC MP3 ALSA SEQ TiMidity RGB zLib MPEG2 A/52 FreeType2 JPEG PNG TinyGL OpenGL

I did not encountered this issue before on Centos7 (although I compiled an older release of scummvm, so not sure if something has changed in the mean time).

I'm using the Myst Masterpiece edition (DVD version).

Thank you in advance for your help!

[naatje80]

The issue is caused by this change:
​https://github.com/scummvm/scummvm/commit/54b0b4ac4cd4a42de660ea426edb27840ddfd4f0
Reverting this change fixes the issue for me. But most likely this a general issue and not specific for Centos8....

[naatje80]

Updated title, because this is most likely a general issue.

[eriktorbjorn]

The problem seems to be this bit in the MystAreaVideo constructor (see myst_areas.cpp):

	do {
		c = rlstStream->readByte();
		_videoFile += c;
	} while (c);

	rlstStream->skip(_videoFile.size() & 1);

In your example, it will read the string "/qtw/myst/vlt1watr" into videoFile.

Before the str-common.cpp change, _videoFile.size() was 19 because it apparently counts the string terminator. It would therefore skip the next byte of the stream.

After the change, _videoFile.size() is 18 because the terminator isn't included in the count. It won't skip anything, and everything it reads from the stream after that will be just garbage.

But I'm not sure what the correct fix is. The change to str-common.cpp was made because it broke other engines (I forget which, but I think Blade Runner may have been one of them), so simply reverting it probably isn't a good idea...

[eriktorbjorn]

Changing the last line to something like this might fix it:

	if (!(_videoFile.size() & 1)) {
		rlstStream->skip(1);
	}

But I don't know if it's the correct fix or not. I think that size() is supposed to return the length of the string without counting the string terminator, but...

[eriktorbjorn]

By the way, it also affects the pre-Masterpice edition of Myst.

[naatje80]

@eriktorbjorn: thank you for your feedback. I indeed also understand reverting the change would most likely not be an option. I will test your suggested change to see it this indeed also resolves the issue from my side.

[naatje80]

As expected, this indeed solved the issue. How do we need tot continue? Should someone else review this ticket, or are you going te create a pull request for this? I could also create one, but I think you should get the credits for it. 😉

[eriktorbjorn]

I'm not sure. I was hoping to find someone familiar with the Mohawk engine to discuss it with, but so far I've had no luck. Anyway, I should probably play through the game from start to finish first to make reasonably sure nothing else broke, but I haven't had the time for that either.

[eriktorbjorn]

I've made a slightly modified patch, and played through the non-Masterpiece version of Myst without any crashes. (I figured that one would be the least common one. I don't have the stomach to play through Myst twice in one evening, so this probably makes it easier for someone else to pick up that torch.)

See ​https://github.com/scummvm/scummvm/pull/2652

[eriktorbjorn]

And now I've played through the Masterpiece version of Myst without any crashes. If there are other supported versions, I don't have them so I can't test them.

[naatje80]

Great! Thank you for your time! I did not have the time yet to test the full game, but the parts that I was able to test indeed did seem to work correctly.

[eriktorbjorn]

On second thought, it may have been the Stark engine (The Longest Journey), not the Blade Runner engine that had problems before the change. But if it was, I still have no idea where.

Point is, the only game I know for a fact had problems before the change is Myst III, and the only game I know that has problems after is Myst. If anyone knows more than that, feel free to chime in.

The pull request has a possible fix for Myst III that should make it work regardless of the change. But I'm not as familiar with that game, so I can't vouch for it myself.

[eriktorbjorn]

The pull request has been merged, so hopefully the next daily build should be fine.

[naatje80]

Great, thank you again for all you help!