AUDIO: (FM-TOWNS/PC-98) - fix regression from 0e734722

My commit 0e734722 causes lockups in SCUMM (sometimes) and SCI (very often). I didn't like the way I had fixed this before, but in the end I now had to do it in a similar way.

10 files changed, 91 insertions, 37 deletions

diff --git a/audio/softsynth/fmtowns_pc98/pc98_audio.cpp b/audio/softsynth/fmtowns_pc98/pc98_audio.cpp
index 9ab1ec7659..144e9834b0 100644
--- a/audio/softsynth/fmtowns_pc98/pc98_audio.cpp
+++ b/audio/softsynth/fmtowns_pc98/pc98_audio.cpp
@@ -47,6 +47,7 @@ public:
 	void ssgSetVolume(int volume);
 
 	Common::Mutex &mutex();
+	int mixerThreadLockCounter() const;
 
 private:
 	bool assignPluginDriver(PC98AudioCore *owner, PC98AudioPluginDriver *driver, bool externalMutexHandling = false);
@@ -192,6 +193,10 @@ Common::Mutex &PC98AudioCoreInternal::mutex() {
 	return _mutex;
 }
 
+int PC98AudioCoreInternal::mixerThreadLockCounter() const {
+	return _mixerThreadLockCounter;
+}
+
 bool PC98AudioCoreInternal::assignPluginDriver(PC98AudioCore *owner, PC98AudioPluginDriver *driver, bool externalMutexHandling) {
 	Common::StackLock lock(_mutex);
 	if (_refCount <= 1)
@@ -215,13 +220,11 @@ void PC98AudioCoreInternal::removePluginDriver(PC98AudioCore *owner) {
 }
 
 void PC98AudioCoreInternal::timerCallbackA() {
-	Common::StackLock lock(_mutex);
 	if (_drv && _ready)
 		_drv->timerCallbackA();
 }
 
 void PC98AudioCoreInternal::timerCallbackB() {
-	Common::StackLock lock(_mutex);
 	if (_drv && _ready)
 		_drv->timerCallbackB();
 }
@@ -283,12 +286,30 @@ PC98AudioCore::MutexLock PC98AudioCore::stackLockMutex() {
 	return MutexLock(_internal);
 }
 
-PC98AudioCore::MutexLock::MutexLock(PC98AudioCoreInternal *pc98int) : _pc98int(pc98int) {
-	if (_pc98int)
+PC98AudioCore::MutexLock PC98AudioCore::stackUnlockMutex() {
+	return MutexLock(_internal, _internal->mixerThreadLockCounter());
+}
+
+PC98AudioCore::MutexLock::MutexLock(PC98AudioCoreInternal *pc98int, int reverse) : _pc98int(pc98int), _count(reverse) {
+	if (!_pc98int)
+		return;
+
+	if (!reverse) {
 		_pc98int->mutex().lock();
+		return;
+	}
+
+	while (reverse--)
+		_pc98int->mutex().unlock();
 }
 
 PC98AudioCore::MutexLock::~MutexLock() {
-	if (_pc98int)
+	if (!_pc98int)
+		return;
+
+	if (!_count)
 		_pc98int->mutex().unlock();
+
+	while (_count--)
+		_pc98int->mutex().lock();
 }
diff --git a/audio/softsynth/fmtowns_pc98/pc98_audio.h b/audio/softsynth/fmtowns_pc98/pc98_audio.h
index 30950e4882..016e242de4 100644
--- a/audio/softsynth/fmtowns_pc98/pc98_audio.h
+++ b/audio/softsynth/fmtowns_pc98/pc98_audio.h
@@ -71,11 +71,13 @@ public:
 	public:
 		~MutexLock();
 	private:
-		MutexLock(PC98AudioCoreInternal *pc98int);
+		MutexLock(PC98AudioCoreInternal *pc98int, int reverse = 0);
 		PC98AudioCoreInternal *_pc98int;
+		int _count;
 	};
 
 	MutexLock stackLockMutex();
+	MutexLock stackUnlockMutex();
 
 private:
 	PC98AudioCoreInternal *_internal;
diff --git a/audio/softsynth/fmtowns_pc98/towns_audio.cpp b/audio/softsynth/fmtowns_pc98/towns_audio.cpp
index cc847e39fe..49961dee32 100644
--- a/audio/softsynth/fmtowns_pc98/towns_audio.cpp
+++ b/audio/softsynth/fmtowns_pc98/towns_audio.cpp
@@ -135,11 +135,11 @@ private:
 
 class TownsAudioInterfaceInternal : public TownsPC98_FmSynth {
 private:
-	TownsAudioInterfaceInternal(Audio::Mixer *mixer, TownsAudioInterface *owner, TownsAudioInterfacePluginDriver *driver);
+	TownsAudioInterfaceInternal(Audio::Mixer *mixer, TownsAudioInterface *owner, TownsAudioInterfacePluginDriver *driver, bool externalMutex);
 public:
 	~TownsAudioInterfaceInternal();
 
-	static TownsAudioInterfaceInternal *addNewRef(Audio::Mixer *mixer, TownsAudioInterface *owner, TownsAudioInterfacePluginDriver *driver);
+	static TownsAudioInterfaceInternal *addNewRef(Audio::Mixer *mixer, TownsAudioInterface *owner, TownsAudioInterfacePluginDriver *driver, bool externalMutex);
 	static void releaseRef(TownsAudioInterface *owner);
 
 	bool init();
@@ -263,6 +263,7 @@ private:
 
 	TownsAudioInterfacePluginDriver *_drv;
 	void *_drvOwner;
+	bool _externalMutex;
 	bool _ready;
 
 	static TownsAudioInterfaceInternal *_refInstance;
@@ -274,9 +275,9 @@ private:
 	static const uint8 _fmDefaultInstrument[];
 };
 
-TownsAudioInterfaceInternal::TownsAudioInterfaceInternal(Audio::Mixer *mixer, TownsAudioInterface *owner, TownsAudioInterfacePluginDriver *driver) :
+TownsAudioInterfaceInternal::TownsAudioInterfaceInternal(Audio::Mixer *mixer, TownsAudioInterface *owner, TownsAudioInterfacePluginDriver *driver, bool externalMutex) :
 	TownsPC98_FmSynth(mixer, kTypeTowns), _fmInstruments(0), _pcmInstruments(0), _pcmChan(0), _waveTables(0), _waveTablesTotalDataSize(0),
-	_tickLength(0x08), _envDuration(0x30), _timer(0), _drv(driver), _drvOwner(owner), _pcmSfxChanMask(0), _outputVolumeFlags(0),
+	_tickLength(0x08), _envDuration(0x30), _timer(0), _drv(driver), _drvOwner(owner), _externalMutex(externalMutex), _pcmSfxChanMask(0), _outputVolumeFlags(0),
 	_fmChanPlaying(0), _musicVolume(Audio::Mixer::kMaxMixerVolume), _sfxVolume(Audio::Mixer::kMaxMixerVolume),
 	_numReservedChannels(0), _numWaveTables(0), _updateOutputVol(false), _ready(false) {
 
@@ -410,10 +411,10 @@ TownsAudioInterfaceInternal::~TownsAudioInterfaceInternal() {
 	delete[] _pcmChan;
 }
 
-TownsAudioInterfaceInternal *TownsAudioInterfaceInternal::addNewRef(Audio::Mixer *mixer, TownsAudioInterface *owner, TownsAudioInterfacePluginDriver *driver) {
+TownsAudioInterfaceInternal *TownsAudioInterfaceInternal::addNewRef(Audio::Mixer *mixer, TownsAudioInterface *owner, TownsAudioInterfacePluginDriver *driver, bool externalMutex) {
 	_refCount++;
 	if (_refCount == 1 && _refInstance == 0)
-		_refInstance = new TownsAudioInterfaceInternal(mixer, owner, driver);
+		_refInstance = new TownsAudioInterfaceInternal(mixer, owner, driver, externalMutex);
 	else if (_refCount < 2 || _refInstance == 0)
 		error("TownsAudioInterfaceInternal::addNewRef(): Internal reference management failure");
 	else if (!_refInstance->assignPluginDriver(owner, driver))
@@ -576,16 +577,30 @@ void TownsAudioInterfaceInternal::nextTickEx(int32 *buffer, uint32 bufferSize) {
 }
 
 void TownsAudioInterfaceInternal::timerCallbackA() {
-	Common::StackLock lock(_mutex);
-	if (_drv && _ready)
+	if (_drv && _ready) {
+		int restore = 0;
+		if (_externalMutex) {
+			for (; restore < _mixerThreadLockCounter; ++restore)
+				_mutex.unlock();
+		}
 		_drv->timerCallback(0);
+		while (restore--)
+			_mutex.lock();
+	}
 }
 
 void TownsAudioInterfaceInternal::timerCallbackB() {
-	Common::StackLock lock(_mutex);
 	if (_ready) {
-		if (_drv)
+		if (_drv) {
+			int restore = 0;
+			if (_externalMutex) {
+				for (; restore < _mixerThreadLockCounter; ++restore)
+					_mutex.unlock();
+			}
 			_drv->timerCallback(1);
+			while (restore--)
+				_mutex.lock();
+		}
 		callback(80);
 	}
 }
@@ -1935,8 +1950,8 @@ void TownsAudio_WaveTable::clear() {
 	data = 0;
 }
 
-TownsAudioInterface::TownsAudioInterface(Audio::Mixer *mixer, TownsAudioInterfacePluginDriver *driver) {
-	_intf = TownsAudioInterfaceInternal::addNewRef(mixer, this, driver);
+TownsAudioInterface::TownsAudioInterface(Audio::Mixer *mixer, TownsAudioInterfacePluginDriver *driver, bool externalMutex) {
+	_intf = TownsAudioInterfaceInternal::addNewRef(mixer, this, driver, externalMutex);
 }
 
 TownsAudioInterface::~TownsAudioInterface() {
diff --git a/audio/softsynth/fmtowns_pc98/towns_audio.h b/audio/softsynth/fmtowns_pc98/towns_audio.h
index 815875c1ac..fce82c7868 100644
--- a/audio/softsynth/fmtowns_pc98/towns_audio.h
+++ b/audio/softsynth/fmtowns_pc98/towns_audio.h
@@ -37,7 +37,7 @@ public:
 
 class TownsAudioInterface {
 public:
-	TownsAudioInterface(Audio::Mixer *mixer, TownsAudioInterfacePluginDriver *driver);
+	TownsAudioInterface(Audio::Mixer *mixer, TownsAudioInterfacePluginDriver *driver, bool externalMutex = false);
 	~TownsAudioInterface();
 
 	enum ErrorCode {
diff --git a/audio/softsynth/fmtowns_pc98/towns_pc98_fmsynth.cpp b/audio/softsynth/fmtowns_pc98/towns_pc98_fmsynth.cpp
index 558c389ebd..db81485426 100644
--- a/audio/softsynth/fmtowns_pc98/towns_pc98_fmsynth.cpp
+++ b/audio/softsynth/fmtowns_pc98/towns_pc98_fmsynth.cpp
@@ -963,7 +963,7 @@ TownsPC98_FmSynth::TownsPC98_FmSynth(Audio::Mixer *mixer, EmuType type) :
 	_samplesPerWaitCycle(type == kType26 ? 72 : 144),
 #endif
 	_outputRate(mixer->getOutputRate()), _rateScale(type == kType26 ? 0 : 1),
-	_volMaskA(0), _volMaskB(0), _volumeA(255), _volumeB(255), _ready(false) {
+	_volMaskA(0), _volMaskB(0), _volumeA(255), _volumeB(255), _mixerThreadLockCounter(0), _ready(false) {
 
 	memset(&_timers[0], 0, sizeof(ChipTimer));
 	memset(&_timers[1], 0, sizeof(ChipTimer));
@@ -1119,10 +1119,13 @@ uint8 TownsPC98_FmSynth::readReg(uint8 part, uint8 regAddress) {
 }
 
 int TownsPC98_FmSynth::readBuffer(int16 *buffer, const int numSamples) {
-	Common::StackLock lock(_mutex);
-	if (!_ready)
+	_mutex.lock();
+	if (!_ready) {
+		_mutex.unlock();
 		return 0;
-
+	}
+	_mixerThreadLockCounter++;
+	
 	// This assumes that the numSamples parameter will be the same on most (if not on all) calls to readBuffer(),
 	// although I don't know whether this is true for all backends. There is no need to reallocate the temp
 	// buffer every time, unless its size needs to be increased.
@@ -1236,6 +1239,9 @@ int TownsPC98_FmSynth::readBuffer(int16 *buffer, const int numSamples) {
 		_numPending = render + _numPending - consumed;
 	}
 
+	_mutex.unlock();
+	_mixerThreadLockCounter--;
+
 	return numSamples;
 }
 
diff --git a/audio/softsynth/fmtowns_pc98/towns_pc98_fmsynth.h b/audio/softsynth/fmtowns_pc98/towns_pc98_fmsynth.h
index dea7d9f84b..8b81cb8307 100644
--- a/audio/softsynth/fmtowns_pc98/towns_pc98_fmsynth.h
+++ b/audio/softsynth/fmtowns_pc98/towns_pc98_fmsynth.h
@@ -114,6 +114,7 @@ protected:
 	const bool _hasPercussion;
 
 	Common::Mutex _mutex;
+	int _mixerThreadLockCounter;
 
 private:
 	void generateTables();
diff --git a/engines/sci/sound/drivers/fmtowns.cpp b/engines/sci/sound/drivers/fmtowns.cpp
index 7475b01a6f..4dd74bc42d 100644
--- a/engines/sci/sound/drivers/fmtowns.cpp
+++ b/engines/sci/sound/drivers/fmtowns.cpp
@@ -405,7 +405,7 @@ int TownsMidiPart::allocateChannel() {
 }
 
 MidiDriver_FMTowns::MidiDriver_FMTowns(Audio::Mixer *mixer, SciVersion version) : _version(version), _timerProc(0), _timerProcPara(0), _baseTempo(10080), _ready(false), _isOpen(false), _masterVolume(0x0f), _soundOn(true) {
-	_intf = new TownsAudioInterface(mixer, this);
+	_intf = new TownsAudioInterface(mixer, this, true);
 	_out = new TownsChannel*[6];
 	for (int i = 0; i < 6; i++)
 		_out[i] = new TownsChannel(this, i);
diff --git a/engines/sci/sound/drivers/pc9801.cpp b/engines/sci/sound/drivers/pc9801.cpp
index a4b634f1dd..13cb9d0371 100644
--- a/engines/sci/sound/drivers/pc9801.cpp
+++ b/engines/sci/sound/drivers/pc9801.cpp
@@ -328,6 +328,7 @@ private:
 	const uint16 _baseTempo;
 
 	PC98AudioCore *_pc98a;
+	Audio::Mixer *_mixer;
 	const SciVersion _version;
 };
 
@@ -1317,27 +1318,28 @@ void MidiPart_PC9801::assignFreeChannels() {
 MidiPart_PC9801 **MidiDriver_PC9801::_parts = 0;
 
 MidiDriver_PC9801::MidiDriver_PC9801(Audio::Mixer *mixer, SciVersion version)
-	: _version(version), _pc98a(0), _chan(0), _numChan(6), _internalVersion(0xFF), _ssgPatchOffset(0xFF), _patchSize(0),
+	: _mixer(mixer), _version(version), _pc98a(0), _chan(0), _numChan(6), _internalVersion(0xFF), _ssgPatchOffset(0xFF), _patchSize(0),
 	_timerProc(0), _timerProcPara(0), _baseTempo(10080), _ready(false), _isOpen(false), _masterVolume(0x0f) ,_soundOn(true), _playID(0),
 	_polyphony(9), _channelMask1(0x10), _channelMask2(0x02) {
-	_pc98a =
-#ifdef SCI_PC98_AUDIO_EXTENDED
-		new PC98AudioCore(mixer, this, kType86);
-#else
-		new PC98AudioCore(mixer, this, kType26);
-#endif
 }
 
 MidiDriver_PC9801::~MidiDriver_PC9801() {
-	_ready = false;
 	close();
-	delete _pc98a;
 }
 
 int MidiDriver_PC9801::open() {
 	if (_isOpen)
 		return MERR_ALREADY_OPEN;
 
+	if (!_pc98a) {
+		_pc98a =
+#ifdef SCI_PC98_AUDIO_EXTENDED
+		new PC98AudioCore(_mixer, this, kType86);
+#else
+		new PC98AudioCore(_mixer, this, kType26);
+#endif
+	}
+
 	if (!_ready) {
 		if (!_pc98a->init())
 			return MERR_CANNOT_CONNECT;
@@ -1413,7 +1415,8 @@ void MidiDriver_PC9801::close() {
 	bool ready = _ready;
 	_isOpen = _ready = false;
 
-	PC98AudioCore::MutexLock lock = _pc98a->stackLockMutex();
+	delete _pc98a;
+	_pc98a = 0;
 
 	if (_parts) {
 		for (int i = 0; i < 16; ++i) {
@@ -1624,8 +1627,14 @@ bool MidiDriver_PC9801::loadInstruments(const SciSpan<const uint8> &data) {
 }
 
 void MidiDriver_PC9801::updateParser() {
-	if (_timerProc)
+	if (_timerProc) {
+		// The mutex lock has to be lifted, before entering the SCI engine space. The engine has its owns mutex and the different threads
+		// will often cause an immediate lockup (each thread caught in the mutex lock of the other). I consider this safe for all realistic
+		// scenarios, since a reentry of the space guarded by the PC98 audio mutex is not possible without triggering another mutex lock.
+		// I have also rearranged the driver deconstruction appropriately.
+		PC98AudioCore::MutexLock tempUnlock = _pc98a->stackUnlockMutex();
 		_timerProc(_timerProcPara);
+	}
 }
 
 void MidiDriver_PC9801::updateChannels() {
diff --git a/engines/scumm/imuse/drivers/fmtowns.cpp b/engines/scumm/imuse/drivers/fmtowns.cpp
index cf15fcdb25..a85b4597e4 100644
--- a/engines/scumm/imuse/drivers/fmtowns.cpp
+++ b/engines/scumm/imuse/drivers/fmtowns.cpp
@@ -827,7 +827,7 @@ const uint8 TownsMidiInputChannel::_programAdjustLevel[] = {
 
 MidiDriver_TOWNS::MidiDriver_TOWNS(Audio::Mixer *mixer) : _timerProc(0), _timerProcPara(0), _channels(0), _out(0),
 	_baseTempo(10080), _chanState(0), _operatorLevelTable(0), _tickCounter(0), _rand(1), _allocCurPos(0), _isOpen(false) {
-	_intf = new TownsAudioInterface(mixer, this);
+	_intf = new TownsAudioInterface(mixer, this, true);
 
 	_channels = new TownsMidiInputChannel*[32];
 	for (int i = 0; i < 32; i++)
diff --git a/engines/scumm/players/player_towns.cpp b/engines/scumm/players/player_towns.cpp
index c5f6b21841..a2b3fdb0ff 100644
--- a/engines/scumm/players/player_towns.cpp
+++ b/engines/scumm/players/player_towns.cpp
@@ -586,7 +586,7 @@ void Player_Towns_v1::playCdaTrack(int sound, const uint8 *data, bool skipTrackV
 Player_Towns_v2::Player_Towns_v2(ScummEngine *vm, Audio::Mixer *mixer, IMuse *imuse, bool disposeIMuse) : Player_Towns(vm, true), _imuse(imuse), _imuseDispose(disposeIMuse), _sblData(0) {
 	_soundOverride = new SoundOvrParameters[_numSoundMax];
 	memset(_soundOverride, 0, _numSoundMax * sizeof(SoundOvrParameters));
-	_intf = new TownsAudioInterface(mixer, 0);
+	_intf = new TownsAudioInterface(mixer, 0, true);
 }
 
 Player_Towns_v2::~Player_Towns_v2() {