From 086055d5b95eaf893d60e5b6a0433d852bcc58e3 Mon Sep 17 00:00:00 2001 From: abduznik <85239936+abduznik@users.noreply.github.com> Date: Sat, 8 Aug 2026 20:39:40 +0300 Subject: [PATCH 1/5] fix(wgc): pull-based frame delivery to stop CopyResource wedging the 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 #252, #305. # Conflicts: # electron/native/wgc-capture/src/main.cpp --- electron/native/wgc-capture/src/main.cpp | 296 ++++++++++++------ .../native/wgc-capture/src/wgc_session.cpp | 216 ++++++++----- electron/native/wgc-capture/src/wgc_session.h | 65 +++- 3 files changed, 402 insertions(+), 175 deletions(-) diff --git a/electron/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index a9e21d45c..0a3689f12 100644 --- a/electron/native/wgc-capture/src/main.cpp +++ b/electron/native/wgc-capture/src/main.cpp @@ -146,6 +146,15 @@ int readEnvInt(const char* name, int fallback) { } } +// Rollback lever for the pull-based WGC frame delivery (default; see +// wgc_session.h). Forces the previously-shipped FrameArrived-callback path +// instead, for anyone hit by a regression the pull-based path was not tested +// against. Kept only until the pull-based path has enough field time to +// retire this flag and the legacy path with it. +bool useLegacyFrameCallback() { + return readEnvInt("OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK", 0) != 0; +} + std::wstring utf8ToWide(const std::string& value) { if (value.empty()) { return {}; @@ -686,10 +695,16 @@ int main(int argc, char* argv[]) { // same frame lock, rather than the writer's readback -- the shape // getopenscreen/openscreen#460 actually reproduced on Intel HD 520 // ("A WGC frame callback did not finish"). Distinct from - // testStallReadbackMs above because quiesceCapture()'s drain only ever - // sees the callback side: a stall placed in the writer instead leaves + // testStallReadbackMs above because quiesceLegacyCallback()'s drain only + // ever sees the callback side: a stall placed in the writer instead leaves // callbacksInFlight_ at zero and wgcDrained true, which cannot exercise // the video-writer-join skip this stall exists to test. + // + // Requires OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK=1: there is no callback + // thread to stall on the default pull path, which is the point of it -- + // the wedge lands on the writer thread instead, where + // testStallReadbackMs already reaches it and the video-writer-join + // watchdog, not the drain, is what bounds it. const int testStallFrameCallbackMs = std::max(0, readEnvInt("OPENSCREEN_WGC_TEST_STALL_FRAME_CALLBACK_MS", 0)); @@ -905,7 +920,20 @@ int main(int argc, char* argv[]) { } } - std::mutex mutex; + // By default, no mutex guards frame handoff: writeVideoFrames is the + // only thread that ever touches WGC or latestFrameTexture. It pulls each + // frame with session.tryGetNextFrame() itself (see wgc_session.h for + // why) instead of a separate thread pushing into a shared, lock-guarded + // texture. A CopyResource that wedges inside the display + // driver (issue #252, and the DXGI path in PR #305 did not avoid it + // either) then blocks only this thread, which is already the thread + // whose job is to notice stopRequested and give up -- there is no second + // thread left for it to take down with it. + // + // OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK=1 reverts to the previously + // shipped push-based design (frameMutex/frameCv guard the handoff from + // WGC's own callback thread) as a rollback lever -- see wgc_session.h. + const bool legacyFrameCallback = useLegacyFrameCallback(); CaptureControl control; std::atomic firstFrameWritten = false; std::atomic encodeFailed = false; @@ -914,51 +942,58 @@ int main(int argc, char* argv[]) { // them is the next bug report, and neither is worth a log line each. std::atomic contendedFrames = 0; Microsoft::WRL::ComPtr latestFrameTexture; - int64_t latestFrameTimestampHns = 0; - int64_t firstFrameTimestampHns = -1; std::vector latestWebcamFrame; int latestWebcamWidth = 0; int latestWebcamHeight = 0; uint64_t latestWebcamSequence = 0; bool hasVisibleWebcamFrame = false; - session.setFrameCallback([&](ID3D11Texture2D* texture, int64_t timestampHns) { - if (control.stopRequested || control.paused) { - return; - } - - std::scoped_lock lock(mutex); - if (!latestFrameTexture) { - D3D11_TEXTURE2D_DESC desc{}; - texture->GetDesc(&desc); - desc.BindFlags = 0; - desc.CPUAccessFlags = 0; - desc.MiscFlags = 0; - if (FAILED(session.device()->CreateTexture2D(&desc, nullptr, &latestFrameTexture))) { - encodeFailed = true; - control.requestStop(); + // Legacy-path-only state. frameMutex guards latestFrameTexture/ + // legacyLatestFrameTimestampHns between WGC's callback thread (writer) + // and writeVideoFrames (reader); frameCv wakes the reader. Both are + // unused on the default pull-based path. + std::timed_mutex frameMutex; + std::condition_variable_any frameCv; + int64_t legacyLatestFrameTimestampHns = 0; + + if (legacyFrameCallback) { + session.setFrameCallback([&](ID3D11Texture2D* texture, int64_t timestampHns) { + if (control.stopRequested || control.paused) { return; } - } + std::scoped_lock lock(frameMutex); + if (!latestFrameTexture) { + D3D11_TEXTURE2D_DESC desc{}; + texture->GetDesc(&desc); + desc.BindFlags = 0; + desc.CPUAccessFlags = 0; + desc.MiscFlags = 0; + if (FAILED(session.device()->CreateTexture2D(&desc, nullptr, &latestFrameTexture))) { + encodeFailed = true; + control.requestStop(); + return; + } + } - // Gated on an already-arrived first frame: main() blocks up to 10s - // waiting for firstFrameWritten before it will even print - // recording-started, a startup budget this stall is meant to outlast - // (it needs to still be asleep when `stop` arrives, seconds later). - // Stalling the first frame trips that unrelated timeout instead of - // reaching the steady-state shutdown path this exists to test, and - // does not match the real report either -- getopenscreen/openscreen - // #460's diagnostic shows recording-started succeeding before the - // hang. - if (testStallFrameCallbackMs > 0 && firstFrameWritten.load()) { - std::this_thread::sleep_for(std::chrono::milliseconds(testStallFrameCallbackMs)); - } - session.context()->CopyResource(latestFrameTexture.Get(), texture); - latestFrameTimestampHns = timestampHns; - if (!firstFrameWritten.exchange(true)) { - control.cv.notify_all(); - } - }); + // Gated on an already-arrived first frame: main() blocks up to 10s + // waiting for firstFrameWritten before it will even print + // recording-started, a startup budget this stall is meant to + // outlast (it needs to still be asleep when `stop` arrives, + // seconds later). Stalling the first frame trips that unrelated + // timeout instead of reaching the steady-state shutdown path this + // exists to test, and does not match the real report either -- + // getopenscreen/openscreen#460's diagnostic shows + // recording-started succeeding before the hang. + if (testStallFrameCallbackMs > 0 && firstFrameWritten.load()) { + std::this_thread::sleep_for(std::chrono::milliseconds(testStallFrameCallbackMs)); + } + session.context()->CopyResource(latestFrameTexture.Get(), texture); + legacyLatestFrameTimestampHns = timestampHns; + if (!firstFrameWritten.exchange(true)) { + frameCv.notify_all(); + } + }); + } auto writeVideoFrames = [&]() { const auto frameDuration = std::chrono::duration_cast( @@ -980,6 +1015,8 @@ int main(int argc, char* argv[]) { const int64_t nominalWebcamIntervalHns = static_cast(10'000'000ULL / std::max(1, webcamCapture.fps())); auto nextFrameDue = std::chrono::steady_clock::now(); + int64_t firstFrameTimestampHns = -1; + int64_t latestFrameTimestampHns = 0; while (!control.stopRequested && !encodeFailed) { Microsoft::WRL::ComPtr videoSample; @@ -987,15 +1024,75 @@ int main(int argc, char* argv[]) { bool hasVideoSample = false; bool hasWebcamSample = false; + std::unique_lock legacyLock; { - std::unique_lock lock(mutex); - control.cv.wait_for(lock, std::chrono::milliseconds(100), [&] { - return control.stopRequested.load() || - encodeFailed.load() || - (!control.paused.load() && latestFrameTexture); - }); - if (control.stopRequested || encodeFailed) { - break; + if (legacyFrameCallback) { + // try_lock_for, not a blocking lock: the WGC callback + // holds frameMutex across CopyResource, which can wedge + // inside the display driver and never return (#252). + // This is the exact failure OPENSCREEN_WGC_LEGACY_FRAME_ + // CALLBACK=1 opts back into; a blocking acquire here + // would let it also stall this thread's stop detection. + legacyLock = std::unique_lock(frameMutex, std::defer_lock); + if (!legacyLock.try_lock_for(std::chrono::milliseconds(100))) { + if (control.stopRequested || encodeFailed) { + break; + } + continue; + } + frameCv.wait_for(legacyLock, std::chrono::milliseconds(100), [&] { + return control.stopRequested.load() || + encodeFailed.load() || + (!control.paused.load() && latestFrameTexture); + }); + if (control.stopRequested || encodeFailed) { + break; + } + if (!latestFrameTexture) { + continue; + } + latestFrameTimestampHns = legacyLatestFrameTimestampHns; + } else { + if (control.paused) { + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + continue; + } + + ID3D11Texture2D* wgcTexture = nullptr; + int64_t wgcTimestampHns = 0; + const bool gotFrame = session.tryGetNextFrame(&wgcTexture, &wgcTimestampHns); + if (gotFrame) { + if (!latestFrameTexture) { + D3D11_TEXTURE2D_DESC desc{}; + wgcTexture->GetDesc(&desc); + desc.BindFlags = 0; + desc.CPUAccessFlags = 0; + desc.MiscFlags = 0; + if (FAILED(session.device()->CreateTexture2D(&desc, nullptr, &latestFrameTexture))) { + encodeFailed = true; + control.requestStop(); + break; + } + } + // The wedge risk this class exists to avoid: this call + // can block inside the display driver and never return + // (#252, still true of PR #305's DXGI path on some + // hardware). It now does so only on this thread, which + // already owns deciding when to give up -- there is no + // separate WGC callback thread left for it to take a + // lock down with it. + session.context()->CopyResource(latestFrameTexture.Get(), wgcTexture); + latestFrameTimestampHns = wgcTimestampHns; + firstFrameWritten = true; + } else if (!latestFrameTexture) { + // No frame captured yet at all: nothing to encode + // this iteration, and nothing gated on it either (the + // first-frame wait below polls firstFrameWritten + // directly, not a condition variable this thread + // would need to notify). + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + continue; + } } if (webcamActive) { WebcamFrameSnapshot candidateWebcamFrame; @@ -1053,10 +1150,9 @@ int main(int argc, char* argv[]) { if (lastWebcamTimestampHns >= 0 && webcamTimestampHns <= lastWebcamTimestampHns) { webcamTimestampHns = lastWebcamTimestampHns + nominalWebcamIntervalHns; } - // Capture the sample under `mutex` (the frame copy), but - // submit it to the sink writer OUTSIDE the mutex below - // (issue #115) so a slow WriteSample can't starve the main - // thread's stop-wait. + // Capture the sample here, but submit it to the sink + // writer OUTSIDE this block below (issue #115) so a + // slow WriteSample can't hold up the next frame pull. hasWebcamSample = webcamEncoder.captureBgraSample(webcamFrame, webcamTimestampHns, webcamSample); if (!hasWebcamSample) { encodeFailed = true; @@ -1076,13 +1172,16 @@ int main(int argc, char* argv[]) { std::this_thread::sleep_for(std::chrono::milliseconds(testStallReadbackMs)); } if (latestFrameTexture) { - // Both entry points do their GPU work on latestFrameTexture, - // which must stay serialized (via `mutex`) against the WGC - // frame-arrival callback above, which writes new data into - // the same texture on another thread. Which one is live is - // the encoder's answer, not this struct's request: it falls - // back to the CPU path on its own when the GPU path does - // not fit the machine. + // Both entry points do their GPU work on + // latestFrameTexture. On the default pull path this thread + // is the only writer of that texture too (the CopyResource + // above), so there is no concurrent access to serialize + // against; on the legacy path frameMutex -- held across + // this whole block -- is what keeps WGC's callback thread + // out of it. Which entry point is live is the encoder's + // answer, not this struct's request: it falls back to the + // CPU path on its own when the GPU path does not fit the + // machine. bool captured = false; if (usesDxgiInput) { captured = encoder.captureDxgiSample( @@ -1113,17 +1212,17 @@ int main(int argc, char* argv[]) { } } - // Submit the captured samples to their sink writers OUTSIDE - // `mutex`. IMFSinkWriter::WriteSample runs the H.264 encode - // synchronously and can be slow (especially the software encoder - // fallback used when preferSoftwareEncoder is set), and every - // millisecond it holds `mutex` is a millisecond the WGC frame - // callback spends queued behind it dropping frames (issue #115). + // Submit the captured samples to their sink writers after the + // pull-and-copy block above has finished. IMFSinkWriter:: + // WriteSample runs the H.264 encode synchronously and can be slow + // (especially the software encoder fallback used when + // preferSoftwareEncoder is set); doing it here rather than inside + // the block keeps a slow encode from delaying the next frame pull + // (issue #115). // - // This no longer has anything to do with noticing a stop -- that - // moved off `mutex` entirely (see CaptureControl::stopMutex) after - // issue #252 showed the readback below can wedge inside the lock - // regardless of how briefly WriteSample is held. + // Stop detection has nothing to do with this ordering -- that is + // CaptureControl::stopMutex/stopCv, checked by the loop condition + // above, unrelated to sample submission (issue #252). if (hasWebcamSample && !webcamEncoder.submitVideoSample(webcamSample.Get())) { encodeFailed = true; control.requestStop(); @@ -1283,24 +1382,34 @@ int main(int argc, char* argv[]) { } }); - // The lock covers the wait and the decision, and nothing else. Every - // teardown call below runs outside it, because session.stop() waits for any - // in-flight WGC callback to finish -- and those callbacks block on this very - // mutex. Tearing down while holding it deadlocks the two against each other, - // on the one path the shutdown watchdog does not cover. + // writeVideoFrames is the only caller of session.tryGetNextFrame() now + // (see wgc_session.h), so it has to be running before anything can wait + // for a first frame to arrive -- there is no separate WGC callback thread + // left to deliver one on its own. + if (audioMixer) { + audioMixer->beginTimeline(); + } + control.recordingStartedAt = std::chrono::steady_clock::now(); + startVideoWriter(); + + // firstFrameWritten is set by writeVideoFrames on its own thread; this + // just polls it with the same 10s ceiling the old condition-variable wait + // used. bool firstFrameArrived = false; { - std::unique_lock lock(mutex); - const bool started = control.cv.wait_for(lock, std::chrono::seconds(10), [&] { - return firstFrameWritten.load() || control.stopRequested.load(); - }); - firstFrameArrived = started && firstFrameWritten.load(); + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(10); + while (!firstFrameWritten.load() && !control.stopRequested.load() && + std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + firstFrameArrived = firstFrameWritten.load(); } if (!firstFrameArrived) { control.requestStop(); if (stdinThread.joinable()) { stdinThread.detach(); } + stopVideoWriter(); microphoneCapture.stop(); loopbackCapture.stop(); webcamCapture.stop(); @@ -1312,12 +1421,6 @@ int main(int argc, char* argv[]) { return 1; } - if (audioMixer) { - audioMixer->beginTimeline(); - } - control.recordingStartedAt = std::chrono::steady_clock::now(); - startVideoWriter(); - std::cout << "{\"event\":\"recording-started\",\"schemaVersion\":2}" << std::endl; std::cout << "Recording started" << std::endl; @@ -1420,13 +1523,24 @@ int main(int argc, char* argv[]) { // Quiesce the frame producer first. Until WGC is closed, callbacks keep // arriving and keep taking the frame lock, racing the writer's last pass on // the shared D3D context at exactly the moment we can least afford a stall. + // + // Only the legacy push path has a producer thread to quiesce: on the + // default pull path writeVideoFrames is the sole caller of + // tryGetNextFrame(), so its own exit from the loop is the producer + // stopping, and quiesceLegacyCallback() returns immediately with nothing + // to drain. The step still runs and still reports on both paths, so the + // traces line up step for step and `mode` says which one produced them -- + // every report on #252/#460 so far has been read by comparing these lines + // against each other. 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 // release, so a report that does not say which happened cannot be read. - const bool wgcDrained = session.quiesceCapture(); + const bool wgcDrained = session.quiesceLegacyCallback(); std::cerr << "[stop-timing] step=wgc-quiesce elapsed_ms=" << stopElapsedMs() - << " drained=" << (wgcDrained ? "true" : "false") << std::endl; + << " drained=" << (wgcDrained ? "true" : "false") + << " mode=" << (legacyFrameCallback ? "legacy-callback" : "pull") + << std::endl; beginStopStep("microphone", stepBudgetMs); microphoneCapture.stop(); logStopStep("microphone"); @@ -1455,7 +1569,7 @@ int main(int argc, char* argv[]) { // one that cannot ever succeed, and this is not the only step that // assumed it would: encoder.finalize() below resets the very D3D // device/context a still-blocked writer thread might resume touching - // the moment that lock frees, and quiesceCapture()/stop() already + // the moment that lock frees, and quiesceLegacyCallback()/stop() already // treat "leave everything alone and let process exit reclaim it" as // the only safe response to exactly this state. So this ends the // process here, on this thread, rather than pretending the rest of a @@ -1538,7 +1652,13 @@ int main(int argc, char* argv[]) { } // Releasing the device goes last: by now no thread can still be holding the - // D3D context. + // D3D context. On the default pull path that is stopVideoWriter() above -- + // the writer thread is the only caller of tryGetNextFrame(), so its join is + // what makes this safe; on the legacy path it is wgc-quiesce's drain. Both + // happen before this line, which is why it stays here rather than moving up + // next to the join: encoder.finalize() still issues GPU work on this + // device, and there is nothing to gain from releasing our reference to it + // any earlier. beginStopStep("wgc-session-close", stepBudgetMs); session.stop(); logStopStep("wgc-session-close"); diff --git a/electron/native/wgc-capture/src/wgc_session.cpp b/electron/native/wgc-capture/src/wgc_session.cpp index 76649a990..b7796fc34 100644 --- a/electron/native/wgc-capture/src/wgc_session.cpp +++ b/electron/native/wgc-capture/src/wgc_session.cpp @@ -237,7 +237,6 @@ bool WgcSession::initialize(HMONITOR monitor, int fps, bool captureCursor) { return false; } - frameArrivedToken_ = framePool_.FrameArrived({this, &WgcSession::onFrameArrived}); return true; } @@ -261,15 +260,9 @@ bool WgcSession::initialize(HWND window, int fps, bool captureCursor) { return false; } - frameArrivedToken_ = framePool_.FrameArrived({this, &WgcSession::onFrameArrived}); return true; } -void WgcSession::setFrameCallback(FrameCallback callback) { - std::scoped_lock lock(callbackMutex_); - frameCallback_ = std::move(callback); -} - bool WgcSession::start() { if (!session_) { return false; @@ -282,12 +275,111 @@ bool WgcSession::start() { return true; } -bool WgcSession::quiesceCapture(int drainTimeoutMs) { +bool WgcSession::tryGetNextFrame(ID3D11Texture2D** outTexture, int64_t* outTimestampHns) { + if (!framePool_) { + return false; + } + + // TryGetNextFrame() and frame.Close() are the only WGC calls this makes; + // neither performs the GPU copy itself, so neither is where a wedge in + // #252 was ever observed. The copy (CopyResource, on whatever the caller + // does with *outTexture) is the caller's own doing on the caller's own + // thread -- this class has no thread of its own left to hang on their + // behalf. + auto frame = framePool_.TryGetNextFrame(); + if (!frame) { + return false; + } + + auto surface = frame.Surface(); + auto access = surface.as<::Windows::Graphics::DirectX::Direct3D11::IDirect3DDxgiInterfaceAccess>(); + Microsoft::WRL::ComPtr texture; + HRESULT hr = access->GetInterface(__uuidof(ID3D11Texture2D), reinterpret_cast(texture.GetAddressOf())); + if (FAILED(hr) || !texture) { + return false; + } + + // Closing the previous frame here (rather than right after this class + // copied out of it) returns it to the pool only once the caller has had a + // full interval to read the one before that -- the pool has 2 buffers, so + // closing eagerly would let WGC recycle a buffer the caller might still + // 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; + + *outTexture = texture.Get(); + *outTimestampHns = timeSpanToHns(frame.SystemRelativeTime()); + return true; +} + +void WgcSession::setFrameCallback(FrameCallback callback) { + if (!legacyCallbackRegistered_ && framePool_) { + frameArrivedToken_ = framePool_.FrameArrived({this, &WgcSession::onFrameArrived}); + legacyCallbackRegistered_ = true; + } + std::scoped_lock lock(callbackMutex_); + frameCallback_ = std::move(callback); +} + +void WgcSession::onFrameArrived( + wgcap::Direct3D11CaptureFramePool const& sender, + wf::IInspectable const&) { + auto frame = sender.TryGetNextFrame(); + if (!frame) { + return; + } + + auto surface = frame.Surface(); + auto access = surface.as<::Windows::Graphics::DirectX::Direct3D11::IDirect3DDxgiInterfaceAccess>(); + Microsoft::WRL::ComPtr texture; + HRESULT hr = access->GetInterface(__uuidof(ID3D11Texture2D), reinterpret_cast(texture.GetAddressOf())); + if (FAILED(hr) || !texture) { + return; + } + + FrameCallback callback; + { + std::scoped_lock lock(callbackMutex_); + callback = frameCallback_; + if (callback) { + // Counted under the same lock quiesceLegacyCallback() clears the + // callback under, so once it has cleared it no new callback can + // start and the counter it then drains cannot go back up. + callbacksInFlight_ += 1; + } + } + + if (callback) { + // Scoped rather than a bare decrement after the call, for two reasons: + // a callback that left by exception would otherwise strand + // quiesceLegacyCallback()'s drain forever, and the guard has to + // outlive frame.Close() -- dropping the count first would let + // quiesce return and close the frame pool while this handler is + // still closing a frame that pool owns. + struct InFlightGuard { + std::atomic& counter; + ~InFlightGuard() { + counter -= 1; + } + } guard{callbacksInFlight_}; + callback(texture.Get(), timeSpanToHns(frame.SystemRelativeTime())); + frame.Close(); + return; + } + frame.Close(); +} + +bool WgcSession::quiesceLegacyCallback(int drainTimeoutMs) { if (quiesced_) { return callbacksInFlight_.load() == 0; } quiesced_ = true; + if (!legacyCallbackRegistered_) { + return true; + } + try { if (framePool_) { framePool_.FrameArrived(frameArrivedToken_); @@ -297,20 +389,21 @@ bool WgcSession::quiesceCapture(int drainTimeoutMs) { // to abandon the rest of the shutdown. } { - // Drop the callback under the same lock onFrameArrived copies it under, - // so any handler that has not read it yet becomes a no-op... + // Drop the callback under the same lock onFrameArrived copies it + // under, so any handler that has not read it yet becomes a no-op... std::scoped_lock lock(callbackMutex_); frameCallback_ = nullptr; } - // ...then wait out the handlers that already read it. Without this, stop() - // could Reset() the D3D context while a callback was still issuing - // CopyResource on it. + // ...then wait out the handlers that already read it. Without this, + // stop() could Reset() the D3D context while a callback was still + // issuing CopyResource on it. // // Bounded, because a callback wedged inside the display driver never - // finishes and this runs on paths that have no watchdog above them (the - // first-frame timeout in main.cpp). Giving up is reported rather than - // papered over: the caller keeps the device alive instead, which leaks it - // until the process exits and is the lesser of the two failures. + // finishes (this is #252 -- the exact failure this legacy path is kept + // around to let a user opt back into, so its own known weakness needs no + // further comment here). Giving up is reported rather than papered over: + // the caller keeps the device alive instead, which leaks it until the + // process exits and is the lesser of the two failures. const auto drainDeadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(drainTimeoutMs); while (callbacksInFlight_.load() > 0) { @@ -321,13 +414,38 @@ bool WgcSession::quiesceCapture(int drainTimeoutMs) { } std::this_thread::sleep_for(std::chrono::milliseconds(1)); } + return true; +} + +void WgcSession::stop() { + if (!started_ && !framePool_) { + return; + } + + if (legacyCallbackRegistered_ && !quiesceLegacyCallback()) { + // A callback is still inside the driver holding this context. + // Releasing it now would pull the device out from under a live + // CopyResource, so leak it and let process exit reclaim it. This is + // the exact hang class the pull-based default avoids; it is only + // reachable via OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK=1. + return; + } // Close() is a C++/WinRT projection and throws hresult_error on failure. // Letting that escape would take the process down through std::terminate - // mid-shutdown, discarding a recording that is already finalized by the time - // this runs. There is nothing to do about a capture session that refuses to - // close except stop caring about it. + // mid-shutdown, discarding a recording that is already finalized by the + // time this runs. There is nothing to do about a capture session that + // refuses to close except stop caring about it. + // + // On the pull-based (default) path, there is no other thread that could + // be mid-copy on currentFrame_'s texture when this runs: the caller only + // ever calls tryGetNextFrame() and stop() from its own thread, so by the + // time stop() is reached whatever the caller was doing with the last + // texture it read is already done. On the legacy path, the + // quiesceLegacyCallback() call above already established the same + // invariant before falling through to here. try { + currentFrame_ = nullptr; if (session_) { session_.Close(); } @@ -343,70 +461,12 @@ bool WgcSession::quiesceCapture(int drainTimeoutMs) { session_ = nullptr; framePool_ = nullptr; started_ = false; - return true; -} - -void WgcSession::stop() { - if (!quiesceCapture()) { - // A callback is still inside the driver holding this context. Releasing - // it now would pull the device out from under a live CopyResource, so - // leak it and let process exit reclaim it. - return; - } item_ = nullptr; winrtDevice_ = nullptr; d3dContext_.Reset(); d3dDevice_.Reset(); } -void WgcSession::onFrameArrived( - wgcap::Direct3D11CaptureFramePool const& sender, - wf::IInspectable const&) { - auto frame = sender.TryGetNextFrame(); - if (!frame) { - return; - } - - auto surface = frame.Surface(); - auto access = surface.as<::Windows::Graphics::DirectX::Direct3D11::IDirect3DDxgiInterfaceAccess>(); - Microsoft::WRL::ComPtr texture; - HRESULT hr = access->GetInterface(__uuidof(ID3D11Texture2D), reinterpret_cast(texture.GetAddressOf())); - if (FAILED(hr) || !texture) { - return; - } - - FrameCallback callback; - { - std::scoped_lock lock(callbackMutex_); - callback = frameCallback_; - if (callback) { - // Counted under the same lock quiesceCapture() clears the callback - // under, so once it has cleared it no new callback can start and - // the counter it then drains cannot go back up. - callbacksInFlight_ += 1; - } - } - - if (callback) { - // Scoped rather than a bare decrement after the call, for two reasons: - // a callback that left by exception would otherwise strand - // quiesceCapture()'s drain forever, and the guard has to outlive - // frame.Close() -- dropping the count first would let quiesce return and - // close the frame pool while this handler is still closing a frame that - // pool owns. - struct InFlightGuard { - std::atomic& counter; - ~InFlightGuard() { - counter -= 1; - } - } guard{callbacksInFlight_}; - callback(texture.Get(), timeSpanToHns(frame.SystemRelativeTime())); - frame.Close(); - return; - } - frame.Close(); -} - int WgcSession::captureWidth() const { return width_; } diff --git a/electron/native/wgc-capture/src/wgc_session.h b/electron/native/wgc-capture/src/wgc_session.h index 33aba29b4..35eb25a54 100644 --- a/electron/native/wgc-capture/src/wgc_session.h +++ b/electron/native/wgc-capture/src/wgc_session.h @@ -10,9 +10,38 @@ #include #include +#include #include #include +// Frame delivery defaults to pull-based, not the WGC FrameArrived event: the +// caller's own thread polls tryGetNextFrame() on its own schedule and does +// the GPU copy itself. The reason is this codebase's threading model, not +// another project's precedent: a FrameArrived handler runs on a WGC-owned +// thread, so any lock a caller takes to synchronize that handler with its +// own pipeline ends up held by a thread the caller does not control. If the +// copy wedges inside the display driver -- which happens on real hardware, +// not hypothetically (see #252 and #460) -- that lock is gone until the +// process exits, and every other thread that ever needs it hangs too, +// however briefly it would otherwise have held it. Pulling on the caller's +// own thread means a wedged copy only ever blocks the one thread already +// responsible for deciding when to give up on it; nothing else can be +// dragged in. +// +// Chromium's WGC capturer also pulls rather than handling FrameArrived, but +// not for this reason -- its comment there is about avoiding a +// DispatcherQueue, and it runs a 1-buffer pool because a dropped frame costs +// a screen-sharing viewer nothing. We record, where a dropped frame is a +// defect in a file someone keeps, so none of its sizing carries over here. +// +// The old FrameArrived-callback path (setFrameCallback/onFrameArrived) is +// kept alongside it, selected by OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK (see +// main.cpp), as a rollback lever: if the pull-based path regresses on some +// hardware/driver combination this was not tested against, a user or +// maintainer can force the previously-shipped behavior back on without +// waiting for a new release. It carries its own known failure mode (#252) +// and is not a recommended default -- remove it once the pull-based path has +// enough field time to retire the flag. class WgcSession { public: using FrameCallback = std::function; @@ -25,16 +54,26 @@ class WgcSession { bool initialize(HMONITOR monitor, int fps, bool captureCursor); bool initialize(HWND window, int fps, bool captureCursor); - void setFrameCallback(FrameCallback callback); bool start(); - // Stops frame delivery and waits out any callback already running, without - // touching the D3D device. Split out of stop() so a caller can quiesce the - // producer early in a shutdown and only release the device once nothing can - // still be using it. Idempotent; stop() calls it. - // - // Returns false if a callback was still running when `drainTimeoutMs` - // expired -- releasing the device after that is unsafe, so stop() skips it. - bool quiesceCapture(int drainTimeoutMs = 5000); + // Returns the most recently arrived frame's texture and timestamp, or + // false if none is available since the last call. The returned pointer + // is only valid until the next tryGetNextFrame() call or stop() -- copy + // out of it (e.g. via CopyResource) before either. Do not mix with + // setFrameCallback() on the same session. + bool tryGetNextFrame(ID3D11Texture2D** outTexture, int64_t* outTimestampHns); + + // Legacy push-based path (OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK=1 only). + // callback runs on a WGC-owned thread inside FrameArrived and may be + // invoked concurrently with stop()/quiesceLegacyCallback() from the + // caller's thread -- see onFrameArrived's locking. Do not mix with + // tryGetNextFrame() on the same session. + void setFrameCallback(FrameCallback callback); + // Stops frame delivery and waits out any callback already running, + // without touching the D3D device. Only meaningful after + // setFrameCallback(); a no-op on the pull-based path. Returns false if a + // callback was still running when drainTimeoutMs expired -- releasing + // the device after that is unsafe, so stop() skips it in that case. + bool quiesceLegacyCallback(int drainTimeoutMs = 5000); void stop(); int captureWidth() const; @@ -57,10 +96,18 @@ class WgcSession { winrt::Windows::Graphics::Capture::GraphicsCaptureItem item_{nullptr}; winrt::Windows::Graphics::Capture::Direct3D11CaptureFramePool framePool_{nullptr}; winrt::Windows::Graphics::Capture::GraphicsCaptureSession session_{nullptr}; + // Keeps the most recent frame's WinRT wrapper (and therefore its + // pool-owned texture) alive between tryGetNextFrame() calls: the pool is + // created with 2 buffers, so holding this reference is what keeps the + // texture valid for the caller to read from until the next call reclaims + // it. + winrt::Windows::Graphics::Capture::Direct3D11CaptureFrame currentFrame_{nullptr}; + // Legacy push-based path state; unused unless setFrameCallback() is called. winrt::event_token frameArrivedToken_{}; FrameCallback frameCallback_; std::mutex callbackMutex_; std::atomic callbacksInFlight_ = 0; + bool legacyCallbackRegistered_ = false; bool quiesced_ = false; int width_ = 0; int height_ = 0; From a5a75d10ea243a9036e12252680aec280c64af4a Mon Sep 17 00:00:00 2001 From: abduznik <85239936+abduznik@users.noreply.github.com> Date: Sat, 8 Aug 2026 20:55:06 +0300 Subject: [PATCH 2/5] fix(wgc): address CodeRabbit review on the pull-based frame delivery PR - legacyLock (main.cpp writeVideoFrames) outlived the block it was scoped for, so on the legacy callback path frameMutex stayed held across submitVideoSample -- reintroducing the #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/ComPtr, 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). --- electron/native/wgc-capture/src/main.cpp | 64 +++++++++++++------ .../native/wgc-capture/src/wgc_session.cpp | 41 ++++++------ 2 files changed, 65 insertions(+), 40 deletions(-) diff --git a/electron/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index 0a3689f12..be8145d6f 100644 --- a/electron/native/wgc-capture/src/main.cpp +++ b/electron/native/wgc-capture/src/main.cpp @@ -1172,16 +1172,22 @@ int main(int argc, char* argv[]) { std::this_thread::sleep_for(std::chrono::milliseconds(testStallReadbackMs)); } if (latestFrameTexture) { - // Both entry points do their GPU work on - // latestFrameTexture. On the default pull path this thread - // is the only writer of that texture too (the CopyResource - // above), so there is no concurrent access to serialize - // against; on the legacy path frameMutex -- held across - // this whole block -- is what keeps WGC's callback thread - // out of it. Which entry point is live is the encoder's - // answer, not this struct's request: it falls back to the - // CPU path on its own when the GPU path does not fit the - // machine. + // captureVideoSample/captureDxgiSample perform the GPU + // readback from latestFrameTexture. On the pull-based + // (default) path no lock is needed around it: this thread + // is the only writer of latestFrameTexture too (the + // CopyResource above), so there is no concurrent access + // to serialize against. On the legacy path the WGC + // callback thread also writes latestFrameTexture, under + // frameMutex -- legacyLock is still held here (see its + // declaration above) and is what keeps this readback safe + // in that case. Do not remove the legacy locking on the + // strength of this comment; it describes the default path + // only. + // + // Which entry point is live is the encoder's answer, not + // this struct's request: it falls back to the CPU path on + // its own when the GPU path does not fit the machine. bool captured = false; if (usesDxgiInput) { captured = encoder.captureDxgiSample( @@ -1211,6 +1217,15 @@ int main(int argc, char* argv[]) { } } } + // Explicitly released here, not left to the end of the loop + // iteration: on the legacy path, legacyLock still owns frameMutex + // at this point (unique_lock's scope is its own lifetime, not the + // braces above), and the submission calls below are synchronous + // H.264 encodes that must not run while the WGC callback thread + // is blocked waiting for this same mutex (issue #115). + if (legacyLock.owns_lock()) { + legacyLock.unlock(); + } // Submit the captured samples to their sink writers after the // pull-and-copy block above has finished. IMFSinkWriter:: @@ -1600,8 +1615,18 @@ int main(int argc, char* argv[]) { if (usesDxgiInput) { std::cerr << "[frame-drops] gpu_bridge_contended=" << contendedFrames.load() << std::endl; } - // No frame lock here, and the ordering above is what makes that safe rather - // than incidental: stopVideoWriter() joined the only thread that calls into + // Finalizing before closing the WGC session, not after: MFEncoder holds + // its own ComPtr/ComPtr (see + // mf_encoder.h), separate from WgcSession's, so session.stop() resetting + // WgcSession's pointers would not by itself invalidate what finalize() + // uses -- COM reference counting keeps the underlying device alive until + // MFEncoder releases its own. This ordering does not rely on that: it + // removes the dependency instead of documenting it, so a future change to + // MFEncoder (taking a raw, non-owning pointer, say) cannot silently + // reintroduce a use-after-free. + // + // No frame lock here either, and the ordering above is what makes that + // safe rather than incidental: stopVideoWriter() joined the only thread that calls into // the encoder's GPU readback, and audioMixer->stop() joined the only other // thread that writes to it. MFEncoder's own writerMutex_ deliberately does // NOT cover copyFrameToBuffer, so finalizing before those joins would race @@ -1651,14 +1676,13 @@ int main(int argc, char* argv[]) { } } - // Releasing the device goes last: by now no thread can still be holding the - // D3D context. On the default pull path that is stopVideoWriter() above -- - // the writer thread is the only caller of tryGetNextFrame(), so its join is - // what makes this safe; on the legacy path it is wgc-quiesce's drain. Both - // happen before this line, which is why it stays here rather than moving up - // next to the join: encoder.finalize() still issues GPU work on this - // device, and there is nothing to gain from releasing our reference to it - // any earlier. + // Releasing the device goes last, after every encoder that might still + // hold a reference to WgcSession's device has released it via finalize() + // above. By now no thread can still be holding the D3D context: on the + // default pull path writeVideoFrames -- already joined by + // stopVideoWriter() -- was the only caller of tryGetNextFrame()/ + // CopyResource, so its own exit from the while loop is the producer + // stopping; on the legacy path it is wgc-quiesce's drain. beginStopStep("wgc-session-close", stepBudgetMs); session.stop(); logStopStep("wgc-session-close"); diff --git a/electron/native/wgc-capture/src/wgc_session.cpp b/electron/native/wgc-capture/src/wgc_session.cpp index b7796fc34..ecc49303c 100644 --- a/electron/native/wgc-capture/src/wgc_session.cpp +++ b/electron/native/wgc-capture/src/wgc_session.cpp @@ -338,34 +338,35 @@ void WgcSession::onFrameArrived( return; } + // Scoped rather than a bare decrement at the end, for two reasons: a + // callback that left by exception would otherwise strand + // quiesceLegacyCallback()'s drain forever, and the guard has to outlive + // frame.Close() -- dropping the count first would let quiesce return and + // close the frame pool while this handler is still closing a frame that + // pool owns. Counted unconditionally (not only when callback is + // non-null): the no-callback path still calls frame.Close() below, and + // that call needs to be covered by the drain too, or quiesceLegacyCallback() + // could return while this handler is still inside it. + struct InFlightGuard { + std::atomic& counter; + ~InFlightGuard() { + counter -= 1; + } + }; + FrameCallback callback; { std::scoped_lock lock(callbackMutex_); callback = frameCallback_; - if (callback) { - // Counted under the same lock quiesceLegacyCallback() clears the - // callback under, so once it has cleared it no new callback can - // start and the counter it then drains cannot go back up. - callbacksInFlight_ += 1; - } + // 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. + callbacksInFlight_ += 1; } + InFlightGuard guard{callbacksInFlight_}; if (callback) { - // Scoped rather than a bare decrement after the call, for two reasons: - // a callback that left by exception would otherwise strand - // quiesceLegacyCallback()'s drain forever, and the guard has to - // outlive frame.Close() -- dropping the count first would let - // quiesce return and close the frame pool while this handler is - // still closing a frame that pool owns. - struct InFlightGuard { - std::atomic& counter; - ~InFlightGuard() { - counter -= 1; - } - } guard{callbacksInFlight_}; callback(texture.Get(), timeSpanToHns(frame.SystemRelativeTime())); - frame.Close(); - return; } frame.Close(); } From 08a7be4b42c4baa7ab92005a6babf1f4de1d4436 Mon Sep 17 00:00:00 2001 From: abduznik <85239936+abduznik@users.noreply.github.com> Date: Sat, 8 Aug 2026 21:01:48 +0300 Subject: [PATCH 3/5] fix(wgc): register onFrameArrived's in-flight guard before touching the 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. --- .../native/wgc-capture/src/wgc_session.cpp | 49 +++++++++++-------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/electron/native/wgc-capture/src/wgc_session.cpp b/electron/native/wgc-capture/src/wgc_session.cpp index ecc49303c..53928963f 100644 --- a/electron/native/wgc-capture/src/wgc_session.cpp +++ b/electron/native/wgc-capture/src/wgc_session.cpp @@ -325,28 +325,12 @@ void WgcSession::setFrameCallback(FrameCallback callback) { void WgcSession::onFrameArrived( wgcap::Direct3D11CaptureFramePool const& sender, wf::IInspectable const&) { - auto frame = sender.TryGetNextFrame(); - if (!frame) { - return; - } - - auto surface = frame.Surface(); - auto access = surface.as<::Windows::Graphics::DirectX::Direct3D11::IDirect3DDxgiInterfaceAccess>(); - Microsoft::WRL::ComPtr texture; - HRESULT hr = access->GetInterface(__uuidof(ID3D11Texture2D), reinterpret_cast(texture.GetAddressOf())); - if (FAILED(hr) || !texture) { - return; - } - // Scoped rather than a bare decrement at the end, for two reasons: a // callback that left by exception would otherwise strand // quiesceLegacyCallback()'s drain forever, and the guard has to outlive - // frame.Close() -- dropping the count first would let quiesce return and - // close the frame pool while this handler is still closing a frame that - // pool owns. Counted unconditionally (not only when callback is - // non-null): the no-callback path still calls frame.Close() below, and - // that call needs to be covered by the drain too, or quiesceLegacyCallback() - // could return while this handler is still inside it. + // every pool-owned object this handler touches -- dropping the count + // first would let quiesce return and close the frame pool while this + // handler still holds a reference into it. struct InFlightGuard { std::atomic& counter; ~InFlightGuard() { @@ -354,17 +338,42 @@ void WgcSession::onFrameArrived( } }; + // Captured and counted before TryGetNextFrame(), not after: this handler + // starts touching the pool (TryGetNextFrame, Surface(), GetInterface()) + // immediately below, and none of that is safe to run concurrently with + // framePool_.Close(). Counting only after those calls succeeded left a + // window where quiesceLegacyCallback() could see callbacksInFlight_ == 0 + // and return while this handler was still mid-frame -- registering the + // guard first, before anything pool-related, closes that window instead + // of narrowing it. FrameCallback callback; { 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. + // 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. callbacksInFlight_ += 1; } InFlightGuard guard{callbacksInFlight_}; + auto frame = sender.TryGetNextFrame(); + if (!frame) { + return; + } + + auto surface = frame.Surface(); + auto access = surface.as<::Windows::Graphics::DirectX::Direct3D11::IDirect3DDxgiInterfaceAccess>(); + Microsoft::WRL::ComPtr texture; + HRESULT hr = access->GetInterface(__uuidof(ID3D11Texture2D), reinterpret_cast(texture.GetAddressOf())); + if (FAILED(hr) || !texture) { + frame.Close(); + return; + } + if (callback) { callback(texture.Get(), timeSpanToHns(frame.SystemRelativeTime())); } From 91475b4413e8900f8e41cfa635a2538d0e6d73d7 Mon Sep 17 00:00:00 2001 From: abduznik <85239936+abduznik@users.noreply.github.com> Date: Sat, 8 Aug 2026 21:07:31 +0300 Subject: [PATCH 4/5] fix(wgc): return before touching the frame pool when the legacy callback 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. --- .../native/wgc-capture/src/wgc_session.cpp | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/electron/native/wgc-capture/src/wgc_session.cpp b/electron/native/wgc-capture/src/wgc_session.cpp index 53928963f..b75d0819e 100644 --- a/electron/native/wgc-capture/src/wgc_session.cpp +++ b/electron/native/wgc-capture/src/wgc_session.cpp @@ -346,16 +346,23 @@ void WgcSession::onFrameArrived( // and return while this handler was still mid-frame -- registering the // guard first, before anything pool-related, closes that window instead // of narrowing it. + // + // Returns here, before incrementing the counter or touching the pool, if + // frameCallback_ is already null: there is nothing to do with a frame in + // that case, so the handler should not acquire one. This also means a + // handler that starts after quiesceLegacyCallback() has cleared + // frameCallback_ is never counted at all -- which is fine, since it never + // reaches the pool either. FrameCallback callback; { std::scoped_lock lock(callbackMutex_); callback = frameCallback_; + if (!callback) { + return; + } // 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. + // and the counter it then drains cannot go back up. callbacksInFlight_ += 1; } InFlightGuard guard{callbacksInFlight_}; @@ -374,9 +381,9 @@ void WgcSession::onFrameArrived( return; } - if (callback) { - callback(texture.Get(), timeSpanToHns(frame.SystemRelativeTime())); - } + // callback is never null here: the only path that reaches this point + // returned earlier if frameCallback_ was null when captured. + callback(texture.Get(), timeSpanToHns(frame.SystemRelativeTime())); frame.Close(); } From f4cafb235431381dda16ed3975ab380726ee5c9d Mon Sep 17 00:00:00 2001 From: EtienneLescot Date: Sat, 29 Aug 2026 17:09:24 +0200 Subject: [PATCH 5/5] test(wgc): run the #460 stall scenario on the path that still has a callback --stall-frame-callback asserts `reason=frame-callback-stuck`, the video-writer-join skip, which only exists where a WGC callback thread does: with pull-based delivery now the default, the scenario stalled nothing and asserted on a branch that can never be taken. It fails rather than silently passes, but a regression test that cannot reach its own subject is not one. Pin OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK=1 for it. Also adds --legacy-frame-callback so any scenario can be run on either delivery path from the same build -- the A/B to ask for from the machines in getopenscreen/openscreen#252 and #460 that reproduce on demand. --- scripts/test-windows-wgc-helper.mjs | 32 +++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/scripts/test-windows-wgc-helper.mjs b/scripts/test-windows-wgc-helper.mjs index ee40838e6..785e48c1e 100644 --- a/scripts/test-windows-wgc-helper.mjs +++ b/scripts/test-windows-wgc-helper.mjs @@ -51,14 +51,28 @@ const STALL_FRAME_CALLBACK_ENV = "OPENSCREEN_WGC_TEST_STALL_FRAME_CALLBACK_MS"; * frame *callback* itself while it holds the frame lock, the shape that issue * actually reproduced on Intel HD 520 ("A WGC frame callback did not finish"). * Distinct from WITH_STALLED_READBACK above -- that stalls the writer's own - * readback, which quiesceCapture()'s drain cannot see (callbacksInFlight_ - * stays at zero), so it cannot exercise the video-writer-join skip this stall - * exists to test. + * readback, which quiesceLegacyCallback()'s drain cannot see + * (callbacksInFlight_ stays at zero), so it cannot exercise the + * video-writer-join skip this stall exists to test. + * + * Forces the legacy push path on (below), because that is the only path with a + * frame callback to stall: the default pull path has no WGC-owned thread, so + * this scenario would otherwise stall nothing and assert on a skip that can + * never be taken. */ const WITH_STALLED_FRAME_CALLBACK = process.env.OPENSCREEN_WGC_TEST_STALL_FRAME_CALLBACK === "true" || process.argv.includes("--stall-frame-callback"); const STALL_FRAME_CALLBACK_MS = Number(process.env[STALL_FRAME_CALLBACK_ENV] ?? 60_000); +const LEGACY_FRAME_CALLBACK_ENV = "OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK"; +/** + * Runs any scenario on the pre-#306 push-based delivery path instead of the + * pull-based default -- the same lever a user gets, so a machine that only + * fails one way can be A/B'd without swapping builds. + */ +const WITH_LEGACY_FRAME_CALLBACK = + process.env.OPENSCREEN_WGC_LEGACY_FRAME_CALLBACK === "1" || + process.argv.includes("--legacy-frame-callback"); const STOP_BUDGET_ENV = "OPENSCREEN_WGC_STOP_BUDGET_MS"; /** * The helper's global shutdown ceiling, pinned into its environment below so @@ -80,14 +94,23 @@ if (WITH_SOFTWARE_ENCODER && WITH_SOFTWARE_FALLBACK) { function runHelper( config, - { injectDefaultSinkWriterFailure = false, stallReadbackMs = 0, stallFrameCallbackMs = 0 } = {}, + { + injectDefaultSinkWriterFailure = false, + stallReadbackMs = 0, + stallFrameCallbackMs = 0, + legacyFrameCallback = false, + } = {}, ) { return new Promise((resolve, reject) => { const env = { ...process.env }; delete env[INJECT_DEFAULT_SINK_WRITER_FAILURE_ENV]; delete env[STALL_READBACK_ENV]; delete env[STALL_FRAME_CALLBACK_ENV]; + delete env[LEGACY_FRAME_CALLBACK_ENV]; env[STOP_BUDGET_ENV] = String(STOP_BUDGET_MS); + if (legacyFrameCallback) { + env[LEGACY_FRAME_CALLBACK_ENV] = "1"; + } if (injectDefaultSinkWriterFailure) { env[INJECT_DEFAULT_SINK_WRITER_FAILURE_ENV] = "1"; } @@ -483,6 +506,7 @@ try { injectDefaultSinkWriterFailure: WITH_SOFTWARE_FALLBACK, stallReadbackMs: WITH_STALLED_READBACK ? STALL_READBACK_MS : 0, stallFrameCallbackMs: WITH_STALLED_FRAME_CALLBACK ? STALL_FRAME_CALLBACK_MS : 0, + legacyFrameCallback: WITH_LEGACY_FRAME_CALLBACK || WITH_STALLED_FRAME_CALLBACK, }); } finally { if (fixtureWindow) {