Skip to content

Commit b65fd7e

Browse files
committed
fix: harden window streaming per adversarial review
- handle streamDidBecomeInactive so a closed captured window reports a stream death instead of leaving isRunning() true forever - decide death reports synchronously on the delegate thread: a death that precedes a concurrent stop() can no longer be suppressed by the async hop racing the stop flag (delivery stays serialized and inside the stop() drain) - guard startStream behind macOS 14 (pointPixelScale/contentRect) and report Unsupported below it instead of crashing at first use - validate WindowStreamOptions.maxFps at the facade (finite, 1-1000) before the int32 timescale conversion - derive frame age from SCStreamFrameInfoDisplayTime (mach absolute ticks, same clock as mach_absolute_time) with the PTS-vs-host-clock math as fallback - make CoreVideo a PUBLIC link dependency: the installed robot/mac bridge hands consumers a CVPixelBufferRef they lock via CoreVideo - widen the build-directory ignore to build*/
1 parent cd08cd8 commit b65fd7e

6 files changed

Lines changed: 93 additions & 28 deletions

File tree

‎.gitignore‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
/example/cmake-build-debug
99

1010

11-
/build
11+
/build*
1212

1313
/.opencode
1414
/AGENTS.md

‎CMakeLists.txt‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -149,15 +149,19 @@ if(APPLE)
149149
find_library(CORE_MEDIA CoreMedia REQUIRED)
150150
find_library(CORE_VIDEO CoreVideo REQUIRED)
151151
find_library(APP_KIT AppKit REQUIRED)
152-
# Platform framework types appear only in the platform .cpp/.mm files
153-
# (behind the backend interfaces), so the dependencies are PRIVATE. Carbon
154-
# is intentionally NOT used.
155-
target_link_libraries(robot PRIVATE
156-
${APPLICATION_SERVICES}
157-
${SCREEN_CAPTURE_KIT}
158-
${CORE_MEDIA}
159-
${CORE_VIDEO}
160-
${APP_KIT})
152+
# Most platform framework types appear only in the platform .cpp/.mm files
153+
# (behind the backend interfaces), so those dependencies are PRIVATE.
154+
# CoreVideo is PUBLIC because the installed robot/mac/VideoFrame.h bridge
155+
# hands consumers a CVPixelBufferRef they lock through CoreVideo calls.
156+
# Carbon is intentionally NOT used.
157+
target_link_libraries(robot
158+
PUBLIC
159+
${CORE_VIDEO}
160+
PRIVATE
161+
${APPLICATION_SERVICES}
162+
${SCREEN_CAPTURE_KIT}
163+
${CORE_MEDIA}
164+
${APP_KIT})
161165
elseif(WIN32)
162166
# user32/gdi32 for SendInput, hooks, GDI capture. Shcore and the DPI context
163167
# API are resolved at runtime (see WinDpi.cpp / WinScreenBackend.cpp), so they

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ robot-cpp is built around the distinctions those libraries collapse:
2828

2929
| Platform | Backend | Injection | Capture | Window streaming | Recording |
3030
| ----------------- | ----------------------------- | ------------------------------- | ------- | ---------------- | --------- |
31-
| macOS | Quartz + ScreenCaptureKit | yes (needs Accessibility) | yes (needs Screen Recording) | yes (needs Screen Recording) | yes (needs Accessibility) |
31+
| macOS | Quartz + ScreenCaptureKit | yes (needs Accessibility) | yes (needs Screen Recording) | yes (macOS 14+, needs Screen Recording) | yes (needs Accessibility) |
3232
| Windows | SendInput + GDI | yes | yes | no | yes |
3333
| Linux (X11) | XTest + XRandR + XRecord | yes | yes | no | yes |
3434
| Linux (Wayland) | uinput (opt-in) | keyboard + relative mouse only | no | no | no |

‎include/robot/WindowStream.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ class IWindowStreamBackend;
1212
}
1313

1414
struct WindowStreamOptions {
15-
double maxFps = 60.0;
15+
double maxFps = 60.0; // must be finite and in [1, 1000]
1616
bool showsCursor = true;
1717
};
1818

