-
Notifications
You must be signed in to change notification settings - Fork 53
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
add macOS support #16
Conversation
// obs-mac-virtualcam is distributed in the hope that it will be useful, | ||
// but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
// GNU General Public License for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License should be mentioned somewhere more prominently
// along with obs-mac-virtualcam. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
#define PLUGIN_NAME "mac-virtualcam" | ||
#define PLUGIN_VERSION "1.3.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if these fields are still relevant
} | ||
static void UVfromRGB(uint8_t* u, uint8_t* v, uint8_t r, uint8_t g, uint8_t b) { | ||
*u = (uint8_t)(-0.148 * r - 0.291 * g + 0.439 * b + 128); | ||
*v = (uint8_t)( 0.439 * r - 0.368 * g - 0.071 * b + 128); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Colorspace for these might be bad. Also horrible performance.
Should be more optimized but I didn't care enough.
cam_output_width = width; | ||
cam_output_height = height; | ||
cam_fps_num = fps * 1000; | ||
cam_fps_den = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calculation of FPS values is pretty bad. I'm not even sure if I tested this version of the code. Originally I had fps:1
|
||
uint8_t* data = (uint8_t*) malloc(buf.shape[1] * buf.shape[0] * 2); | ||
|
||
// Convert RGBA to UYVY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be optimized
"-framework", "Cocoa", | ||
"-framework", "CoreMediaIO", | ||
"-framework", "IOSurface", | ||
"-framework", "IOKit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These come from the macOS obs plugin CMakeList - most of them shouldn't be required.
I was simply too lazy to filter this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #22.
if sys.platform == 'darwin': | ||
darwin_opts = ['-stdlib=libc++', '-mmacosx-version-min=10.7'] | ||
darwin_opts = ['-stdlib=libc++', '-std=gnu++14'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this comes from the macOS obs plugin CMake file; probably not necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #22.
if sys.platform != 'darwin': | ||
opts.append(cpp_flag(self.compiler)) | ||
#if has_flag(self.compiler, '-fvisibility=hidden'): | ||
# opts.append('-fvisibility=hidden') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out during debugging when I had missing-symbol errors, can probably be re-enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even with this commented out, trying to use this package now again yields this:
$ python3 ./samples/simple.py
Traceback (most recent call last):
File "pyvirtualcam/./samples/simple.py", line 4, in <module>
import pyvirtualcam
File "pyvirtualcam/pyvirtualcam/__init__.py", line 3, in <module>
from .camera import Camera
File "pyvirtualcam/pyvirtualcam/camera.py", line 13, in <module>
from pyvirtualcam import _native_macos as _native
ImportError: dlopen(pyvirtualcam/pyvirtualcam/_native_macos.cpython-39-darwin.so, 2): Symbol not found: _OBJC_CLASS_$_NSMachBootstrapServer
Referenced from: pyvirtualcam/pyvirtualcam/_native_macos.cpython-39-darwin.so
Expected in: flat namespace
in pyvirtualcam/pyvirtualcam/_native_macos.cpython-39-darwin.so
Not sure how I fixed it earlier.
(Edit: Looks like setuptools didn't bother recompiling, because I still had a build/-folder and .so-files, so it used old flags - compilation and using worked fine after removing those)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #22.
if has_flag(self.compiler, '-fvisibility=hidden'): | ||
opts.append('-fvisibility=hidden') | ||
if sys.platform != 'darwin': | ||
opts.append(cpp_flag(self.compiler)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition was added when I used some .c
file. I have removed that file by now. So this change can probably be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #22.
ext.extra_compile_args = opts | ||
ext.extra_link_args = link_opts | ||
ext.extra_compile_args += opts | ||
ext.extra_link_args += link_opts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had issues with "-framework" lines not reaching the linker. I' believe this was related to that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that seems right. I copied this as-is from the pybind11 docs, probably worth reporting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if you could report it; I never worked with pybind11 (except for the hacks in this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like they factored that into a component now https://github.com/pybind/python_example/blob/master/setup.py. At some point I'll probably switch to that.
NSRunLoop *runLoop; | ||
NSDate *timeout = [NSDate dateWithTimeIntervalSinceNow:0.001]; | ||
runLoop = [NSRunLoop currentRunLoop]; | ||
[runLoop runUntilDate:timeout]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasted about 1-2 hours to find this solution.
I noticed that someone else had the exact same problem before (https://stackoverflow.com/questions/62253531/cannot-connect-to-nsmachbootstrapserver-from-plugin); at least I found a solution, but it feels very hacky.
There's probably a cleaner way - but I don't usually code on macOS and never really had to do ObjC before.
The timeout value (0.001) seems very janky, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the timeout is one of the reasons for failing CI performance tests. Not sure about the actual time-slice resolution, but I could imagine that this blocks for more than 1ms.
I did not check if it still works when providing timeout in the past (so it returns immediately after handling messages).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Possibly) fixed in #22.
*v = (uint8_t)( 0.439 * r - 0.368 * g - 0.071 * b + 128); | ||
} | ||
|
||
static bool virtual_output_start(int width, int height, double fps, int delay) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delay
argument is being ignored. What's the intention here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It determines the size of the ring buffer for the Windows variant, roughly small delay = small ring buffer = low latency = potentially more instable. Kind of an advanced option. It's fine to leave it unused here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed that was the case.
If you wanted support for this, you could probably play with the presentation-timestamp on macOS to achieve something similar (pre-buffer multiple frames).
I currently use the mach_absolute_time
function to create a timestamp, but I'm not sure if that has any impact (mach_absolute_time
is something I saw mentioned in the OBS plugin, although I did not verify it's the correct timestamp source; and then the DAL plugin might ignore these timestamps anyway).
m.def("send", &send, R"pbdoc( | ||
Send frame to the virtual cam. | ||
)pbdoc"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
README needs information that macOS is supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #21.
uint8_t mixRGB[3]; | ||
mixRGB[0] = (rgba[0+0] + rgba[4+0]) / 2; | ||
mixRGB[1] = (rgba[0+1] + rgba[4+1]) / 2; | ||
mixRGB[2] = (rgba[0+2] + rgba[4+2]) / 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this in 8-bit will reduce quality.
Also should probably do gamma correction before downsampling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've switched to floating-point in #22.
pyvirtualcam/native_macos/main.mm
Outdated
cam_fps_den = 1000; | ||
|
||
blog(LOG_DEBUG, "output_create"); | ||
sMachServer = [[OBSDALMachServer alloc] init]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the OBS DAL plugin is not installed, then software (Zoom, Quicktime, etc.)won't show a virtual camera and therefore it's unlikely that anything will ever connect to the Mach server.
The OBS plugin would first check if the DAL plugin is installed (and the correct version).
If the DAL plugin is not installed, the OBS plugin would attempt to install it, or report an error.
My code does none of those checks. It will always start the server, regardless if the user has a suitable client.
Ideally we should abort creation and report an error about the missing OBS DAL plugin. - However, I was too lazy to add this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practical terms, does that mean for this to work a user would have to use the OBS plugin at least once so that the DAL plugin is installed? I assume so. I agree that checking for /Library/CoreMediaIO/Plug-Ins/DAL/obs-mac-virtualcam.plugin
and printing a useful error message makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The macOS plugin only became part of OBS Studio very recently. Before that, the plugin probably self-installed the DAL plugin to make it easier for the user.
With inclusion of the plugin in OBS Studio I'd assume that the OBS installer installs it automatically these days.
But I'm not sure either.
Thanks for the contribution! I think it's pretty good, hackyness is on par with the Windows variant :) This package is experimental, so that's not a problem. As long as folks can use it for some funny demos or their research prototypes it's good enough. |
I added macOS CI, seems like there are some compile issues. |
I'd like to see this included in master and am willing to fix the worst bugs I've created. CI sounds like a good idea. Make sure it also catches issues like this: #16 (comment). |
CI now passes except for the test that checks whether the target fps are achieved. It seems that it fluctuates a lot judging from the output of the
Compared to Windows:
Do you have any idea how bad this really was when you tested it? Did it ever cause instability? |
I only tested this for about a minute or two. Tests were done with simple.py and https://github.com/JayFoxRox/Fujifilm-FinePix-Driver on an older MacBook Pro. The FinePix camera image felt slow, but it's a 320x240 camera from 2002 which sends JPEG frame and is polled at 35ms per frame with naive decoding by my driver.
I also had one case where my FinePix camera driver crashed, but I believe it was due to the USB code or a bad USB cable; not pyvirtualcam.
I expect slowness, especially at higher resolutions. Things which should help:
However, I don't want to spend time on any of that at the moment. |
I'm on my MacBook Pro (5-inch, 2017) / 2,8 GHz Quad-Core Intel Core i7 now. With Zoom preferences opened to see the image:
So errors likely stem from CI performance fluctuation. |
Great, I disabled that test in CI for macOS, all green now. |
Closes #2
This is pretty hacky, as I only needed it for a little project. Very little effort and testing went into this.
I'll add some review comments about things which still need work.
A bunch of code was stolen from https://github.com/obsproject/obs-studio/tree/516ed4458dbc74b27b425533789d4e9dd95b923b/plugins/mac-virtualcam
Ideally we'd probably have a custom DAL plugin - see https://github.com/johnboiles/coremediaio-dal-minimal-example
I also took the UYVY code from stackoverflow or something, but adapted it. I did not check which colorspace is used or expected.