Merge pull request #759 from TomHarte/CPCCorruption

Resolves a variety of potential startup data races.

Machines/AmstradCPC/AmstradCPC.cpp

@@ -306,9 +306,16 @@ class CRTCBusHandler {
 			visible early. The CPC uses changes in sync to clock the interrupt timer.
 		*/
 		void perform_bus_cycle_phase2(const Motorola::CRTC::BusState &state) {
-			// check for a trailing CRTC hsync; if one occurred then that's the trigger potentially to change
+			// Notify a leading hsync edge to the interrupt timer.
-			// modes, and should also be sent on to the interrupt timer
+			// Per Interrupts in the CPC: "to be confirmed: does gate array count positive or negative edge transitions of HSYNC signal?";
+			// if you take it as given that display mode is latched as a result of hsync then Pipe Mania seems to imply that the count
+			// occurs on a leading edge and the mode lock on a trailing.
 			if(was_hsync_ && !state.hsync) {
+				interrupt_timer_.signal_hsync();
+			}
+
+			// Check for a trailing CRTC hsync; if one occurred then that's the trigger potentially to change modes.
+			if(!was_hsync_ && state.hsync) {
 				if(mode_ != next_mode_) {
 					mode_ = next_mode_;
 					switch(mode_) {
@@ -319,8 +326,6 @@ class CRTCBusHandler {
 					}
 					build_mode_table();
 				}
-
-				interrupt_timer_.signal_hsync();
 			}
 
 			// check for a leading vsync; that also needs to be communicated to the interrupt timer

OSBindings/Mac/Clock Signal/Documents/MachineDocument.swift

@@ -174,7 +174,7 @@ class MachineDocument:
 	// MARK: - Connections Between Machine and the Outside World
 
 	private func setupMachineOutput() {
-		if let machine = self.machine, let openGLView = self.openGLView {
+		if let machine = self.machine, let openGLView = self.openGLView, machine.view != openGLView {
 			// Establish the output aspect ratio and audio.
 			let aspectRatio = self.aspectRatio()
 			machine.setView(openGLView, aspectRatio: Float(aspectRatio.width / aspectRatio.height))
@@ -227,13 +227,17 @@ class MachineDocument:
 		//
 		// TODO: this needs to be threadsafe. FIX!
 		let maximumSamplingRate = CSAudioQueue.preferredSamplingRate()
-		let selectedSamplingRate = self.machine.idealSamplingRate(from: NSRange(location: 0, length: NSInteger(maximumSamplingRate)))
+		let selectedSamplingRate = Float64(self.machine.idealSamplingRate(from: NSRange(location: 0, length: NSInteger(maximumSamplingRate))))
 		let isStereo = self.machine.isStereo()
 		if selectedSamplingRate > 0 {
-			self.audioQueue = CSAudioQueue(samplingRate: Float64(selectedSamplingRate), isStereo:isStereo)
+			// [Re]create the audio queue only if necessary.
-			self.audioQueue.delegate = self
+			if self.audioQueue == nil || self.audioQueue.samplingRate != selectedSamplingRate {
-			self.machine.audioQueue = self.audioQueue
+				self.machine.audioQueue = nil
-			self.machine.setAudioSamplingRate(selectedSamplingRate, bufferSize:audioQueue.preferredBufferSize, stereo:isStereo)
+				self.audioQueue = CSAudioQueue(samplingRate: Float64(selectedSamplingRate), isStereo:isStereo)
+				self.audioQueue.delegate = self
+				self.machine.audioQueue = self.audioQueue
+				self.machine.setAudioSamplingRate(Float(selectedSamplingRate), bufferSize:audioQueue.preferredBufferSize, stereo:isStereo)
+			}
 		}
 	}
 

OSBindings/Mac/Clock Signal/Machine/CSMachine.h

@@ -75,7 +75,7 @@ typedef NS_ENUM(NSInteger, CSMachineKeyboardInputMode) {
 - (void)setMouseButton:(int)button isPressed:(BOOL)isPressed;
 - (void)addMouseMotionX:(CGFloat)deltaX y:(CGFloat)deltaY;
 
-@property (nonatomic, strong, nullable) CSAudioQueue *audioQueue;
+@property (atomic, strong, nullable) CSAudioQueue *audioQueue;
 @property (nonatomic, readonly, nonnull) CSOpenGLView *view;
 @property (nonatomic, weak, nullable) id<CSMachineDelegate> delegate;
 

Outputs/OpenGL/ScanTarget.cpp

@@ -131,6 +131,8 @@ void ScanTarget::set_modals(Modals modals) {
 Outputs::Display::ScanTarget::Scan *ScanTarget::begin_scan() {
 	if(allocation_has_failed_) return nullptr;
 
+	std::lock_guard<std::mutex> lock_guard(write_pointers_mutex_);
+
 	const auto result = &scan_buffer_[write_pointers_.scan_buffer];
 	const auto read_pointers = read_pointers_.load();
 
@@ -154,6 +156,7 @@ Outputs::Display::ScanTarget::Scan *ScanTarget::begin_scan() {
 
 void ScanTarget::end_scan() {
 	if(vended_scan_) {
+		std::lock_guard<std::mutex> lock_guard(write_pointers_mutex_);
 		vended_scan_->data_y = TextureAddressGetY(vended_write_area_pointer_);
 		vended_scan_->line = write_pointers_.line;
 		vended_scan_->scan.end_points[0].data_offset += TextureAddressGetX(vended_write_area_pointer_);
@@ -177,6 +180,8 @@ uint8_t *ScanTarget::begin_data(size_t required_length, size_t required_alignmen
 	assert(required_alignment);
 
 	if(allocation_has_failed_) return nullptr;
+
+	std::lock_guard<std::mutex> lock_guard(write_pointers_mutex_);
 	if(write_area_texture_.empty()) {
 		allocation_has_failed_ = true;
 		return nullptr;
@@ -222,6 +227,8 @@ uint8_t *ScanTarget::begin_data(size_t required_length, size_t required_alignmen
 void ScanTarget::end_data(size_t actual_length) {
 	if(allocation_has_failed_ || !data_is_allocated_) return;
 
+	std::lock_guard<std::mutex> lock_guard(write_pointers_mutex_);
+
 	// Bookend the start of the new data, to safeguard for precision errors in sampling.
 	memcpy(
 		&write_area_texture_[size_t(write_pointers_.write_area - 1) * data_type_size_],
@@ -268,6 +275,7 @@ void ScanTarget::announce(Event event, bool is_visible, const Outputs::Display::
 	if(output_is_visible_ == is_visible) return;
 	if(is_visible) {
 		const auto read_pointers = read_pointers_.load();
+		std::lock_guard<std::mutex> lock_guard(write_pointers_mutex_);
 
 		// Commit the most recent line only if any scans fell on it.
 		// Otherwise there's no point outputting it, it'll contribute nothing.
@@ -336,14 +344,20 @@ void ScanTarget::announce(Event event, bool is_visible, const Outputs::Display::
 
 void ScanTarget::setup_pipeline() {
 	const auto data_type_size = Outputs::Display::size_for_data_type(modals_.input_data_type);
-	if(data_type_size != data_type_size_) {
-		// TODO: flush output.
 
-		data_type_size_ = data_type_size;
+	// Ensure the lock guard here has a restricted scope; this is the only time that a thread
-		write_area_texture_.resize(WriteAreaWidth*WriteAreaHeight*data_type_size_);
+	// other than the main owner of write_pointers_ may adjust it.
+	{
+		std::lock_guard<std::mutex> lock_guard(write_pointers_mutex_);
+		if(data_type_size != data_type_size_) {
+			// TODO: flush output.
 
-		write_pointers_.scan_buffer = 0;
+			data_type_size_ = data_type_size;
-		write_pointers_.write_area = 0;
+			write_area_texture_.resize(WriteAreaWidth*WriteAreaHeight*data_type_size_);
+
+			write_pointers_.scan_buffer = 0;
+			write_pointers_.write_area = 0;
+		}
 	}
 
 	// Prepare to bind line shaders.

Outputs/OpenGL/ScanTarget.hpp

@@ -116,6 +116,11 @@ class ScanTarget: public Outputs::Display::ScanTarget {
 		/// A pointer to the next thing that should be provided to the caller for data.
 		PointerSet write_pointers_;
 
+		/// A mutex for gettng access to write_pointers_; access to write_pointers_,
+		/// data_type_size_ or write_area_texture_ is almost never contended, so this
+		/// is cheap for the main use case.
+		std::mutex write_pointers_mutex_;
+
 		/// A pointer to the final thing currently cleared for submission.
 		std::atomic<PointerSet> submit_pointers_;
 

Outputs/Speaker/Implementation/LowpassSpeaker.hpp

@@ -124,7 +124,8 @@ template <typename SampleSource> class LowpassSpeaker: public Speaker {
 			at construction, filtering it and passing it on to the speaker's delegate if there is one.
 		*/
 		void run_for(const Cycles cycles) {
-			if(!delegate_) return;
+			const auto delegate = delegate_.load();
+			if(!delegate) return;
 
 			std::size_t cycles_remaining = size_t(cycles.as_integral());
 			if(!cycles_remaining) return;
@@ -138,7 +139,7 @@ template <typename SampleSource> class LowpassSpeaker: public Speaker {
 			}
 			if(filter_parameters.parameters_are_dirty) update_filter_coefficients(filter_parameters);
 			if(filter_parameters.input_rate_changed) {
-				delegate_->speaker_did_change_input_clock(this);
+				delegate->speaker_did_change_input_clock(this);
 			}
 
 			switch(conversion_) {

Outputs/Speaker/Speaker.hpp

@@ -9,6 +9,7 @@
 #ifndef Speaker_hpp
 #define Speaker_hpp
 
+#include <atomic>
 #include <cstdint>
 #include <vector>
 
@@ -82,12 +83,16 @@ class Speaker {
 
 	protected:
 		void did_complete_samples(Speaker *speaker, const std::vector<int16_t> &buffer, bool is_stereo) {
+			// Test the delegate for existence again, as it may have changed.
+			const auto delegate = delegate_.load();
+			if(!delegate) return;
+
 			++completed_sample_sets_;
 
 			// Hope for the fast path first: producer and consumer agree about
 			// number of channels.
 			if(is_stereo == stereo_output_) {
-				delegate_->speaker_did_complete_samples(this, buffer);
+				delegate->speaker_did_complete_samples(this, buffer);
 				return;
 			}
 
@@ -106,9 +111,9 @@ class Speaker {
 					mix_buffer_[(c << 1) + 0] = mix_buffer_[(c << 1) + 1] = buffer[c];
 				}
 			}
-			delegate_->speaker_did_complete_samples(this, mix_buffer_);
+			delegate->speaker_did_complete_samples(this, mix_buffer_);
 		}
-		Delegate *delegate_ = nullptr;
+		std::atomic<Delegate *> delegate_ = nullptr;
 
 	private:
 		void compute_output_rate() {
@@ -121,7 +126,7 @@ class Speaker {
 		float input_rate_multiplier_ = 1.0f;
 		float output_cycles_per_second_ = 1.0f;
 		int output_buffer_size_ = 1;
-		bool stereo_output_ = false;
+		std::atomic<bool> stereo_output_ = false;
 		std::vector<int16_t> mix_buffer_;
 };
 

Storage/Disk/Drive.hpp

@@ -205,7 +205,7 @@ class Drive: public ClockingHint::Source, public TimedEventLoop {
 
 		// Contains the multiplier that converts between track-relative lengths
 		// to real-time lengths. So it's the reciprocal of rotation speed.
-		float rotational_multiplier_;
+		float rotational_multiplier_ = 1.0f;
 
 		// A count of time since the index hole was last seen. Which is used to
 		// determine how far the drive is into a full rotation when switching to