Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mac-virtualcam: Avoid transcoding where possible #6573

Merged
merged 9 commits into from
Jun 12, 2022

Conversation

fabianishere
Copy link
Contributor

This pull request updates the mac-virtualcam plugin to avoid transcoding the video feed if possible.

Additionally, I have updated the DAL plugin to build for ARM64e as well (trying to fix #6285)

Motivation and Context

Currently, the mac-virtualcam plugin transcodes the video feed into UYVY, since the DAL plugin exposes that format to the consuming application. However, having to do this transformation incurs higher CPU usage (see johnboiles/obs-mac-virtualcam#102 for more information).

This pull request updates the plugin to support transferring the raw video feed to the DAL plugin without transformation for the pixel formats most commonly used in OBS (NV12 and I444, I believe). We achieve this by communicating with CVPixelBuffers between the OBS plugin and the DAL plugin, as opposed to raw data buffers. CVPixelBuffer objects carry additional information (such as pixel format and video dimensions) and can be converted quite easily to CMSampleBuffer objects that are submitted to the consuming application.

How Has This Been Tested?

These changes have been tested on a Macbook Pro (M1) running macOS Monterey and have been verified to work with Google Chrome and Zoom.

Types of changes

  • Performance enhancement: transcoding the video feed is avoided if possible. Pixel buffers are properly shared across processes (zero-copy, via IOSurface).
  • Code cleanup: removal of unused functions.
  • (Potentially) breaking change: communication protocol between OBS plugin and DAL plugin has changed. If user is still using the old DAL plugin, the virtual cam will not work.

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@WizardCM WizardCM added Enhancement Improvement to existing functionality Code Cleanup Non-breaking change which makes code smaller or more readable labels Jun 1, 2022
@derrod derrod requested review from PatTheMav and gxalpha June 1, 2022 10:28
@WizardCM WizardCM added the macOS Affects macOS label Jun 1, 2022
Copy link
Member

@gxalpha gxalpha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial impressions, didn’t look too deeply into the code or build or test yet.

Regarding your comments about this being breaking: Changing the communication protocol is completely fine, OBS will automatically update the DAL plugin when a new version comes out, running the old one isn’t a supported use case.

plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.h Outdated Show resolved Hide resolved
plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm Outdated Show resolved Hide resolved
plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm Outdated Show resolved Hide resolved
plugins/mac-virtualcam/src/dal-plugin/OBSDALPlugIn.mm Outdated Show resolved Hide resolved
plugins/mac-virtualcam/src/obs-plugin/plugin-main.mm Outdated Show resolved Hide resolved
@fabianishere
Copy link
Contributor Author

@gxalpha Thanks for the comments. I have updated the pull request according to your suggestions.

@PatTheMav
Copy link
Member

Looks fine to me overall - using a CVPixelBuffer brings it closer to how Apple's own virtual camera extensions work, so seems like the more "correct" way to achieve this.

@PatTheMav
Copy link
Member

I did some tests on my local machines:

  • On my M1-based Mac mini this worked without issue
  • On my x86_64-based iMac the virtual camera output was either stuck on a single-coloured frame or wasn't updated at all.

Both Macs run macOS 12.4.

@PatTheMav
Copy link
Member

Seems I spoke to soon, it seized to work on my M1 Mac as well - will have to investigate..

@fabianishere
Copy link
Contributor Author

@PatTheMav How did you test the virtual cam? Hopefully I can reproduce your issue as well.

@PatTheMav
Copy link
Member

@PatTheMav How did you test the virtual cam? Hopefully I can reproduce your issue as well.

Nothing special, pulled the code, built OBS, activated the cam, had it overwrite the preexisting one. Tested with https://github.com/lvsti/Cameo.

I got crashes in Cameo as well as OBS now, both of which I have to further investigate right now.

@fabianishere
Copy link
Contributor Author

@PatTheMav Looks like I am able to reproduce the issue with Cameo. Will investigate.

This change fixes an issue where the DAL plugin would not load due to
not supporting the architecture arm64e. We update the build
configuration to build a universal binary that includes arm64e as well.

See obsproject#6285 for more
information regarding this issue.
This change updates the virtual camera implementation on macOS to
utilize IOSurface to share the output feed with the virtual cameras.

By using IOSurface, we remove the need for copying the frames across
multiple buffers, since they can be shared across Mach connections using
zero-copy.
This change updates the mac-virtualcam implementation to pool the
CVPixelBuffers used to share the output frames. This allows the plugin
to recycle the pixel buffers used by the plugin.
This change updates the plugin to support video formats that contain
multiple planes (such as NV12). Such functionality is necessary to
prevent transcoding the raw video data, which is often delivered in a
planar format.
This change updates the mac-virtualcam implementation to conditionally
enable conversion of the output video format. Previously, the output
video was always converted into UYVY. However, this conversion exhibits
high CPU usage, as reported in:
johnboiles/obs-mac-virtualcam#102

Therefore, we disable conversion when the selected output format (e.g.,
NV12) is natively supported by CoreVideo's pixel buffers.
This change removes the unused CMSampleBuffer utility functions that
were still left from the previous implementation. Since we construct the
CMSampleBuffer directly from an IOSurface, we do not need any custom
construction logic anymore, since that is now performed by the OBS
plugin.
This change updates the implementation of the mac-virtualcam plugin to
not use any global state and instead rely on the state object that is
passed by the OBS module system.

This approach is similar to the virtual camera implementations for Linux
and Windows.
This change fixes an issue in the Mach server exposed by the macOS
virtual camera for OBS, where it would not invalidate ports that were
disconnected by the remote application, causing sporadic crashes.

These crashes can be reproduced in the previous builds by opening the
virtual camera in a remote application and closing the application
(without stopping the virtual camera).
This change fixes an issue with the CMIO DAL plugin where the CMIO
subsystem would log multiple errors when starting the virtual camera,
due to certain properties that could not be set (frame rate and format).

For now, we just ignore the assignment, but mark the property as
settable to suppress the error messages that are reported by the CMIO
subsystem.
@fabianishere
Copy link
Contributor Author

fabianishere commented Jun 10, 2022

@PatTheMav I think I found the cause of the issues (which the updated pull request should fix):

  1. Virtual camera output stuck on a single-coloured frame or not updated at all
    I originally did not use CMIOSampleBufferCreateForImageBuffer to create the CMSampleBuffer. Apparently, this is fine for some applications (e.g., Zoom and Chrome), but not for others (e.g., Cameo).
  2. OBS or Application crashing
    It seems the original code tried to double-free the Mach port that was used to exchange the IOSurfaces.

Cameo now works on my device as well (where it did not before).

@PatTheMav
Copy link
Member

@PatTheMav I think I found the cause of the issues (which the updated pull request should fix):

  1. Virtual camera output stuck on a single-coloured frame or not updated at all
    I originally did not use CMIOSampleBufferCreateForImageBuffer to create the CMSampleBuffer. Apparently, this is fine for some applications (e.g., Zoom and Chrome), but not for others (e.g., Cameo).
  2. OBS or Application crashing
    It seems the original code tried to double-free the Mach port that was used to exchange the IOSurfaces.

Cameo now works on my device as well (where it did not before).

Great, seems to work in my tests on both architectures now as well.

@PatTheMav
Copy link
Member

@gxalpha did you test this already?

Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through the code again, looks fine to me, changes are straight-forward enough.

At some point we should move to a HelperTool + XPCService setup, probably around the time we add a modern VirtualCamera output, but should be fine for now.

@fabianishere
Copy link
Contributor Author

fabianishere commented Jun 12, 2022

@PatTheMav I have already worked a bit on a prototype using XPC as well as one using the new CMIO System Extension framework (in Swift). I could create a (draft) pull request once those changes are ready for integration.

Using the new CMIO framework vastly simplifies the implementation of the virtual camera, but the development experience is quite painful, since System Extensions require special entitlements and other strict requirements (e.g., application must be located in /Applications). This means you cannot install your own extensions unless you have an Apple Developer account and have been granted the System Extension entitlement.

@PatTheMav
Copy link
Member

@fabianishere that'd be amazing, @gxalpha and I have been looking at that precisely and we'd be very happy to check out what you did so far. 👍🏻

@PatTheMav PatTheMav merged commit 1f72dad into obsproject:master Jun 12, 2022
@RytoEX RytoEX added this to PRs Pending Review in OBS Studio 28.0 via automation Jun 12, 2022
@RytoEX RytoEX added this to the OBS Studio 28.0 milestone Jun 12, 2022
@RytoEX RytoEX moved this from PRs Pending Review to Completed in OBS Studio 28.0 Jun 12, 2022
@SciTechNick
Copy link

Hi all, after running the build with this PR merged, I get a massive memory leak on my M1 Ultra Mac Studio when using the virtual camera. The virtual camera must be accessed by an application (in my case, latest version of Zoom - 5.11.0). The second I start my mac virtualcam with this build and enable the camera within zoom, OBS memory usage will gradually climb to enormous amounts until the video hangs in zoom and OBS crashes. Stopping and restarting the video capture in zoom does not resolve the problem. Closing/exiting zoom completely BEFORE OBS crashes will release the memory, however. The second I start zoom back up and use virtual cam output, the memory starts climbing again until crash. Have not tested other apps that can leverage the virtual cam yet, only zoom so far.

DAL plug-in completely removed via rm -rf /Library/CoreMediaIO/Plug-Ins/DAL/obs-mac-virtualcam.plugin before each build is installed/ran.

Decklink Quad HDMI, capturing from a Panasonic Lumix GH5M2: 3840x2160, 59.94 FPS, Input format: UYVY - 422YpCbCr8, Color space Auto (autoselect: Rec. 709), video range: Full, Buffering On or Off

Rolling back to a previous build without this PR merged resolves the issue.

Screen Shot 2022-06-21 at 11 21 36 AM

@gxalpha
Copy link
Member

gxalpha commented Jun 21, 2022

Thanks for reporting, we'll investigate.
I can confirm the behavior you describe.

@fabianishere
Copy link
Contributor Author

fabianishere commented Jun 21, 2022

I can only reproduce this with Zoom, not with Chrome or Cameo. It looks like Zoom is somehow not releasing the IOSurfaces to be re-used by OBS Studio, causing it to allocate more and more memory.

EDIT: I updated the plugin to explicitly invalidate the Mach port, but it does not look like it resolves the issue.

diff --git a/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm b/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm
index 452ee6fbc..f13e4dd51 100644
--- a/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm
+++ b/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm
@@ -132,6 +132,8 @@
 
 			CVPixelBufferRelease(frame);
 			CFRelease(surface);
+
+			[framePort invalidate];
 		}
 		break;
 	case MachMsgIdStop:
diff --git a/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm b/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm
index d2857766e..8df468598 100644
--- a/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm
+++ b/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm
@@ -164,6 +164,8 @@
 						 fpsNumeratorData,
 						 fpsDenominatorData
 					 ]];
