feat(macos/capture): ScreenCaptureKit backend with gated EDR (HDR) for 10-bit pixel formats#5190
Open
Nottlespike wants to merge 3 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces a macOS ScreenCaptureKit (SCK) video capture backend and switches the display capture path to select SCK on macOS 12.3+ while retaining the legacy AVCaptureScreenInput fallback for older releases.
Changes:
- Added
SCVideo(ScreenCaptureKit-based) capture implementation and header. - Unified macOS capture backends behind a
SunshineVideoCaptureprotocol and updateddisplay.mmto choose backend at runtime. - Updated macOS CMake to locate/link ScreenCaptureKit and compile
sc_video.mwith ARC.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/macos/sc_video.m | New ScreenCaptureKit capture backend implementation (ARC). |
| src/platform/macos/sc_video.h | New public interface for SCVideo conforming to shared capture protocol. |
| src/platform/macos/display.mm | Switches capture backend to SCVideo on macOS 12.3+ and generalizes types to the new protocol. |
| src/platform/macos/av_video.h | Introduces SunshineVideoCapture protocol shared by AVVideo and SCVideo. |
| cmake/dependencies/macos.cmake | Adds ScreenCaptureKit framework lookup. |
| cmake/compile_definitions/macos.cmake | Links ScreenCaptureKit and adds new source files; compiles sc_video.m with ARC. |
Comments suppressed due to low confidence (1)
cmake/compile_definitions/macos.cmake:1
- Linking ScreenCaptureKit as a normal (strong) dependency will prevent the app from launching on macOS versions where ScreenCaptureKit is absent (pre-12.3), even though
display.mmhas an@available(macOS 12.3, *)runtime fallback. To preserve the intended backward compatibility, link ScreenCaptureKit weakly (e.g., via a weak framework link option) or conditionally add the framework only when targeting 12.3+.
# macos specific compile definitions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+61
to
+81
| [SCShareableContent getShareableContentExcludingDesktopWindows:NO | ||
| onScreenWindowsOnly:NO | ||
| completionHandler:^(SCShareableContent *_Nullable content, NSError *_Nullable error) { | ||
| if (error || !content) { | ||
| enumerationError = error; | ||
| } else { | ||
| for (SCDisplay *d in content.displays) { | ||
| if (d.displayID == displayID) { | ||
| selectedDisplay = d; | ||
| break; | ||
| } | ||
| } | ||
| // If the requested display wasn't found (display reconfigured, | ||
| // unplugged, etc.) fall back to the first display SCK reports. | ||
| if (!selectedDisplay && content.displays.count > 0) { | ||
| selectedDisplay = content.displays.firstObject; | ||
| } | ||
| } | ||
| dispatch_semaphore_signal(ready); | ||
| }]; | ||
| dispatch_semaphore_wait(ready, DISPATCH_TIME_FOREVER); |
Comment on lines
+170
to
+176
| __block NSError *startError = nil; | ||
| dispatch_semaphore_t started = dispatch_semaphore_create(0); | ||
| [self.stream startCaptureWithCompletionHandler:^(NSError *_Nullable error) { | ||
| startError = error; | ||
| dispatch_semaphore_signal(started); | ||
| }]; | ||
| dispatch_semaphore_wait(started, DISPATCH_TIME_FOREVER); |
Comment on lines
+159
to
+168
| if (!self.streamRunning) { | ||
| NSError *outputError = nil; | ||
| if (![self.stream addStreamOutput:self | ||
| type:SCStreamOutputTypeScreen | ||
| sampleHandlerQueue:self.sampleQueue | ||
| error:&outputError]) { | ||
| NSLog(@"SCVideo: addStreamOutput failed: %@", outputError); | ||
| dispatch_semaphore_signal(self.currentSignal); | ||
| return self.currentSignal; | ||
| } |
| auto device = std::make_unique<nv12_zero_device>(); | ||
|
|
||
| device->init(static_cast<void *>(av_capture), pix_fmt, setResolution, setPixelFormat); | ||
| device->init((__bridge void *) av_capture, pix_fmt, setResolution, setPixelFormat); |
| */ | ||
| static void setResolution(void *display, int width, int height) { | ||
| [static_cast<AVVideo *>(display) setFrameWidth:width frameHeight:height]; | ||
| [(__bridge id<SunshineVideoCapture>) display setFrameWidth:width frameHeight:height]; |
|
|
||
| static void setPixelFormat(void *display, OSType pixelFormat) { | ||
| static_cast<AVVideo *>(display).pixelFormat = pixelFormat; | ||
| ((__bridge id<SunshineVideoCapture>) display).pixelFormat = pixelFormat; |
21 tasks
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 395e2a26f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+161
to
+164
| if (![self.stream addStreamOutput:self | ||
| type:SCStreamOutputTypeScreen | ||
| sampleHandlerQueue:self.sampleQueue | ||
| error:&outputError]) { |
There was a problem hiding this comment.
capture: re-adds self as an SCStreamOutput every time streamRunning is false, but this class never calls removeStreamOutput when stopping. SCVideo is reused for multiple capture probes (for example, validate_config repeatedly calls dummy_img via the same display object in src/video.cpp), so the second probe can fail at addStreamOutput, causing immediate semaphore release with no frame delivery and false encoder/capture failures. Keep the output attached once (or remove it on stop) before attempting another add.
Useful? React with 👍 / 👎.
Nottlespike
force-pushed
the
feat/macos/capture/screencapturekit-backend
branch
from
395e2a2 to
443fe9e
Compare
Contributor
Author
|
Thanks for the catches — pushed an amended commit (force-push to this branch) addressing the genuinely-broken items:
The Copilot summary comment is purely descriptive (no actionable inline suggestions in it). |
…12.3+
AVCaptureScreenInput was deprecated in macOS 13 (October 2022) and is
fundamentally limited to 8-bit BGRA, blocking any honest HDR or 10-bit
work on the macOS capture path. ScreenCaptureKit has been available
since macOS 12.3 (March 2022) and is the only forward path; this
commit lays the foundation by adding a drop-in SCK-based backend that
preserves behaviour exactly (same pixel format, frame rate, display
selection) so it can be reviewed independently of the HDR work that
builds on top.
Changes:
* Add SunshineVideoCapture protocol in av_video.h declaring the
capture-side surface both backends expose.
* Make AVVideo conform to the protocol (no behaviour change; pure
declaration).
* Add SCVideo (sc_video.h / sc_video.m) implementing the same
protocol against SCStream + SCContentFilter + SCStreamConfiguration.
Built with -fobjc-arc for SCK's block-heavy API surface; objects
cross the MRC boundary via the standard +1-retain alloc/init
convention so display.mm continues to work in MRC.
* Drop incomplete frames from SCK output by inspecting
SCStreamFrameInfoStatus on each sample-buffer attachment, matching
the reliability the legacy path got for free from AVCaptureSession.
* display.mm now holds an id<SunshineVideoCapture> and branches at
construction via @available(macOS 12.3, *): SCVideo on supported
systems, AVVideo as fallback for older macOS.
* Wire ScreenCaptureKit framework into cmake/dependencies/macos.cmake
and cmake/compile_definitions/macos.cmake; set ARC compile flag on
sc_video.m only.
Pixel format stays 32BGRA for this commit; 10-bit + EDR metadata
follow in a subsequent change.
Nottlespike
force-pushed
the
feat/macos/capture/screencapturekit-backend
branch
from
443fe9e to
b49a34b
Compare
Contributor
Author
|
Force-pushed. Addressed the remaining Copilot suggestion (the one that wasn't already fixed by the earlier amend):
For the other Copilot points on this PR's older review — they were already addressed in the earlier amend and are still in place at slightly different line numbers post-rebase. Specifically:
Copilot's diff-relative line numbers shifted post-rebase, so its older comments now point at unrelated code. Sorry for the noise. |
…pixel formats
With AVCaptureScreenInput, asking the capture surface for a 10-bit
pixel format silently produced 8-bit BGRA — the OS-level lie that
made HEVC Main10 / AV1 Main10 / ProRes 10-bit profiles on macOS into
fake HDR (color-tagged 8-bit data). With ScreenCaptureKit landing in
the previous commit, 10-bit pixel formats are actually honoured, but
SCK needs an explicit signal to attach HDR metadata to those buffers
instead of treating them as 10-bit Rec.709.
This commit wires SCStreamConfiguration.captureDynamicRange:
* Add +pixelFormatIsHighBitDepth: classifier covering the YUV 4:2:0,
4:2:2 and 4:4:4 10-bit BiPlanar formats plus ARGB2101010 packed
and 64-bit RGBA formats.
* On the synchronous init path, set captureDynamicRange immediately
if the starting pixel format is high bit depth so the very first
sample buffer carries HDR metadata.
* On the setPixelFormat: path (called by nv12_zero_device when the
encoder selects p010), also update captureDynamicRange and push
the new config to a running stream via -updateConfiguration:.
* Use SCCaptureDynamicRangeHDRLocalDisplay rather than canonical
HDR: game streaming wants the host display's actual HDR
characteristics (peak luminance, primaries) so the receiver shows
what a local user would see, not Apple's idealised reference.
* Guard the whole block behind @available(macOS 14.0, *); on
12.3-13.x SCK still honours the 10-bit pixel format request but
doesn't auto-tag buffers, so Sunshine's existing colorspace logic
continues to drive the encoder's color fields.
Validated on M4 Max: Sunshine's encoder probe matrix now includes
successful 10-bit HEVC and 10-bit ProRes entries that previously
could not have validated because the capture surface couldn't
deliver matching pixel data. ProRes-specific VideoToolbox color tags
land in a separate follow-up commit.
…pth alone
The previous EDR commit flipped SCStreamConfiguration.captureDynamicRange
to HDRLocalDisplay whenever the chosen CVPixelBuffer format was 10-bit.
That is necessary but not sufficient: a 10-bit format may be selected
for codec reasons (e.g., a ProRes profile that requires 4:4:4 10-bit
input) without the client ever requesting HDR ingest. The result was a
silent control/data-plane mismatch — Sunshine would tell the client
"HDR mode false" in the SDP while emitting BT.2020 PQ-tagged buffers,
leaving the decoder to interpret tagged HDR content however its display
pipeline saw fit.
Plumb the negotiated session's HDR state down to SCK:
rtsp.cpp (x-nv-video[0].dynamicRangeMode → config.dynamicRange)
→ video.cpp (existing)
→ platf::display(... video::config_t)
→ display.mm (hdr_allowed = config.dynamicRange ? YES : NO)
→ SCVideo initWithDisplay:frameRate:hdrAllowed:
→ applyDynamicRangeForPixelFormat: (gates HDR on both pixel
format depth AND hdrAllowed; defaults to SDR otherwise)
The convenience initializer without hdrAllowed defaults to NO so any
out-of-tree caller stays on the safe SDR path until they opt in. The
new "Using ScreenCaptureKit capture backend (HDR allowed|blocked)" log
line makes the negotiated state visible at the same place the backend
selection is logged.
Nottlespike
changed the title
Contributor
Author
|
Force-pushed. Restructured per maintainer guidance that each PR must be mergeable in any order:
PR title and body updated to reflect the combined scope. Closes #5191. #5192 (ProRes) was rebased onto this new tip and updated to state explicit any-order-mergeable independence (ProRes encoder probe gracefully falls back to standard codecs if the underlying capture path can't supply 10-bit buffers — same as any failed-probe encoder). |
|
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
Adds a ScreenCaptureKit (SCK) backend for macOS screen capture (12.3+) and layers properly-gated EDR/HDR support on top of it. Replaces the deprecated
AVCaptureScreenInputpath for users on modern macOS while leaving the legacy path intact as a runtime fallback.This PR was previously split across #5190 (SCK) and #5191 (EDR). Folded together per maintainer feedback that PRs must be mergeable in any order — the EDR work textually depends on the SCK API surface and would not compile without it, so the split was artificial. The HDR-gating hardening (third commit) was added during review to fix a real defect in the EDR work.
Commits
feat(macos/capture): add ScreenCaptureKit backend, runtime-select on 12.3+— introducesSCVideo(insc_video.{h,m}), runtime-selected via@available(macOS 12.3, *)indisplay.mm. SCVideo conforms to the sameSunshineVideoCaptureprotocol as the legacyAVVideoso the rest of the macOS capture pipeline is unchanged. ARC-compiled for clarity; lifecycle hardened (singleaddStreamOutput, bounded SCK completion-handler timeouts,@synchronizedon shared state).feat(macos/capture): enable EDR (HDR) output on macOS 14+ for 10-bit pixel formats— when SCK is in use and the chosenCVPixelBufferformat is 10-bit, requestSCCaptureDynamicRangeHDRLocalDisplayso the OS tags buffers with BT.2020 PQ metadata matching the local panel. SDK-guarded so older Xcode versions still compile.feat(macos/capture): gate EDR on negotiated session HDR, not pixel depth alone— the previous commit gated EDR purely on pixel format depth. That's not enough: a 10-bit format may be selected for codec reasons (e.g., ProRes 4:4:4) without the client ever requesting HDR ingest. We were silently emitting BT.2020 PQ buffers into streams the SDP described as SDR. This commit plumbsenable_hdrfromlaunch_session_t(via the existingconfig.dynamicRangefield onvideo::config_t) down toSCVideo init, and requires both 10-bit pixel format AND negotiated HDR before flippingcaptureDynamicRange = HDRLocalDisplay.Verification
__MAC_OS_X_VERSION_MAX_ALLOWED >= 140000) and current SDK.Using ScreenCaptureKit capture backend (HDR allowed|blocked)at session start so the negotiated state is auditable.Independence
Stands alone — does not depend on any other open PR. The 10-bit pixel format selection path (e.g., for ProRes capture in #5192) is itself an additive change that exercises this PR's EDR path when both are present; neither requires the other to compile or pass CI.
closes #5191