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.

5 files changed, 70 insertions, 12 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
 	,