Skip to content

Commit

Permalink
SDL: Implement a hellish workaround to fix bug #3368143.
Browse files Browse the repository at this point in the history
The bug in question is "SDL/OpenGL: Crash when switching renderer backend". To
fix it I added a stupid graphics state copying to the SDL backend, in case the
graphics manager is switched. The implementation of this is considered a pure
workaround, no one should ever do it like this in reality... I just want to
die when looking at this... Not sure why I actually committed it.

Anyway it at least makes the OpenGL backend testable for those who do not
want to fiddle with the config file directly.
  • Loading branch information
Johannes Schickel committed Aug 11, 2011
1 parent 2739d8f commit 0f6e231
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 2 deletions.
5 changes: 4 additions & 1 deletion backends/graphics/surfacesdl/surfacesdl-graphics.cpp
Expand Up @@ -249,7 +249,10 @@ void SurfaceSdlGraphicsManager::setFeatureState(OSystem::Feature f, bool enable)
}

bool SurfaceSdlGraphicsManager::getFeatureState(OSystem::Feature f) {
assert(_transactionMode == kTransactionNone);
// We need to allow this to be called from within a transaction, since we
// currently use it to retreive the graphics state, when switching from
// SDL->OpenGL mode for example.
//assert(_transactionMode == kTransactionNone);

switch (f) {
case OSystem::kFeatureFullscreenMode:
Expand Down
58 changes: 57 additions & 1 deletion backends/platform/sdl/sdl.cpp
Expand Up @@ -49,6 +49,7 @@
#include "backends/graphics/surfacesdl/surfacesdl-graphics.h"
#ifdef USE_OPENGL
#include "backends/graphics/openglsdl/openglsdl-graphics.h"
#include "graphics/cursorman.cpp"
#endif

#include "icons/scummvm.xpm"
Expand Down Expand Up @@ -524,6 +525,22 @@ bool OSystem_SDL::setGraphicsMode(int mode) {
i = _sdlModesCount;
}

// Very hacky way to set up the old graphics manager state, in case we
// switch from SDL->OpenGL or OpenGL->SDL.
//
// This is a probably temporary workaround to fix bugs like #3368143
// "SDL/OpenGL: Crash when switching renderer backend".
const int screenWidth = _graphicsManager->getWidth();
const int screenHeight = _graphicsManager->getHeight();
const bool arState = _graphicsManager->getFeatureState(kFeatureAspectRatioCorrection);
const bool fullscreen = _graphicsManager->getFeatureState(kFeatureFullscreenMode);
const bool cursorPalette = _graphicsManager->getFeatureState(kFeatureCursorPalette);
#ifdef USE_RGB_COLOR
const Graphics::PixelFormat pixelFormat = _graphicsManager->getScreenFormat();
#endif

bool switchedManager = false;

// Loop through modes
while (srcMode->name) {
if (i == mode) {
Expand All @@ -535,16 +552,55 @@ bool OSystem_SDL::setGraphicsMode(int mode) {
_graphicsManager = new SurfaceSdlGraphicsManager(_eventSource);
((SurfaceSdlGraphicsManager *)_graphicsManager)->initEventObserver();
_graphicsManager->beginGFXTransaction();

switchedManager = true;
} else if (_graphicsMode < _sdlModesCount && mode >= _sdlModesCount) {
debug(1, "switching to OpenGL graphics");
delete _graphicsManager;
_graphicsManager = new OpenGLSdlGraphicsManager(_eventSource);
((OpenGLSdlGraphicsManager *)_graphicsManager)->initEventObserver();
_graphicsManager->beginGFXTransaction();

switchedManager = true;
}

_graphicsMode = mode;
return _graphicsManager->setGraphicsMode(srcMode->id);

if (switchedManager) {
#ifdef USE_RGB_COLOR
_graphicsManager->initSize(screenWidth, screenHeight, &pixelFormat);
#else
_graphicsManager->initSize(screenWidth, screenHeight, 0);
#endif
_graphicsManager->setFeatureState(kFeatureAspectRatioCorrection, arState);
_graphicsManager->setFeatureState(kFeatureFullscreenMode, fullscreen);
_graphicsManager->setFeatureState(kFeatureCursorPalette, cursorPalette);

// Worst part about this right now, tell the cursor manager to
// resetup the cursor + cursor palette if necessarily

// First we need to try to setup the old state on the new manager...
if (_graphicsManager->endGFXTransaction() != kTransactionSuccess) {
// Oh my god if this failed the client code might just explode.
return false;
}

// Next setup the cursor again
CursorMan.pushCursor(0, 0, 0, 0, 0, 0);
CursorMan.popCursor();

// Next setup cursor palette if needed
if (cursorPalette) {
CursorMan.pushCursorPalette(0, 0, 0);
CursorMan.popCursorPalette();
}

_graphicsManager->beginGFXTransaction();
// Oh my god if this failed the client code might just explode.
return _graphicsManager->setGraphicsMode(srcMode->id);
} else {
return _graphicsManager->setGraphicsMode(srcMode->id);
}
}

i++;
Expand Down

2 comments on commit 0f6e231

@dreammaster
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the inclusion of 'cursorman.cpp' rather than 'cursorman.h' was intentionally part of fixing the problem or not, but it broke compilation under MSVC, since the CursorManager.cpp file is now present in multiple .obj files, and causes the linking to fail. For now, I've changed the include to be cursorman.h, which at least fixes the error.

@lordhoto
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that was one of the most stupid typos I ever made ;-P. Thanks for fixing this.

Please sign in to comment.