PEGASUS: colossal memory leak, wan't start

[fedor4ever]

scummvm-2.2.0-winxp-win32>scummvm.exe -v
ScummVM 2.2.0 (Sep 14 2020 10:41:13)

mingw-w64-master-9a33d895>scummvm.exe -v
ScummVM 2.3.0git12153-g9a33d895ed (Feb 5 2021 05:21:58)
Features compiled in: TAINTED Vorbis FLAC MP3 RGB zLib MPEG2 FluidSynth Theora AAC A/52 FreeType2 JPEG PNG cloud (servers, local) TinyGL OpenGL (withshaders) GLEW

mingw-w32-master-9a33d895>scummvm.exe -v
ScummVM 2.3.0git12153-g9a33d895ed (Feb 5 2021 06:22:42)
Features compiled in: TAINTED Vorbis FLAC MP3 RGB zLib MPEG2 FluidSynth Theora AAC A/52 FreeType2 JPEG PNG cloud (local) TinyGL OpenGL (with shaders) GLEW

Tried Symbian build

How reproduce: run demo, push Start button to run game,instead intro memory start leaking. This leak consume almost 2Gb Ram!

Os: windows, symbian.

ScummVM 1.8.1 run demo fine.

[sev-]

I cannot reproduce. The memory usage goes from initial 300MB, eventually to 700MB. Did not see 2Gb on a 64bit Mac.

Please, provide more information. Also, since you're a developer, please run a MemoryDoctor on Windows and tell where do you see the memory leaks.

[fedor4ever]

First line in description: scummvm-2.2.0-winxp-win32
It means ScummVM 2.2.0 32 bit edition.

I try run Pegasus demo with ScummVM 2.2.0 on my symbian phone with memory monitor and see 2 things: black screen and free ram started disappear. App crashed when 10mb free ram ends.
I try 1.8.1 - memory doesn't disappear, demo runs fine.

I run Pegasus with on winxp x64 sp2 with task manager started. I see black screen free ram started disappear. When it feed 1974mb ram it stopped. No sounds, only black screen.

Do you say on mac os game use 400 mb? Can you try 1.8.1? I haven't mac.

[criezy]

I tried the demo on Linux with both the current master and branch-2-2, and I can't see any issue. With branch-2-2, it uses 14.1 Mb before starting the game, and then a steady 22.7 Mb while playing. With master it uses a little more memory, 14.6 Mb before starting the game, and then 23.0 Mb while playing. And in both cases I still have plenty of free memory.

I also tried with valgrind, and it didn't detect any memory leak in the pegasus engine.

[criezy]

And I also just tried on macOS 64 bits on branch-2-2 and while playing the pegasus prime demo ScummVM uses a steady 61.6 Mb (it uses 46.7 Mb before starting the game).

So I can't reproduce any bad memory leak either (neither on Linux, nor on mac).

[fedor4ever]

Do you press start button? Do you try windows build? Try x64 version on win7 x64 - memory leaked too.

[criezy]

Yes I did press the Start button.
And no I did not try the windows build (and I can't try it since I don't have a Windows computer).

[fedor4ever]

Okay. Waiting someone else with windows computer.

[fedor4ever]

Still crashes.

Tried:
mingw-w64-master-9a33d895>scummvm.exe -v
ScummVM 2.3.0git12153-g9a33d895ed (Feb 5 2021 05:21:58)
Features compiled in: TAINTED Vorbis FLAC MP3 RGB zLib MPEG2 FluidSynth Theora AAC A/52 FreeType2 JPEG PNG cloud (servers, local) TinyGL OpenGL (withshaders) GLEW

mingw-w32-master-9a33d895>scummvm.exe -v
ScummVM 2.3.0git12153-g9a33d895ed (Feb 5 2021 06:22:42)
Features compiled in: TAINTED Vorbis FLAC MP3 RGB zLib MPEG2 FluidSynth Theora AAC A/52 FreeType2 JPEG PNG cloud (local) TinyGL OpenGL (with shaders) GLEW

Symbian build 74fd09e830211de9c138b43e475f74b014942586 crashes too.

[fedor4ever]

Played with prebuild debuged scummvm.exe with Eclipse debugger on windows. Memory start leaking while try to open file "Images/Items/Inventory/Inventory Panel Movie". Current trace:

Thread #1 0 (Suspended : Step)

