feat(macos/capture): enable EDR (HDR) output on macOS 14+ for 10-bit pixel formats#5191
feat(macos/capture): enable EDR (HDR) output on macOS 14+ for 10-bit pixel formats#5191Nottlespike wants to merge 2 commits into
Conversation
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 screen-capture backend using ScreenCaptureKit (macOS 12.3+) and wires it into the existing display capture pipeline while keeping the legacy AVCaptureScreenInput path for older macOS versions.
Changes:
- Introduce
SCVideo(ScreenCaptureKit-based) as an alternative capture implementation conforming to a shared protocol. - Update
display.mmto select betweenSCVideoandAVVideoat runtime and to store the capture object behindid<SunshineVideoCapture>. - Update CMake to link
ScreenCaptureKitand compilesc_video.mwith ARC.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/macos/sc_video.m | New ScreenCaptureKit-based capture implementation (ARC) with dynamic range handling and callback-based frame delivery. |
| src/platform/macos/sc_video.h | Declares SCVideo and aligns it with the shared capture protocol used by callers. |
| src/platform/macos/display.mm | Switch capture backend by OS availability and adapt NV12 device initialization/type handling. |
| src/platform/macos/av_video.h | Introduces SunshineVideoCapture protocol shared by AVVideo and SCVideo. |
| cmake/dependencies/macos.cmake | Adds ScreenCaptureKit framework discovery. |
| cmake/compile_definitions/macos.cmake | Links ScreenCaptureKit, adds new sources, and enables ARC for sc_video.m. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.streamConfig.width = self.frameWidth; | ||
| self.streamConfig.height = self.frameHeight; |
| CGDisplayModeRelease(mode); | ||
| } | ||
|
|
||
| self.sampleQueue = dispatch_queue_create("dev.lizardbyte.sunshine.sckCapture", dispatch_queue_attr_make_with_qos_class(DISPATCH_QUEUE_SERIAL, QOS_CLASS_USER_INTERACTIVE, DISPATCH_QUEUE_PRIORITY_HIGH)); |
| [SCShareableContent getShareableContentExcludingDesktopWindows:NO | ||
| onScreenWindowsOnly:NO | ||
| completionHandler:^(SCShareableContent *_Nullable content, NSError *_Nullable error) { |
| } | ||
| dispatch_semaphore_signal(ready); | ||
| }]; | ||
| dispatch_semaphore_wait(ready, DISPATCH_TIME_FOREVER); |
| if (!self.streamRunning) { | ||
| NSError *outputError = nil; | ||
| if (![self.stream addStreamOutput:self | ||
| type:SCStreamOutputTypeScreen | ||
| sampleHandlerQueue:self.sampleQueue | ||
| error:&outputError]) { |
| if (self.streamRunning) { | ||
| [self.stream stopCaptureWithCompletionHandler:^(NSError *_Nullable error) { | ||
| (void) error; | ||
| }]; | ||
| self.streamRunning = NO; | ||
| } |
| if (@available(macOS 14.0, *)) { | ||
| if ([SCVideo pixelFormatIsHighBitDepth:pixelFormat]) { |
| self.streamConfig.captureDynamicRange = SCCaptureDynamicRangeHDRLocalDisplay; | ||
| } else { | ||
| self.streamConfig.captureDynamicRange = SCCaptureDynamicRangeSDR; | ||
| } | ||
| } |
| "${CMAKE_SOURCE_DIR}/src/platform/macos/sc_video.h" | ||
| "${CMAKE_SOURCE_DIR}/src/platform/macos/sc_video.m" |
| set_source_files_properties( | ||
| "${CMAKE_SOURCE_DIR}/src/platform/macos/sc_video.m" | ||
| PROPERTIES COMPILE_FLAGS "-fobjc-arc") |
![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: 88208f69c6
ℹ️ 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".
| if (![self.stream addStreamOutput:self | ||
| type:SCStreamOutputTypeScreen | ||
| sampleHandlerQueue:self.sampleQueue | ||
| error:&outputError]) { |
There was a problem hiding this comment.
capture: adds self as an SCStream output every time streamRunning is false, but the stop paths only call stopCapture and never remove that output, so restarts reuse a stream with stale output registration. This is exercised by the current flow (e.g., dummy_img warm-up then real capture in src/video.cpp), and can cause the second start to fail (addStreamOutput returns NO) or to deliver duplicated callbacks if the framework accepts duplicate registrations.
Useful? React with 👍 / 👎.
| if (startError) { | ||
| NSLog(@"SCVideo: startCapture failed: %@", startError); | ||
| dispatch_semaphore_signal(self.currentSignal); | ||
| return self.currentSignal; |
There was a problem hiding this comment.
On addStreamOutput/startCapture failure, this method signals and returns a semaphore instead of propagating an error, and display.mm subsequently waits and returns capture_e::ok unconditionally. That means startup failures are reported as successful capture sessions, which can leave the pipeline in a silent no-frame state instead of triggering error/reinit handling.
Useful? React with 👍 / 👎.
Nottlespike
force-pushed
the
feat/macos/capture/edr-hdr-output
branch
from
88208f6 to
50b75e8
Compare
|
Most of the inline suggestions are duplicated from #5190 and have been addressed there (this PR's diff includes #5190's commit; once that lands the diff reduces to just the EDR commit). Specifically resolved in the amended SCK commit:
EDR-specific:
Force-pushed both branches. |
|
…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.
Nottlespike
force-pushed
the
feat/macos/capture/edr-hdr-output
branch
from
50b75e8 to
fa43fb1
Compare
|
Force-pushed (rebased on new #5190 tip). Same Re Copilot's latest inline pass: all six suggestions it surfaced are already fixed in the rebased branch, just at shifted line numbers post-amend. Quick map:
The Copilot bot doesn't re-evaluate inline comments after force-pushes; the suggestions are valid critiques of the pre-amend code that's no longer present in the branch. |
|
Closing and folding into #5190. Per the 'each PR must be mergeable in any order' guidance, the EDR work is textually dependent on the SCK API surface in #5190 (it would not compile against the legacy AVCaptureScreenInput path), so splitting it across two PRs is artificial. The EDR commit will be squashed onto #5190 along with the HDR-gating hardening (gating EDR on negotiated |
Description
With
AVCaptureScreenInput, asking the capture surface for a 10-bit pixel format silently produced 8-bit BGRA — an OS-level lie that made HEVC Main10 / AV1 Main10 / future ProRes 10-bit profiles on macOS into fake HDR (color-tagged 8-bit data). WithScreenCaptureKitlanding in #5190, 10-bit pixel formats are actually honoured, but SCK needs an explicit signal (captureDynamicRange) to attach HDR metadata to those buffers instead of treating them as 10-bit Rec.709.Changes:
+pixelFormatIsHighBitDepth:classifier inSCVideocovering the YUV 4:2:0 / 4:2:2 / 4:4:4 10-bit BiPlanar formats plusARGB2101010packed and 64-bit RGBA formats. Returning YES is the signal that the capture surface is HDR-capable.captureDynamicRangeimmediately if the starting pixel format is high bit depth, so the very first sample buffer carries HDR metadata (no first-frame-SDR flash).setPixelFormat:path (called bynv12_zero_devicewhen the encoder selectspix_fmt_e::p010): also updatescaptureDynamicRangeand pushes the new config to a running stream via-updateConfiguration:.SCCaptureDynamicRangeHDRLocalDisplayrather than the canonical HDR mode: 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 encoding.@available(macOS 14.0, *)guard: on 12.3-13.x SCK still honours the 10-bit pixel format request fromnv12_zero_devicebut the OS doesn't auto-tag buffers; Sunshine's existing colorspace logic continues to drive the encoder's color fields in that case.Validated on M4 Max (macOS 14+):
Sunshine's encoder probe matrix at startup now contains:
Previously these 10-bit probes could not validate because the capture surface couldn't deliver matching 10-bit data —
validate_configwould fail at encoder init time. With this change, real 10-bit data flows through the pipeline for the first time on macOS, and the per-codecDYNAMIC_RANGEcapability bit is set honestly based on actual probe results rather than optimistic.capabilities.set().What this does NOT yet do (separate follow-ups):
kVTCompressionPropertyKey_ColorPrimaries/_TransferFunction/_YCbCrMatrix) — ProRes carries color in the frame header rather than SEI, so this is its own discrete change.Encoder side is already wired generically (Sunshine sets
ctx->color_primaries / color_trc / colorspacefromavcodec_colorspaceatsrc/video.cpp:1781-1783for all encoders), so HEVC Main10 HDR via VideoToolbox should now produce honest BT.2020 PQ output for any HDR-requesting client.Screenshot
Issues Fixed or Closed
Roadmap Issues
Type of Change
Checklist
AI Usage