@@ -24,7 +24,9 @@ using VideoFrameCallback = std::function<void(const VideoFrame&)>;
2424

2525
// Invoked on the same capture thread (serialized with frame delivery) when
2626
// the stream dies outside stop(): the window closed, the OS revoked capture,
27-
// the display reconfigured. After it fires, isRunning() is false and no
27+
// the display reconfigured. Fires at most once; a death that happens before a
28+
// concurrent stop() is always reported, while stop() winning the race means a
29+
// clean stop and no error. After it fires, isRunning() is false and no
2830
// further frames arrive. Same rules as VideoFrameCallback.
2931
using StreamErrorCallback = std::function<void(const Error&)>;
3032

‎src/common/Windows.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#include "robot/Windows.h"
22

3+
#include <cmath>
4+
#include <format>
35
#include <utility>
46

57
#include "robot/backend/IWindowBackend.h"
@@ -69,6 +71,12 @@ std::expected<WindowStream, Error> Windows::stream(
6971
Error::invalidArgument("stream() requires a frame callback")
7072
);
7173
}
74+
if (!std::isfinite(options.maxFps) || options.maxFps < 1.0 ||
75+
options.maxFps > 1000.0) {
76+
return std::unexpected(Error::invalidArgument(std::format(
77+
"maxFps must be a finite value in [1, 1000], got {}", options.maxFps
78+
)));
79+
}
7280

7381
auto stream = backend_->startStream(
7482
windowId, options, std::move(onFrame), std::move(onError)

‎src/platform/macos/MacWindowBackend.mm‎

Lines changed: 66 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
#import <CoreMedia/CoreMedia.h>
66
#import <ScreenCaptureKit/ScreenCaptureKit.h>
77

8+
#include <mach/mach_time.h>
9+
810
#include <atomic>
911
#include <chrono>
1012
#include <memory>
@@ -52,6 +54,7 @@ - (void)stream:(SCStream*)stream
5254
double scaleFactor = 0.0;
5355
double contentScale = 0.0;
5456
CGRect contentRect = CGRectZero;
57+
std::optional<std::uint64_t> displayTicks;
5558
CFArrayRef attachments =
5659
CMSampleBufferGetSampleAttachmentsArray(sampleBuffer, false);
5760
if (attachments != nullptr && CFArrayGetCount(attachments) > 0) {
@@ -88,19 +91,45 @@ - (void)stream:(SCStream*)stream
8891
if (contentRectDict != nullptr) {
8992
CGRectMakeWithDictionaryRepresentation(contentRectDict, &contentRect);
9093
}
94+
const auto* displayTimeNumber =
95+
static_cast<CFNumberRef>(CFDictionaryGetValue(
96+
attachment, (__bridge CFStringRef)SCStreamFrameInfoDisplayTime
97+
));
98+
if (displayTimeNumber != nullptr) {
99+
std::int64_t ticks = 0;
100+
CFNumberGetValue(displayTimeNumber, kCFNumberSInt64Type, &ticks);
101+
if (ticks > 0) displayTicks = static_cast<std::uint64_t>(ticks);
102+
}
91103
}
92104

93105
CVPixelBufferRef pixels = CMSampleBufferGetImageBuffer(sampleBuffer);
94106
if (pixels == nullptr) return;
95107

96108
// Stamp the frame with when it was presented, not when this callback ran -
97109
// a sample delayed inside ScreenCaptureKit must not look fresh to
98-
// freshness-gated consumers. Presentation time and the host clock share the
99-
// same time base, so the age subtraction is exact.
110+
// freshness-gated consumers. SCStreamFrameInfoDisplayTime is the display
111+
// time in mach absolute ticks, the same clock mach_absolute_time() reads,
112+
// so the age subtraction stays within one clock domain. When the attachment
113+
// is absent, fall back to the sample's presentation timestamp measured
114+
// against the CoreMedia host clock it is produced on.
100115
auto capturedAt = std::chrono::steady_clock::now();
101-
const CMTime presentation =
102-
CMSampleBufferGetPresentationTimeStamp(sampleBuffer);
103-
if (CMTIME_IS_VALID(presentation)) {
116+
if (displayTicks) {
117+
static const double nanosPerTick = [] {
118+
mach_timebase_info_data_t timebase{};
119+
mach_timebase_info(&timebase);
120+
return static_cast<double>(timebase.numer) /
121+
static_cast<double>(timebase.denom);
122+
}();
123+
const double ageNanos =
124+
(static_cast<double>(mach_absolute_time()) -
125+
static_cast<double>(*displayTicks)) *
126+
nanosPerTick;
127+
capturedAt -= std::chrono::duration_cast<std::chrono::steady_clock::duration>(
128+
std::chrono::duration<double, std::nano>(ageNanos)
129+
);
130+
} else if (const CMTime presentation =
131+
CMSampleBufferGetPresentationTimeStamp(sampleBuffer);
132+
CMTIME_IS_VALID(presentation)) {
104133
const CMTime hostNow = CMClockGetTime(CMClockGetHostTimeClock());
105134
const double ageSeconds =
106135
CMTimeGetSeconds(CMTimeSubtract(hostNow, presentation));
@@ -135,21 +164,33 @@ - (void)stream:(SCStream*)stream
135164
));
136165
}
137166

