Fix thread safety/data races between game and sound effect mixer threads

src/framerate_gui.cpp

@@ -25,8 +25,16 @@
 #include "game/game_instance.hpp"
 
 #include "widgets/framerate_widget.h"
+
+#include <atomic>
+#include <mutex>
+#include <vector>
+
 #include "safeguards.h"
 
+static std::mutex _sound_perf_lock;
+static std::atomic<bool> _sound_perf_pending;
+static std::vector<TimingMeasurement> _sound_perf_measurements;
 
 /**
  * Private declarations for performance measurement implementation
@@ -251,6 +259,20 @@ PerformanceMeasurer::~PerformanceMeasurer()
 			return;
 		}
 	}
+	if (this->elem == PFE_SOUND) {
+		/* PFE_SOUND measurements are made from the mixer thread.
+		 * _pf_data cannot be concurrently accessed from the mixer thread
+		 * and the main thread, so store the measurement results in a
+		 * mutex-protected queue which is drained by the main thread.
+		 * See: ProcessPendingPerformanceMeasurements() */
+		TimingMeasurement end = GetPerformanceTimer();
+		std::lock_guard lk(_sound_perf_lock);
+		if (_sound_perf_measurements.size() >= NUM_FRAMERATE_POINTS * 2) return;
+		_sound_perf_measurements.push_back(this->start_time);
+		_sound_perf_measurements.push_back(end);
+		_sound_perf_pending.store(true, std::memory_order_release);
+		return;
+	}
 	_pf_data[this->elem].Add(this->start_time, GetPerformanceTimer());
 }
 
@@ -1079,3 +1101,22 @@ void ConPrintFramerate()
 		IConsolePrint(CC_ERROR, "No performance measurements have been taken yet.");
 	}
 }
+
+/**
+ * This drains the PFE_SOUND measurement data queue into _pf_data.
+ * PFE_SOUND measurements are made by the mixer thread and so cannot be stored
+ * into _pf_data directly, because this would not be thread safe and would violate
+ * the invariants of the FPS and frame graph windows.
+ * @see PerformanceMeasurement::~PerformanceMeasurement()
+ */
+void ProcessPendingPerformanceMeasurements()
+{
+	if (_sound_perf_pending.load(std::memory_order_acquire)) {
+		std::lock_guard lk(_sound_perf_lock);
+		for (size_t i = 0; i < _sound_perf_measurements.size(); i += 2) {
+			_pf_data[PFE_SOUND].Add(_sound_perf_measurements[i], _sound_perf_measurements[i + 1]);
+		}
+		_sound_perf_measurements.clear();
+		_sound_perf_pending.store(false, std::memory_order_relaxed);
+	}
+}

src/mixer.cpp

@@ -10,6 +10,7 @@
 #include "stdafx.h"
 #include <math.h>
 #include <mutex>
+#include <atomic>
 #include "core/math_func.hpp"
 #include "framerate_type.h"
 #include "settings_type.h"
