fix(wgc): pull-based frame delivery to stop CopyResource wedging the stop path - #306
fix(wgc): pull-based frame delivery to stop CopyResource wedging the stop path#306abduznik wants to merge 5 commits into
Conversation
…stop path WgcSession no longer pushes frames via the WGC FrameArrived event onto a callback thread of its own. writeVideoFrames now pulls each frame with session.tryGetNextFrame() on its own thread and does the CopyResource itself, matching Chromium's WgcCaptureSession (modules/desktop_capture/win/wgc_capture_session.cc), which comments "we don't listen for the FrameArrived event" for the same reason. Root cause: onFrameArrived held the shared frame-state mutex across CopyResource. On hardware where that call wedges inside the display driver, the lock is gone until the process exits, and the video-writer thread blocks trying to acquire the same lock -- so both wgc-quiesce's drain and video-writer-join hang, and the shutdown watchdog TerminateProcess()es the helper before encoder-finalize ever runs. Confirmed with the standalone diagnostic tool: wgc-quiesce hung 5s (drained=false), video-writer-join was abandoned at 13s, 0-byte MP4 -- under both the default and preferSoftwareEncoder paths, so this is not specific to one encoder pipeline. OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK=1 restores the previous push-based implementation (kept alongside the new one in WgcSession) as a rollback lever, since the pull-based path has only been verified on one machine so far. Re-running the same diagnostic tool with the flag set reproduces the original hang exactly (video-writer-join abandoned at 8020ms), confirming the flag is a working escape hatch and not just a comment. Refs getopenscreen#252, getopenscreen#305.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWGC capture now uses pull-based frame retrieval on the video-writer thread by default. An environment-controlled legacy callback path remains available. Startup and shutdown ordering now follow writer-thread ownership. ChangesWGC capture pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VideoWriter
participant WgcSession
participant WGCFramePool
VideoWriter->>WgcSession: tryGetNextFrame
WgcSession->>WGCFramePool: Retrieve frame
WGCFramePool-->>WgcSession: Return texture and timestamp
WgcSession-->>VideoWriter: Return retained frame
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 894-899: Update the comment above captureVideoSample to qualify
that this thread is the only writer on the pull path. Document that on the
legacy path the WGC callback writes latestFrameTexture under frameMutex, and
that the lock held here protects the readback; preserve the existing legacy
locking.
- Around line 1211-1220: Reorder shutdown so encoder finalization completes
before WGC teardown: update both shutdown paths in
electron/native/wgc-capture/src/main.cpp at lines 1211-1220 and 1094-1103 to
call encoder.finalize()/webcamEncoder.finalize() before session.stop(),
preserving the existing stop-step logging. In
electron/native/wgc-capture/src/wgc_session.cpp lines 457-460, make no direct
change; WgcSession::stop() remains responsible for resetting device/context
pointers after finalization.
- Around line 747-748: Explicitly unlock legacyLock immediately after the scoped
block ending near the legacy frame-processing section and before the submission
section. Ensure both submitVideoSample calls execute without holding frameMutex,
while preserving the existing lock behavior inside the block.
In `@electron/native/wgc-capture/src/wgc_session.cpp`:
- Around line 334-364: Update the handler around frameCallback_ retrieval so
callbacksInFlight_ is incremented for every handler that pulls a frame,
regardless of whether the callback is null. Move InFlightGuard construction
outside the callback conditional so it remains active through frame.Close(),
while preserving callback invocation only when callback is non-null and ensuring
the guard is released after all frame cleanup.
🪄 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: 314476d3-190a-44f1-ae10-17e5fc469030
📒 Files selected for processing (3)
electron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/wgc_session.cppelectron/native/wgc-capture/src/wgc_session.h
- legacyLock (main.cpp writeVideoFrames) outlived the block it was scoped for, so on the legacy callback path frameMutex stayed held across submitVideoSample -- reintroducing the getopenscreen#115 hazard for that path. Unlock explicitly before submission. - Qualify the "only writer of latestFrameTexture" comment: true on the pull-based path only, not the legacy path, where the WGC callback thread also writes it under frameMutex. - Reorder shutdown so encoder.finalize()/webcamEncoder.finalize() run before session.stop(). Not a live bug -- MFEncoder holds its own ComPtr<ID3D11Device>/ComPtr<ID3D11DeviceContext>, so COM reference counting already kept things alive -- but the old order relied on that implicitly, and finalizing first removes the dependency structurally instead of documenting around it. - onFrameArrived only counted a handler as in-flight when frameCallback_ was non-null, leaving frame.Close() on the no-callback path uncounted and outside quiesceLegacyCallback()'s drain. Count unconditionally. Re-verified after these changes with the standalone diagnostic tool: default path still stops in ~85ms, legacy-flag path still reproduces the original hang unchanged (confirms the lock-scope fix didn't affect the flag's intended rollback behavior).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/wgc_session.cpp`:
- Around line 334-359: Move callback capture and callbacksInFlight_ registration
under callbackMutex_ to the start of the handler, before TryGetNextFrame(), and
construct InFlightGuard before acquiring or creating the frame so cleanup is
covered on exceptions. If the captured frameCallback_ is null, return
immediately without accessing sender or the frame pool; otherwise preserve the
existing frame processing and callback behavior.
🪄 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: 18e09673-ce26-4382-97fb-8ea6bb519fce
📒 Files selected for processing (2)
electron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/wgc_session.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/native/wgc-capture/src/main.cpp
…he frame pool CodeRabbit's second pass caught what the first fix (9a0c4e4) missed: callbacksInFlight_ was incremented after TryGetNextFrame()/Surface()/ GetInterface() already ran, not before. quiesceLegacyCallback() could still observe callbacksInFlight_ == 0 and return while a handler was mid-acquisition, letting stop() close framePool_ concurrently with this handler's use of it. Move the callback capture and counter increment to before TryGetNextFrame() is called at all, so the entire window this handler spends touching the pool is covered by the drain. Also closes a frame.Close() gap on the GetInterface-failure path noticed while reordering. Re-verified: default path still stops in ~83ms, legacy-flag path still reproduces the original hang unchanged.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
electron/native/wgc-capture/src/wgc_session.cpp (1)
342-356: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn before frame-pool access when
frameCallback_is null.At Line 352, a handler can block on
callbackMutex_whilequiesceLegacyCallback()clears the callback and observescallbacksInFlight_ == 0. The handler can then increment the counter and callsender.TryGetNextFrame()at Line 356 after quiesce returns.stop()can closeframePool_during that access.If
frameCallback_is null, return while holdingcallbackMutex_. IncrementcallbacksInFlight_only for a handler that captured a non-null callback.Proposed fix
{ std::scoped_lock lock(callbackMutex_); callback = frameCallback_; - // Counted under the same lock quiesceLegacyCallback() clears the - // callback under, so once it has cleared it no new handler can start - // and the counter it then drains cannot go back up. Counted - // unconditionally (not only when callback is non-null): a handler - // that observes a cleared callback still touches the frame pool - // below and needs to be covered by the drain too. + if (!callback) { + return; + } callbacksInFlight_ += 1; }🤖 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/wgc_session.cpp` around lines 342 - 356, Update the callback acquisition block in the frame handler to return immediately while holding callbackMutex_ when frameCallback_ is null, before any frame-pool access. Only increment callbacksInFlight_ and create InFlightGuard after capturing a non-null callback, preserving the existing guarded path for active callbacks.
🤖 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.
Duplicate comments:
In `@electron/native/wgc-capture/src/wgc_session.cpp`:
- Around line 342-356: Update the callback acquisition block in the frame
handler to return immediately while holding callbackMutex_ when frameCallback_
is null, before any frame-pool access. Only increment callbacksInFlight_ and
create InFlightGuard after capturing a non-null callback, preserving the
existing guarded path for active callbacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4823697f-7d48-4fb5-8b26-f2f766db106c
📒 Files selected for processing (1)
electron/native/wgc-capture/src/wgc_session.cpp
…ack is null CodeRabbit's third pass on onFrameArrived: a null-callback handler had nothing useful to do with a frame, but still called TryGetNextFrame() and incremented callbacksInFlight_. Return immediately, before either, once frameCallback_ is observed null under callbackMutex_ -- there is no reason for that handler to touch the pool at all. The `if (callback)` guard before invoking it is now dead code (the only path reaching that point already has a non-null callback) and is removed. Re-verified: default path still stops in ~84ms, legacy-flag path still reproduces the original hang unchanged.
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis is the useful part, and it is well argued. Pulling on the consumer's own thread is the right shape, and citing Chromium's capturer for the same reason is the right precedent. The write-up is unusually honest about what was and was not exercised, which made it reviewable.
On whether this replaces #305: I do not think it does, and I do not think you have to choose. The two touch different stages of the same pipeline:
WGC frame pool --[delivery]--> latestFrameTexture --[encoder input]--> sink writer
^ ^
#306 #305
#305 removes Map(D3D11_MAP_READ) from the encoder input. #306 removes the shared lock by removing the second thread. They collide textually in main.cpp, not functionally.
The evidence says each one leaves the other's wedge standing:
- On @Seb1900's machine, #305 took display and window capture from a 13 s hang to a 105 ms stop. So the
Mapwedge was real and #305 killed it. - On yours, #305 still hangs in
CopyResource. So there is a second wedge #305 does not touch, which is what this PR removes. - But this PR alone leaves
captureVideoSample'sMap(D3D11_MAP_READ)on the video-writer thread. If that wedges — which is exactly what was observed on Seb1900's hardware —stopVideoWriter()joins a thread that never returns andvideo-writer-joinis abandoned again. "The failure stays local" is true, but local here is the one thread whose join is the abandoned step.
So my read is: two distinct wedges, one per PR, both real, neither sufficient alone. That argues for landing both rather than picking, with this one rebased on top of #305 once that merges — the conflict is in the frame loop you rewrote, which you know better than the rebase would.
Four things below. Only the first is a behaviour change I would want fixed before merge; the rest are worth a look but would not block.
On the rollback flag: keeping it is defensible for one release given the pull path has one machine behind it, and I would rather have your honest diff than a smaller one. But it preserves the exact code path that causes #252, so please open a follow-up issue to remove it — your own comment already says "remove it once the pull-based path has enough field time", and that ages better as an issue than as a comment.
One note on the artifact: main has moved since you opened this. Your branch has picked it up, so your CI build now links the helper against the static CRT (/MT, commit 7f68e9a) — a different binary from the one you measured on. Nothing about your diagnosis depends on it, but if you re-run the diagnostic tool, use a fresh build so we are not comparing across that change.
| // for a first frame to arrive -- there is no separate WGC callback thread | ||
| // left to deliver one on its own. | ||
| if (audioMixer) { | ||
| audioMixer->beginTimeline(); |
There was a problem hiding this comment.
This shifts the audio timeline origin ahead of the video's, which was not the case before.
The three tracks anchor to three different clocks:
- screen video:
firstFrameTimestampHns, the first WGCSystemRelativeTimethe writer sees (line 838-839) - audio:
audioMixer->beginTimeline(), which clears the queues and zeroesemittedFrames_ - separate webcam file:
control.recordingStartedAt(line 854)
Before this PR all three were established after the first frame had arrived — the old code waited on the condition variable first, then called beginTimeline() and stamped recordingStartedAt. Here both move ahead of startVideoWriter(), so they are stamped before WGC has delivered anything. Audio and the separate webcam file now lead the screen video by the whole time-to-first-frame: thread start, StartCapture(), and the first FrameArrived. The 10 s ceiling below is the worst case; typical is tens of milliseconds, which is already inside the range where audio leading video reads as a lip-sync error.
The reordering itself is necessary — the writer thread is the producer now, so it has to be running before anything can wait for a first frame. It is only these two stamps that need to stay behind.
Moving them back below the wait is not quite enough on its own, though: the writer reads control.recordingStartedAt at line 854 as soon as it has a frame, so main() stamping it afterwards races the writer's first iteration and would give the webcam branch a default-constructed time_point. The clean version is to have the writer establish both at the moment it captures its first frame — where it already sets firstFrameTimestampHns — and let main() only wait. That restores the old invariant exactly (timeline origin is the first video frame) and removes the race instead of narrowing it.
Worth confirming with a recording of something with a sharp transient — a clap, or any hard audio/video cut — rather than by eye on desktop footage.
| latestFrameTimestampHns = legacyLatestFrameTimestampHns; | ||
| } else { | ||
| if (control.paused) { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); |
There was a problem hiding this comment.
Pause behaves differently now, in a way that shows on resume.
On the legacy path, onFrameArrived still called TryGetNextFrame() and Close()d the frame; it was the callback that returned early on paused, after the frame had already been consumed and returned to the pool. So frames kept flowing and being discarded during a pause.
Here, tryGetNextFrame() is not called at all while paused, so nothing is consumed. On resume, the first TryGetNextFrame() returns whatever WGC last queued — a frame captured during the pause. Its SystemRelativeTime falls inside the paused window, so after - control.pausedDurationHns() it lands behind lastEncodedVideoTimestampHns and gets pushed forward by the monotonic guard at line 845. The timestamp ends up correct; the pixels are one frame stale.
One stale frame at each resume is minor, but it is a visible artifact on a pause-heavy recording and it is new. Calling tryGetNextFrame() and dropping the result while paused would keep the old behaviour for two lines.
| // be mid-CopyResource on across the two-call boundary. currentFrame_ | ||
| // holds the reference that keeps *outTexture valid until this class's | ||
| // next call or stop() closes it. | ||
| currentFrame_ = frame; |
There was a problem hiding this comment.
Holding the frame pins one of only two pool buffers, permanently.
The reasoning for holding it is right — the caller needs the texture to stay valid across the return, and Direct3D11CaptureFrame's reference is what guarantees that. But both initialize() overloads create the pool with CreateFreeThreaded(..., 2, ...), and with one frame always checked out, WGC is left rotating through a single buffer for the entire recording. There is no slack: any jitter in the writer's cadence (a slow WriteSample, a scheduling hiccup) lands while WGC has nowhere to put the next frame, and it drops it.
The push model never had this problem — the callback consumed and closed each frame immediately, so both buffers stayed available.
Probably worth a third buffer, which costs one texture and removes the constraint entirely. Either way it is measurable rather than theoretical: a 60 fps display recording, count encoded frames against elapsed wall time, this branch versus main. If the delivered rate holds at 60, ignore me.
(Combined with the pause behaviour noted in main.cpp, the pinned buffer also lasts for the whole duration of a pause, not just a frame interval.)
| // the shared D3D context at exactly the moment we can least afford a stall. | ||
| beginStopStep("wgc-quiesce", stepBudgetMs); | ||
| // The drain outcome decides the shape of the whole rest of the shutdown: | ||
| // a callback that never came back makes wgc-session-close skip the device |
There was a problem hiding this comment.
Removing the step is right; losing the line from the trace is a real cost.
The step genuinely has nothing left to do on the pull path — the writer's own loop exit is the producer stopping, exactly as your comment says. No argument there, and I checked: nothing in the TypeScript or the diagnostic tooling parses wgc-quiesce, so no consumer breaks.
The cost is diagnostic. Every field report on #252 so far, from two different machines, is read through this pair of lines:
[stop-timing] step=wgc-quiesce elapsed_ms=5002 drained=false
[stop-timing] step=video-writer-join elapsed_ms=13047 phase=abandoned
drained=false is what tells us a producer sat on the frame lock rather than the writer simply being slow. On this branch a hang produces only the second line, and the first piece of evidence disappears — on the one bug where we are still collecting traces from users, and where your diagnosis and #305's differ precisely on which thread is stuck.
Suggestion: keep emitting a line for the step with a value that says the question no longer applies — drained=n/a or producer=inline — so an old trace and a new one can still be laid side by side. Cheap, and it keeps the vocabulary the reporters already use.
|
Correction to my review above. I wrote that citing Chromium's capturer "for the same reason" was the right precedent. I hadn't opened the file. The comment in full: // Cast to FramePoolStatics2 so we can use CreateFreeThreaded and avoid the
// need to have a DispatcherQueue. We don't listen for the FrameArrived event,
// so there's no difference.It is about avoiding a Your design argument is untouched by this: pulling removes the second thread, and with it the lock that thread forces on everyone else. That stands on its own, and honestly it is stronger without the borrowed authority, because it is about our threading model rather than theirs. But One more thing while I'm correcting myself: they use |
2 participants
Reported by
@LuniteLang-Sys in #292: "timed out waiting for native windows capture to stop. Record could not save."
I hit the identical error and dug in. Root cause and fix below.
Root cause
onFrameArrived(the WGCFrameArrivedcallback) holds the shared frame-state mutex acrossCopyResource. On hardware where that call wedges inside the display driver, the lock is gone until the process exits. The video-writer thread then blocks trying to acquire the same lock, so bothwgc-quiesce's drain andvideo-writer-joinhang, and the shutdown watchdogTerminateProcess()s the helper beforeencoder-finalizeever runs. That's the 0-byte MP4.Confirmed with the standalone diagnostic tool (
scripts/diagnostic-tool) on my machine:wgc-quiescehangs 5s (drained=false),video-writer-joingets abandoned at 13s. This happens under both the default andpreferSoftwareEncoderpaths — it's not specific to one encoder pipeline.What #254 and #305 do, and why they don't cover this
Map/Unmapreadback with a GPU DXGI path, becauseUnmapwas the call observed wedging on the original [Bug]: v1.8.0 native Windows recorder still hangs on stop; next attempt says capture is not running #252 reporter's multi-adapter machine. I built and ran it — on my hardware (single GPU, no virtual adapters) it still hangs, in the same place, because the wedge is inCopyResourceinsideonFrameArrived, upstream of whichever readback path fix(wgc): GPU DXGI encode path for Windows capture, with a fallback at every step #305 touches. Neither PR looked at theFrameArrivedcallback itself.What this PR does
Removes the callback thread instead of trying to make its lock safer.
WgcSessionno longer registersFrameArrivedby default.writeVideoFramespulls each frame itself withsession.tryGetNextFrame(), on its own schedule, and does theCopyResourcethere. This is the same design Chromium's WGC capturer uses (modules/desktop_capture/win/wgc_capture_session.cc), which literally comments "we don't listen for the FrameArrived event, so there's no difference" and pulls viaTryGetNextFrame()instead, for this exact reason.With no separate callback thread, there's no second thread for a wedged
CopyResourceto take a lock down with it. If the call still wedges, it now only blocks the one thread already responsible for noticingstopRequestedand giving up — the failure stays local instead of cascading intovideo-writer-join.Net diff is smaller than it looks at a glance because the pull-based design deletes the mutex, the in-flight callback counter, and the bounded-drain logic that existed only to make the push model's shutdown safe. None of that is needed when there's nothing pushing.
Why this PR is long
Two reasons, and I want to be upfront about both:
OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK=1restores the previous push-based implementation, which is kept alongside the new one inWgcSessionrather than deleted. The pull-based path is only verified on my hardware so far — if it regresses on some driver/GPU combination I don't have, this flag gets someone back to the previously-shipped behavior without waiting on a release. I verified the flag is a real escape hatch, not a decorative one: running the same diagnostic tool with it set reproduces the original hang exactly (video-writer-joinabandoned at 8020ms). This roughly doubles the diff versus a flag-less version.session.stop()moved to run right afterstopVideoWriter()instead of before it, and the stalewgc-quiescestep is gone. That touches more of the shutdown sequence inmain.cppthan the frame-delivery change alone would.I'd rather ship the flag and the honest diff size than a smaller PR that leaves people with no way back if I've missed something.
Testing
Machine: Windows 10 22H2, Ryzen 5 4500, RTX 4060 Ti (single GPU, no virtual/remote-desktop display adapters — a different profile than the original #252 reporter's multi-adapter machine, which is useful: this isn't a multi-adapter-only bug).
Built
wgc-capture.exelocally (MSVC 14.44, Windows SDK 26100) and drove it directly withscripts/diagnostic-tool/diagnostic.mjs, bypassing Electron:ftyp/moov/mdatatoms and is playable.preferSoftwareEncoder: true: same result, confirms the fix isn't encoder-path-specific.OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK=1: reproduces the original hang exactly, confirming the flag genuinely restores prior behavior.wgc-capture.exe) and did a full manual pass through the actual app: started a display recording, ran it for about 2 minutes, hit stop (immediate, no hang), opened the recording in the editor, and it loaded and played correctly — no dropped frames or corruption noticed over that length.Not tested: webcam-overlay recording, real window capture (vs. display capture — the diagnostic tool can't pass a real HWND), recordings longer than a few minutes, or any hardware other than the one machine above. All of those go through the same
writeVideoFramesloop so I'd expect them to work, but I want to say plainly what's actually been exercised versus what's just architecturally covered.Type of change
Desktop impact
Summary by CodeRabbit
Performance
Bug Fixes