Merge pull request #597 from TomHarte/MacRaceCondition

Splits OpenGL ScanTarget update and draw functions.

OSBindings/Mac/Clock Signal.xcodeproj/xcshareddata/xcschemes/Clock Signal.xcscheme

@@ -68,7 +68,7 @@
       </AdditionalOptions>
    </TestAction>
    <LaunchAction
-      buildConfiguration = "Debug"
+      buildConfiguration = "Release"
       selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB"
       selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB"
       enableASanStackUseAfterReturn = "YES"

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

@@ -198,17 +198,21 @@ class MachineDocument:
 	}
 
 	// MARK: CSOpenGLViewDelegate
-	final func openGLView(_ view: CSOpenGLView, drawViewOnlyIfDirty onlyIfDirty: Bool) {
-		bestEffortLock.lock()
-		if let bestEffortUpdater = bestEffortUpdater {
-			bestEffortLock.unlock()
-			bestEffortUpdater.update()
-			if drawLock.try() {
-				self.machine.drawView(forPixelSize: view.backingSize, onlyIfDirty: onlyIfDirty)
-				drawLock.unlock()
-			}
-		} else {
-			bestEffortLock.unlock()
+	final func openGLViewRedraw(_ view: CSOpenGLView, event redrawEvent: CSOpenGLViewRedrawEvent) {
+		switch redrawEvent {
+			case .timer:
+				bestEffortLock.lock()
+				if let bestEffortUpdater = bestEffortUpdater {
+					bestEffortLock.unlock()
+					bestEffortUpdater.update()
+				} else {
+					bestEffortLock.unlock()
+				}
+				self.machine.updateView(forPixelSize: view.backingSize)
+				fallthrough
+
+			case .appKit:
+				self.machine.drawView(forPixelSize: view.backingSize)
 		}
 	}
 

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

@@ -54,7 +54,9 @@ typedef NS_ENUM(NSInteger, CSMachineKeyboardInputMode) {
 - (void)setAudioSamplingRate:(float)samplingRate bufferSize:(NSUInteger)bufferSize;
 
 - (void)setView:(nullable CSOpenGLView *)view aspectRatio:(float)aspectRatio;
-- (void)drawViewForPixelSize:(CGSize)pixelSize onlyIfDirty:(BOOL)onlyIfDirty;
+
+- (void)updateViewForPixelSize:(CGSize)pixelSize;
+- (void)drawViewForPixelSize:(CGSize)pixelSize;
 
 - (void)setKey:(uint16_t)key characters:(nullable NSString *)characters isPressed:(BOOL)isPressed;
 - (void)clearAllKeys;

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

@@ -240,8 +240,12 @@ struct ActivityObserver: public Activity::Observer {
 	_machine->crt_machine()->set_scan_target(_scanTarget.get());
 }
 
-- (void)drawViewForPixelSize:(CGSize)pixelSize onlyIfDirty:(BOOL)onlyIfDirty {
-	_scanTarget->draw(true, (int)pixelSize.width, (int)pixelSize.height);
+- (void)updateViewForPixelSize:(CGSize)pixelSize {
+	_scanTarget->update((int)pixelSize.width, (int)pixelSize.height);
+}
+
+- (void)drawViewForPixelSize:(CGSize)pixelSize {
+	_scanTarget->draw((int)pixelSize.width, (int)pixelSize.height);
 }
 
 - (void)paste:(NSString *)paste {

OSBindings/Mac/Clock Signal/Views/CSOpenGLView.h

@@ -11,15 +11,24 @@
 
 @class CSOpenGLView;
 
+typedef NS_ENUM(NSInteger, CSOpenGLViewRedrawEvent) {
+	/// Indicates that AppKit requested a redraw for some reason (mostly likely, the window is being resized). So,
+	/// if the delegate doesn't redraw the view, the user is likely to see a graphical flaw.
+	CSOpenGLViewRedrawEventAppKit,
+	/// Indicates that the view's display-linked timer has triggered a redraw request. So, if the delegate doesn't
+	/// redraw the view, the user will just see the previous drawing without interruption.
+	CSOpenGLViewRedrawEventTimer
+};
+
 @protocol CSOpenGLViewDelegate
 /*!
 	Requests that the delegate produce an image of its current output state. May be called on
 	any queue or thread.
 	@param view The view making the request.
-	@param onlyIfDirty If @c YES then the delegate may decline to redraw if its output would be
+	@param redrawEvent If @c YES then the delegate may decline to redraw if its output would be
 	identical to the previous frame. If @c NO then the delegate must draw.
 */
-- (void)openGLView:(nonnull CSOpenGLView *)view drawViewOnlyIfDirty:(BOOL)onlyIfDirty;
+- (void)openGLViewRedraw:(nonnull CSOpenGLView *)view event:(CSOpenGLViewRedrawEvent)redrawEvent;
 
 /*!
 	Announces receipt of a file by drag and drop to the delegate.

OSBindings/Mac/Clock Signal/Views/CSOpenGLView.m

@@ -53,7 +53,15 @@ static CVReturn DisplayLinkCallback(CVDisplayLinkRef displayLink, const CVTimeSt
 - (void)drawAtTime:(const CVTimeStamp *)now frequency:(double)frequency
 {
 	// Draw the display now regardless of other activity.
-	[self drawViewOnlyIfDirty:YES];
+	[self performWithGLContext:^{
+		[self.delegate openGLViewRedraw:self event:CSOpenGLViewRedrawEventTimer];
+		CGLFlushDrawable([[self openGLContext] CGLContextObj]);
+	}];
+}
+
+- (void)drawRect:(NSRect)dirtyRect
+{
+	[self.delegate openGLViewRedraw:self event:CSOpenGLViewRedrawEventAppKit];
 }
 
 - (void)invalidate
@@ -119,19 +127,6 @@ static CVReturn DisplayLinkCallback(CVDisplayLinkRef displayLink, const CVTimeSt
 	[self registerForDraggedTypes:@[(__bridge NSString *)kUTTypeFileURL]];
 }
 
-- (void)drawRect:(NSRect)dirtyRect
-{
-	[self drawViewOnlyIfDirty:NO];
-}
-
-- (void)drawViewOnlyIfDirty:(BOOL)onlyIfDirty
-{
-	[self performWithGLContext:^{
-		[self.delegate openGLView:self drawViewOnlyIfDirty:onlyIfDirty];
-		CGLFlushDrawable([[self openGLContext] CGLContextObj]);
-	}];
-}
-
 - (void)performWithGLContext:(dispatch_block_t)action
 {
 	CGLLockContext([[self openGLContext] CGLContextObj]);

OSBindings/SDL/main.cpp

@@ -776,7 +776,8 @@ int main(int argc, char *argv[]) {
 
 		// Display a new frame and wait for vsync.
 		updater.update();
-		scan_target.draw(true, int(window_width), int(window_height));
+		scan_target.update(int(window_width), int(window_height));
+		scan_target.draw(int(window_width), int(window_height));
 		if(activity_observer) activity_observer->draw();
 		SDL_GL_SwapWindow(window);
 	}

Outputs/OpenGL/ScanTarget.cpp

@@ -383,10 +383,10 @@ void ScanTarget::setup_pipeline() {
 	input_shader_->set_uniform("textureName", GLint(SourceDataTextureUnit - GL_TEXTURE0));
 }
 
-void ScanTarget::draw(bool synchronous, int output_width, int output_height) {
+void ScanTarget::update(int output_width, int output_height) {
 	if(fence_ != nullptr) {
 		// if the GPU is still busy, don't wait; we'll catch it next time
-		if(glClientWaitSync(fence_, GL_SYNC_FLUSH_COMMANDS_BIT, synchronous ? GL_TIMEOUT_IGNORED : 0) == GL_TIMEOUT_EXPIRED) {
+		if(glClientWaitSync(fence_, GL_SYNC_FLUSH_COMMANDS_BIT, 0) == GL_TIMEOUT_EXPIRED) {
 			return;
 		}
 		fence_ = nullptr;
@@ -666,15 +666,6 @@ void ScanTarget::draw(bool synchronous, int output_width, int output_height) {
 		test_gl(glDisable, GL_BLEND);
 	}
 
-	// Copy the accumulatiion texture to the target.
-	test_gl(glBindFramebuffer, GL_FRAMEBUFFER, target_framebuffer_);
-	test_gl(glViewport, 0, 0, (GLsizei)output_width, (GLsizei)output_height);
-
-	test_gl(glClearColor, 0.0f, 0.0f, 0.0f, 0.0f);
-	test_gl(glClear, GL_COLOR_BUFFER_BIT);
-	accumulation_texture_->bind_texture();
-	accumulation_texture_->draw(float(output_width) / float(output_height), 4.0f / 255.0f);
-
 	// All data now having been spooled to the GPU, update the read pointers to
 	// the submit pointer location.
 	read_pointers_.store(submit_pointers);
@@ -684,3 +675,18 @@ void ScanTarget::draw(bool synchronous, int output_width, int output_height) {
 	fence_ = glFenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 0);
 	is_drawing_.clear();
 }
+
+void ScanTarget::draw(int output_width, int output_height) {
+	while(is_drawing_.test_and_set());
+
+	// Copy the accumulation texture to the target.
+	test_gl(glBindFramebuffer, GL_FRAMEBUFFER, target_framebuffer_);
+	test_gl(glViewport, 0, 0, (GLsizei)output_width, (GLsizei)output_height);
+
+	test_gl(glClearColor, 0.0f, 0.0f, 0.0f, 0.0f);
+	test_gl(glClear, GL_COLOR_BUFFER_BIT);
+	accumulation_texture_->bind_texture();
+	accumulation_texture_->draw(float(output_width) / float(output_height), 4.0f / 255.0f);
+
+	is_drawing_.clear();
+}

Outputs/OpenGL/ScanTarget.hpp

@@ -42,7 +42,10 @@ class ScanTarget: public Outputs::Display::ScanTarget {
 
 		void set_target_framebuffer(GLuint);
 
-		void draw(bool synchronous, int output_width, int output_height);
+		/*! Pushes the current state of output to the target framebuffer. */
+		void draw(int output_width, int output_height);
+		/*! Processes all the latest input, at a resolution suitable for later output to a framebuffer of the specified size. */
+		void update(int output_width, int output_height);
 
 	private:
 #ifndef NDEBUG