SCI32: Fix race conditions in Audio32

5 files changed, 191 insertions, 128 deletions

diff --git a/engines/sci/engine/ksound.cpp b/engines/sci/engine/ksound.cpp
index e7cad21d67..ad0d836e51 100644
--- a/engines/sci/engine/ksound.cpp
+++ b/engines/sci/engine/ksound.cpp
@@ -356,23 +356,19 @@ reg_t kDoAudioPlay(EngineState *s, int argc, reg_t *argv) {
 }
 
 reg_t kDoAudioStop(EngineState *s, int argc, reg_t *argv) {
-	const int16 channelIndex = g_sci->_audio32->findChannelByArgs(argc, argv, 0, argc > 1 ? argv[1] : NULL_REG);
-	return make_reg(0, g_sci->_audio32->stop(channelIndex));
+	return g_sci->_audio32->kernelStop(argc, argv);
 }
 
 reg_t kDoAudioPause(EngineState *s, int argc, reg_t *argv) {
-	const int16 channelIndex = g_sci->_audio32->findChannelByArgs(argc, argv, 0, argc > 1 ? argv[1] : NULL_REG);
-	return make_reg(0, g_sci->_audio32->pause(channelIndex));
+	return g_sci->_audio32->kernelPause(argc, argv);
 }
 
 reg_t kDoAudioResume(EngineState *s, int argc, reg_t *argv) {
-	const int16 channelIndex = g_sci->_audio32->findChannelByArgs(argc, argv, 0, argc > 1 ? argv[1] : NULL_REG);
-	return make_reg(0, g_sci->_audio32->resume(channelIndex));
+	return g_sci->_audio32->kernelResume(argc, argv);
 }
 
 reg_t kDoAudioPosition(EngineState *s, int argc, reg_t *argv) {
-	const int16 channelIndex = g_sci->_audio32->findChannelByArgs(argc, argv, 0, argc > 1 ? argv[1] : NULL_REG);
-	return make_reg(0, g_sci->_audio32->getPosition(channelIndex));
+	return g_sci->_audio32->kernelPosition(argc, argv);
 }
 
 reg_t kDoAudioRate(EngineState *s, int argc, reg_t *argv) {
@@ -391,14 +387,7 @@ reg_t kDoAudioRate(EngineState *s, int argc, reg_t *argv) {
 }
 
 reg_t kDoAudioVolume(EngineState *s, int argc, reg_t *argv) {
-	const int16 volume = argc > 0 ? argv[0].toSint16() : -1;
-	const int16 channelIndex = g_sci->_audio32->findChannelByArgs(argc, argv, 1, argc > 2 ? argv[2] : NULL_REG);
-
-	if (volume != -1) {
-		g_sci->_audio32->setVolume(channelIndex, volume);
-	}
-
-	return make_reg(0, g_sci->_audio32->getVolume(channelIndex));
+	return g_sci->_audio32->kernelVolume(argc, argv);
 }
 
 reg_t kDoAudioGetCapability(EngineState *s, int argc, reg_t *argv) {
@@ -421,11 +410,7 @@ reg_t kDoAudioBitDepth(EngineState *s, int argc, reg_t *argv) {
 }
 
 reg_t kDoAudioMixing(EngineState *s, int argc, reg_t *argv) {
-	if (argc > 0) {
-		g_sci->_audio32->setAttenuatedMixing(argv[0].toUint16());
-	}
-
-	return make_reg(0, g_sci->_audio32->getAttenuatedMixing());
+	return g_sci->_audio32->kernelMixing(argc, argv);
 }
 
 reg_t kDoAudioChannels(EngineState *s, int argc, reg_t *argv) {
@@ -457,21 +442,7 @@ reg_t kDoAudioPreload(EngineState *s, int argc, reg_t *argv) {
 }
 
 reg_t kDoAudioFade(EngineState *s, int argc, reg_t *argv) {
-	if (argc < 4) {
-		return make_reg(0, 0);
-	}
-
-	// NOTE: Sierra did a nightmarish hack here, temporarily replacing
-	// the argc of the kernel arguments with 2 and then restoring it
-	// after findChannelByArgs was called.
-	const int16 channelIndex = g_sci->_audio32->findChannelByArgs(2, argv, 0, argc > 5 ? argv[5] : NULL_REG);
-
-	const int16 volume = argv[1].toSint16();
-	const int16 speed = argv[2].toSint16();
-	const int16 steps = argv[3].toSint16();
-	const bool stopAfterFade = argc > 4 ? (bool)argv[4].toUint16() : false;
-
-	return make_reg(0, g_sci->_audio32->fadeChannel(channelIndex, volume, speed, steps, stopAfterFade));
+	return g_sci->_audio32->kernelFade(argc, argv);
 }
 
 reg_t kDoAudioHasSignal(EngineState *s, int argc, reg_t *argv) {
@@ -479,11 +450,7 @@ reg_t kDoAudioHasSignal(EngineState *s, int argc, reg_t *argv) {
 }
 
 reg_t kDoAudioSetLoop(EngineState *s, int argc, reg_t *argv) {
-	const int16 channelIndex = g_sci->_audio32->findChannelByArgs(argc, argv, 0, argc == 3 ? argv[2] : NULL_REG);
-
-	const bool loop = argv[0].toSint16() != 0 && argv[0].toSint16() != 1;
-
-	g_sci->_audio32->setLoop(channelIndex, loop);
+	g_sci->_audio32->kernelLoop(argc, argv);
 	return s->r_acc;
 }
 
diff --git a/engines/sci/sound/audio32.cpp b/engines/sci/sound/audio32.cpp
index 5d008bff46..df21a5fdcb 100644
--- a/engines/sci/sound/audio32.cpp
+++ b/engines/sci/sound/audio32.cpp
@@ -875,6 +875,12 @@ int16 Audio32::stop(const int16 channelIndex) {
 	return oldNumChannels;
 }
 
+uint16 Audio32::restart(const ResourceId resourceId, const bool autoPlay, const bool loop, const int16 volume, const reg_t soundNode, const bool monitor) {
+	Common::StackLock lock(_mutex);
+	stop(resourceId, soundNode);
+	return play(kNoExistingChannel, resourceId, autoPlay, loop, volume, soundNode, monitor);
+}
+
 int16 Audio32::getPosition(const int16 channelIndex) const {
 	Common::StackLock lock(_mutex);
 	if (channelIndex == kNoExistingChannel || _numActiveChannels == 0) {
@@ -920,82 +926,6 @@ void Audio32::setLoop(const int16 channelIndex, const bool loop) {
 	channel.loop = loop;
 }
 
-reg_t Audio32::kernelPlay(const bool autoPlay, const int argc, const reg_t *const argv) {
-	if (argc == 0) {
-		return make_reg(0, _numActiveChannels);
-	}
-
-	const int16 channelIndex = findChannelByArgs(argc, argv, 0, NULL_REG);
-	ResourceId resourceId;
-	bool loop;
-	int16 volume;
-	bool monitor = false;
-	reg_t soundNode = NULL_REG;
-
-	if (argc >= 5) {
-		resourceId = ResourceId(kResourceTypeAudio36, argv[0].toUint16(), argv[1].toUint16(), argv[2].toUint16(), argv[3].toUint16(), argv[4].toUint16());
-
-		if (argc < 6 || argv[5].toSint16() == 1) {
-			loop = false;
-		} else {
-			// NOTE: Uses -1 for infinite loop. Presumably the
-			// engine was supposed to allow counter loops at one
-			// point, but ended up only using loop as a boolean.
-			loop = (bool)argv[5].toSint16();
-		}
-
-		if (getSciVersion() == SCI_VERSION_3) {
-			if (argc < 7) {
-				volume = Audio32::kMaxVolume;
-			} else {
-				volume = argv[6].toSint16() & Audio32::kMaxVolume;
-				monitor = argv[6].toSint16() & Audio32::kMonitorAudioFlagSci3;
-			}
-		} else {
-			if (argc < 7 || argv[6].toSint16() < 0 || argv[6].toSint16() > Audio32::kMaxVolume) {
-				volume = Audio32::kMaxVolume;
-
-				if (argc >= 7) {
-					monitor = true;
-				}
-			} else {
-				volume = argv[6].toSint16();
-			}
-		}
-	} else {
-		resourceId = ResourceId(kResourceTypeAudio, argv[0].toUint16());
-
-		if (argc < 2 || argv[1].toSint16() == 1) {
-			loop = false;
-		} else {
-			loop = (bool)argv[1].toSint16();
-		}
-
-		if (getSciVersion() == SCI_VERSION_3) {
-			if (argc < 3) {
-				volume = Audio32::kMaxVolume;
-			} else {
-				volume = argv[2].toSint16() & Audio32::kMaxVolume;
-				monitor = argv[2].toSint16() & Audio32::kMonitorAudioFlagSci3;
-			}
-		} else {
-			if (argc < 3 || argv[2].toSint16() < 0 || argv[2].toSint16() > Audio32::kMaxVolume) {
-				volume = Audio32::kMaxVolume;
-
-				if (argc >= 3) {
-					monitor = true;
-				}
-			} else {
-				volume = argv[2].toSint16();
-			}
-		}
-
-		soundNode = argc == 4 ? argv[3] : NULL_REG;
-	}
-
-	return make_reg(0, play(channelIndex, resourceId, autoPlay, loop, volume, soundNode, monitor));
-}
-
 #pragma mark -
 #pragma mark Effects
 
@@ -1100,4 +1030,159 @@ bool Audio32::hasSignal() const {
 	return false;
 }
 
+#pragma mark -
+#pragma mark Kernel
+
+reg_t Audio32::kernelPlay(const bool autoPlay, const int argc, const reg_t *const argv) {
+	Common::StackLock lock(_mutex);
+
+	if (argc == 0) {
+		return make_reg(0, _numActiveChannels);
+	}
+
+	const int16 channelIndex = findChannelByArgs(argc, argv, 0, NULL_REG);
+	ResourceId resourceId;
+	bool loop;
+	int16 volume;
+	bool monitor = false;
+	reg_t soundNode = NULL_REG;
+
+	if (argc >= 5) {
+		resourceId = ResourceId(kResourceTypeAudio36, argv[0].toUint16(), argv[1].toUint16(), argv[2].toUint16(), argv[3].toUint16(), argv[4].toUint16());
+
+		if (argc < 6 || argv[5].toSint16() == 1) {
+			loop = false;
+		} else {
+			// NOTE: Uses -1 for infinite loop. Presumably the
+			// engine was supposed to allow counter loops at one
+			// point, but ended up only using loop as a boolean.
+			loop = (bool)argv[5].toSint16();
+		}
+
+		if (getSciVersion() == SCI_VERSION_3) {
+			if (argc < 7) {
+				volume = Audio32::kMaxVolume;
+			} else {
+				volume = argv[6].toSint16() & Audio32::kMaxVolume;
+				monitor = argv[6].toSint16() & Audio32::kMonitorAudioFlagSci3;
+			}
+		} else {
+			if (argc < 7 || argv[6].toSint16() < 0 || argv[6].toSint16() > Audio32::kMaxVolume) {
+				volume = Audio32::kMaxVolume;
+
+				if (argc >= 7) {
+					monitor = true;
+				}
+			} else {
+				volume = argv[6].toSint16();
+			}
+		}
+	} else {
+		resourceId = ResourceId(kResourceTypeAudio, argv[0].toUint16());
+
+		if (argc < 2 || argv[1].toSint16() == 1) {
+			loop = false;
+		} else {
+			loop = (bool)argv[1].toSint16();
+		}
+
+		if (getSciVersion() == SCI_VERSION_3) {
+			if (argc < 3) {
+				volume = Audio32::kMaxVolume;
+			} else {
+				volume = argv[2].toSint16() & Audio32::kMaxVolume;
+				monitor = argv[2].toSint16() & Audio32::kMonitorAudioFlagSci3;
+			}
+		} else {
+			if (argc < 3 || argv[2].toSint16() < 0 || argv[2].toSint16() > Audio32::kMaxVolume) {
+				volume = Audio32::kMaxVolume;
+
+				if (argc >= 3) {
+					monitor = true;
+				}
+			} else {
+				volume = argv[2].toSint16();
+			}
+		}
+
+		soundNode = argc == 4 ? argv[3] : NULL_REG;
+	}
+
+	return make_reg(0, play(channelIndex, resourceId, autoPlay, loop, volume, soundNode, monitor));
+}
+
+reg_t Audio32::kernelStop(const int argc, const reg_t *const argv) {
+	Common::StackLock lock(_mutex);
+	const int16 channelIndex = findChannelByArgs(argc, argv, 0, argc > 1 ? argv[1] : NULL_REG);
+	return make_reg(0, stop(channelIndex));
+}
+
+reg_t Audio32::kernelPause(const int argc, const reg_t *const argv) {
+	Common::StackLock lock(_mutex);
+	const int16 channelIndex = findChannelByArgs(argc, argv, 0, argc > 1 ? argv[1] : NULL_REG);
+	return make_reg(0, pause(channelIndex));
+}
+
+reg_t Audio32::kernelResume(const int argc, const reg_t *const argv) {
+	Common::StackLock lock(_mutex);
+	const int16 channelIndex = findChannelByArgs(argc, argv, 0, argc > 1 ? argv[1] : NULL_REG);
+	return make_reg(0, resume(channelIndex));
+}
+
+reg_t Audio32::kernelPosition(const int argc, const reg_t *const argv) {
+	Common::StackLock lock(_mutex);
+	const int16 channelIndex = findChannelByArgs(argc, argv, 0, argc > 1 ? argv[1] : NULL_REG);
+	return make_reg(0, getPosition(channelIndex));
+}
+
+reg_t Audio32::kernelVolume(const int argc, const reg_t *const argv) {
+	Common::StackLock lock(_mutex);
+
+	const int16 volume = argc > 0 ? argv[0].toSint16() : -1;
+	const int16 channelIndex = findChannelByArgs(argc, argv, 1, argc > 2 ? argv[2] : NULL_REG);
+
+	if (volume != -1) {
+		setVolume(channelIndex, volume);
+	}
+
+	return make_reg(0, getVolume(channelIndex));
+}
+
+reg_t Audio32::kernelMixing(const int argc, const reg_t *const argv) {
+	Common::StackLock lock(_mutex);
+
+	if (argc > 0) {
+		setAttenuatedMixing(argv[0].toUint16());
+	}
+
+	return make_reg(0, getAttenuatedMixing());
+}
+
+reg_t Audio32::kernelFade(const int argc, const reg_t *const argv) {
+	if (argc < 4) {
+		return make_reg(0, 0);
+	}
+
+	Common::StackLock lock(_mutex);
+
+	// NOTE: In SSCI, this call to find the channel is hacked up; argc is
+	// set to 2 before the call, and then restored after the call.
+	const int16 channelIndex = findChannelByArgs(2, argv, 0, argc > 5 ? argv[5] : NULL_REG);
+	const int16 volume = argv[1].toSint16();
+	const int16 speed = argv[2].toSint16();
+	const int16 steps = argv[3].toSint16();
+	const bool stopAfterFade = argc > 4 ? (bool)argv[4].toUint16() : false;
+
+	return make_reg(0, fadeChannel(channelIndex, volume, speed, steps, stopAfterFade));
+}
+
+void Audio32::kernelLoop(const int argc, const reg_t *const argv) {
+	Common::StackLock lock(_mutex);
+
+	const int16 channelIndex = findChannelByArgs(argc, argv, 0, argc == 3 ? argv[2] : NULL_REG);
+	const bool loop = argv[0].toSint16() != 0 && argv[0].toSint16() != 1;
+
+	setLoop(channelIndex, loop);
+}
+
 } // End of namespace Sci
diff --git a/engines/sci/sound/audio32.h b/engines/sci/sound/audio32.h
index f631de2bdf..c953582965 100644
--- a/engines/sci/sound/audio32.h
+++ b/engines/sci/sound/audio32.h
@@ -451,6 +451,11 @@ public:
 	}
 
 	/**
+	 * Restarts playback of the given audio resource.
+	 */
+	uint16 restart(const ResourceId resourceId, const bool autoPlay, const bool loop, const int16 volume, const reg_t soundNode, const bool monitor);
+
+	/**
 	 * Returns the playback position for the given channel
 	 * number, in ticks.
 	 */
@@ -469,8 +474,6 @@ public:
 		setLoop(findChannelById(resourceId, soundNode), loop);
 	}
 
-	reg_t kernelPlay(const bool autoPlay, const int argc, const reg_t *const argv);
-
 private:
 	/**
 	 * The tick when audio was globally paused.
@@ -594,6 +597,19 @@ private:
 	 * monitoring buffer.
 	 */
 	int _numMonitoredSamples;
+
+#pragma mark -
+#pragma mark Kernel
+public:
+	reg_t kernelPlay(const bool autoPlay, const int argc, const reg_t *const argv);
+	reg_t kernelStop(const int argc, const reg_t *const argv);
+	reg_t kernelPause(const int argc, const reg_t *const argv);
+	reg_t kernelResume(const int argc, const reg_t *const argv);
+	reg_t kernelPosition(const int argc, const reg_t *const argv);
+	reg_t kernelVolume(const int argc, const reg_t *const argv);
+	reg_t kernelMixing(const int argc, const reg_t *const argv);
+	reg_t kernelFade(const int argc, const reg_t *const argv);
+	void kernelLoop(const int argc, const reg_t *const argv);
 };
 
 } // End of namespace Sci
diff --git a/engines/sci/sound/music.cpp b/engines/sci/sound/music.cpp
index dd9fbcb456..ca5644d90b 100644
--- a/engines/sci/sound/music.cpp
+++ b/engines/sci/sound/music.cpp
@@ -484,10 +484,7 @@ void SciMusic::soundPlay(MusicEntry *pSnd) {
 	if (pSnd->isSample) {
 #ifdef ENABLE_SCI32
 		if (_soundVersion >= SCI_VERSION_2_1_EARLY) {
-			g_sci->_audio32->stop(ResourceId(kResourceTypeAudio, pSnd->resourceId), pSnd->soundObj);
-
-			g_sci->_audio32->play(kNoExistingChannel, ResourceId(kResourceTypeAudio, pSnd->resourceId), true, pSnd->loop != 0 && pSnd->loop != 1, pSnd->volume, pSnd->soundObj, false);
-
+			g_sci->_audio32->restart(ResourceId(kResourceTypeAudio, pSnd->resourceId), true, pSnd->loop != 0 && pSnd->loop != 1, pSnd->volume, pSnd->soundObj, false);
 			return;
 		} else
 #endif
diff --git a/engines/sci/sound/soundcmd.cpp b/engines/sci/sound/soundcmd.cpp
index e0cbb97d19..07e01fb759 100644
--- a/engines/sci/sound/soundcmd.cpp
+++ b/engines/sci/sound/soundcmd.cpp
@@ -344,12 +344,10 @@ reg_t SoundCommandParser::kDoSoundPause(EngineState *s, int argc, reg_t *argv) {
 		// implementation is so different that it doesn't
 		// matter here
 		if (_soundVersion >= SCI_VERSION_2_1_EARLY && musicSlot->isSample) {
-			const int16 channelIndex = g_sci->_audio32->findChannelById(ResourceId(kResourceTypeAudio, musicSlot->resourceId), musicSlot->soundObj);
-
 			if (shouldPause) {
-				g_sci->_audio32->pause(channelIndex);
+				g_sci->_audio32->pause(ResourceId(kResourceTypeAudio, musicSlot->resourceId), musicSlot->soundObj);
 			} else {
-				g_sci->_audio32->resume(channelIndex);
+				g_sci->_audio32->resume(ResourceId(kResourceTypeAudio, musicSlot->resourceId), musicSlot->soundObj);
 			}
 		} else
 #endif
@@ -504,7 +502,7 @@ void SoundCommandParser::processUpdateCues(reg_t obj) {
 	if (musicSlot->isSample) {
 #ifdef ENABLE_SCI32
 		if (_soundVersion >= SCI_VERSION_2_1_EARLY) {
-			const int position = g_sci->_audio32->getPosition(g_sci->_audio32->findChannelById(ResourceId(kResourceTypeAudio, musicSlot->resourceId), musicSlot->soundObj));
+			const int position = g_sci->_audio32->getPosition(ResourceId(kResourceTypeAudio, musicSlot->resourceId), musicSlot->soundObj);
 
 			if (position == -1) {
 				processStopSound(musicSlot->soundObj, true);