MacOS getDevice() optimization, set CI env var, & add getVideoDevices() test#1691
Open
sandboxcoder wants to merge 7 commits intostagingfrom
Open
MacOS getDevice() optimization, set CI env var, & add getVideoDevices() test#1691sandboxcoder wants to merge 7 commits intostagingfrom
sandboxcoder wants to merge 7 commits intostagingfrom
Conversation
3a0c9ba to
a935b64
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reintroduces and refines a getDevices optimization to avoid creating dummy sources (notably preventing CoreAudio from spinning up unnecessary reconnect threads), adds a macOS-specific video device enumeration path to avoid plugin initialization requirements, and updates CI/test utilities to better handle Darwin CI constraints.
Changes:
- Replace dummy-source-based property enumeration with
obs_get_source_properties()for device listing, and add warning logs on property lookup failures. - Implement manual macOS video device enumeration via AVFoundation and route
OBS_settings_getVideoDevices(macOS) through it. - Add/adjust OSN tests and CI flags to better detect Darwin CI and validate video device enumeration output shape.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/osn-tests/util/obs_handler.ts | Adds isOnDarwinCI() helper for test gating on macOS CI. |
| tests/osn-tests/util/error_messages.ts | Adds new error messages for video device enumeration tests. |
| tests/osn-tests/src/test_osn_video.ts | Adds a test for OBS_settings_getVideoDevices() return shape and (non-CI) non-empty expectation. |
| tests/osn-tests/src/test_osn_audio.ts | Gates default output device expectation behind isOnDarwinCI(). |
| obs-studio-server/source/nodeobs_settings.cpp | Updates getDevices() to avoid dummy source creation; macOS video devices now enumerated via new macOS helper. |
| obs-studio-server/source/nodeobs_settings-osx.mm | New AVFoundation-based macOS video device enumeration implementation. |
| obs-studio-server/source/nodeobs_settings-osx.h | Declares macOS video device enumeration helper. |
| obs-studio-server/CMakeLists.txt | Adds new macOS settings enumeration sources to the build. |
| .github/workflows/main.yml | Explicitly sets CI=true for the macOS test job environment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5ce44ad to
e05ad58
Compare
e05ad58 to
7d202e7
Compare
* isDarwin() now checks CI env var. This way all tests can be run locally * test_osn_audio: do not check for default audio device on the test runner * test_osn_advanced_replayBuffer.ts: still flaky on macOS test runner so disable on those * main.yml: add CI environment variable
32817d2 to
8b48df9
Compare
Comment on lines
+3870
to
3879
| auto props = obs_get_source_properties(source_id); | ||
| if (!props) { | ||
| obs_source_release(dummy_source); | ||
| obs_data_release(settings); | ||
| blog(LOG_WARNING, "Could not get source properties for source id: %s", source_id); | ||
| return; | ||
| } | ||
|
|
||
| auto prop = obs_properties_get(props, property_name); | ||
| if (!prop) { | ||
| blog(LOG_WARNING, "Could not get the property [%s] for source id: %s", property_name, source_id); | ||
| obs_properties_destroy(props); |
Comment on lines
133
to
+136
| it('Start advanced replay buffer - Use Recording', async function() { | ||
| if (obs.isDarwin()) { | ||
| this.skip(); | ||
| } |
Comment on lines
263
to
+266
| it('Start advanced replay buffer - Use Stream through Recording', async function() { | ||
| if (obs.isDarwin()) { | ||
| this.skip(); | ||
| } |
| } | ||
| } | ||
| if (!obs.isDarwin()) { // On virtual mac the default output device is not included in the list of output devices | ||
| // On the CI, mac-coreaudio will not return a default device unless there is another output audio device. |
Comment on lines
+195
to
+202
| it('Get output video devices', function() { | ||
| const devices = osn.NodeObs.OBS_settings_getVideoDevices(); | ||
| expect(devices).to.not.equal(undefined, GetErrorMessage(ETestErrorMsg.VideoDevices)); | ||
| expect(Array.isArray(devices)).to.equal(true, GetErrorMessage(ETestErrorMsg.VideoDevicesIsArray)); | ||
| for (const device of devices) { | ||
| expect(device).to.have.property('id'); | ||
| expect(device).to.have.property('description'); | ||
| logInfo(testName, `Output Video Device Found: ${device.description} with id: ${device.id}`); |
65e6812 to
9b7e04f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
reconnect_threadwhenever Desktop queries for audio devices. This was because the previous implementation passed in a dummy device which causes coreaudio to create a thread to poll if this dummy device is later initialized. This will cleanup the logs eliminating this warning (32 occurrences in a log I checked from Desktop):[Warning] [coreaudio_get_device_name] failed to get name: 560947818obs.isCI()function to determine if we're running in a CI env (no GPU) very quickly from the tests.getDeviceswon't work for video plugins that requires to be initialized before returning properties thus requiring a workaround now. However, the bonus is now we do not have to worry about video plugins (like mac-avcapture-fast) breaking that requires the graphics pipeline to be init before calling it (so now mac-avcapture-fast plugin can be utilized on frontend if desired).OBS_settings_getVideoDevices.disable on CI runner
Performance:
New implementation (nanoseconds)
Old (nanoseconds)
The new
getDevicesimplementation is more efficient since it does not create adummy_source. Also, it avoids triggering coreaudio to create a reconnect_thread for a dummy device (which saves us another 2ms or so in an async thread because mac-coreaudio polls within the reconnect_thread). However, the new implementation to collect video devices is very similar performance wise (about 1ms difference) but that is only invoked once at startup whereas audio devices are queried much more frequently.Motivation and Context
When I was looking into profiling missing source devices it reminded me that getDevices() triggers coreaudio to create a reconnect_thread which does later get reaped however it did not seem ideal. But it would be nice to avoid this. Currently, desktop queries getAudioDevices() at least around 14 times so it is nice to optimize this to get the app to load up faster and spin up less threads. Also, it is good to avoid spinning up a
dummy_sourcefor mac-capture video plugins specifically- since the newer ones (like mac-avcapture-fast) require graphics to be init first. So were unable to use that plugin because Desktop queries video devices before initializing graphics. But now that we manually enumerate video devices we should be to use that plugin now.How Has This Been Tested?
Locally, I verified the same audio/video devices are output. Originally, commit 9279b8a was reverted due to breaking the old onboarding flow (no webcam detected) but that is addressed in this PR since we will enumerate video devices a different way to avoid that.
Types of changes
Checklist: