fix(win): pace the capture loop to a deadline instead of sleeping a full period - #331
Conversation
…ull period The video writer ended each iteration with `sleep_for(1/fps)`, after the frame had already been captured, converted and submitted to the sink writer. The real period was therefore `work + 1/fps`, never `1/fps`: at 1080p the work is ~11 ms, so a 30 fps recording ran at 22. Keep a `nextFrameDue` deadline and `sleep_until` it. When a frame runs long the deadline is resynced to now rather than carried forward, so a stall costs the frames it costs instead of being repaid as a burst of catch-up frames -- the same rule the webcam cadence a few lines above already follows. Measured on a Ryzen 5 7520U / Radeon iGPU, 15 s display capture at 1080p with 30 fps requested: before 317 frames / 14.672 s = 21.6 fps after 440 frames / 14.675 s = 30.0 fps The frame count comes from the new `[pacing]` line on stderr because the recording cannot answer this question: the sink writer re-times its output to nominal CFR, so `nb_frames / duration` reads exactly 30.000 whatever the loop actually did -- it stayed at 30 even with a 300 ms stall injected through OPENSCREEN_WGC_TEST_STALL_READBACK_MS. The line sits next to the existing [stop-timing] instrumentation and is picked up by the diagnostic tool's stderr capture. macOS and Linux do not share the pattern: ScreenCaptureKit paces the callbacks itself via `minimumFrameInterval`, and the pipewire helper derives its output frame index from the wall clock.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe video writer now uses a steady-clock deadline schedule for frame pacing. It resynchronizes after processing overruns and logs final frame-count and elapsed-time diagnostics. ChangesVideo Frame Pacing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 908-912: Update the pacing metric around the writer loop and
stopVideoWriter() so teardown work is excluded from elapsed_ms. Record the
stop-request timestamp before WGC, audio, and webcam shutdown, or establish a
writer-local timing boundary, and use that boundary instead of
control.recordingStartedAt while preserving the existing frame logging.
- Around line 898-906: Update the capture loop’s paused-to-running transition
around control.paused and nextFrameDue so resuming resets the deadline to now
plus frameDuration before submitting a frame. Track the prior paused state as
needed, keeping the existing slow-frame resynchronization for normal running
frames and ensuring the first post-resume frame is separated by one frame
interval.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d97da350-5504-40e8-b1ec-1dff71530836
📒 Files selected for processing (1)
electron/native/wgc-capture/src/main.cpp
| nextFrameDue += frameDuration; | ||
| const auto now = std::chrono::steady_clock::now(); | ||
| if (nextFrameDue < now) { | ||
| // Fell behind (slow frame, or waiting on the first one). Resync | ||
| // to now rather than firing a burst of catch-up frames, same as | ||
| // the webcam cadence above. | ||
| nextFrameDue = now; | ||
| } | ||
| std::this_thread::sleep_until(nextFrameDue); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the deadline when a pause ends.
When control.paused is true, nextFrameDue becomes stale. After a long pause, Line 904 sets it to now, and Line 906 does not wait. Because latestFrameTexture is still set, the loop submits two frames without one frameDuration interval after resume. Reset the deadline on the paused-to-running transition separately from slow-frame resynchronization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/native/wgc-capture/src/main.cpp` around lines 898 - 906, Update the
capture loop’s paused-to-running transition around control.paused and
nextFrameDue so resuming resets the deadline to now plus frameDuration before
submitting a frame. Track the prior paused state as needed, keeping the existing
slow-frame resynchronization for normal running frames and ensuring the first
post-resume frame is separated by one frame interval.
| std::cerr << "[pacing] frames=" << frameIndex << " elapsed_ms=" | ||
| << std::chrono::duration_cast<std::chrono::milliseconds>( | ||
| std::chrono::steady_clock::now() - control.recordingStartedAt) | ||
| .count() | ||
| << std::endl; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exclude shutdown time from the pacing metric.
The writer logs this interval after control.recordingStartedAt, while the main thread can perform WGC, audio, and webcam shutdown before stopVideoWriter() joins it. If the writer is still running during that work, teardown time is included in elapsed_ms, which under-reports loop pacing. Record the stop-request timestamp before teardown or use a writer-local measurement boundary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/native/wgc-capture/src/main.cpp` around lines 908 - 912, Update the
pacing metric around the writer loop and stopVideoWriter() so teardown work is
excluded from elapsed_ms. Record the stop-request timestamp before WGC, audio,
and webcam shutdown, or establish a writer-local timing boundary, and use that
boundary instead of control.recordingStartedAt while preserving the existing
frame logging.
1 participant
The Windows capture helper delivers about 75% of the requested frame rate.
writeVideoFramesended each iteration withsleep_for(frameDuration), after the frame had been captured, converted and submitted to the sink writer. So the real period waswork + 1/fps, never1/fps. At 1080p the work is ~11 ms: 33.3 + 11 = 44.4 ms, i.e. 22.5 fps for a 30 fps recording.The fix keeps a
nextFrameDuedeadline andsleep_untils it. If a frame ran long the deadline is resynced to now rather than carried forward, so a stall costs the frames it costs instead of being repaid as a burst of catch-up frames — the same rule the webcam cadence a few lines above already follows.Measured
Same instrumented build, only the pacing differing. Ryzen 5 7520U / Radeon iGPU, Windows 11 26200, 15 s display capture at 1080p, 30 fps requested:
Why the frame count comes from stderr
nb_frames / durationon the recording cannot answer this question. The sink writer re-times its output to nominal CFR, so ffprobe read exactly 30.000 fps on every run here — including a run with a 300 ms stall injected throughOPENSCREEN_WGC_TEST_STALL_READBACK_MS, where the loop was managing about 3 iterations a second. The new[pacing] frames=N elapsed_ms=Mline on stop is the ground truth; it sits next to the existing[stop-timing]instrumentation and is captured by the diagnostic tool's stderr collection:Other platforms
Neither shares the pattern:
configuration.minimumFrameIntervaland writes from the ScreenCaptureKit delegate callback — the OS paces it, there is no sleep loop.elapsed * fps / 1e9incapture.rs), so it is deadline-based by construction.The two cursor samplers (
cursor-sampler.cpp, the macOS cursor helper) do sleep a fixed interval after their work, but that work is aGetCursorInfoplus a stdout line, so the drift is a couple of percent of sample density rather than 25% of the frame rate. Left alone.Notes
Pre-existing on main — not introduced by #305 or #306, and independent of the #252 stop-hang work. The native helpers have no PR CI, so build.yml needs a manual dispatch on this branch to get a compiled artifact.
Summary by CodeRabbit