SCI: (FB01 sound driver) - several fixes

Fix SCI0 (LATE/EARLY) variants of the driver which were broken (didn't play at all). This might be of my own doing, since I introduced the initTrack() method into the driver class and the fb01 driver didn't have one so far.

SCI1 didn't seem to require much fixing. I modified some things according to my findings in the driver disasms. QFG2 and JONES seem to be fine. I am not too happy with KQ5. It has volume issues, but they might be present in the original, too.

I also added an isOpen() check and a mutex to avoid threading issues. When aborting SCI (either quitting ScummVM or returning ot the launcher) while using the fb01 driver I frequently (more often than not) got the assert from backends/midi/windows.cpp, line 95. This fixes that.

I've done plenty of checks and experiments with the sound bank initialization. But I found no bugs or possible improvements there. Hard to tell whether the sound is right. That device seems to have a mind of its own...

2 files changed, 203 insertions, 38 deletions

diff --git a/engines/sci/POTFILES b/engines/sci/POTFILES
index 015776c7a1..24576e871f 100644
--- a/engines/sci/POTFILES
+++ b/engines/sci/POTFILES
@@ -1,4 +1,5 @@
 engines/sci/detection.cpp
+engines/sci/sound/drivers/fb01.cpp
 engines/sci/engine/guest_additions.cpp
 engines/sci/engine/kfile.cpp
 engines/sci/engine/kgraphics.cpp
diff --git a/engines/sci/sound/drivers/fb01.cpp b/engines/sci/sound/drivers/fb01.cpp
index 3f3f581ee2..6f104b2ccd 100644
--- a/engines/sci/sound/drivers/fb01.cpp
+++ b/engines/sci/sound/drivers/fb01.cpp
@@ -29,6 +29,14 @@
 #include "common/file.h"
 #include "common/system.h"
 #include "common/textconsole.h"
+#include "common/mutex.h"
+
+// The original driver uses the master volume setting of the hardware device by sending sysex messages
+// (applies to SCI0 and SCI1). We have a software mastervolume implementation instead. I don't know the
+// reason for this. Nor do I know whether our software effective volume calculation matches the device's
+// internal volume calculation algorithm.
+// I add the original style master volume handling, but make it optional via a #define
+#define		HARDWARE_MASTERVOLUME
 
 namespace Sci {
 
@@ -55,15 +63,22 @@ public:
 
 	int open(ResourceManager *resMan);
 	void close();
+	void initTrack(SciSpan<const byte>& header);
 	void send(uint32 b);
 	void sysEx(const byte *msg, uint16 length);
 	bool hasRhythmChannel() const { return false; }
 	byte getPlayId() const;
-	int getPolyphony() const { return kVoices; } // 9 in SCI1?
+	int getPolyphony() const { return _version <= SCI_VERSION_0_LATE ? 8 : 9; }
 	void setVolume(byte volume);
 	int getVolume();
 	void playSwitch(bool play);
 
+	bool isOpen() const { return _isOpen; }
+	void lockMutex() { _mutex.lock(); }
+	void unlockMutex() { _mutex.unlock(); }
+
+	const char *reportMissingFiles() { return _missingFiles; }
+
 private:
 	void noteOn(int channel, int note, int velocity);
 	void noteOff(int channel, int note);
@@ -102,18 +117,23 @@ private:
 
 	struct Voice {
 		int8 channel;			// MIDI channel that this voice is assigned to or -1
+		uint8 poly;				// Number of hardware voices (SCI0); for SCI1 we just set this to 1
 		int8 note;				// Currently playing MIDI note or -1
 		int bank;				// Current bank setting or -1
 		int patch;				// Currently playing patch or -1
-		uint8 velocity;			// Note velocity
-		bool isSustained;		// Flag indicating a note that is being sustained by the hold pedal
+		//uint8 velocity;		// Note velocity
+		//bool isSustained;		// Flag indicating a note that is being sustained by the hold pedal
 		uint16 age;				// Age of the current note
 
-		Voice() : channel(-1), note(-1), bank(-1), patch(-1), velocity(0), isSustained(false), age(0) { }
+		Voice() : channel(-1), note(-1), bank(-1), patch(-1), /*velocity(0), isSustained(false),*/ age(0), poly(1) { }
 	};
 
 	bool _playSwitch;
 	int _masterVolume;
+	int _numParts;
+
+	bool _isOpen;
+	Common::Mutex _mutex;
 
 	Channel _channels[16];
 	Voice _voices[kVoices];
@@ -123,10 +143,14 @@ private:
 	static void midiTimerCallback(void *p);
 	void setTimerCallback(void *timer_param, Common::TimerManager::TimerProc timer_proc);
 
+	const char *_missingFiles;
+	static const char _requiredFiles[2][12];
+
 	byte _sysExBuf[kMaxSysExSize];
 };
 
-MidiPlayer_Fb01::MidiPlayer_Fb01(SciVersion version) : MidiPlayer(version), _playSwitch(true), _masterVolume(15), _timerParam(NULL), _timerProc(NULL) {
+MidiPlayer_Fb01::MidiPlayer_Fb01(SciVersion version) : MidiPlayer(version), _playSwitch(true), _masterVolume(15), _timerParam(NULL), _timerProc(NULL),
+	_numParts(version > SCI_VERSION_0_LATE ? kVoices : 0), _isOpen(false), _missingFiles(0) {
 	MidiDriver::DeviceHandle dev = MidiDriver::detectDevice(MDT_MIDI);
 	_driver = MidiDriver::createMidi(dev);
 
@@ -135,10 +159,23 @@ MidiPlayer_Fb01::MidiPlayer_Fb01(SciVersion version) : MidiPlayer(version), _pla
 }
 
 MidiPlayer_Fb01::~MidiPlayer_Fb01() {
+	Common::StackLock lock(_mutex);
+	close();
 	delete _driver;
 }
 
 void MidiPlayer_Fb01::voiceMapping(int channel, int voices) {
+	if (_version <= SCI_VERSION_0_LATE) {
+		// The original SCI0 drivers don't do any software voice mapping. Only inside the device...
+		for (int i = 0; i < _numParts; ++i) {
+			if (_voices[i].channel == channel && _voices[i].poly != voices) {
+				_voices[i].poly = voices;
+				setVoiceParam(i, 0, voices);
+			}
+		}
+		return;
+	}
+
 	int curVoices = 0;
 
 	for (int i = 0; i < kVoices; i++)
@@ -163,6 +200,8 @@ void MidiPlayer_Fb01::assignVoices(int channel, int voices) {
 	for (int i = 0; i < kVoices; i++) {
 		if (_voices[i].channel == -1) {
 			_voices[i].channel = channel;
+			if (_voices[i].note != -1)
+				voiceOff(i);
 			if (--voices == 0)
 				break;
 		}
@@ -264,16 +303,23 @@ int MidiPlayer_Fb01::findVoice(int channel) {
 }
 
 void MidiPlayer_Fb01::sendToChannel(byte channel, byte command, byte op1, byte op2) {
-	for (int i = 0; i < kVoices; i++) {
+	for (int i = 0; i < _numParts; i++) {
 		// Send command to all voices assigned to this channel
+		// I case of SCI0 the voice mapping is done inside the device.
 		if (_voices[i].channel == channel)
-			_driver->send(command | i, op1, op2);
+			_driver->send(command | (_version <= SCI_VERSION_0_LATE ? channel : i), op1, op2);
 	}
 }
 
 void MidiPlayer_Fb01::setPatch(int channel, int patch) {
 	int bank = 0;
 
+	if (_version <= SCI_VERSION_0_LATE && channel == 15) {
+		// The original driver has some parsing related handling for program 127.
+		// We can't handle that here.
+		return;
+	}
+
 	_channels[channel].patch = patch;
 
 	if (patch >= 48) {
@@ -281,13 +327,13 @@ void MidiPlayer_Fb01::setPatch(int channel, int patch) {
 		bank = 1;
 	}
 
-	for (int voice = 0; voice < kVoices; voice++) {
+	for (int voice = 0; voice < _numParts; voice++) {
 		if (_voices[voice].channel == channel) {
 			if (_voices[voice].bank != bank) {
 				_voices[voice].bank = bank;
 				setVoiceParam(voice, 4, bank);
 			}
-			_driver->send(0xc0 | voice, patch, 0);
+			_driver->send(0xc0 | (_version <= SCI_VERSION_0_LATE ? channel : voice), patch, 0);
 		}
 	}
 }
@@ -320,12 +366,20 @@ void MidiPlayer_Fb01::noteOn(int channel, int note, int velocity) {
 		return noteOff(channel, note);
 
 	if (_version > SCI_VERSION_0_LATE)
-		velocity = volumeTable[velocity >> 1] << 1;
+		velocity >>= 1;
 
 	int voice;
 	for (voice = 0; voice < kVoices; voice++) {
 		if ((_voices[voice].channel == channel) && (_voices[voice].note == note)) {
 			voiceOff(voice);
+
+			// Original bug #1:	The table is only applied here, but not to the voiceOn() call below.
+			// Original bug #2:	The velocity value also gets another right shift although the table
+			//					size of the volume table is 64 and not 32.
+			// --> disable this ? It certainly can't do any good.
+			if (_version > SCI_VERSION_0_LATE)
+				velocity = volumeTable[velocity >> 1] << 1;
+
 			voiceOn(voice, note, velocity);
 			return;
 		}
@@ -342,6 +396,12 @@ void MidiPlayer_Fb01::noteOn(int channel, int note, int velocity) {
 }
 
 void MidiPlayer_Fb01::controlChange(int channel, int control, int value) {
+	// Events for the control channel shouldn't arrive here.
+	// The original driver handles some specific parsing related midi events
+	// sent on channel 15, but we (hopefully) do that in the SCI music engine.
+	if (_version <= SCI_VERSION_0_LATE && channel == 15)
+		return;
+
 	switch (control) {
 	case 0x07: {
 		_channels[channel].volume = value;
@@ -349,12 +409,16 @@ void MidiPlayer_Fb01::controlChange(int channel, int control, int value) {
 		if (_version > SCI_VERSION_0_LATE)
 			value = volumeTable[value >> 1] << 1;
 
+#ifdef HARDWARE_MASTERVOLUME
+		sendToChannel(channel, 0xb0, control, value);
+#else
 		byte vol = _masterVolume;
 
 		if (vol > 0)
 			vol = CLIP<byte>(vol + 3, 0, 15);
 
 		sendToChannel(channel, 0xb0, control, (value * vol / 15) & 0x7f);
+#endif
 		break;
 	}
 	case 0x0a:
@@ -366,16 +430,19 @@ void MidiPlayer_Fb01::controlChange(int channel, int control, int value) {
 		sendToChannel(channel, 0xb0, control, value);
 		break;
 	case 0x4b:
-		// In early SCI0, voice count 15 signifies that the channel should be ignored
-		// for this song. Assuming that there are no embedded voice count commands in
-		// the MIDI stream, we should be able to get away with simply setting the voice
-		// count for this channel to 0.
-		voiceMapping(channel, (value != 15 ? value : 0));
+		voiceMapping(channel, value);
 		break;
 	case 0x7b:
-		for (int i = 0; i < kVoices; i++)
-			if ((_voices[i].channel == channel) && (_voices[i].note != -1))
-				voiceOff(i);
+		for (int i = 0; i < _numParts; i++) {
+			if ((_voices[i].channel == channel) && (_voices[i].note != -1)) {
+				_voices[i].note = -1;
+				sendToChannel(channel, 0xb0, control, value);
+			}
+		}
+		break;
+	default:
+		sendToChannel(channel, 0xb0, control, value);
+		break;
 	}
 }
 
@@ -385,6 +452,16 @@ void MidiPlayer_Fb01::send(uint32 b) {
 	byte op1 = (b >> 8) & 0x7f;
 	byte op2 = (b >> 16) & 0x7f;
 
+	if (_version <= SCI_VERSION_0_LATE && command != 0xB0 && command != 0xC0) {
+		// Since the voice mapping takes place inside the hardware, most messages
+		// are simply passed through. Channel 15 is never assigned to a part but is
+		// used for certain parsing related events which we cannot handle here.
+		// Just making sure that no nonsense is sent to the device...
+		if (channel != 15)
+			sendToChannel(channel, command, op1, op2);
+		return;
+	}
+
 	switch (command) {
 	case 0x80:
 		noteOff(channel, op1);
@@ -409,9 +486,12 @@ void MidiPlayer_Fb01::send(uint32 b) {
 
 void MidiPlayer_Fb01::setVolume(byte volume) {
 	_masterVolume = volume;
-
+#ifdef HARDWARE_MASTERVOLUME
+	setSystemParam(0, 0x24, (CLIP<byte>(_masterVolume + 3, 0, 15) << 3) + 7);
+#else
 	for (uint i = 0; i < MIDI_CHANNELS; i++)
 		controlChange(i, 0x07, _channels[i].volume & 0x7f);
+#endif
 }
 
 int MidiPlayer_Fb01::getVolume() {
@@ -419,11 +499,20 @@ int MidiPlayer_Fb01::getVolume() {
 }
 
 void MidiPlayer_Fb01::playSwitch(bool play) {
+	_playSwitch = play;
 }
 
 void MidiPlayer_Fb01::midiTimerCallback(void *p) {
+	if (!p)
+		return;
+
 	MidiPlayer_Fb01 *m = (MidiPlayer_Fb01 *)p;
 
+	m->lockMutex();
+
+	if (!m->isOpen())
+		return;
+
 	// Increase the age of the notes
 	for (int i = 0; i < kVoices; i++) {
 		if (m->_voices[i].note != -1)
@@ -432,6 +521,8 @@ void MidiPlayer_Fb01::midiTimerCallback(void *p) {
 
 	if (m->_timerProc)
 		m->_timerProc(m->_timerParam);
+
+	m->unlockMutex();
 }
 
 void MidiPlayer_Fb01::setTimerCallback(void *timer_param, Common::TimerManager::TimerProc timer_proc) {
@@ -456,11 +547,15 @@ void MidiPlayer_Fb01::sendBanks(const SciSpan<const byte> &data) {
 	}
 
 	// Send second bank if available
-	if (data.size() >= 6146 && data.getUint16BEAt(3072) == 0xabcd) {
-		for (int i = 0; i < 48; i++) {
-			sendVoiceData(0, data.subspan(3074 + i * 64));
-			storeVoiceData(0, 1, i);
-		}
+	if (data.size() < 6146)
+		return;
+
+	if (data.getUint16BEAt(3072) != 0xabcd)
+		return;
+
+	for (int i = 0; i < 48; i++) {
+		sendVoiceData(0, data.subspan(3074 + i * 64));
+		storeVoiceData(0, 1, i);
 	}
 }
 
@@ -473,9 +568,8 @@ int MidiPlayer_Fb01::open(ResourceManager *resMan) {
 		return retval;
 	}
 
-	// Set system channel to 0. We send this command over all 16 system channels
-	for (int i = 0; i < 16; i++)
-		setSystemParam(i, 0x20, 0);
+	// Set system channel to 0.
+	setSystemParam(0, 0x20, 0);
 
 	// Turn off memory protection
 	setSystemParam(0, 0x21, 0);
@@ -511,9 +605,19 @@ int MidiPlayer_Fb01::open(ResourceManager *resMan) {
 			if (offset >= buf->size())
 				error("Failed to locate start of FB-01 sound bank");
 
+			// The newer IMF.DRV versions without the sound bank will still have the 32 byte
+			// "SIERRA 1" header, but after that they want to send the patch.002 resource.
+			// These driver version are easy to identify by their small size.
+			if (buf->subspan(offset).size() < 3072) {
+				_missingFiles = _requiredFiles[1];
+				return MidiDriver::MERR_DEVICE_NOT_AVAILABLE;
+			}
+
 			sendBanks(buf->subspan(offset));
-		} else
-			error("Failed to open IMF.DRV");
+		} else {
+			_missingFiles = _version == SCI_VERSION_0_EARLY ? _requiredFiles[0] : _requiredFiles[1];
+			return MidiDriver::MERR_DEVICE_NOT_AVAILABLE;
+		}
 	}
 
 	// Set up voices to use MIDI channels 0 - 7
@@ -525,13 +629,65 @@ int MidiPlayer_Fb01::open(ResourceManager *resMan) {
 	// Set master volume
 	setSystemParam(0, 0x24, 0x7f);
 
+	_isOpen = true;
+
 	return 0;
 }
 
 void MidiPlayer_Fb01::close() {
+	Common::StackLock lock(_mutex);
+	_isOpen = false;
 	_driver->close();
 }
 
+void MidiPlayer_Fb01::initTrack(SciSpan<const byte>& header) {
+	if (!_isOpen || _version > SCI_VERSION_0_LATE)
+		return;
+
+	uint8 readPos = 0;
+	uint8 caps = header.getInt8At(readPos++);
+	if (caps != 0 && (_version == SCI_VERSION_0_EARLY || caps != 2))
+		return;
+
+	for (int i = 0; i < 8; ++i)
+		_voices[i] = Voice();
+
+	_numParts = 0;
+	for (int i = 0; i < 16; ++i) {
+		if (_version == SCI_VERSION_0_LATE) {
+			uint8 num = header.getInt8At(readPos++) & 0x7F;
+			uint8 flags = header.getInt8At(readPos++);
+
+			if (flags & 0x02) {
+				_voices[_numParts].channel = i;
+				_voices[_numParts].poly = num;
+				_numParts++;
+			}
+		} else {
+			uint8 val = header.getInt8At(readPos++);
+			if (val & 0x01) {
+				if (val & 0x08) {
+					if (val >> 4)
+						debugC(9, kDebugLevelSound, "MidiPlayer_Fb01::initTrack(): Unused rhythm channel found: 0x%.02x", i);
+				} else if ((val >> 4) != 0x0F) {
+					_voices[_numParts].channel = i;
+					_voices[_numParts].poly = val >> 4;
+					_numParts++;
+				}
+			} else if (val & 0x08) {
+				debugC(9, kDebugLevelSound, "MidiPlayer_Fb01::initTrack(): Control channel found: 0x%.02x", i);
+			}
+		}
+	}
+
+	for (int i = 0; i < _numParts; ++i)
+		setVoiceParam(i, 1, _voices[i].channel);
+
+	initVoices();
+	
+	setVolume(_masterVolume);
+}
+
 void MidiPlayer_Fb01::setVoiceParam(byte voice, byte param, byte value) {
 	_sysExBuf[2] = 0x00;
 	_sysExBuf[3] = 0x18 | voice;
@@ -592,28 +748,29 @@ void MidiPlayer_Fb01::initVoices() {
 		_sysExBuf[i++] = 0x00;
 	}
 
-	// Set up the 8 MIDI channels we will be using
-	for (int j = 0; j < 8; j++) {
+	// Set up the MIDI channels we will be using
+	for (int j = 0; j < _numParts; j++) {
+		int8 chan = _version > SCI_VERSION_0_LATE ? j : _voices[j].channel;
 		// One voice
-		_sysExBuf[i++] = 0x70 | j;
+		_sysExBuf[i++] = 0x70 | chan;
 		_sysExBuf[i++] = 0x00;
-		_sysExBuf[i++] = 0x01;
+		_sysExBuf[i++] = _voices[j].poly;
 
 		// Full range of keys
-		_sysExBuf[i++] = 0x70 | j;
+		_sysExBuf[i++] = 0x70 | chan;
 		_sysExBuf[i++] = 0x02;
 		_sysExBuf[i++] = 0x7f;
-		_sysExBuf[i++] = 0x70 | j;
+		_sysExBuf[i++] = 0x70 | chan;
 		_sysExBuf[i++] = 0x03;
 		_sysExBuf[i++] = 0x00;
 
 		// Voice bank 0
-		_sysExBuf[i++] = 0x70 | j;
+		_sysExBuf[i++] = 0x70 | chan;
 		_sysExBuf[i++] = 0x04;
 		_sysExBuf[i++] = 0x00;
 
 		// Voice 10
-		_sysExBuf[i++] = 0x70 | j;
+		_sysExBuf[i++] = 0x70 | chan;
 		_sysExBuf[i++] = 0x05;
 		_sysExBuf[i++] = 0x0a;
 	}
@@ -636,7 +793,7 @@ void MidiPlayer_Fb01::sysEx(const byte *msg, uint16 length) {
 byte MidiPlayer_Fb01::getPlayId() const {
 	switch (_version) {
 	case SCI_VERSION_0_EARLY:
-		return 0x01;
+		return 0x09;
 	case SCI_VERSION_0_LATE:
 		return 0x02;
 	default:
@@ -644,8 +801,15 @@ byte MidiPlayer_Fb01::getPlayId() const {
 	}
 }
 
+const char MidiPlayer_Fb01::_requiredFiles[2][12] = {
+	"'IMF.DRV'",
+	"'PATCH.002'"
+};
+
 MidiPlayer *MidiPlayer_Fb01_create(SciVersion version) {
 	return new MidiPlayer_Fb01(version);
 }
 
 } // End of namespace Sci
+
+#undef HARDWARE_MASTERVOLUME
\ No newline at end of file