SCI32: Implement kLock & kDoAudio(1) for SCI32

1. Unlocking all resources of a type using a resource ID of -1 is
   gone in SCI32;
2. Audio locks need to be serialized starting in GK2 for the game's
   modified kDoAudio(1) call;
3. Audio locks in SCI3 must work more like SSCI, since at least
   Lighthouse's `BackMusic::fade` method will attempt to unlock
   audio that was never locked by a script. In SSCI (and now in
   ScummVM too) this is a no-op; previously in ScummVM, it would
   remove Audio32's own lock on the audio resource, resulting in a
   use-after-free;
4. kDoAudio(1) starting in GK2 returns the number of active
   *not-in-memory* channels being played, not the total number of
   active channels.

Fixes Trac#9675.

7 files changed, 140 insertions, 17 deletions

diff --git a/engines/sci/engine/features.h b/engines/sci/engine/features.h
index 6d9460d61e..08611981cf 100644
--- a/engines/sci/engine/features.h
+++ b/engines/sci/engine/features.h
@@ -113,6 +113,10 @@ public:
 		}
 	}
 
+	inline bool hasSci3Audio() const {
+		return getSciVersion() == SCI_VERSION_3 || g_sci->getGameId() == GID_GK2;
+	}
+
 	inline bool hasTransparentPicturePlanes() const {
 		const SciGameId &gid = g_sci->getGameId();
 
diff --git a/engines/sci/engine/kscripts.cpp b/engines/sci/engine/kscripts.cpp
index 7bcbd0e359..5a330237e9 100644
--- a/engines/sci/engine/kscripts.cpp
+++ b/engines/sci/engine/kscripts.cpp
@@ -27,6 +27,10 @@
 #include "sci/engine/state.h"
 #include "sci/engine/selector.h"
 #include "sci/engine/kernel.h"
+#ifdef ENABLE_SCI32
+#include "sci/engine/features.h"
+#include "sci/sound/audio32.h"
+#endif
 
 #include "common/file.h"
 
@@ -49,6 +53,11 @@ reg_t kLoad(EngineState *s, int argc, reg_t *argv) {
 //  behavior of this call didn't change between sci0->sci1.1 parameter wise, which means getting called with
 //  1 or 3+ parameters is not right according to sierra sci
 reg_t kUnLoad(EngineState *s, int argc, reg_t *argv) {
+	// NOTE: Locked resources in SSCI could be disposed by kUnLoad regardless
+	// of lock state. With this ScummVM implementation of kUnLoad, game scripts
+	// that dispose locked resources via kUnLoad without unlocking them with
+	// kLock will leak the resource until the engine is restarted.
+
 	ResourceType restype = g_sci->getResMan()->convertResType(argv[0].toUint16());
 	reg_t resnr = argv[1];
 
@@ -59,27 +68,48 @@ reg_t kUnLoad(EngineState *s, int argc, reg_t *argv) {
 }
 
 reg_t kLock(EngineState *s, int argc, reg_t *argv) {
-	int state = argc > 2 ? argv[2].toUint16() : 1;
+	// NOTE: In SSCI, kLock uses a boolean lock flag, not a lock counter.
+	// ScummVM's current counter-based implementation should be better than SSCI
+	// at dealing with game scripts that unintentionally lock & unlock the same
+	// resource multiple times (e.g. through recursion), but it will introduce
+	// memory bugs (resource leaks lasting until the engine is restarted, or
+	// destruction of kernel locks that lead to a use-after-free) that are
+	// masked by ResourceManager's LRU cache if scripts rely on kLock being
+	// idempotent like it was in SSCI.
+	//
+	// Like SSCI, resource locks are not persisted in save games in ScummVM
+	// until GK2, so it is also possible that kLock bugs will appear only after
+	// restoring a save game.
+	//
+	// See also kUnLoad.
+
 	ResourceType type = g_sci->getResMan()->convertResType(argv[0].toUint16());
 	if (type == kResourceTypeSound && getSciVersion() >= SCI_VERSION_1_1) {
 		type = g_sci->_soundCmd->getSoundResourceType(argv[1].toUint16());
 	}
 
 	const ResourceId id(type, argv[1].toUint16());
+	const bool lock = argc > 2 ? argv[2].toUint16() : true;
+
+#ifdef ENABLE_SCI32
+	// SSCI GK2+SCI3 also saves lock states for View, Pic, and Sync resources,
+	// but so far it seems like audio resources are the only ones that actually
+	// need to be handled
+	if (g_sci->_features->hasSci3Audio() && type == kResourceTypeAudio) {
+		g_sci->_audio32->lockResource(id, lock);
+		return s->r_acc;
+	}
+#endif
 
 	if (getSciVersion() == SCI_VERSION_1_1 &&
 		(type == kResourceTypeAudio36 || type == kResourceTypeSync36)) {
 		return s->r_acc;
 	}
 
-	Resource *which;
-
-	switch (state) {
-	case 1 :
-		g_sci->getResMan()->findResource(id, 1);
-		break;
-	case 0 :
-		if (id.getNumber() == 0xFFFF) {
+	if (lock) {
+		g_sci->getResMan()->findResource(id, true);
+	} else {
+		if (getSciVersion() < SCI_VERSION_2 && id.getNumber() == 0xFFFF) {
 			// Unlock all resources of the requested type
 			Common::List<ResourceId> resources = g_sci->getResMan()->listResources(type);
 			Common::List<ResourceId>::iterator itr;
@@ -89,7 +119,7 @@ reg_t kLock(EngineState *s, int argc, reg_t *argv) {
 					g_sci->getResMan()->unlockResource(res);
 			}
 		} else {
-			which = g_sci->getResMan()->findResource(id, 0);
+			Resource *which = g_sci->getResMan()->findResource(id, false);
 
 			if (which)
 				g_sci->getResMan()->unlockResource(which);
@@ -103,7 +133,6 @@ reg_t kLock(EngineState *s, int argc, reg_t *argv) {
 					debugC(kDebugLevelResMan, "[resMan] Attempt to unlock non-existent resource %s", id.toString().c_str());
 			}
 		}
-		break;
 	}
 	return s->r_acc;
 }
diff --git a/engines/sci/engine/ksound.cpp b/engines/sci/engine/ksound.cpp
index ad0d836e51..ad137566a2 100644
--- a/engines/sci/engine/ksound.cpp
+++ b/engines/sci/engine/ksound.cpp
@@ -348,10 +348,22 @@ reg_t kDoAudioInit(EngineState *s, int argc, reg_t *argv) {
 }
 
 reg_t kDoAudioWaitForPlay(EngineState *s, int argc, reg_t *argv) {
+	if (argc == 0) {
+		if (g_sci->_features->hasSci3Audio()) {
+			return make_reg(0, g_sci->_audio32->getNumUnlockedChannels());
+		} else {
+			return make_reg(0, g_sci->_audio32->getNumActiveChannels());
+		}
+	}
+
 	return g_sci->_audio32->kernelPlay(false, argc, argv);
 }
 
 reg_t kDoAudioPlay(EngineState *s, int argc, reg_t *argv) {
+	if (argc == 0) {
+		return make_reg(0, g_sci->_audio32->getNumActiveChannels());
+	}
+
 	return g_sci->_audio32->kernelPlay(true, argc, argv);
 }
 
diff --git a/engines/sci/engine/savegame.cpp b/engines/sci/engine/savegame.cpp
index c8e8813470..e0f9ec34db 100644
--- a/engines/sci/engine/savegame.cpp
+++ b/engines/sci/engine/savegame.cpp
@@ -435,6 +435,10 @@ void EngineState::saveLoadWithSerializer(Common::Serializer &s) {
 		g_sci->_gfxPalette32->saveLoadWithSerializer(s);
 		g_sci->_gfxRemap32->saveLoadWithSerializer(s);
 		g_sci->_gfxCursor32->saveLoadWithSerializer(s);
+		// TODO: SCI2 should be using Audio32 too, but is not yet.
+		if (g_sci->_audio32) {
+			g_sci->_audio32->saveLoadWithSerializer(s);
+		}
 		g_sci->_video32->saveLoadWithSerializer(s);
 	} else
 #endif
@@ -1017,6 +1021,14 @@ void GfxCursor32::saveLoadWithSerializer(Common::Serializer &s) {
 	}
 }
 
+void Audio32::saveLoadWithSerializer(Common::Serializer &s) {
+	if (!g_sci->_features->hasSci3Audio() || s.getVersion() < 44) {
+		return;
+	}
+
+	syncArray(s, _lockedResourceIds);
+}
+
 void Video32::beforeSaveLoadWithSerializer(Common::Serializer &s) {
 	if (getSciVersion() < SCI_VERSION_3 || s.isSaving()) {
 		return;
diff --git a/engines/sci/engine/savegame.h b/engines/sci/engine/savegame.h
index 151585ac77..29930241b1 100644
--- a/engines/sci/engine/savegame.h
+++ b/engines/sci/engine/savegame.h
@@ -37,6 +37,7 @@ struct EngineState;
  *
  * Version - new/changed feature
  * =============================
+ *      44 - GK2+SCI3 audio resource locks
  *      43 - stop saving SCI3 mustSetViewVisible array
  *      42 - SCI3 robots and VM objects
  *      41 - palette support for newer SCI2.1 games; stable SCI2/2.1 save games
@@ -68,7 +69,7 @@ struct EngineState;
  */
 
 enum {
-	CURRENT_SAVEGAME_VERSION = 43,
+	CURRENT_SAVEGAME_VERSION = 44,
 	MINIMUM_SAVEGAME_VERSION = 14
 #ifdef ENABLE_SCI32
 	,
diff --git a/engines/sci/sound/audio32.cpp b/engines/sci/sound/audio32.cpp
index b61bbd9a4e..f47002bb59 100644
--- a/engines/sci/sound/audio32.cpp
+++ b/engines/sci/sound/audio32.cpp
@@ -354,6 +354,20 @@ int Audio32::readBuffer(Audio::st_sample_t *buffer, const int numSamples) {
 #pragma mark -
 #pragma mark Channel management
 
+uint8 Audio32::getNumUnlockedChannels() const {
+	Common::StackLock lock(_mutex);
+
+	uint8 numChannels = 0;
+	for (uint i = 0; i < _numActiveChannels; ++i) {
+		const AudioChannel &channel = getChannel(i);
+		if (!channel.robot && Common::find(_lockedResourceIds.begin(), _lockedResourceIds.end(), channel.id) == _lockedResourceIds.end()) {
+			++numChannels;
+		}
+	}
+
+	return numChannels;
+}
+
 int16 Audio32::findChannelByArgs(int argc, const reg_t *argv, const int startIndex, const reg_t soundNode) const {
 	// NOTE: argc/argv are already reduced by one in our engine because
 	// this call is always made from a subop, so no reduction for the
@@ -420,6 +434,21 @@ int16 Audio32::findChannelById(const ResourceId resourceId, const reg_t soundNod
 	return kNoExistingChannel;
 }
 
+void Audio32::lockResource(const ResourceId resourceId, const bool lock) {
+	Common::StackLock slock(_mutex);
+
+	LockList::iterator it = Common::find(_lockedResourceIds.begin(), _lockedResourceIds.end(), resourceId);
+	if (it != _lockedResourceIds.end()) {
+		if (!lock) {
+			_lockedResourceIds.erase(it);
+		}
+	} else {
+		if (lock) {
+			_lockedResourceIds.push_back(resourceId);
+		}
+	}
+}
+
 void Audio32::freeUnusedChannels() {
 	Common::StackLock lock(_mutex);
 	for (int channelIndex = 0; channelIndex < _numActiveChannels; ++channelIndex) {
@@ -1037,10 +1066,6 @@ bool Audio32::hasSignal() const {
 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;
@@ -1214,6 +1239,20 @@ void Audio32::printAudioList(Console *con) const {
 							 channel.stopChannelOnFade ? ", stopping" : "");
 		}
 	}
+
+	if (g_sci->_features->hasSci3Audio()) {
+		con->debugPrintf("\nLocks: ");
+		if (_lockedResourceIds.size()) {
+			const char *separator = "";
+			for (LockList::const_iterator it = _lockedResourceIds.begin(); it != _lockedResourceIds.end(); ++it) {
+				con->debugPrintf("%s%s", separator, it->toString().c_str());
+				separator = ", ";
+			}
+		} else {
+			con->debugPrintf("none");
+		}
+		con->debugPrintf("\n");
+	}
 }
 
 } // End of namespace Sci
diff --git a/engines/sci/sound/audio32.h b/engines/sci/sound/audio32.h
index dd48b39865..27f7e8f3a1 100644
--- a/engines/sci/sound/audio32.h
+++ b/engines/sci/sound/audio32.h
@@ -157,11 +157,13 @@ enum AudioChannelIndex {
  * engine, since the system mixer does not support all the
  * features of SCI.
  */
-class Audio32 : public Audio::AudioStream {
+class Audio32 : public Audio::AudioStream, public Common::Serializable {
 public:
 	Audio32(ResourceManager *resMan);
 	~Audio32();
 
+	virtual void saveLoadWithSerializer(Common::Serializer &s);
+
 	enum {
 		/**
 		 * The maximum channel volume.
@@ -205,6 +207,19 @@ public:
 	}
 
 	/**
+	 * Gets the number of currently active channels that are playing from
+	 * unlocked resources.
+	 *
+	 * @note In SSCI, this function would actually return the number of channels
+	 * whose audio data were not loaded into memory. In practice, the signal for
+	 * placing audio data into memory was a call to kLock, so since we do not
+	 * follow how SSCI works when it comes to resource management, the lock
+	 * state is used as an (apparently) successful proxy for this information
+	 * instead.
+	 */
+	uint8 getNumUnlockedChannels() const;
+
+	/**
 	 * Finds a channel that is already configured for the
 	 * given audio sample.
 	 *
@@ -219,7 +234,13 @@ public:
 	 */
 	int16 findChannelById(const ResourceId resourceId, const reg_t soundNode = NULL_REG) const;
 
+	/**
+	 * Sets or clears a lock on the given resource ID.
+	 */
+	void lockResource(const ResourceId resourceId, const bool lock);
+
 private:
+	typedef Common::Array<ResourceId> LockList;
 	typedef Common::Array<Resource *> UnlockList;
 
 	/**
@@ -252,6 +273,11 @@ private:
 	UnlockList _resourcesToUnlock;
 
 	/**
+	 * The list of resource IDs that have been locked by game scripts.
+	 */
+	LockList _lockedResourceIds;
+
+	/**
 	 * Gets the audio channel at the given index.
 	 */
 	inline AudioChannel &getChannel(const int16 channelIndex) {