+
+		[framePort invalidate];
 	}
 }

@SciTechNick
Copy link

FYI: I've just confirmed the same issue in Microsoft Edge + Teams Progressive Web App in a video call.

@fabianishere
Copy link
Contributor Author

Looks like OBS is leaking Mach ports and we need to manually deallocate the ports using mach_port_deallocate. The following patch fixes the memory leak for me:

diff --git a/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm b/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm
index 452ee6fbc..f781b29f5 100644
--- a/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm
+++ b/plugins/mac-virtualcam/src/dal-plugin/OBSDALMachClient.mm
@@ -101,14 +101,32 @@
 		break;
 	case MachMsgIdFrame:
 		VLog(@"Received frame message");
-		if (components.count >= 4) {
+
+		if (components.count < 4) {
+			break;
+		}
+
+		@autoreleasepool {
 			NSMachPort *framePort = (NSMachPort *)components[0];
+
+			if (!framePort) {
+				return;
+			}
+
 			IOSurfaceRef surface = IOSurfaceLookupFromMachPort(
 				[framePort machPort]);
+			mach_port_deallocate(mach_task_self(),
+					     [framePort machPort]);
+
+			if (!surface) {
+				ELog(@"Failed to obtain IOSurface from Mach port");
+				return;
+			}
 
 			CVPixelBufferRef frame;
 			CVPixelBufferCreateWithIOSurface(kCFAllocatorDefault,
 							 surface, NULL, &frame);
+			CFRelease(surface);
 
 			uint64_t timestamp;
 			[components[1] getBytes:&timestamp
@@ -131,7 +149,6 @@
 					    fpsDenominator:fpsDenominator];
 
 			CVPixelBufferRelease(frame);
-			CFRelease(surface);
 		}
 		break;
 	case MachMsgIdStop:
diff --git a/plugins/mac-virtualcam/src/dal-plugin/OBSDALStream.mm b/plugins/mac-virtualcam/src/dal-plugin/OBSDALStream.mm
index b318f6fb7..571eefd70 100644
--- a/plugins/mac-virtualcam/src/dal-plugin/OBSDALStream.mm
+++ b/plugins/mac-virtualcam/src/dal-plugin/OBSDALStream.mm
@@ -397,7 +397,10 @@
 	}
 
 	err = CMSimpleQueueEnqueue(self.queue, sampleBuffer);