Common::MacResManager::open() at macresman.cpp:123 0x26a4302
Common::MacResManager::open() at macresman.cpp:119 0x26a42eb
Common::QuickTimeParser::parseFile() at quicktime.cpp:63 0x26a9126
Video::QuickTimeDecoder::loadFile() at qt_decoder.cpp:60 0x25626d9
Pegasus::Movie::initFromMovieFile() at movie.cpp:62 0x180afd7
Pegasus::InventoryPicture::initInventoryImage() at inventorypicture.cpp:60 0x180ec3f
Pegasus::Interface::validateInventoryPanel() at interface.cpp:175 0x1805299
Pegasus::Interface::createInterface() at interface.cpp:289 0x180594f
Pegasus::PegasusEngine::createInterface() at pegasus.cpp:1 485 0x17a5bf9
Pegasus::PegasusEngine::startNewGame() at pegasus.cpp:1 645 0x17a62e7
Pegasus::PegasusEngine::doGameMenuCommand() at pegasus.cpp:870 0x17a35d2
Pegasus::PegasusEngine::checkGameMenu() at pegasus.cpp:856 0x17a3556
Pegasus::PegasusEngine::handleInput() at pegasus.cpp:1 012 0x17a3fee
Pegasus::InputHandler::handleInput() at input.cpp:263 0x1804350
Pegasus::MainMenu::handleInput() at menu.cpp:344 0x1807fd7
Pegasus::InputHandler::pollForInput() at input.cpp:214 0x18040d7
Pegasus::PegasusEngine::processShell() at pegasus.cpp:1 477 0x17a5bb7
Pegasus::PegasusEngine::run() at pegasus.cpp:195 0x17a0c67
runGame() at main.cpp:307 0x41b44b
scummvm_main() at main.cpp:592 0x41c742
SDL_main() at win32-main.cpp:71 0x418e4c
WinMain@16() at win32-main.cpp:52 0x418da8
main() at 0x30177b4

[fedor4ever]

scummvm-2.1.2-win32 last working version.

[fedor4ever]

Problem in bool MacResManager::open(const String &fileName, Archive &archive) - treats video "Images/Items/Inventory/Inventory Panel Movie" as mac binary.

[criezy]

Thank you for taking the time to look into this further.

This is possibly a regression from ​https://github.com/scummvm/scummvm/commit/1bb7386?
Since it is a simple and short commit, could you try to revert that change locally to see if that fixes the issue for you?

Here is the change to try.

--- a/common/macresman.cpp
+++ b/common/macresman.cpp
@@ -317,10 +317,10 @@ bool MacResManager::isMacBinary(SeekableReadStream &stream) {
 
                uint32 dataSizePad = (((dataSize + 127) >> 7) << 7);
                // Files produced by ISOBuster are not padded, thus, compare with the actual size
-               //uint32 rsrcSizePad = (((rsrcSize + 127) >> 7) << 7);
+               uint32 rsrcSizePad = (((rsrcSize + 127) >> 7) << 7);
 
                // Length check
-               if (MBI_INFOHDR + dataSizePad + rsrcSize <= (uint32)stream.size()) {
+               if (MBI_INFOHDR + dataSizePad + rsrcSizePad == (uint32)stream.size()) {
                        resForkOffset = MBI_INFOHDR + dataSizePad;
                }
        }

[fedor4ever]

Replying to criezy:

Thank you for taking the time to look into this further.

This is possibly a regression from ​https://github.com/scummvm/scummvm/commit/1bb7386?
Since it is a simple and short commit, could you try to revert that change locally to see if that fixes the issue for you?

Here is the change to try:

--- a/common/macresman.cpp
+++ b/common/macresman.cpp
@@ -317,10 +317,10 @@ bool MacResManager::isMacBinary(SeekableReadStream &stream) {
 
                uint32 dataSizePad = (((dataSize + 127) >> 7) << 7);
                // Files produced by ISOBuster are not padded, thus, compare with the actual size
-               //uint32 rsrcSizePad = (((rsrcSize + 127) >> 7) << 7);
+               uint32 rsrcSizePad = (((rsrcSize + 127) >> 7) << 7);
 
                // Length check
-               if (MBI_INFOHDR + dataSizePad + rsrcSize <= (uint32)stream.size()) {
+               if (MBI_INFOHDR + dataSizePad + rsrcSizePad == (uint32)stream.size()) {
                        resForkOffset = MBI_INFOHDR + dataSizePad;
                }
        }

It works for symbian!!! try for windows tomorrow =)

[fedor4ever]

Work for windows too. I open PR asap.

[criezy]

Thanks for testing and confirming this is the source of the issue.
But actually the change here is probably not what we want to do (but opening a pull request could allow to move the discussion there).

Eugene, do you have a suggestion to fix this issue without breaking loading macbinary data from ISOBuster? Would the following work to have a stricter check?

if (MBI_INFOHDR + dataSizePad + rsrcSizePad == (uint32)stream.size() ||
    MBI_INFOHDR + dataSizePad + rsrcSize == (uint32)stream.size()) {
    resForkOffset = MBI_INFOHDR + dataSizePad;
}

[fedor4ever]

I try to reduce that case as minimal as possible. I'm give up because code is crap because it's impossible to write simple test.

I add my testcase with hope someone can go further. It is console app and use scummvm.ini to find video files for PEGASUS demo. It uses existed scummvm code as much as possible. All logic in main.cpp and backends directory. I hope it helps. Currently windows only.