138-
- (void)stream:(SCStream*)stream didStopWithError:(NSError*)error {
139-
const auto chan = self->channel;
167+
// Shared death path for didStopWithError and streamDidBecomeInactive. The
168+
// active flag flips synchronously on the delegate thread, so a death that
169+
// happens before a concurrent stop() wins the exchange and its report cannot
170+
// be suppressed; delivery still hops onto the sample handler queue so it is
171+
// serialized with frame delivery and covered by stop()'s drain.
172+
static void reportStreamDeath(
173+
const std::shared_ptr<robot::mac::StreamChannel>& chan, NSString* message
174+
) {
140175
if (chan == nullptr || chan->queue == nil) return;
141-
// Delegate callbacks arrive on ScreenCaptureKit's own queue; hop onto the
142-
// sample handler queue so error delivery is serialized with frame delivery
143-
// and covered by stop()'s drain. Only report deaths the caller did not
144-
// initiate - stop() deactivates the channel first.
145-
NSString* message = error.localizedDescription;
176+
if (!chan->active.exchange(false)) return; // stop() already initiated
177+
if (chan->onError == nullptr) return;
146178
dispatch_async(chan->queue, ^{
147-
if (chan->active.exchange(false) && chan->onError != nullptr) {
148-
chan->onError(robot::Error::captureFailed(message.UTF8String));
149-
}
179+
chan->onError(robot::Error::captureFailed(message.UTF8String));
150180
});
151181
}
152182

183+
- (void)stream:(SCStream*)stream didStopWithError:(NSError*)error {
184+
reportStreamDeath(self->channel, error.localizedDescription);
185+
}
186+
187+
- (void)streamDidBecomeInactive:(SCStream*)stream {
188+
reportStreamDeath(
189+
self->channel,
190+
@"stream became inactive (the captured window closed or capture ended)"
191+
);
192+
}
193+
153194
@end
154195

155196
namespace robot::mac {
@@ -379,6 +420,16 @@ void stop() override {
379420
const std::uint32_t windowId, const WindowStreamOptions& options,
380421
VideoFrameCallback onFrame, StreamErrorCallback onError
381422
) {
423+
// SCContentFilter.pointPixelScale and .contentRect - the metadata the whole
424+
// coordinate contract rests on - exist since macOS 14.
425+
if (@available(macOS 14.0, *)) {
426+
// Streaming is available; continue below.
427+
} else {
428+
return std::unexpected(
429+
Error::unsupported("window streaming requires macOS 14 or newer")
430+
);
431+
}
432+
382433
auto contentResult = fetchShareableContent();
383434
if (!contentResult) return std::unexpected(contentResult.error());
384435

0 commit comments

Comments
 (0)