+
 	if (err != noErr) {
+		CFRelease(sampleBuffer);
+
 		DLog(@"CMSimpleQueueEnqueue err %d", err);
 		return;
 	}
diff --git a/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm b/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm
index d2857766e..efd33c1b4 100644
--- a/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm
+++ b/plugins/mac-virtualcam/src/obs-plugin/OBSDALMachServer.mm
@@ -147,23 +147,33 @@
 			dataWithBytes:&fpsDenominator
 			       length:sizeof(fpsDenominator)];
 
-		NSPort *framePort = [NSMachPort
-			portWithMachPort:IOSurfaceCreateMachPort(
-						 CVPixelBufferGetIOSurface(
-							 frame))];
+		IOSurfaceRef surface = CVPixelBufferGetIOSurface(frame);
+
+		if (!surface) {
+			blog(LOG_ERROR,
+			     "unable to access IOSurface associated with CVPixelBuffer");
+			return;
+		}
+
+		mach_port_t framePort = IOSurfaceCreateMachPort(surface);
 
 		if (!framePort) {
 			blog(LOG_ERROR,
-			     "unable to allocate mach port for pixel buffer");
+			     "unable to allocate mach port for IOSurface");
 			return;
 		}
 
 		[self sendMessageToClientsWithMsgId:MachMsgIdFrame
 					 components:@[
-						 framePort, timestampData,
+						 [NSMachPort
+							 portWithMachPort:framePort
+								  options:NSMachPortDeallocateNone],
+						 timestampData,
 						 fpsNumeratorData,
 						 fpsDenominatorData
 					 ]];
