From 447adfbe385249ba721dec2ad9d2e4aaa9f0376a Mon Sep 17 00:00:00 2001
From: PatTheMav <PatTheMav@users.noreply.github.com>
Date: Wed, 11 Jan 2023 18:50:04 +0100
Subject: [PATCH] mac-virtualcam: Fix memory access issues for shared
 IOSurfaces

The DAL plugin-based virtualcamera shares data between OBS and the
plugin using an IOSurface. IOSurface locks are necessary to ensure
race conditions between data generation (OBS side) and consumption
(virtual camera side) and also that an IOSurface is not offloaded to
GPU memory when it is indeed needed in CPU memory.

Also moves the invalidation of the NSMachPort for the frames to after
the IOSurface data has been converted into a pixelbuffer and added to
the frame queue of the virtual camera, as an early invalidation will
cut off access to the pixel data shared with the DAL plugin.
---
 .../src/dal-plugin/OBSDALMachClient.mm        | 93 ++++++++++---------
 .../src/obs-plugin/OBSDALMachServer.mm        |  7 ++
 2 files changed, 56 insertions(+), 44 deletions(-)

diff --git a/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm b/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm
index 9514eee9f..303a3a4dc 100644
--- a/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm
+++ b/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm
@@ -93,6 +93,8 @@
 
 - (void)handlePortMessage:(NSPortMessage *)message
 {
+	__strong id<MachClientDelegate> strongDelegate = self.delegate;
+
 	VLogFunc(@"");
 	NSArray *components = message.components;
 	switch (message.msgid) {
@@ -113,57 +115,60 @@
 
 			IOSurfaceRef surface = IOSurfaceLookupFromMachPort(
 				[framePort machPort]);
+
+			if (surface) {
+				/*
+                 * IOSurfaceLocks are only necessary on non Apple Silicon devices, as those have
+                 * unified memory. On Intel machines, the lock ensures that the IOSurface is copied back
+                 * from GPU memory to CPU memory so we can process the pixel buffer.
+                 */
+#ifndef __aarch64__
+				IOSurfaceLock(surface, kIOSurfaceLockReadOnly,
+					      NULL);
+#endif
+				CVPixelBufferRef frame;
+				CVPixelBufferCreateWithIOSurface(
+					kCFAllocatorDefault, surface, NULL,
+					&frame);
+#ifndef __aarch64__
+				IOSurfaceUnlock(surface, kIOSurfaceLockReadOnly,
+						NULL);
+#endif
+				CFRelease(surface);
+
+				uint64_t timestamp;
+				[components[1] getBytes:&timestamp
+						 length:sizeof(timestamp)];
+
+				VLog(@"Received frame data: %zux%zu (%llu)",
+				     CVPixelBufferGetWidth(frame),
+				     CVPixelBufferGetHeight(frame), timestamp);
+
+				uint32_t fpsNumerator;
+				[components[2] getBytes:&fpsNumerator
+						 length:sizeof(fpsNumerator)];
+				uint32_t fpsDenominator;
+				[components[3] getBytes:&fpsDenominator
+						 length:sizeof(fpsDenominator)];
+
+				[strongDelegate
+					receivedPixelBuffer:frame
+						  timestamp:timestamp
+					       fpsNumerator:fpsNumerator
+					     fpsDenominator:fpsDenominator];
+
+				CVPixelBufferRelease(frame);
+			} else {
+				ELog(@"Failed to obtain IOSurface from Mach port");
+			}
 			[framePort invalidate];
 			mach_port_deallocate(mach_task_self(),
 					     [framePort machPort]);
-
-			if (!surface) {
-				ELog(@"Failed to obtain IOSurface from Mach port");
-				return;
-			}
-
-			/*
-			 * IOSurfaceLocks are only necessary on non Apple Silicon devices, as those have
-			 * unified memory. On Intel machines, the lock ensures that the IOSurface is copied back
-			 * from GPU memory to CPU memory so we can process the pixel buffer.
-			 */
-#ifndef __aarch64__
-			IOSurfaceLock(surface, kIOSurfaceLockReadOnly, NULL);
-#endif
-			CVPixelBufferRef frame;
-			CVPixelBufferCreateWithIOSurface(kCFAllocatorDefault,
-							 surface, NULL, &frame);
-#ifndef __aarch64__
-			IOSurfaceUnlock(surface, kIOSurfaceLockReadOnly, NULL);
-#endif
-			CFRelease(surface);
-
-			uint64_t timestamp;
-			[components[1] getBytes:&timestamp
-					 length:sizeof(timestamp)];
-
-			VLog(@"Received frame data: %zux%zu (%llu)",
-			     CVPixelBufferGetWidth(frame),
-			     CVPixelBufferGetHeight(frame), timestamp);
-
-			uint32_t fpsNumerator;
-			[components[2] getBytes:&fpsNumerator
-					 length:sizeof(fpsNumerator)];
-			uint32_t fpsDenominator;
-			[components[3] getBytes:&fpsDenominator
-					 length:sizeof(fpsDenominator)];
-
-			[self.delegate receivedPixelBuffer:frame
-						 timestamp:timestamp
-					      fpsNumerator:fpsNumerator
-					    fpsDenominator:fpsDenominator];
-
-			CVPixelBufferRelease(frame);
 		}
 		break;
 	case MachMsgIdStop:
 		DLog(@"Received stop message");
-		[self.delegate receivedStop];
+		[strongDelegate receivedStop];
 		break;
 	default:
 		ELog(@"Received unexpected response msgid %u",
diff --git a/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm b/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm
index efd33c1b4..450e99466 100644
--- a/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm
+++ b/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm
@@ -148,6 +148,9 @@
 			       length:sizeof(fpsDenominator)];
 
 		IOSurfaceRef surface = CVPixelBufferGetIOSurface(frame);
+#ifndef __aarch64__
+		IOSurfaceLock(surface, 0, NULL);
+#endif
 
 		if (!surface) {
 			blog(LOG_ERROR,
@@ -174,6 +177,10 @@
 					 ]];
 
 		mach_port_deallocate(mach_task_self(), framePort);
+
+#ifndef __aarch64__
+		IOSurfaceUnlock(surface, 0, NULL);
+#endif
 	}
 }