@@ -18,8 +19,6 @@
 #include "mixer.h"
 
 struct MixerChannel {
-	bool active;
-
 	/* pointer to allocated buffer memory */
 	int8 *memory;
 
@@ -36,11 +35,13 @@ struct MixerChannel {
 	bool is16bit;
 };
 
+static std::atomic<uint8> _active_channels;
 static MixerChannel _channels[8];
 static uint32 _play_rate = 11025;
 static uint32 _max_size = UINT_MAX;
 static MxStreamCallback _music_stream = nullptr;
 static std::mutex _music_stream_mutex;
+static std::atomic<uint8> _effect_vol;
 
 /**
  * The theoretical maximum volume for a single sound sample. Multiple sound
@@ -135,9 +136,9 @@ static void mix_int8_to_int16(MixerChannel *sc, int16 *buffer, uint samples, uin
 	sc->pos = b - sc->memory;
 }
 
-static void MxCloseChannel(MixerChannel *mc)
+static void MxCloseChannel(uint8 channel_index)
 {
-	mc->active = false;
+	_active_channels.fetch_and(~(1 << channel_index), std::memory_order_release);
 }
 
 void MxMixSamples(void *buffer, uint samples)
@@ -149,8 +150,6 @@ void MxMixSamples(void *buffer, uint samples)
 		last_samples = samples;
 	}
 
-	MixerChannel *mc;
-
 	/* Clear the buffer */
 	memset(buffer, 0, sizeof(int16) * 2 * samples);
 
@@ -164,34 +163,36 @@ void MxMixSamples(void *buffer, uint samples)
 	 * perceived difference in loudness to better match expectations. effect_vol
 	 * is expected to be in the range 0-127 hence the division by 127 * 127 to
 	 * get back into range. */
-	uint8 effect_vol = (_settings_client.music.effect_vol *
-	                    _settings_client.music.effect_vol *
-	                    _settings_client.music.effect_vol) / (127 * 127);
+	uint8 effect_vol_setting = _effect_vol.load(std::memory_order_relaxed);
+	uint8 effect_vol = (effect_vol_setting *
+	                    effect_vol_setting *
+	                    effect_vol_setting) / (127 * 127);
 
 	/* Mix each channel */
-	for (mc = _channels; mc != endof(_channels); mc++) {
-		if (mc->active) {
-			if (mc->is16bit) {
-				mix_int16(mc, (int16*)buffer, samples, effect_vol);
-			} else {
-				mix_int8_to_int16(mc, (int16*)buffer, samples, effect_vol);
-			}
-			if (mc->samples_left == 0) MxCloseChannel(mc);
+	uint8 active = _active_channels.load(std::memory_order_acquire);
+	for (uint8 idx : SetBitIterator(active)) {
+		MixerChannel *mc = &_channels[idx];
+		if (mc->is16bit) {
+			mix_int16(mc, (int16*)buffer, samples, effect_vol);
+		} else {
+			mix_int8_to_int16(mc, (int16*)buffer, samples, effect_vol);
 		}
+		if (mc->samples_left == 0) MxCloseChannel(idx);
 	}
 }
 
 MixerChannel *MxAllocateChannel()
 {
-	MixerChannel *mc;
-	for (mc = _channels; mc != endof(_channels); mc++) {
-		if (!mc->active) {
-			free(mc->memory);
-			mc->memory = nullptr;
-			return mc;
-		}
-	}
-	return nullptr;
+	uint8 currently_active = _active_channels.load(std::memory_order_acquire);
+	uint8 available = ~currently_active;
+	if (available == 0) return nullptr;
+
+	uint8 channel_index = FindFirstBit(available);
+
+	MixerChannel *mc = &_channels[channel_index];
+	free(mc->memory);
+	mc->memory = nullptr;
+	return mc;
 }
 
 void MxSetChannelRawSrc(MixerChannel *mc, int8 *mem, size_t size, uint rate, bool is16bit)
@@ -231,7 +232,8 @@ void MxSetChannelVolume(MixerChannel *mc, uint volume, float pan)
 
 void MxActivateChannel(MixerChannel *mc)
 {
-	mc->active = true;
+	uint8 channel_index = mc - _channels;
+	_active_channels.fetch_or((1 << channel_index), std::memory_order_release);
 }
 
 /**
@@ -255,3 +257,8 @@ bool MxInitialize(uint rate)
 	_music_stream = nullptr; /* rate may have changed, any music source is now invalid */
 	return true;
 }
+
+void SetEffectVolume(uint8 volume)
+{
+	_effect_vol.store(volume, std::memory_order_relaxed);
+}

src/mixer.h

@@ -30,4 +30,6 @@ void MxActivateChannel(MixerChannel*);
 
 uint32 MxSetMusicSource(MxStreamCallback music_callback);
 
+void SetEffectVolume(uint8 volume);
+
 #endif /* MIXER_H */

src/music_gui.cpp

@@ -27,6 +27,7 @@
 #include "widgets/dropdown_func.h"
 #include "widgets/dropdown_type.h"
 #include "widgets/slider_func.h"
+#include "mixer.h"
 
 #include "widgets/music_widget.h"
 
@@ -788,7 +789,11 @@ struct MusicWindow : public Window {
 			case WID_M_MUSIC_VOL: case WID_M_EFFECT_VOL: { // volume sliders
 				byte &vol = (widget == WID_M_MUSIC_VOL) ? _settings_client.music.music_vol : _settings_client.music.effect_vol;
 				if (ClickVolumeSliderWidget(this->GetWidget<NWidgetBase>(widget)->GetCurrentRect(), pt, vol)) {
-					if (widget == WID_M_MUSIC_VOL) MusicDriver::GetInstance()->SetVolume(vol);
+					if (widget == WID_M_MUSIC_VOL) {
+						MusicDriver::GetInstance()->SetVolume(vol);
+					} else {
+						SetEffectVolume(vol);
+					}
 					this->SetWidgetDirty(widget);
 					SetWindowClassesDirty(WC_GAME_OPTIONS);
 				}

src/openttd.cpp

@@ -13,6 +13,7 @@
 #include "sound/sound_driver.hpp"
 #include "music/music_driver.hpp"
 #include "video/video_driver.hpp"
+#include "mixer.h"
 
 #include "fontcache.h"
 #include "error.h"
@@ -452,8 +453,9 @@ struct AfterNewGRFScan : NewGRFScanCallback {
 		/* We have loaded the config, so we may possibly save it. */
 		_save_config = save_config;
 
-		/* restore saved music volume */
+		/* restore saved music and effects volumes */
 		MusicDriver::GetInstance()->SetVolume(_settings_client.music.music_vol);
+		SetEffectVolume(_settings_client.music.effect_vol);
 
 		if (startyear != INVALID_YEAR) IConsoleSetSetting("game_creation.starting_year", startyear);
 		if (generation_seed != GENERATE_NEW_SEED) _settings_newgame.game_creation.generation_seed = generation_seed;

src/window.cpp

@@ -3067,6 +3067,9 @@ void UpdateWindows()
 	PerformanceMeasurer framerate(PFE_DRAWING);
 	PerformanceAccumulator::Reset(PFE_DRAWWORLD);
 
+	extern void ProcessPendingPerformanceMeasurements();
+	ProcessPendingPerformanceMeasurements();
+
 	CallWindowRealtimeTickEvent(delta_ms);
 
 	static GUITimer network_message_timer = GUITimer(1);