[Scummvm-git-logs] scummvm master -> dcc37481dcb4655f93a0614fc09f5ca26455bd4b

[AndywinXp]

This automated email contains information about 1 new commit which have been
pushed to the 'scummvm' repo located at https://github.com/scummvm/scummvm .

Summary:
dcc37481dc SWORD1: Add screen access mutex in critical places


Commit: dcc37481dcb4655f93a0614fc09f5ca26455bd4b
    https://github.com/scummvm/scummvm/commit/dcc37481dcb4655f93a0614fc09f5ca26455bd4b
Author: AndywinXp (andywinxp at gmail.com)
Date: 2023-09-25T09:28:56+02:00

Commit Message:
SWORD1: Add screen access mutex in critical places

This was deemed necessary by the fact that we have the main thread
and the timer thread which are constantly reaching out to functions which
update the screen in one form or another. By putting locks around calls to
_system->updateScreen() and _system->getPaletteManager()->setPalette()
we should be able to avoid issues like the one described in bug #14638.

I marked all places which seem sensitive enough when the main thread and
the timer are both in full operation. I might dial it back in the future after
we are sure that the original issue is gone.

Changed paths:
    engines/sword1/control.cpp
    engines/sword1/screen.cpp
    engines/sword1/screen.h
    engines/sword1/sword1.cpp


diff --git a/engines/sword1/control.cpp b/engines/sword1/control.cpp
index 07b7f09e821..4d646bf7a2a 100644
--- a/engines/sword1/control.cpp
+++ b/engines/sword1/control.cpp
@@ -2756,7 +2756,10 @@ void Control::delay(uint32 msecs) {
 			}
 		}
 
+		_screen->_screenAccessMutex.lock();
 		_system->updateScreen();
+		_screen->_screenAccessMutex.unlock();
+
 		_system->delayMillis(10);
 	} while (_system->getMillis() < endTime);
 }
diff --git a/engines/sword1/screen.cpp b/engines/sword1/screen.cpp
index 51ac9adb729..a40eed1431c 100644
--- a/engines/sword1/screen.cpp
+++ b/engines/sword1/screen.cpp
@@ -156,7 +156,9 @@ void Screen::startFadePaletteDown(int speed) {
 		_paletteFadeInfo.fadeCount = 0;
 		_paletteFadeInfo.paletteStatus = FADE_DOWN;
 	} else {
+		_screenAccessMutex.lock();
 		_system->getPaletteManager()->setPalette(_zeroPalette, 0, 256);
+		_screenAccessMutex.unlock();
 	}
 }
 
@@ -190,7 +192,9 @@ void Screen::startFadePaletteUp(int speed) {
 			shiftedPalette[i] = _currentPalette[i] << 2;
 		}
 
+		_screenAccessMutex.lock();
 		_system->getPaletteManager()->setPalette(shiftedPalette, 0, 256);
+		_screenAccessMutex.unlock();
 	}
 }
 
@@ -211,7 +215,9 @@ void Screen::fnSetPalette(uint8 start, uint16 length, uint32 id) {
 	}
 	_resMan->resClose(id);
 
+	_screenAccessMutex.lock();
 	_system->getPaletteManager()->setPalette(_targetPalette + 3 * start, start, length);