+
+		mach_port_deallocate(mach_task_self(), framePort);
 	}
 }

@gxalpha
Copy link
Member

gxalpha commented Jun 22, 2022

@fabianishere Thanks for the quick fix, would you be interested in submitting another PR? Otherwise we could test and submit it as well.

fabianishere added a commit to fabianishere/obs-studio that referenced this pull request Jun 22, 2022
This change fixes a memory leak in the mac-virtualcam plugin that causes
OBS to not release the CVPixelBuffers (and underlying IOSurfaces)
it emits to the virtual camera consumers.

Pull request obsproject#6573 (Avoid
transcoding where possible) updated the mac-virtualcam to share the
virtual camera feed with other processes via IOSurfaces.

Although the changes work correctly, users have observed that OBS memory
usage keeps increasing when the virtual camera is active until OBS runs
out of memory or the consuming application is closed.
See the report by @SciTechNick for more information:
obsproject#6573 (comment)

After some debugging, I have found that the plugin is leaking Mach ports
associated with IOSurfaces, preventing them from being re-used. The
previous approach using `NSMachPort` does not seem to properly release
the Mach port allocated via `CVPixelBufferGetIOSurface` and
`IOSurfaceLookupFromMachPort`. Instead, we must explicitly deallocate
the port using `mach_port_deallocate`.

I have tested the changes on a Macbook Pro (M1) running macOS Monterey with
Google Chrome, Zoom, and Cameo. OBS shows no signs of memory leakage
after multiple minutes.
@fabianishere
Copy link
Contributor Author

@gxalpha I have opened a pull request with the fix: #6639

fabianishere added a commit to fabianishere/obs-studio that referenced this pull request Jun 23, 2022
This change fixes a memory leak in the mac-virtualcam plugin that causes
OBS to not release the CVPixelBuffers (and underlying IOSurfaces)
it emits to the virtual camera consumers.

Pull request obsproject#6573 (Avoid
transcoding where possible) updated the mac-virtualcam to share the
virtual camera feed with other processes via IOSurfaces.

Although the changes work correctly, users have observed that OBS memory
usage keeps increasing when the virtual camera is active until OBS runs
out of memory or the consuming application is closed.
See the report by @SciTechNick for more information:
obsproject#6573 (comment)

After some debugging, I have found that the plugin is leaking Mach ports
associated with IOSurfaces, preventing them from being re-used. The
previous approach using `NSMachPort` does not seem to properly release
the Mach port allocated via `CVPixelBufferGetIOSurface` and
`IOSurfaceLookupFromMachPort`. Instead, we must explicitly deallocate
the port using `mach_port_deallocate`.

I have tested the changes on a Macbook Pro (M1) running macOS Monterey with
Google Chrome, Zoom, and Cameo. OBS shows no signs of memory leakage
after multiple minutes.
jp9000 pushed a commit that referenced this pull request Jun 25, 2022
This change fixes a memory leak in the mac-virtualcam plugin that causes
OBS to not release the CVPixelBuffers (and underlying IOSurfaces)
it emits to the virtual camera consumers.

Pull request #6573 (Avoid
transcoding where possible) updated the mac-virtualcam to share the
virtual camera feed with other processes via IOSurfaces.

Although the changes work correctly, users have observed that OBS memory
usage keeps increasing when the virtual camera is active until OBS runs
out of memory or the consuming application is closed.
See the report by @SciTechNick for more information:
#6573 (comment)

After some debugging, I have found that the plugin is leaking Mach ports
associated with IOSurfaces, preventing them from being re-used. The
previous approach using `NSMachPort` does not seem to properly release
the Mach port allocated via `CVPixelBufferGetIOSurface` and
`IOSurfaceLookupFromMachPort`. Instead, we must explicitly deallocate
the port using `mach_port_deallocate`.

I have tested the changes on a Macbook Pro (M1) running macOS Monterey with
Google Chrome, Zoom, and Cameo. OBS shows no signs of memory leakage
after multiple minutes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Cleanup Non-breaking change which makes code smaller or more readable Enhancement Improvement to existing functionality macOS Affects macOS
Projects
OBS Studio 28.0
  
Completed
Development

Successfully merging this pull request may close these issues.

mac-virtualcam DAL plugin has incompatible architectures
6 participants