Fix: Various reliability and correctness improvements to MIDI on Windows. (#7620)

src/music/dmusic.cpp

@@ -538,19 +538,19 @@ static void TransmitChannelMsg(IDirectMusicBuffer *buffer, REFERENCE_TIME rt, by
 	}
 }
 
-static void TransmitSysex(IDirectMusicBuffer *buffer, REFERENCE_TIME rt, byte *&msg_start, size_t &remaining)
+static void TransmitSysex(IDirectMusicBuffer *buffer, REFERENCE_TIME rt, const byte *&msg_start, size_t &remaining)
 {
 	/* Find end of message. */
-	byte *msg_end = msg_start;
+	const byte *msg_end = msg_start;
 	while (*msg_end != MIDIST_ENDSYSEX) msg_end++;
 	msg_end++; // Also include SysEx end byte.
 
-	if (buffer->PackUnstructured(rt, 0, msg_end - msg_start, msg_start) == E_OUTOFMEMORY) {
+	if (buffer->PackUnstructured(rt, 0, msg_end - msg_start, const_cast<LPBYTE>(msg_start)) == E_OUTOFMEMORY) {
 		/* Buffer is full, clear it and try again. */
 		_port->PlayBuffer(buffer);
 		buffer->Flush();
 
-		buffer->PackUnstructured(rt, 0, msg_end - msg_start, msg_start);
+		buffer->PackUnstructured(rt, 0, msg_end - msg_start, const_cast<LPBYTE>(msg_start));
 	}
 
 	/* Update position in buffer. */
@@ -558,9 +558,11 @@ static void TransmitSysex(IDirectMusicBuffer *buffer, REFERENCE_TIME rt, byte *&
 	msg_start = msg_end;
 }
 
-static void TransmitSysexConst(IDirectMusicBuffer *buffer, REFERENCE_TIME rt, byte *msg_start, size_t length)
+static void TransmitStandardSysex(IDirectMusicBuffer *buffer, REFERENCE_TIME rt, MidiSysexMessage msg)
 {
-	TransmitSysex(buffer, rt, msg_start, length);
+	size_t length = 0;
+	const byte *data = MidiGetStandardSysexMessage(msg, length);
+	TransmitSysex(buffer, rt, data, length);
 }
 
 /** Transmit 'Note off' messages to all MIDI channels. */
@@ -571,27 +573,16 @@ static void TransmitNotesOff(IDirectMusicBuffer *buffer, REFERENCE_TIME block_ti
 		TransmitChannelMsg(_buffer, block_time + 10, MIDIST_CONTROLLER | ch, MIDICT_SUSTAINSW, 0);
 		TransmitChannelMsg(_buffer, block_time + 10, MIDIST_CONTROLLER | ch, MIDICT_MODE_RESETALLCTRL, 0);
 	}
-	/* Explicitly flush buffer to make sure the note off messages are processed
+
-	 * before we send any additional control messages. */
+	/* Performing a GM reset stops all sound and resets all parameters. */
+	TransmitStandardSysex(_buffer, block_time + 20, MidiSysexMessage::ResetGM);
+	TransmitStandardSysex(_buffer, block_time + 30, MidiSysexMessage::RolandSetReverb);
+
+	/* Explicitly flush buffer to make sure the messages are processed,
+	 * as we want sound to stop immediately. */
 	_port->PlayBuffer(_buffer);
 	_buffer->Flush();
 
-	/* Some songs change the "Pitch bend range" registered parameter. If
-	 * this doesn't get reset, everything else will start sounding wrong. */
-	for (int ch = 0; ch < 16; ch++) {
-		/* Running status, only need status for first message
-		 * Select RPN 00.00, set value to 02.00, and de-select again */
-		TransmitChannelMsg(_buffer, block_time + 10, MIDIST_CONTROLLER | ch, MIDICT_RPN_SELECT_LO, 0x00);
-		TransmitChannelMsg(_buffer, block_time + 10, MIDICT_RPN_SELECT_HI, 0x00);
-		TransmitChannelMsg(_buffer, block_time + 10, MIDICT_DATAENTRY, 0x02);
-		TransmitChannelMsg(_buffer, block_time + 10, MIDICT_DATAENTRY_LO, 0x00);
-		TransmitChannelMsg(_buffer, block_time + 10, MIDICT_RPN_SELECT_LO, 0x7F);
-		TransmitChannelMsg(_buffer, block_time + 10, MIDICT_RPN_SELECT_HI, 0x7F);
-
-		_port->PlayBuffer(_buffer);
-		_buffer->Flush();
-	}
-
 	/* Wait until message time has passed. */
 	Sleep(Clamp((block_time - cur_time) / MS_TO_REFTIME, 5, 1000));
 }
@@ -616,13 +607,6 @@ static void MidiThreadProc(void *)
 	REFERENCE_TIME cur_time;
 	clock->GetTime(&cur_time);
 
-	/* Standard "Enable General MIDI" message */
-	static byte gm_enable_sysex[] = { 0xF0, 0x7E, 0x00, 0x09, 0x01, 0xF7 };
-	TransmitSysexConst(_buffer, cur_time, &gm_enable_sysex[0], sizeof(gm_enable_sysex));
-	/* Roland-specific reverb room control, used by the original game */
-	static byte roland_reverb_sysex[] = { 0xF0, 0x41, 0x10, 0x42, 0x12, 0x40, 0x01, 0x30, 0x02, 0x04, 0x00, 0x40, 0x40, 0x00, 0x00, 0x09, 0xF7 };
-	TransmitSysexConst(_buffer, cur_time, &roland_reverb_sysex[0], sizeof(roland_reverb_sysex));
-
 	_port->PlayBuffer(_buffer);
 	_buffer->Flush();
 
@@ -665,7 +649,7 @@ static void MidiThreadProc(void *)
 					_playback.do_start = false;
 				}
 
