Conversation
|
The remaining QualityGate issue is not mine; it's from 4 months ago. I can certainly fix it but I would rather keep this PR on topic. |
|
Agree on the sonar issue. Could you fix the doxygen errors before I review this? https://app.readthedocs.org/projects/sunshinestream/builds/34086280/ You can expand the failed section and |
|
Done. Sorry I missed them last night. |
|
This PR compiles against the latest |
9204144 to
6adfca6
Compare
|
Sorry for the delay in getting to this. I don't know much about Apple so the following is according to GPT 5.6. Main10 capture uses an unsupported ScreenCaptureKit format. At display.mm:590, 10-bit sessions assign x420 to SCStreamConfiguration.pixelFormat. Apple documents only BGRA, l10r, 420v, and 420f as supported formats. Sunshine can reach this path through VideoToolbox’s P010/Main10 capability. Capture errors are then silently discarded at sc_capture.m:270, so the backend neither produces real frames nor falls back to AVFoundation. Keep 10-bit capture on AVFoundation or add a supported ScreenCaptureKit format/conversion path, and propagate permanent screenshot errors. Apple’s pixel-format contract. Additionally, can you add tests to cover the changes? At minimum, cover delayed packets, dropped PTS values, missing timestamps, and queue eviction. |
|
Re the Main10 / Minimal standalone probe calling macOS 26.5.2 (25F84), Apple M4, display 2048x1152:
So on this system Caveat: single machine, single OS version. I have not tested macOS 14.x or 15.x, which is where the question is still open given the The second part of the review stands regardless: sc_pixfmt_probe.m/*
* sc_pixfmt_probe.m
*
* Minimal standalone probe for LizardByte/Sunshine PR #5511.
*
* Question: what does SCScreenshotManager actually do when
* SCStreamConfiguration.pixelFormat is set to a format that Apple's docs do
* not list as supported? In particular 'x420'
* (kCVPixelFormatType_420YpCbCr10BiPlanarVideoRange), which display.mm assigns
* for 10-bit sessions.
*
* Four possible outcomes, each leading to a different conclusion:
* 1. NSError is returned -> the review is right, needs a fix.
* 2. Buffer returned as 'x420' -> the format works despite the docs.
* 3. Buffer returned in another -> silent degradation: worse than an error,
* format the encoder gets an unexpected format.
* 4. Buffer returned as 'x420' -> format accepted but no real data. This is
* but all-zero content what "no frames, no error" would look like.
*
* Build:
* clang -fobjc-arc -framework Foundation -framework ScreenCaptureKit \
* -framework CoreMedia -framework CoreVideo -framework CoreGraphics \
* -o sc_pixfmt_probe sc_pixfmt_probe.m
*
* Run:
* ./sc_pixfmt_probe
*
* Requires Screen Recording permission for the terminal app launching it
* (System Settings > Privacy & Security > Screen Recording).
*/
#import <Foundation/Foundation.h>
#import <ScreenCaptureKit/ScreenCaptureKit.h>
#import <CoreMedia/CoreMedia.h>
#import <CoreVideo/CoreVideo.h>
#import <CoreGraphics/CoreGraphics.h>
static NSString *FourCC(OSType c) {
if (c == 0) return @"<none>";
char s[5] = {
(char) ((c >> 24) & 0xFF),
(char) ((c >> 16) & 0xFF),
(char) ((c >> 8) & 0xFF),
(char) (c & 0xFF),
0
};
for (int i = 0; i < 4; i++) {
if (s[i] < 32 || s[i] > 126) s[i] = '?';
}
return [NSString stringWithFormat:@"'%s' (0x%08X)", s, (unsigned) c];
}
static void probe(SCDisplay *display, OSType fmt, NSString *label) {
SCContentFilter *filter = [[SCContentFilter alloc] initWithDisplay:display
excludingWindows:@[]];
// Same configuration fields the PR sets in SCCapture.captureVideo.
SCStreamConfiguration *cfg = [[SCStreamConfiguration alloc] init];
cfg.width = display.width;
cfg.height = display.height;
cfg.pixelFormat = fmt;
cfg.showsCursor = YES;
cfg.captureResolution = SCCaptureResolutionBest;
cfg.preservesAspectRatio = YES;
printf("\n--- %s: requested %s\n", label.UTF8String, FourCC(fmt).UTF8String);
printf(" cfg.pixelFormat after assignment: %s\n",
FourCC(cfg.pixelFormat).UTF8String);
dispatch_semaphore_t sem = dispatch_semaphore_create(0);
// Same API the PR polls: SCScreenshotManager, not SCStream.
[SCScreenshotManager captureSampleBufferWithFilter:filter
configuration:cfg
completionHandler:^(CMSampleBufferRef sb, NSError *err) {
if (err) {
printf(" RESULT: NSError -> domain=%s code=%ld %s\n",
err.domain.UTF8String,
(long) err.code,
err.localizedDescription.UTF8String);
dispatch_semaphore_signal(sem);
return;
}
if (!sb) {
printf(" RESULT: no error, but sampleBuffer == NULL\n");
dispatch_semaphore_signal(sem);
return;
}
printf(" sampleBuffer valid: %s\n",
CMSampleBufferIsValid(sb) ? "yes" : "no");
CVImageBufferRef px = CMSampleBufferGetImageBuffer(sb);
if (!px) {
printf(" RESULT: sampleBuffer has no imageBuffer\n");
dispatch_semaphore_signal(sem);
return;
}
OSType got = CVPixelBufferGetPixelFormatType(px);
printf(" RESULT: buffer delivered with format %s\n",
FourCC(got).UTF8String);
printf(" dimensions: %zux%zu planar=%s planes=%zu\n",
CVPixelBufferGetWidth(px),
CVPixelBufferGetHeight(px),
CVPixelBufferIsPlanar(px) ? "yes" : "no",
CVPixelBufferIsPlanar(px) ? CVPixelBufferGetPlaneCount(px) : (size_t) 0);
printf(" matches requested: %s\n",
got == fmt ? "YES" : "NO <-- silent degradation");
// Outcome 4: format accepted but empty content (black frame).
// Sample plane 0 (Y for biplanar formats, the only plane for packed ones).
if (CVPixelBufferLockBaseAddress(px, kCVPixelBufferLock_ReadOnly) == kCVReturnSuccess) {
const uint8_t *base;
size_t bpr, h;
if (CVPixelBufferIsPlanar(px)) {
base = CVPixelBufferGetBaseAddressOfPlane(px, 0);
bpr = CVPixelBufferGetBytesPerRowOfPlane(px, 0);
h = CVPixelBufferGetHeightOfPlane(px, 0);
} else {
base = CVPixelBufferGetBaseAddress(px);
bpr = CVPixelBufferGetBytesPerRow(px);
h = CVPixelBufferGetHeight(px);
}
size_t nonzero = 0, total = 0;
// Sample 1 row out of 64; enough to detect an all-black frame.
// Note for 'x420': samples are 16-bit LE with the 10-bit value in the high
// bits, so the low byte is often zero on real images. Expect ~50-70% nonzero.
for (size_t row = 0; row < h; row += 64) {
const uint8_t *r = base + row * bpr;
for (size_t i = 0; i < bpr; i++) {
total++;
if (r[i] != 0) nonzero++;
}
}
CVPixelBufferUnlockBaseAddress(px, kCVPixelBufferLock_ReadOnly);
printf(" plane 0 content: %zu/%zu sampled bytes nonzero -> %s\n",
nonzero, total,
nonzero == 0 ? "BLACK FRAME <-- format accepted but no data" : "has image");
} else {
printf(" plane 0 content: could not lock buffer for reading\n");
}
dispatch_semaphore_signal(sem);
}];
// 10 s headroom: the first call can be slow while permission is being granted.
if (dispatch_semaphore_wait(sem,
dispatch_time(DISPATCH_TIME_NOW, 10ull * NSEC_PER_SEC)) != 0) {
printf(" RESULT: timeout, completion handler not invoked within 10 s\n");
}
}
int main(void) {
@autoreleasepool {
NSOperatingSystemVersion v =
[[NSProcessInfo processInfo] operatingSystemVersion];
printf("macOS %ld.%ld.%ld\n",
(long) v.majorVersion, (long) v.minorVersion, (long) v.patchVersion);
__block SCShareableContent *content = nil;
dispatch_semaphore_t sem = dispatch_semaphore_create(0);
[SCShareableContent getShareableContentWithCompletionHandler:
^(SCShareableContent *c, NSError *e) {
if (e) {
printf("getShareableContent failed: %s\n",
e.localizedDescription.UTF8String);
} else {
content = c;
}
dispatch_semaphore_signal(sem);
}];
dispatch_semaphore_wait(sem, DISPATCH_TIME_FOREVER);
if (!content || content.displays.count == 0) {
printf("No displays available. Check Screen Recording permission.\n");
return 1;
}
SCDisplay *display = content.displays.firstObject;
printf("display id=%u %zux%zu\n\n",
(unsigned) display.displayID,
(size_t) display.width,
(size_t) display.height);
// Documented baseline: what Sunshine uses today for 8-bit.
probe(display, kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange, @"420v 8-bit (documented)");
// The disputed one: what display.mm assigns for 10-bit sessions.
probe(display, kCVPixelFormatType_420YpCbCr10BiPlanarVideoRange, @"x420 10-bit (DISPUTED)");
// Documented 10-bit alternative, in case it is useful as a fallback path.
probe(display, kCVPixelFormatType_ARGB2101010LEPacked, @"l10r 10-bit (documented)");
// Control: should always work.
probe(display, kCVPixelFormatType_32BGRA, @"BGRA 8-bit (documented)");
printf("\nDone.\n");
}
return 0;
}Full output on macOS 26.5.2 / Apple M4 |
|
Glad to hear the PR is alive. I will address the issues raised - but @Optimiza, if you could hold off reviewing/testing until after I push changes that'd be great. Reason: the current implementation is limited to ~30fps (inherent >20ms latency in SCScreenshotManager, only one in-flight request). I have refactored the PR to use SCStream after all. The original issue (detections dropped) was worked around by using the method OBS uses: cadence set to 0.9*expected_fps. (Credit to Claude for digging this up.) Result: 10ms host-processing latency all-in, smooth 60fps (limited by my displays). It will never reach Windows' NVEnc 4ms number but it's close enough, and I'm very happy with it. I've been using the refactored version for over a week. Let me look at the 10-bit issue in detail and I'll push an update. |
|
Sounds good, I'll hold off until you push the changes and test everything together then, including the Sonoma/Intel path on the iMac. |
|
Capture is now SCStream, not SCScreenshotManager polling. With the refactor and necessary fixes to my initial SCStream approach, it delivers reliably: pitch-perfect typing, a solid 60fps under motion, and ~10ms host processing latency all-in on my hardware. The screenshot engine is gone entirely, which also removes the 30-40fps ceiling its serial ~25ms-per-request cost imposed. Re: Main10/x420 — @Optimiza's probe showed x420 working, but it exercised SCScreenshotManager, and the branch now uses SCStream, whose format validation may differ; and either way the docs don't promise it. So instead of relying on it: 10-bit sessions attempt x420, and if the runtime rejects it at stream start, capture retries once with 420v and logs a warning. VideoToolbox accepts 8-bit input into a Main10 session, so the negotiated stream survives with 8-bit-sourced content. A reasonable degradation given macOS HDR capture isn't a thing here yet. I didn't move 10-bit to AVFoundation because that would reintroduce the disappearing-cursor bug (#3433) and AVFoundation's higher capture latency for a fringe path. Re: silent error handling — sc_capture is now Objective-C++ and all its logging goes through BOOST_LOG into sunshine.log. Stream start failures propagate out of the capture backend (capture_e::error -> normal reinit machinery), and a mid-stream stream death wakes the capture loop into reinit instead of hanging. The finishScreenshotSampleBuffer: error swallowing called out in the review no longer exists along with the rest of the screenshot path. Tests: added coverage for the frame-timestamp bookkeeping: delayed packets, dropped PTS values, missing timestamps, and queue eviction, plus in-order round-trip, unknown pts, empty queue, and double-consume. The helpers were re-targeted at a plain queue type to make them testable without an encoder session (which collided with the software-encode-device relocation on master; same idea, resolved in the rebase). @Optimiza, good to test now, and the Sonoma/Intel pass would be especially valuable: that's the configuration where both the SCStream format handling and the x420 fallback are most likely to behave differently from my Apple Silicon machines. With capture fixed, the dominant latency term on the default config is now the ffmpeg VideoToolbox encoder itself (~120ms on my test systems, vs ~10-15ms with encoder = software). I have a native VideoToolbox encoder path queued as a follow-up PR that brings hardware encode to ~10ms. Not pushed yet because I'm out of slots. |
|
The remaining Sonar finding is not mine & from a year ago; I can fix but would prefer to keep the PR on topic. @ReenigneArcher your call, I'm happy either way. |
98b1ad7 to
25994e9
Compare
There was a problem hiding this comment.
We're no longer using inputtino, looks like you may have accidentally commited this when rebasing?
Test report: macOS 26.5 / Apple M4I built this branch and ran it against a real Moonlight client. Summary: the Forcing I also hit three build problems that stop the Environment
Unit tests pass: 580 run, 579 passed, 1 skipped (Windows-only), 0 failed. The eight 1. Cursor after typing (#3433) looks fixedIn the one session where the picture was usable (see section 5), the cursor came back on its own at any position on screen after typing, with no need for the usual workaround of dragging it down to the Dock. That is the behaviour #3433 asks for, and I could not reproduce the old symptom at all in that session. Worth noting how this was observed: it was through 2. The
|
| Encoder | SCStream scaling | Result |
|---|---|---|
h264_videotoolbox (zero-copy) |
yes, 1920x1080 on a 2048x1152 display | green / frozen |
h264_videotoolbox (zero-copy) |
no, 1920x1080 on a 1920x1080 display | frozen, brief artefacts on cursor movement |
h264_videotoolbox (zero-copy) |
no, with capture_buffer_size lowered to 6 |
green with artefacts |
libx264 (encoder = software) |
no, native capture | correct image, cursor correct |
Two hypotheses were tested and ruled out:
- Scaling. Setting the host display to exactly the requested resolution, so
SCStreamcaptures native, does not fix it. - Surface exhaustion.
capture_buffer_size(video.cpp:1577) is 12 whileSCStreamConfiguration.queueDepthis capped at 8, so the pipeline can retain more surfaces than the stream owns. Lowering it to 6 and rebuilding does not fix it either. (The mismatch still looks worth addressing on its own, see below.)
Note that the encoder choice and the capture geometry are coupled and cannot be varied independently through configuration: resolution_fn is called from exactly one place in the tree, nv12_zero_device::set_frame (nv12_zero_device.cpp:62), so selecting VideoToolbox is what makes SCStream scale, while the software path leaves capture at the display's native size. Equalizing the two resolutions on the host is what allowed a clean comparison.
Where this points
nv12_zero_device::convert hands the CVPixelBufferRef straight to data[3] for AV_PIX_FMT_VIDEOTOOLBOX, so img->data is unused on this path and the encoder reads the ScreenCaptureKit IOSurface directly. av_img_t.h and nv12_zero_device.cpp are unchanged from master, so what changed is the origin and lifetime of the surfaces now being handed to VideoToolbox. One thing worth a look: av_pixel_buf_t holds CVPixelBufferLockBaseAddress(..., kCVPixelBufferLock_ReadOnly) for the whole lifetime of the pooled image, which was harmless for AVFoundation buffers but pins a CPU mapping on a surface WindowServer owns and recycles.
I did not manage to pin the exact mechanism, so this last paragraph is a lead, not a finding.
6. Separate: the capture loop never signals "no new frame"
push_captured_image_cb_t takes (img, bool frame_captured) (common.h:690), where false means "no new frame, reuse the previous one". The SCKit loop only ever calls it with true (display.mm:440), and when copyLatestSampleBuffer returns null it does continue without pushing anything (display.mm:417-423). The result of the bounded wait is explicitly discarded (display.mm:409-410) even though the poll interval is 1/60 s (display.mm:85).
video.cpp:2438 sets a minimum FPS target (half the client framerate) that exists precisely to keep frames flowing while the screen is static. With no false push, that mechanism never fires on this backend, so a completely idle desktop produces no frames at all.
This is not what caused the issue in section 5 (frames flow at full rate there), but it should bite on a static screen.
7. Minor
config.queueDepth = 8 (sc_capture.mm:275) writes the value that Apple documents as both the default and the maximum: "If not set the default value is 8 frames ... and should not exceed 8 frames." The comment above it states the invariant that the value must exceed the number of buffers the pipeline holds at once, which cannot be satisfied while capture_buffer_size is 12. Whichever way this is resolved, the line as written sets nothing.
Evidence
Session log, both key sessions
Failing, default encoder, capture size equal to the display size so SCStream is not scaling:
[05:17:20.790]: Info: Found H.264 encoder: h264_videotoolbox [videotoolbox]
[05:17:21.298]: Info: CLIENT CONNECTED
[05:17:21.374]: Info: Minimum FPS target set to ~30fps (33.3333ms)
[05:17:21.417]: Info: SCCapture stream configured: 1920x1080 @ 60 fps
[05:17:27.106]: Error: Encoder did not produce IDR frame when requested!
[05:17:27.496]: Error: Encoder did not produce IDR frame when requested!
[05:17:39.194]: Error: Encoder did not produce IDR frame when requested!
Working, software encoder, same capture backend:
[05:09:51.037]: Info: Found H.264 encoder: libx264 [software]
[05:09:51.493]: Info: CLIENT CONNECTED
[05:09:51.588]: Info: Minimum FPS target set to ~30fps (33.3333ms)
[05:09:51.639]: Info: SCCapture stream configured: 2048x1152 @ 60 fps
All sessions, with the numbers quoted above
Host display was 2048x1152 until 05:16 and 1920x1080 from then on, so the SCStream is scaling column is the capture size compared against the display size at that moment.
| Time | Encoder | Capture | Scaling | Result | Sent | Avg fps | Dupe |
IDR errors |
|---|---|---|---|---|---|---|---|---|
| 04:54 | videotoolbox | 1920x1080 | yes | frozen | n/a | n/a | n/a | 6 |
| 05:03 | videotoolbox | 1920x1080 | yes | green | 628 | 48.3 | 3.5% | 0 |
| 05:09 | libx264 | 2048x1152 | no | correct | 10275 | 55.2 | 8.0% | 0 |
| 05:12 | videotoolbox | 1920x1080 | yes | green | 431 | 53.9 | 0.7% | 0 |
| 05:17 | videotoolbox | 1920x1080 | no | frozen | 1239 | 53.9 | 2.3% | 4 |
| 05:21 | videotoolbox, capture_buffer_size = 6 |
1920x1080 | no | green | 591 | 39.4 | 9.0% | 0 |
n/a for the 04:54 row is not zero: that session predates raising the log level, and Sent Frame is logged at verbose. It is an absence of logging, not an absence of frames.
How the numbers were derived
Server-side frame rate and duplicate share, per session log (min_log_level = verbose):
grep -h "Sent Frame" "$LOG" | awk '
{ split($2, t, ":"); c[t[1]":"t[2]":"int(t[3])]++; n++; if (/Dupe/) d++ }
END { for (x in c) { s += c[x]; k++ }
printf "avg_fps=%.1f seconds=%d sent=%d dupe=%.1f%%\n", s/k, k, n, 100*d/n }'Freeze measurement on the client recording, restricted to the remote video area so that client-side UI does not count as movement. First the per-frame mean difference, then the runs of frames that did not change:
ffmpeg -v error -i recording.mp4 \
-vf "crop=1084:608:204:174,tblend=all_mode=difference,signalstats,metadata=print:file=diff.txt" \
-f null -
grep YAVG diff.txt | awk -F= '{ printf "%.3f\n", $2 }' > yavg.txt
awk '$1 < 0.5 { c++ } END { printf "%d of %d frames unchanged (%.0f%%)\n", c, NR, 100*c/NR }' yavg.txtThe recording is 30 fps, so a run of 30 unchanged frames is one second of frozen picture.
recording-PR5511-cropped.mp4
…error handling; improve 10-bit capture handling
25994e9 to
f051973
Compare
|
Apologies for the late response, life got in the way. Thanks for the thorough pass @Optimiza, this was genuinely useful. Point by point: 1. Cursor (#3433): good to hear it holds. That's the capture layer, so it applies to both encoder paths. 2/3/4. Web UI race, missing apps.json, CWD-relative assets dir: agreed on all three, but they're pre-existing master behavior when running the raw build tree rather than the packaged bundle (the PR's only cmake changes are adding the ScreenCaptureKit framework and the two sc_capture sources). Worth an issue of their own; I'd rather not fold them into this PR. 5. Frozen/green picture on the VT H.264 path: really great catch, but it's a pre-existing issue, not an SCKit one. It 6. No 7. queueDepth comment: 8 is the API cap, so the value can't enforce the invariant the comment claimed. Kept the explicit 8 (the default has varied across macOS releases) and rewrote the comment to say what's actually true. @ReenigneArcher: rebased onto current master, and the stray inputtino gitlink is gone. It came from a stale submodule checkout that survived the libvirtualhid change and got swept into a commit on my side. |
|
|
Re point 5, confirming the master repro: yes, it reproduces here too, and I have production logs from before this thread that already show it. Setup: a separate Homebrew install of current Log evidence, Today I reverted A mobile Moonlight client connected against this session and had no freeze with Let me know if there's anything else worth testing while I have that window. |
|
Thanks for this. I guess what'd be helpful is to run the master branch (and possibly this PR) and test it as widely as possible. My Moonlight clients are all Nvidia and they all exhibit the HW decode issue as long as the source is VT h.264. M3 and M5 Macs. You said the client-side issue was also present on Intel integrated, but not on mobile; odd, and encouraging. |
|
Follow-up on item 5: root cause found, and it predates this PR. VideoToolbox's low-latency H.264 sessions emit long-term reference pictures, which some client hardware decoders mishandle; #5200 exposed it by dropping the all-IDR behaviour. Full analysis, the fix (EnableLTR=false on the VT session, verified end to end), and a proposal for the ffmpeg encoder are in #5469. It's a host-side encoder setting, not a capture issue, and it needs an ffmpeg patch in build-deps rather than anything in this PR, so I'd like to keep it out of scope here. |
|
Follow-up on the frozen H.264 picture reported here: tested on a Windows NVIDIA client with controls, results in #5469. The native encoder with |
|
The build-tree defects from my report are now filed separately as #5787, as suggested. One correction there: section 4 of my report (the |
This branch has not been deployed
4 participants
Description
Replaces AVFoundation's AVCaptureScreenInput as the default macOS capture backend with a ScreenCaptureKit-based implementation (macOS 14+; AVFoundation remains as fallback for older systems or SCKit setup failures).
Capture runs through SCScreenshotManager polling paced to the session frame rate, rather than an SCStream.
Two reasons:
Polling a single consistent source resolves both, at capture latency measured equal to AVFoundation. Since polling goes through the zero-copy VideoToolbox path by default, there's no CPU cost to it.
Fixes
Testing
Tested on macOS 26 (Apple silicon, VM, Mac Studio and MBP): cursor reappearance, static-screen behavior, display reconfiguration, latency stat parity, extended interactive sessions at 1080p60/4K60 with hardware (VideoToolbox) and software (x264) encoders.
Screenshot
Issues Fixed or Closed
Closes #3433
Roadmap Issues
Type of Change
Checklist
AI Usage
See our AI usage policy.