From 0e6c4f37738c845633cf52406dcab9ef4c6cdb97 Mon Sep 17 00:00:00 2001
From: threefins <threefins@gardenizr.com>
Date: Sun, 4 Feb 2024 15:40:12 +1100
Subject: [PATCH] DIRECTOR: Correct for invalid loop bounds in D4

In some D4 version 400 (file version 0x45B) files, sound resource loop bounds are
invalid but their cast member flags are set to loop. Examples are
"Barbie and her Magical Dreamhouse" and "Necrobius" (Windows demo).
Invalid is any situation where the following does not hold:
`loop_start < loop_end <= size`

In the original environment, these resources are handled by disabling
the looping flag and resetting the loop bounds on the sound resources to
0 -> sample size.

This commit checks for invalid loop bounds and applies roughly the same
fix. This fixes the invalid sound looping behaviours present in the
aforementioned titles.
---
 engines/director/castmember/sound.cpp | 15 +++++++++++++++
 engines/director/sound.cpp            | 12 ++++++++++--
 engines/director/sound.h              |  2 ++
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/engines/director/castmember/sound.cpp b/engines/director/castmember/sound.cpp
index 8620b1d9eb2..80619eb9847 100644
--- a/engines/director/castmember/sound.cpp
+++ b/engines/director/castmember/sound.cpp
@@ -99,6 +99,21 @@ void SoundCastMember::load() {
 			// The looping flag wasn't added to sound cast members until D4.
 			// In older versions, always loop sounds that contain a loop start and end.
 			_looping = audio->hasLoopBounds();
+		} else {
+			// Some sound cast members at version kFileVer400 have looping=true with
+			// invalid loop bounds (bigger than sample size or non-consecutive).
+			// Resetting loop bounds to sample bounds and disabling looping similar
+			// to how D4 playback seems to work.
+			if (!audio->hasValidLoopBounds()) {
+				// only emit a warning for files > kFileVer400 as it's only kFileVer400 files that should be affected
+				if (_cast->_version > kFileVer400) {
+					warning("Sound::load(): Invalid loop bounds detected. Disabling looping for cast member id %d, sndId %d", _castId, sndId);
+				} else {
+					debugC(2, "Sound::load(): Invalid loop bounds detected. Disabling looping for cast member id %d, sndId %d", _castId, sndId);
+				}
+				_looping = false;
+				audio->resetLoopBounds();
+			}
 		}
 	}
 	if (sndData)
diff --git a/engines/director/sound.cpp b/engines/director/sound.cpp
index f89cb512323..0ed9645fabe 100644
--- a/engines/director/sound.cpp
+++ b/engines/director/sound.cpp
@@ -153,8 +153,8 @@ void DirectorSound::playCastMember(CastMemberID memberID, uint8 soundChannel, bo
 		//   4. maybe more?
 		if (shouldStopOnZero(soundChannel)) {
 			stopSound(soundChannel);
-		// Director 4 will stop after the current loop iteration, but
-		// Director 3 will continue looping until the sound is replaced.
+			// Director 4 will stop after the current loop iteration, but
+			// Director 3 will continue looping until the sound is replaced.
 		} else if (g_director->getVersion() >= 400) {
 			// If there is a loopable stream specified, set the loop to expire by itself
 			if (_channels[soundChannel]->loopPtr) {
@@ -848,6 +848,14 @@ bool SNDDecoder::hasLoopBounds() {
 	return _loopStart != 0 || _loopEnd != 0;
 }
 
+bool SNDDecoder::hasValidLoopBounds() {
+	return hasLoopBounds() && _loopStart < _loopEnd && _loopEnd <= _size;
+}
+
+void SNDDecoder::resetLoopBounds() {
+	_loopStart = _loopEnd = 0;
+}
+
 AudioFileDecoder::AudioFileDecoder(Common::String &path)
 		: AudioDecoder() {
 	_path = path;
diff --git a/engines/director/sound.h b/engines/director/sound.h
index 2e2afbdd6db..0071be127ef 100644
--- a/engines/director/sound.h
+++ b/engines/director/sound.h
@@ -240,6 +240,8 @@ public:
 	bool processBufferCommand(Common::SeekableReadStreamEndian &stream);
 	Audio::AudioStream *getAudioStream(bool looping = false, bool forPuppet = false, DisposeAfterUse::Flag disposeAfterUse = DisposeAfterUse::YES) override;
 	bool hasLoopBounds();
+	void resetLoopBounds();
+	bool hasValidLoopBounds();
 
 private:
 	byte *_data;