SHERLOCK: Game crashes lightning matches on lab table

[popovi76]

ScummVM Version 2.6.0git1521-g93ebb65979 (nov 20 2021 21:54:57)
and
ScummVM Version 2.5.0 (Oct 1 2021 17:49:58)
OS Version Windows 10 64 Bit

Game crashes on the lab table screen, when using the matches, during the powdery specimen test.
This didn't happen in previous versions, at least in ScummVM Version 2.2.0 (Sep 14 2020 10:39:25), that i used to play the game to the end.

I'm attaching a save game to test the crash.

[eriktorbjorn]

I get the impression that there has always been something wrong here (even in versions where it works, there is a noticeable pause), but other changes caused the error to be reported. I'll see if I can figure anything out...

The good news is that it's easy to reproduce, so it doesn't appear to be specific to any one operating system.

[eriktorbjorn]

I can see that ImageFile::load() loads a bunch of images with really weird dimensions. And I have a feeling it's because it doesn't stop in time. I added some debug messages to print the dimensions of the frames it loaded:

=> Loading image file from name: 59MATCH4.vgs
    ImageFile::load: 53x98
    ImageFile::load: 53x97
    ImageFile::load: 53x101
    ImageFile::load: 53x105
    ImageFile::load: 53x95
    ImageFile::load: 58x83
    ImageFile::load: 67x74
    ImageFile::load: 69x78
    ImageFile::load: 63x68
    ImageFile::load: 55x65
    ImageFile::load: 47x59
    ImageFile::load: 37x52
    ImageFile::load: 26x39
    ImageFile::load: 0x26
    ImageFile::load: 2348x0
    ImageFile::load: 0x0
    ImageFile::load: 0x0
    ImageFile::load: 0x18944
    ImageFile::load: 0x0
    ImageFile::load: 0x0
    ImageFile::load: 0x0
    ImageFile::load: 0x0
    ImageFile::load: 0x0
    ImageFile::load: 0x0
    ImageFile::load: 0x0
    ImageFile::load: 0x0
    ImageFile::load: 0x0
    ImageFile::load: 45641x0
scummvm: graphics/surface.cpp:67: void Graphics::Surface::create(int16, int16, const Graphics::PixelFormat&): Assertion `width >= 0 && height >= 0' failed.
Aborted

Note that after the normal-looking sizes, one of the dimensions of the frame is always 0, which probably means it's not advancing very far in the stream. It doesn't crash, though, until it encounters a frame with dimensions that overflow into the negative when cast to a signed 16-bit integer.

[eriktorbjorn]

I don't see any obvious errors in this function, though. I wonder if the problem is that ScummVM reads too far ahead into the stream, or if there is garbage in the data after the last frame. (I believe the original engine only created an index of the frames at this point, it didn't actually decode them yet?)

[eriktorbjorn]

These are the dimensions of the frames that the engine tries to load:

59MATCH4.vgs: 53x98
59MATCH4.vgs: 53x97
59MATCH4.vgs: 53x101
59MATCH4.vgs: 53x105
59MATCH4.vgs: 53x95
59MATCH4.vgs: 58x83
59MATCH4.vgs: 67x74
59MATCH4.vgs: 69x78
59MATCH4.vgs: 63x68
59MATCH4.vgs: 55x65
59MATCH4.vgs: 47x59
59MATCH4.vgs: 37x52
59MATCH4.vgs: 26x39
59MATCH4.vgs: 0x26
59MATCH4.vgs: 2348x0         <-- This is the last frame that's actually drawn
59MATCH4.vgs: 0x0
59MATCH4.vgs: 0x0
59MATCH4.vgs: 0x18944
59MATCH4.vgs: 0x0
59MATCH4.vgs: 0x0
59MATCH4.vgs: 0x0
59MATCH4.vgs: 0x0
59MATCH4.vgs: 0x0
59MATCH4.vgs: 0x0
59MATCH4.vgs: 0x0
59MATCH4.vgs: 0x0
59MATCH4.vgs: 0x0
59MATCH4.vgs: 45641x0        <-- This is the frame that causes the crash
59MATCH4.vgs: 0x0
59MATCH4.vgs: 0x0
59MATCH4.vgs: 18761x25165    <-- This frame probably caused the delay earlier
59MATCH4.vgs: 53x98

So it is trying to draw some bad frames, but that doesn't cause any graphical glitches that I can see so it's probably harmless.

The real harm comes from the engine trying to decode some even worse frames. So I guess one way would be to have it decode the frames on demand, rather than in advance. I've attached a proof-of-concept patch for doing that.

[eriktorbjorn]

It's unknown if this bug also affects the 3DO version, since that one's almost completely unsupported. (I tried playing it far enough to use the lab table, and it crashed for what was probably completely different reasons.)

[eriktorbjorn]

Here are two possible ways ahead. One tries to do frame-decoding on demand rather than in advance (but it won't work for all cases), and another simply adds a workaround for the bad animation:

​https://github.com/eriktorbjorn/scummvm/tree/sherlock-frames-on-demand
​https://github.com/eriktorbjorn/scummvm/tree/sherlock-workaround-bad-frames

Whichever way we proceed, both of the Sherlock games will have to be tested before 2.5.1. (Oh dear. I like the Serrated Scalpel, but the Rose Tatto is such a slow and ponderous game...)

[dreammaster]

Since the first one is cleaner, I'm pulling into master and the 2.5 branch