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.

1 files changed, 40 insertions, 11 deletions

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;
 }