feat(macos/capture): ScreenCaptureKit backend with gated EDR for 10-bit pixel formats#5201
Open
Nottlespike wants to merge 6 commits into
Open
Conversation
…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.
…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.
…session HDR Two improvements to the macOS ScreenCaptureKit backend that landed in the upstream review cycle (LizardByte#5190) and never made it back onto our fork's dev: 1. **Lifecycle hardening**: - Register the SCStreamOutput exactly once in -init, not on every -capture: call. SCStream retains outputs across stop/start cycles, so re-registering would fail or silently duplicate delivery. - Bound all SCK completion-handler waits to 5s. SCK should always invoke them, but a misbehaving system service must not hang the whole startup path. - @synchronized(self) around all reads/writes of currentCallback / currentSignal / streamRunning. The sample-handler queue, the -capture: caller, and the SCStream delegate all touch these from different threads. - dispatch_queue_attr_make_with_qos_class's third argument is a RELATIVE priority (range -15..0), not one of the legacy DISPATCH_QUEUE_PRIORITY_* constants. Using 0 keeps the queue at its QoS class's nominal priority. - CGDisplayBounds fallback when CGDisplayCopyDisplayMode returns NULL (display reconfiguration races). 2. **EDR gating fix**: The previous EDR code flipped captureDynamicRange = HDRLocalDisplay whenever the chosen CVPixelBuffer format was 10-bit. That's 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. Without gating, Sunshine would tell the client "HDR mode false" in the SDP while emitting BT.2020 PQ-tagged buffers — a silent control/data-plane mismatch. Now EDR requires BOTH 10-bit pixel format AND the negotiated session's enable_hdr (plumbed from launch_session_t via the existing config.dynamicRange field on video::config_t). Default is SDR; HDR is opt-in per session. New init signature: initWithDisplay:frameRate:hdrAllowed: The old initializer is preserved as a convenience that passes NO. New log line at session start makes the gating visible: "Using ScreenCaptureKit capture backend (HDR allowed|blocked)"
FIND_LIBRARY(SCREEN_CAPTURE_KIT_LIBRARY ScreenCaptureKit REQUIRED) so configure fails fast on environments without the SDK rather than later at header-lookup time. sc_video.m is compiled unconditionally; the REQUIRED keyword ensures the build prerequisites are surfaced clearly when the build host's Xcode/SDK is older than 13.3 / 12.3 (well past routine compatibility). Addresses Copilot inline feedback from the closed upstream PR cycle.
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a new macOS ScreenCaptureKit (SCK) capture backend and wires it into the existing display pipeline so Sunshine can use SCK on macOS 12.3+ while retaining the legacy AVCaptureScreenInput fallback for older systems.
Changes:
- Introduce
SCVideo(SCK-based) capture implementation andSunshineVideoCaptureprotocol to unify backends. - Switch
display.mmto hold anid<SunshineVideoCapture>and selectSCVideovsAVVideoat runtime via@available. - Update macOS CMake to link ScreenCaptureKit and compile
sc_video.mwith ARC.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/macos/sc_video.m | New ScreenCaptureKit-based capture backend with stream lifecycle management and HDR dynamic range gating logic. |
| src/platform/macos/sc_video.h | Public header exposing SCVideo as a SunshineVideoCapture implementation. |
| src/platform/macos/display.mm | Uses unified capture protocol and chooses SCK backend on macOS 12.3+. |
| src/platform/macos/av_video.h | Introduces SunshineVideoCapture protocol and makes AVVideo conform to it. |
| cmake/dependencies/macos.cmake | Adds required ScreenCaptureKit framework discovery. |
| cmake/compile_definitions/macos.cmake | Links ScreenCaptureKit, adds new files, and enables ARC for sc_video.m. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+323
to
+342
| - (void)dealloc { | ||
| BOOL running; | ||
| SCStream *stream; | ||
| @synchronized(self) { | ||
| running = self.streamRunning; | ||
| stream = self.stream; | ||
| self.streamRunning = NO; | ||
| self.currentCallback = nil; | ||
| } | ||
| if (running && stream) { | ||
| // Best-effort synchronous stop with a bounded wait so a | ||
| // misbehaving SCK doesn't hang teardown. | ||
| dispatch_semaphore_t stopped = dispatch_semaphore_create(0); | ||
| [stream stopCaptureWithCompletionHandler:^(NSError *_Nullable error) { | ||
| (void) error; | ||
| dispatch_semaphore_signal(stopped); | ||
| }]; | ||
| dispatch_semaphore_wait(stopped, dispatch_time(DISPATCH_TIME_NOW, 2 * NSEC_PER_SEC)); | ||
| } | ||
| } |
Comment on lines
+46
to
+47
| @property (nonatomic, assign) BOOL streamRunning; | ||
| @property (nonatomic, assign) BOOL streamOutputAdded; |
Comment on lines
+28
to
+47
| @protocol SunshineVideoCapture <NSObject> | ||
|
|
||
| @property (nonatomic, assign) CGDirectDisplayID displayID; | ||
| @property (nonatomic, assign) CMTime minFrameDuration; | ||
| @property (nonatomic, assign) OSType pixelFormat; | ||
| @property (nonatomic, assign) int frameWidth; | ||
| @property (nonatomic, assign) int frameHeight; | ||
|
|
||
| typedef bool (^FrameCallbackBlock)(CMSampleBufferRef); | ||
| - (void)setFrameWidth:(int)frameWidth frameHeight:(int)frameHeight; | ||
| - (dispatch_semaphore_t)capture:(FrameCallbackBlock)frameCallback; | ||
|
|
||
| @end | ||
|
|
||
| @interface AVVideo: NSObject <AVCaptureVideoDataOutputSampleBufferDelegate, SunshineVideoCapture> | ||
|
|
||
| @property (nonatomic, assign) CGDirectDisplayID displayID; | ||
| @property (nonatomic, assign) CMTime minFrameDuration; | ||
| @property (nonatomic, assign) OSType pixelFormat; | ||
| @property (nonatomic, assign) int frameWidth; | ||
| @property (nonatomic, assign) int frameHeight; |
Comment on lines
+14
to
+20
| # deprecated AVCaptureScreenInput-based capture path. Sunshine's | ||
| # sc_video.{h,m} is unconditionally compiled into the macOS target; | ||
| # fail configure with a clear message rather than failing the build | ||
| # later on header lookup when the SDK doesn't ship the framework | ||
| # (e.g., when building with an Xcode older than 13.3 / SDK older than | ||
| # 12.3, which dropped out of routine compatibility long ago). | ||
| FIND_LIBRARY(SCREEN_CAPTURE_KIT_LIBRARY ScreenCaptureKit REQUIRED) |
ScreenCaptureKit became available in macOS 12.3; Sunshine's deployment target (MACOSX_DEPLOYMENT_TARGET=14.2) is well above that. The @available(macOS 12.3, *) runtime branch in display.mm and the entire AVCaptureScreenInput-based AVVideo class were therefore dead code on every supported build. Changes: - Remove @available(macOS 12.3, *) check in display.mm; SCK is the only branch. - Replace `id<SunshineVideoCapture>` with `SCVideo *` directly — the protocol existed to abstract over both AVVideo and SCVideo, and is no longer needed with a single concrete capture class. - Move the small bits we still need (FrameCallbackBlock typedef, +displayNames / +getDisplayName: helpers) from av_video.{h,m} into sc_video.{h,m}. - Delete src/platform/macos/av_video.{h,m} (208 lines). - Drop both from PLATFORM_TARGET_FILES. Addresses andygrundman + ReenigneArcher review feedback on the original PR: "shouldn't keep workarounds for versions older than what we support."
Contributor
Author
|
Force-pushed. Addressing the maintainer feedback I missed during the original review cycle:
Right call — the `@available(macOS 12.3, *)` branch and the entire `AVVideo` class were dead code on every supported build. New commit on the branch removes them:
Net diff: -208 / +75 lines, single concrete macOS capture backend, no runtime feature flags to chase. |
Two Copilot findings on the SCK PR: 1. **dealloc signals pending semaphore.** -dealloc was clearing currentCallback but never signalling currentSignal. If the stream stopped without firing -stream:didStopWithError:, any caller still waiting on the semaphore returned by -capture: would stall forever. Snapshot the pending signal in the @synchronized block, then send it after clearing the callback so the waiter wakes up to observe their callback is nil and exits. 2. **Drop unused streamOutputAdded property.** Set in -init but never read. Removed both the @Property declaration and the assignment. The "register output exactly once at init" invariant is now structural (the call is in -init, not gated on a flag) and the dead state can't drift.
Contributor
Author
|
Pushed Real bugs fixed (2):
Defended (2) — with rationale:
Build clean on macOS 26.5 / Apple clang 21. The bug fix for #1 is the substantive one — it eliminates a real wait-forever path during shutdown. |
|
ReenigneArcher
added
the
ai
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.
Reopens #5190 (closed prematurely; GitHub blocked direct reopen due to regenerated branch SHAs). All review feedback from the original cycle is addressed in additional commits. See the original PR for full discussion; rebased on current master.