GRAPHICS: HQScaler crash when exiting Indy3-Mac in HQ3X

[dwatteau]

Current Git HEAD.

I can't say if this is an issue in the graphics code, or in what the SCUMM engine does in that case.

To reproduce:

1. Use the attached .ini file for ScummVM, so that the graphics settings are set to HQ3X (so: don't disable HQ scalers in your build).
2. Start the Macintosh release of Indy3 (which is now displayed with the original interface).
3. Go to the first playable room. Move your cursor to the upper left corner of the old Macintosh interface, on the Apple logo. The game pauses.
4. At the same time, exit ScummVM with the close button of your actual window manager.

ScummVM will then crash.

GDB logs attached below.

[dwatteau]

GDB backtrace when the crash occurs

[dwatteau]

scummvm.ini settings to reproduce the issue

[dwatteau]

Screenshot showing what is being done just before the crash

[eriktorbjorn]

I wasn't able to reproduce it on my initial try, so I'll have to give it another go later. Maybe with Valgrind. (Though a 3x scaler on a game that runs at 640x480 pixels... My monitor doesn't have that kind of resolution! :-)

[eriktorbjorn]

Still no luck in reproducing it, and Valgrind says nothing. I did run into some other strange graphics corruption where there was a black box instead of part of the background, but I haven't been able to reproduce that.

Valgrind told me nothing.

[eriktorbjorn]

Using the attached .ini file, I was able to reproduce the crash. Valgrind doesn't tell me anything really new:

==25802== Invalid read of size 4
==25802==    at 0x6E3870C: void HQ3x_implementation<Graphics::ColorMasks<888> >(unsigned char const*, unsigned int, unsigned char*, unsigned int, int, int, unsigned int const*) (hq.cpp:2143)
==25802==    by 0x6E49409: HQScaler::HQ3x32(unsigned char const*, unsigned int, unsigned char*, unsigned int, int, int) (hq.cpp:5033)
==25802==    by 0x6DCADD4: HQScaler::scaleIntern(unsigned char const*, unsigned int, unsigned char*, unsigned int, int, int, int, int) (hq.cpp:5055)
==25802==    by 0x688786D: Scaler::scale(unsigned char const*, unsigned int, unsigned char*, unsigned int, int, int, int, int) (scalerplugin.cpp:56)
==25802==    by 0x672C3A7: SurfaceSdlGraphicsManager::blitCursor() (surfacesdl-graphics.cpp:2318)
==25802==    by 0x6726A8C: SurfaceSdlGraphicsManager::setGraphicsModeIntern() (surfacesdl-graphics.cpp:702)
==25802==    by 0x6725889: SurfaceSdlGraphicsManager::endGFXTransaction() (surfacesdl-graphics.cpp:428)
==25802==    by 0x669D547: ModularGraphicsBackend::endGFXTransaction() (modular-backend.cpp:145)
==25802==    by 0x26638A2: setupGraphics(OSystem&) (main.cpp:381)
==25802==    by 0x26658B7: scummvm_main (main.cpp:884)
==25802==    by 0x266015B: main (posix-main.cpp:44)
==25802==  Address 0x40dc9d260 is not stack'd, malloc'd or (recently) free'd

However, I did notice two ways which each individually avoid the crash. Neither are solutions, of course, but perhaps they provide a clue:

• If I uncheck the global GUI option "Return to the launcher when leaving a game", it doesn't crash.
• If I change the global scaler from 3x to 4x, making the ScummVM launcher window almost as large as the game window, it doesn't crash.

So I guess that explains why it crashes in the HQ3x scaler, even though the game uses a 2x scaler?

[eriktorbjorn]

By the way, you don't even have to play to the first interactive part of the game, you can reproduce it on the Lucasfilm logo screen. (The mouse is hidden, but will still activate the menus.)

I tried doing the same thing in another game with menus (one of the Pink Panther games), but unfortunately (?) the crash did not happen there.

So other than it apparently being in the process of scaling the mouse cursor, I don't know what's going on here.

[eriktorbjorn]

Well, something's odd about the HQ3x scaler or, more likely, the way it's used here. When it gets to the end of the HQ3x_implementation() function, it does this:

p += nextlineSrc - width;
q += (nextlineDst - width) * 3;

At this point, width is 16, nextlineSrc is 6, and nextlineDst is 12. And I think that through the wonders of signed math, adding "nextlineSrc - width" is not the same as first adding nextlineSrc and then subtracting width, because the value of p increases by a lot.

Breaking it up like that isn't the solution to the problem, though it does make the code run for a bit longer before it crashes. But perhaps it is a clue?

[eriktorbjorn]

Also, from what I understand the HQ scaler shouldn't be allowed to scale the cursor, but at this point it apparently incorrectly thinks it's using the Normal scaler instead? That needs to be investigated.

[eriktorbjorn]

It looks like when SurfaceSdlGraphicsManager creates a new _scalerPlugin and _scaler, it does not create a new _mouseScaler. So while the scaler changes from HQ3x (not allowed to scale the cursor) to Normal2x (allowed to scale the cursor), _mouseScaler will still be a HQ scaler.

So it ought to be possible to reproduce this with other games, except perhaps it also requires the cursor to be a particular size?

I'm not really familiar with the graphical backend, but something like this seems to fix it for me:

diff --git a/backends/graphics/surfacesdl/surfacesdl-graphics.cpp b/backends/graphics/surfacesdl/surfacesdl-graphics.cpp
index 6990aa19c73..0eb3ee43d18 100644
--- a/backends/graphics/surfacesdl/surfacesdl-graphics.cpp
+++ b/backends/graphics/surfacesdl/surfacesdl-graphics.cpp
@@ -683,6 +683,11 @@ void SurfaceSdlGraphicsManager::setGraphicsModeIntern() {

                _scalerPlugin = &_scalerPlugins[_videoMode.scalerIndex]->get<ScalerPluginObject>();
                _scaler = _scalerPlugin->createInstance(format);
+
+               if (_mouseScaler != nullptr) {
+                       delete _mouseScaler;
+                       _mouseScaler = _scalerPlugin->createInstance(_cursorFormat);
+               }
        }

        _scaler->setFactor(_videoMode.scaleFactor);

Maybe it should also call _mouseScaler->setFactor(_videoMode.scaleFactor), but that's also done before the mouse is scaled in SurfaceSdlGraphicsManager::blitCursor(). As I said, I don't know...

[eriktorbjorn]

I forgot to mention that I've submitted that as a pull request: ​https://github.com/scummvm/scummvm/pull/5644

I'm hoping for some feedback before doing anything more.

[eriktorbjorn]

The pull request has been merged (both to master and to branch-2-8), so I'm going to assume that this bug report is fixed now.