SCUMM: Prevent music channels from drifting out of sync in Mac MI1

In looped music, prevent the music channels from drifting out of
sync over time. This was noticeable after a few minutes in the
SCUMM Bar. We do this by extending the last note (which is just
zeroes, so we didn't even use to play it) so that it has the
exact number of samples needed to make all channels the exact
same length. (This is calculated when the music is loaded, so it
does not need any extra data in the save games, thankfully.)

As a result, the getNextNote() is now responsible for converting
the duration to number of samples (out of necessity) and for
converting the note to a pitch modifier (out of symmetry). I made
several false starts before I realized how much easier it would
be this way.

6 files changed, 80 insertions, 40 deletions

diff --git a/engines/scumm/player_mac.cpp b/engines/scumm/player_mac.cpp
index 7c3e2c22b0..470bdaff30 100644
--- a/engines/scumm/player_mac.cpp
+++ b/engines/scumm/player_mac.cpp
@@ -276,6 +276,34 @@ int Player_Mac::getSoundStatus(int nr) const {
 	return _soundPlaying == nr;
 }
 
+uint32 Player_Mac::durationToSamples(uint16 duration) {
+	// The correct formula should be:
+	//
+	// (duration * 473 * _sampleRate) / (4 * 480 * 480)
+	//
+	// But that's likely to cause integer overflow, so we do it in two
+	// steps and hope that the rounding error won't be noticeable.
+	//
+	// The original code is a bit unclear on if it should be 473 or 437,
+	// but since the comments indicated 473 I'm assuming 437 was a typo.
+	uint32 samples = (duration * _sampleRate) / (4 * 480);
+	samples = (samples * 473) / 480;
+	return samples;
+}
+
+int Player_Mac::noteToPitchModifier(byte note, Instrument *instrument) {
+	if (note > 1) {
+		const int pitchIdx = note + 60 - instrument->_baseFreq;
+		// I don't want to use floating-point arithmetics here, but I
+		// ran into overflow problems with the church music in Monkey
+		// Island. It's only once per note, so it should be ok.
+		double mult = (double)instrument->_rate / (double)_sampleRate;
+		return (int)(mult * _pitchTable[pitchIdx]);
+	} else {
+		return 0;
+	}
+}
+
 int Player_Mac::readBuffer(int16 *data, const int numSamples) {
 	Common::StackLock lock(_mutex);
 
@@ -297,36 +325,14 @@ int Player_Mac::readBuffer(int16 *data, const int numSamples) {
 		while (samplesLeft > 0) {
 			int generated;
 			if (_channel[i]._remaining == 0) {
-				uint16 duration;
-				byte note, velocity;
-				if (getNextNote(i, duration, note, velocity)) {
-					if (note > 1) {
-						const int pitchIdx = note + 60 - _channel[i]._instrument._baseFreq;
-						assert(pitchIdx >= 0);
-						// I don't want to use floating-point arithmetics here,
-						// but I ran into overflow problems with the church
-						// music. It's only once per note, so it should be ok.
-						double mult = (double)(_channel[i]._instrument._rate) / (double)_sampleRate;
-						_channel[i]._pitchModifier = (int)(mult * _pitchTable[pitchIdx]);
-						_channel[i]._velocity = velocity;
-					} else {
-						_channel[i]._pitchModifier = 0;
-						_channel[i]._velocity = 0;
-					}
-
-					// The correct formula should be:
-					//
-					// (duration * 473 * _sampleRate) / (4 * 480 * 480)
-					//
-					// But that's likely to cause integer overflow, so
-					// we do it in two steps and hope that the rounding
-					// error won't be noticeable.
-					//
-					// The original code is a bit unclear on if it should
-					// be 473 or 437, but since the comments indicated
-					// 473 I'm assuming 437 was a typo.
-					_channel[i]._remaining = (duration * _sampleRate) / (4 * 480);
-					_channel[i]._remaining = (_channel[i]._remaining * 473) / 480;
+				uint32 samples;
+				int pitchModifier;
+				byte velocity;
+				if (getNextNote(i, samples, pitchModifier, velocity)) {
+					_channel[i]._remaining = samples;
+					_channel[i]._pitchModifier = pitchModifier;
+					_channel[i]._velocity = velocity;
+
 				} else {
 					_channel[i]._pitchModifier = 0;
 					_channel[i]._velocity = 0;
diff --git a/engines/scumm/player_mac.h b/engines/scumm/player_mac.h
index 3f5184d2d8..c46495c333 100644
--- a/engines/scumm/player_mac.h
+++ b/engines/scumm/player_mac.h
@@ -99,7 +99,7 @@ private:
 
 	virtual bool checkMusicAvailable() { return false; }
 	virtual bool loadMusic(const byte *ptr) { return false; }
-	virtual bool getNextNote(int ch, uint16 &duration, byte &value, byte &velocity) { return false; }
+	virtual bool getNextNote(int ch, uint32 &samples, int &pitchModifier, byte &velocity) { return false; }
 
 protected:
 	struct Channel {
@@ -122,6 +122,9 @@ protected:
 
 	ScummEngine *const _vm;
 	Channel *_channel;
+
+	uint32 durationToSamples(uint16 duration);
+	int noteToPitchModifier(byte note, Instrument *instrument);
 };
 
 } // End of namespace Scumm
diff --git a/engines/scumm/player_v3m.cpp b/engines/scumm/player_v3m.cpp
index a1e69e2434..db532a9f6e 100644
--- a/engines/scumm/player_v3m.cpp
+++ b/engines/scumm/player_v3m.cpp
@@ -156,7 +156,7 @@ bool Player_V3M::loadMusic(const byte *ptr) {
 	return true;
 }
 
-bool Player_V3M::getNextNote(int ch, uint16 &duration, byte &note, byte &velocity) {
+bool Player_V3M::getNextNote(int ch, uint32 &samples, int &pitchModifier, byte &velocity) {
 	_channel[ch]._instrument.newNote();
 	if (_channel[ch]._pos >= _channel[ch]._length) {
 		if (!_channel[ch]._looped) {
@@ -165,8 +165,10 @@ bool Player_V3M::getNextNote(int ch, uint16 &duration, byte &note, byte &velocit
 		}
 		_channel[ch]._pos = 0;
 	}
-	duration = READ_BE_UINT16(&_channel[ch]._data[_channel[ch]._pos]);
-	note = _channel[ch]._data[_channel[ch]._pos + 2];
+	uint16 duration = READ_BE_UINT16(&_channel[ch]._data[_channel[ch]._pos]);
+	byte note = _channel[ch]._data[_channel[ch]._pos + 2];
+	samples = durationToSamples(duration);
+	pitchModifier = noteToPitchModifier(note, &_channel[ch]._instrument);
 	velocity = 127;
 	_channel[ch]._pos += 3;
 	return true;
diff --git a/engines/scumm/player_v3m.h b/engines/scumm/player_v3m.h
index b39783fb9a..359bab32a9 100644
--- a/engines/scumm/player_v3m.h
+++ b/engines/scumm/player_v3m.h
@@ -46,7 +46,7 @@ public:
 
 	virtual bool checkMusicAvailable();
 	virtual bool loadMusic(const byte *ptr);
-	virtual bool getNextNote(int ch, uint16 &duration, byte &note, byte &velocity);
+	virtual bool getNextNote(int ch, uint32 &samples, int &pitchModifier, byte &velocity);
 };
 
 } // End of namespace Scumm
diff --git a/engines/scumm/player_v5m.cpp b/engines/scumm/player_v5m.cpp
index 20519097bd..d59cf9ade2 100644
--- a/engines/scumm/player_v5m.cpp
+++ b/engines/scumm/player_v5m.cpp
@@ -119,7 +119,7 @@ bool Player_V5M::loadMusic(const byte *ptr) {
 		uint32 len = READ_BE_UINT32(ptr + 4);
 		uint32 instrument = READ_BE_UINT32(ptr + 8);
 
-		_channel[i]._length = len - 24;
+		_channel[i]._length = len - 20;
 		_channel[i]._data = ptr + 12;
 		_channel[i]._looped = (READ_BE_UINT32(ptr + len - 8) == MKTAG('L', 'o', 'o', 'p'));
 		_channel[i]._pos = 0;
@@ -147,10 +147,30 @@ bool Player_V5M::loadMusic(const byte *ptr) {
 	}
 
 	resource.close();
+
+	// The last note of each channel is just zeroes. We will adjust this
+	// note so that all the channels end at the same time.
+
+	uint32 samples[3];
+	uint32 maxSamples = 0;
+	for (i = 0; i < 3; i++) {
+		samples[i] = 0;
+		for (uint j = 0; j < _channel[i]._length; j += 4) {
+			samples[i] += durationToSamples(READ_BE_UINT16(&_channel[i]._data[j]));
+		}
+		if (samples[i] > maxSamples) {
+			maxSamples = samples[i];
+		}
+	}
+
+	for (i = 0; i < 3; i++) {
+		_lastNoteSamples[i] = maxSamples - samples[i];
+	}
+
 	return true;
 }
 
-bool Player_V5M::getNextNote(int ch, uint16 &duration, byte &note, byte &velocity) {
+bool Player_V5M::getNextNote(int ch, uint32 &samples, int &pitchModifier, byte &velocity) {
 	_channel[ch]._instrument.newNote();
 	if (_channel[ch]._pos >= _channel[ch]._length) {
 		if (!_channel[ch]._looped) {
@@ -163,10 +183,16 @@ bool Player_V5M::getNextNote(int ch, uint16 &duration, byte &note, byte &velocit
 		// MI1 Lookout music, where I was hearing problems.
 		_channel[ch]._pos = 0;
 	}
-	duration = READ_BE_UINT16(&_channel[ch]._data[_channel[ch]._pos]);
-	note = _channel[ch]._data[_channel[ch]._pos + 2];
+	uint16 duration = READ_BE_UINT16(&_channel[ch]._data[_channel[ch]._pos]);
+	byte note = _channel[ch]._data[_channel[ch]._pos + 2];
+	samples = durationToSamples(duration);
+	pitchModifier = noteToPitchModifier(note, &_channel[ch]._instrument);
 	velocity = _channel[ch]._data[_channel[ch]._pos + 3];
 	_channel[ch]._pos += 4;
+
+	if (_channel[ch]._pos >= _channel[ch]._length) {
+		samples = _lastNoteSamples[ch];
+	}
 	return true;
 }
 
diff --git a/engines/scumm/player_v5m.h b/engines/scumm/player_v5m.h
index 169fa89320..b2079ee331 100644
--- a/engines/scumm/player_v5m.h
+++ b/engines/scumm/player_v5m.h
@@ -46,7 +46,10 @@ public:
 
 	virtual bool checkMusicAvailable();
 	virtual bool loadMusic(const byte *ptr);
-	virtual bool getNextNote(int ch, uint16 &duration, byte &note, byte &velocity);
+	virtual bool getNextNote(int ch, uint32 &samples, int &pitchModifier, byte &velocity);
+
+private:
+	uint32 _lastNoteSamples[3];
 };
 
 } // End of namespace Scumm