+	_screenAccessMutex.unlock();
 }
 
 void Screen::fnSetFadeTargetPalette(uint8 start, uint16 length, uint32 id, int singleColor) {
@@ -238,8 +244,11 @@ void Screen::fnSetFadeTargetPalette(uint8 start, uint16 length, uint32 id, int s
 
 void Screen::fullRefresh(bool soft) {
 	_fullRefresh = true;
-	if (!soft)
+	if (!soft) {
+		_screenAccessMutex.lock();
 		_system->getPaletteManager()->setPalette(_targetPalette, 0, 256);
+		_screenAccessMutex.unlock();
+	}
 }
 
 void Screen::setNextFadeOutToBlack() {
@@ -281,7 +290,9 @@ void Screen::fadePalette() {
 		outPal[i] = ((byte)curValueSigned) << 2;
 	}
 
+	_screenAccessMutex.lock();
 	_system->getPaletteManager()->setPalette((const byte *)outPal, 0, 256);
+	_screenAccessMutex.unlock();
 
 	_paletteFadeInfo.paletteCount--;
 
@@ -317,7 +328,11 @@ bool Screen::showScrollFrame() {
 	uint16 avgScrlY = (uint16)(_oldScrollY + Logic::_scriptVars[SCROLL_OFFSET_Y]) / 2;
 
 	_system->copyRectToScreen(_screenBuf + avgScrlY * _scrnSizeX + avgScrlX, _scrnSizeX, 0, 40, SCREEN_WIDTH, SCREEN_DEPTH);
+
+	_screenAccessMutex.lock();
 	_system->updateScreen();
+	_screenAccessMutex.unlock();
+
 	return true;
 }
 
@@ -430,7 +445,10 @@ void Screen::updateScreen() {
 			scrnBuf += _scrnSizeX * SCRNGRID_Y;
 		}
 	}
+
+	_screenAccessMutex.lock();
 	_system->updateScreen();
+	_screenAccessMutex.unlock();
 }
 
 void Screen::newScreen(uint32 screen) {
@@ -595,7 +613,9 @@ void Screen::initFadePaletteServer() {
 	memset(_paletteFadeInfo.dstPalette, 0, sizeof(_paletteFadeInfo.dstPalette));
 	memset(_paletteFadeInfo.srcPalette, 0, sizeof(_paletteFadeInfo.srcPalette));
 
+	_screenAccessMutex.lock();
 	_system->getPaletteManager()->setPalette((const byte *)_paletteFadeInfo.srcPalette, 0, 256);
+	_screenAccessMutex.unlock();
 }
 
 void Screen::processImage(uint32 id) {
@@ -1376,7 +1396,9 @@ void Screen::fnFlash(uint8 color) {
 		return;
 	}
 
+	_screenAccessMutex.lock();
 	_system->getPaletteManager()->setPalette(targetColor, 0, 1);
+	_screenAccessMutex.unlock();
 
 	if (color == FLASH_RED || color == FLASH_BLUE) {
 		// This is what the original did here to induce a small wait cycle
@@ -1387,7 +1409,9 @@ void Screen::fnFlash(uint8 color) {
 		// We induce a delay instead
 		_system->delayMillis(200);
 
+		_screenAccessMutex.lock();
 		_system->getPaletteManager()->setPalette(_black, 0, 1);
+		_screenAccessMutex.unlock();
 	}
 }
 
diff --git a/engines/sword1/screen.h b/engines/sword1/screen.h
index 6701bf28f7a..2ca6ba05dca 100644
--- a/engines/sword1/screen.h
+++ b/engines/sword1/screen.h
@@ -23,6 +23,7 @@
 #define SWORD1_SCREEN_H
 
 #include "sword1/sworddefs.h"
+#include "common/mutex.h"
 
 class OSystem;
 
@@ -111,6 +112,8 @@ public:
 	void plotPoint(int32 x, int32 y, uint8 color);
 	void bresenhamLine(int32 x1, int32 y1, int32 x2, int32 y2, uint8 color);
 
+	Common::Mutex _screenAccessMutex; // To coordinate actions between the main thread and the palette fade thread
+
 private:
 	// The original values are 6-bit RGB numbers, so they have to be shifted,
 	// except for white, which for some reason has to stay unshifted in order
diff --git a/engines/sword1/sword1.cpp b/engines/sword1/sword1.cpp
index c4f67d5aaca..12f85737337 100644
--- a/engines/sword1/sword1.cpp
+++ b/engines/sword1/sword1.cpp
@@ -1150,7 +1150,9 @@ void SwordEngine::pollInput(uint32 delay) { //copied and mutilated from sky.cpp
 			}
 		}
 
+		_screen->_screenAccessMutex.lock();
 		_system->updateScreen();
+		_screen->_screenAccessMutex.unlock();
 
 		if (delay > 0)
 			_system->delayMillis(10);