Opened 3 years ago

Closed 3 years ago

#11758 closed defect (worksforme)

PEGASUS: colossal memory leak, wan't start

Reported by: fedor4ever Owned by: sev-
Priority: high Component: Engine: Pegasus
Version: Keywords:
Cc: Game: Journeyman Project Pegasus Prime

Description (last modified by 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.

Attachments (1)

mac_file_test.zip (211.9 KB ) - added by fedor4ever 3 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 by sev-, 3 years ago

Owner: set to sev-
Resolution: worksforme
Status: newpending

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.

comment:2 by sev-, 3 years ago

Priority: highlow

comment:3 by fedor4ever, 3 years ago

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.

comment:4 by criezy, 3 years ago

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.

comment:5 by criezy, 3 years ago

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).

comment:6 by fedor4ever, 3 years ago

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

comment:7 by criezy, 3 years ago

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).

comment:8 by fedor4ever, 3 years ago

Okay. Waiting someone else with windows computer.

comment:9 by sev-, 3 years ago

Summary: Pegasus prime demo: colossal memory leak, wan't startPEGASUS: colossal memory leak, wan't start

comment:10 by sev-, 3 years ago

Resolution: worksformefixed
Status: pendingclosed

comment:11 by fedor4ever, 3 years ago

Description: modified (diff)
Priority: lowhigh
Resolution: fixed
Status: closednew

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.

comment:12 by fedor4ever, 3 years ago

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

comment:13 by fedor4ever, 3 years ago

scummvm-2.1.2-win32 last working version.

comment:14 by fedor4ever, 3 years ago

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

comment:15 by criezy, 3 years ago

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;
                }
        }
Last edited 3 years ago by criezy (previous) (diff)

in reply to:  15 comment:16 by fedor4ever, 3 years ago

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 =)

comment:17 by fedor4ever, 3 years ago

Work for windows too. I open PR asap.

comment:18 by fedor4ever, 3 years ago

Owner: changed from sev- to fedor4ever
Resolution: fixed
Status: newclosed

comment:19 by criezy, 3 years ago

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;
}

Last edited 3 years ago by criezy (previous) (diff)

by fedor4ever, 3 years ago

Attachment: mac_file_test.zip added

comment:20 by fedor4ever, 3 years ago

Resolution: fixed
Status: closednew

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.

comment:21 by sev-, 3 years ago

Owner: changed from fedor4ever to sev-
Resolution: worksforme
Status: newclosed
Note: See TracTickets for help on using tickets.