-				/* Turn all notes off in case we are seeking between music titles. */
+				/* Reset playback device between songs. */
 				clock->GetTime(&cur_time);
 				TransmitNotesOff(_buffer, block_time, cur_time);
 
@@ -751,7 +735,7 @@ static void MidiThreadProc(void *)
 				block_time = playback_start_time + block.realtime * MIDITIME_TO_REFTIME;
 				DEBUG(driver, 9, "DMusic thread: Streaming block " PRINTF_SIZE " (cur=" OTTD_PRINTF64 ", block=" OTTD_PRINTF64 ")", current_block, (long long)(current_time / MS_TO_REFTIME), (long long)(block_time / MS_TO_REFTIME));
 
-				byte *data = block.data.Begin();
+				const byte *data = block.data.Begin();
 				size_t remaining = block.data.Length();
 				byte last_status = 0;
 				while (remaining > 0) {

src/music/midi.h

@@ -141,4 +141,19 @@ enum MidiController {
 	MIDICT_MODE_POLY         = 127,
 };
 
+
+/** Well-known MIDI system exclusive message values for use with the MidiGetStandardSysexMessage function. */
+enum class MidiSysexMessage {
+	/** Reset device to General MIDI defaults */
+	ResetGM,
+	/** Reset device to (Roland) General Standard defaults */
+	ResetGS,
+	/** Reset device to (Yamaha) XG defaults */
+	ResetXG,
+	/** Set up Roland SoundCanvas reverb room as TTD does */
+	RolandSetReverb,
+};
+
+const byte *MidiGetStandardSysexMessage(MidiSysexMessage msg, size_t &length);
+
 #endif /* MUSIC_MIDI_H */

src/music/midifile.cpp

@@ -27,6 +27,37 @@
 
 static MidiFile *_midifile_instance = NULL;
 
+/**
+ * Retrieve a well-known MIDI system exclusive message.
+ * @param msg Which sysex message to retrieve
+ * @param[out] length Receives the length of the returned buffer
+ * @return Pointer to byte buffer with sysex message
+ */
+const byte *MidiGetStandardSysexMessage(MidiSysexMessage msg, size_t &length)
+{
+	static byte reset_gm_sysex[] = { 0xF0, 0x7E, 0x7F, 0x09, 0x01, 0xF7 };
+	static byte reset_gs_sysex[] = { 0xF0, 0x41, 0x10, 0x42, 0x12, 0x40, 0x00, 0x7F, 0x00, 0x41, 0xF7 };
+	static byte reset_xg_sysex[] = { 0xF0, 0x43, 0x10, 0x4C, 0x00, 0x00, 0x7E, 0x00, 0xF7 };
+	static byte roland_reverb_sysex[] = { 0xF0, 0x41, 0x10, 0x42, 0x12, 0x40, 0x01, 0x30, 0x02, 0x04, 0x00, 0x40, 0x40, 0x00, 0x00, 0x09, 0xF7 };
+
+	switch (msg) {
+		case MidiSysexMessage::ResetGM:
+			length = lengthof(reset_gm_sysex);
+			return reset_gm_sysex;
+		case MidiSysexMessage::ResetGS:
+			length = lengthof(reset_gs_sysex);
+			return reset_gs_sysex;
+		case MidiSysexMessage::ResetXG:
+			length = lengthof(reset_xg_sysex);
+			return reset_xg_sysex;
+		case MidiSysexMessage::RolandSetReverb:
+			length = lengthof(roland_reverb_sysex);
+			return roland_reverb_sysex;
+		default:
+			NOT_REACHED();
+	}
+}
+
 /**
  * Owning byte buffer readable as a stream.
  * RAII-compliant to make teardown in error situations easier.

src/music/win32_m.cpp

@@ -35,7 +35,7 @@ static struct {
 	CRITICAL_SECTION lock; ///< synchronization for playback status fields
 
 	bool playing;        ///< flag indicating that playback is active
-	bool do_start;       ///< flag for starting playback of next_file at next opportunity
+	int do_start;        ///< flag for starting playback of next_file at next opportunity
 	bool do_stop;        ///< flag for stopping playback at next opportunity
 	byte current_volume; ///< current effective volume setting
 	byte new_volume;     ///< volume setting to change to
@@ -73,10 +73,10 @@ static void TransmitChannelMsg(byte status, byte p1, byte p2 = 0)
 	midiOutShortMsg(_midi.midi_out, status | (p1 << 8) | (p2 << 16));
 }
 
-static void TransmitSysex(byte *&msg_start, size_t &remaining)
+static void TransmitSysex(const byte *&msg_start, size_t &remaining)
 {
 	/* find end of message */
-	byte *msg_end = msg_start;
+	const byte *msg_end = msg_start;
 	while (*msg_end != MIDIST_ENDSYSEX) msg_end++;
 	msg_end++; /* also include sysex end byte */
 
@@ -97,9 +97,11 @@ static void TransmitSysex(byte *&msg_start, size_t &remaining)
 	msg_start = msg_end;
 }
 
