Opened 12 years ago

Closed 12 years ago

#3135 closed defect (fixed)

BASS: Crash when changing scalers during intro

Reported by: eriktorbjorn Owned by: fingolfin
Priority: high Component: Engine: Sky
Keywords: Cc:
Game: Beneath a Steel Sky

Description

BASS 0.0372
Current SVN snapshot (but also tested with 0.9.1)

It seems that if you change scaler (or, I guess, do anything else that forces the screen to redraw) during the BASS intro, there is a tiny chance that ScummVM will crash.

I managed to capture the crash in Valgrind, though since I had been making local changes to BASS at the time (touching screen.h, intro.cpp, sky.cpp and screen.cpp), the line number may be slightly off:

==24221== Thread 2:
==24221== Invalid read of size 1
==24221== at 0x422F11D: (within /usr/lib/libSDL-1.2.so.0.11.0)
==24221== by 0x424B4A3: SDL_LowerBlit (in /usr/lib/libSDL-1.2.so.0.11.0)
==24221== by 0x424B6D3: SDL_UpperBlit (in /usr/lib/libSDL-1.2.so.0.11.0)
==24221== by 0x8089CA8: OSystem_SDL::internUpdateScreen() (graphics.cpp:678)
==24221== by 0x8088558: OSystem_SDL::updateScreen() (graphics.cpp:512)
==24221== by 0x4E36830: Sky::Screen::processSequence() (screen.cpp:516)
==24221== by 0x4E368DA: Sky::Screen::handleTimer() (screen.cpp:418)
==24221== by 0x4E37B8E: Sky::SkyEngine::gotTimerTick() (sky.cpp:510)
==24221== by 0x4E37BB2: Sky::SkyEngine::timerHandler(void*) (sky.cpp:505)
==24221== by 0x814F840: DefaultTimerManager::handler() (default-timer.cpp:107)
==24221== by 0x808C34A: timer_handler(unsigned, void*) (sdl.cpp:49)
==24221== by 0x422B30B: (within /usr/lib/libSDL-1.2.so.0.11.0)
==24221== Address 0x0 is not stack'd, malloc'd or (recently) free'd

Maybe it has to do with the animations being run by a timer, i.e. not the main thread? That sort of thing has haunted us in the past...

Ticket imported from: #1690813. Ticket imported from: bugs/3135.

Change History (4)

comment:1 Changed 12 years ago by fingolfin

Priority: normalhigh

comment:2 Changed 12 years ago by fingolfin

This is an odd one. I wonder whether it's really caused by a bug in the BASS code, though, or if it's not really a problem in the backend... ?
Maybe the problem is triggered because a redraw is initiated from within a timer? Although at least in theory, a mutex should protect us from problems there.

The source code has shifted since you posted this, but looking at <http://scummvm.svn.sourceforge.net/viewvc/scummvm/scummvm/trunk/backends/platform/sdl/graphics.cpp?revision=24706&view=markup>,
it seems the offending line in the backend is this:

SDL_BlitSurface(_osdSurface, 0, _hwscreen, 0);

so it crashed while trying to draw the OSD surface. Hu... could it be that in another thread, we are right in the middle of OSystem_SDL::displayMessageOnOSD(), which doesn't perform thread locking? If that's the case, maybe the following simple trick will help: Only change _osdAlpha at the very end of OSystem_SDL::displayMessageOnOSD(), to make sure the OSD won't be drawn before everything is fully setup.

comment:3 Changed 12 years ago by fingolfin

Owner: set to fingolfin
Resolution: fixed
Status: newclosed

comment:4 Changed 12 years ago by fingolfin

OK, found and fixed the problem. The displayMessageOnOSD() simply needs to lock the graphics mutex. Otherwise it might be called *while* updateScreen is blitting the OSD surface, which obviously can cause troubles.

Note: See TracTickets for help on using tickets.