-static void TransmitSysexConst(byte *msg_start, size_t length)
+static void TransmitStandardSysex(MidiSysexMessage msg)
 {
-	TransmitSysex(msg_start, length);
+	size_t length = 0;
+	const byte *data = MidiGetStandardSysexMessage(msg, length);
+	TransmitSysex(data, length);
 }
 
 /**
@@ -108,10 +110,9 @@ static void TransmitSysexConst(byte *msg_start, size_t length)
  */
 void CALLBACK TimerCallback(UINT uTimerID, UINT, DWORD_PTR dwUser, DWORD_PTR, DWORD_PTR)
 {
-	/* Try to check playback status changes.
+	/* Ensure only one timer callback is running at once, and prevent races on status flags */
-	 * If _midi is already locked, skip checking for this cycle and try again
+	if (!TryEnterCriticalSection(&_midi.lock)) return;
-	 * next cycle, instead of waiting for locks in the realtime callback. */
+
-	if (TryEnterCriticalSection(&_midi.lock)) {
 	/* check for stop */
 	if (_midi.do_stop) {
 		DEBUG(driver, 2, "Win32-MIDI: timer: do_stop is set");
@@ -123,33 +124,50 @@ void CALLBACK TimerCallback(UINT uTimerID, UINT, DWORD_PTR dwUser, DWORD_PTR, DW
 	}
 
 	/* check for start/restart/change song */
-		if (_midi.do_start) {
+	if (_midi.do_start != 0) {
-			DEBUG(driver, 2, "Win32-MIDI: timer: do_start is set");
+		/* Have a delay between playback start steps, prevents jumbled-together notes at the start of song */
-			if (_midi.playing) {
+		if (timeGetTime() - _midi.playback_start_time < 50) {
+			LeaveCriticalSection(&_midi.lock);
+			return;
+		}
+		DEBUG(driver, 2, "Win32-MIDI: timer: do_start step %d", _midi.do_start);
+
+		if (_midi.do_start == 1) {
+			/* Send "all notes off" */
 			midiOutReset(_midi.midi_out);
-				/* Some songs change the "Pitch bend range" registered
+			_midi.playback_start_time = timeGetTime();
-				 * parameter. If this doesn't get reset, everything else
+			_midi.do_start = 2;
-				 * will start sounding wrong. */
+
-				for (int ch = 0; ch < 16; ch++) {
+			LeaveCriticalSection(&_midi.lock);
-					/* Running status, only need status for first message */
+			return;
-					/* Select RPN 00.00, set value to 02.00, and unselect again */
+		} else if (_midi.do_start == 2) {
-					TransmitChannelMsg(MIDIST_CONTROLLER | ch, MIDICT_RPN_SELECT_LO, 0x00);
+			/* Reset the device to General MIDI defaults */
-					TransmitChannelMsg(MIDICT_RPN_SELECT_HI, 0x00);
+			TransmitStandardSysex(MidiSysexMessage::ResetGM);
-					TransmitChannelMsg(MIDICT_DATAENTRY,     0x02);
+			_midi.playback_start_time = timeGetTime();
-					TransmitChannelMsg(MIDICT_DATAENTRY_LO,  0x00);
+			_midi.do_start = 3;
-					TransmitChannelMsg(MIDICT_RPN_SELECT_LO, 0x7F);
+
-					TransmitChannelMsg(MIDICT_RPN_SELECT_HI, 0x7F);
+			LeaveCriticalSection(&_midi.lock);
-				}
+			return;
-			}
+		} else if (_midi.do_start == 3) {
+			/* Set up device-specific effects */
+			TransmitStandardSysex(MidiSysexMessage::RolandSetReverb);
+			_midi.playback_start_time = timeGetTime();
+			_midi.do_start = 4;
+
+			LeaveCriticalSection(&_midi.lock);
+			return;
+		} else if (_midi.do_start == 4) {
+			/* Load the new file */
 			_midi.current_file.MoveFrom(_midi.next_file);
 			std::swap(_midi.next_segment, _midi.current_segment);
 			_midi.current_segment.start_block = 0;
 			_midi.playback_start_time = timeGetTime();
 			_midi.playing = true;
-			_midi.do_start = false;
+			_midi.do_start = 0;
 			_midi.current_block = 0;
 
 			MemSetT<byte>(_midi.channel_volumes, 127, lengthof(_midi.channel_volumes));
+		}
 	} else if (!_midi.playing) {
 		/* not playing, stop the timer */
 		DEBUG(driver, 2, "Win32-MIDI: timer: not playing, stopping timer");
@@ -167,18 +185,14 @@ void CALLBACK TimerCallback(UINT uTimerID, UINT, DWORD_PTR dwUser, DWORD_PTR, DW
 			_midi.current_volume = _midi.new_volume;
 			volume_throttle = 20 / _midi.time_period;
 			for (int ch = 0; ch < 16; ch++) {
-					int vol = ScaleVolume(_midi.channel_volumes[ch], _midi.current_volume);
+				byte vol = ScaleVolume(_midi.channel_volumes[ch], _midi.current_volume);
 				TransmitChannelMsg(MIDIST_CONTROLLER | ch, MIDICT_CHANVOLUME, vol);
 			}
-			}
+		} else {
-			else {
 			volume_throttle--;
 		}
 	}
 
-		LeaveCriticalSection(&_midi.lock);
-	}
-
 	/* skip beginning of file? */
 	if (_midi.current_segment.start > 0 && _midi.current_block == 0 && _midi.current_segment.start_block == 0) {
 		/* find first block after start time and pretend playback started earlier
@@ -200,7 +214,7 @@ void CALLBACK TimerCallback(UINT uTimerID, UINT, DWORD_PTR dwUser, DWORD_PTR, DW
 					 * The delay compensation is needed to avoid time-compression of following messages.
 					 */
 					DEBUG(driver, 2, "Win32-MIDI: timer: start from block %d (ticktime %d, realtime %.3f, bytes %d)", (int)bl, (int)block.ticktime, ((int)block.realtime) / 1000.0, (int)preload_bytes);
-					_midi.playback_start_time -= block.realtime / 1000 - preload_bytes * 1000 / 3125;
+					_midi.playback_start_time -= block.realtime / 1000 - (DWORD)(preload_bytes * 1000 / 3125);
 					break;
 				}
 			}
@@ -229,7 +243,7 @@ void CALLBACK TimerCallback(UINT uTimerID, UINT, DWORD_PTR dwUser, DWORD_PTR, DW
 			break;
 		}
 
-		byte *data = block.data.Begin();
+		const byte *data = block.data.Begin();
 		size_t remaining = block.data.Length();
 		byte last_status = 0;
 		while (remaining > 0) {
@@ -308,25 +322,28 @@ void CALLBACK TimerCallback(UINT uTimerID, UINT, DWORD_PTR dwUser, DWORD_PTR, DW
 			_midi.do_stop = true;
 		}
 	}
+
+	LeaveCriticalSection(&_midi.lock);
 }
 
 void MusicDriver_Win32::PlaySong(const MusicSongInfo &song)
 {
 	DEBUG(driver, 2, "Win32-MIDI: PlaySong: entry");
+
+	MidiFile new_song;
+	if (!new_song.LoadSong(song)) return;
+	DEBUG(driver, 2, "Win32-MIDI: PlaySong: Loaded song");
+
 	EnterCriticalSection(&_midi.lock);
 
-	if (!_midi.next_file.LoadSong(song)) {
+	_midi.next_file.MoveFrom(new_song);
-		LeaveCriticalSection(&_midi.lock);
-		return;
-	}
-
 	_midi.next_segment.start = song.override_start;
 	_midi.next_segment.end = song.override_end;
 	_midi.next_segment.loop = song.loop;
 
 	DEBUG(driver, 2, "Win32-MIDI: PlaySong: setting flag");
 	_midi.do_stop = _midi.playing;
-	_midi.do_start = true;
+	_midi.do_start = 1;
 
 	if (_midi.timer_id == 0) {
 		DEBUG(driver, 2, "Win32-MIDI: PlaySong: starting timer");
@@ -347,7 +364,7 @@ void MusicDriver_Win32::StopSong()
 
 bool MusicDriver_Win32::IsSongPlaying()
 {
-	return _midi.playing || _midi.do_start;
+	return _midi.playing || (_midi.do_start != 0);
 }
 
 void MusicDriver_Win32::SetVolume(byte vol)
@@ -381,14 +398,6 @@ const char *MusicDriver_Win32::Start(const char * const *parm)
 
 	midiOutReset(_midi.midi_out);
 
-	/* Standard "Enable General MIDI" message */
-	static byte gm_enable_sysex[] = { 0xF0, 0x7E, 0x00, 0x09, 0x01, 0xF7 };
-	TransmitSysexConst(&gm_enable_sysex[0], sizeof(gm_enable_sysex));
-
-	/* Roland-specific reverb room control, used by the original game */
-	static byte roland_reverb_sysex[] = { 0xF0, 0x41, 0x10, 0x42, 0x12, 0x40, 0x01, 0x30, 0x02, 0x04, 0x00, 0x40, 0x40, 0x00, 0x00, 0x09, 0xF7 };
-	TransmitSysexConst(&roland_reverb_sysex[0], sizeof(roland_reverb_sysex));
-
 	/* prepare multimedia timer */
 	TIMECAPS timecaps;
 	if (timeGetDevCaps(&timecaps, sizeof(timecaps)) == MMSYSERR_NOERROR) {