From c3ddbeddccbb4e7564637932c0abe1eeabe73fae 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/4] 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. --- electron/native/wgc-capture/src/main.cpp | 260 ++++++++++++------ .../native/wgc-capture/src/wgc_session.cpp | 216 +++++++++------ electron/native/wgc-capture/src/wgc_session.h | 60 +++- 3 files changed, 363 insertions(+), 173 deletions(-) diff --git a/electron/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index 63cbcbba7..a91634b19 100644 --- a/electron/native/wgc-capture/src/main.cpp +++ b/electron/native/wgc-capture/src/main.cpp @@ -145,6 +145,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 {}; @@ -648,44 +657,64 @@ 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 + // -- matches Chromium's WgcCaptureSession, which pulls for the same + // reason) 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; 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; } - } - - session.context()->CopyResource(latestFrameTexture.Get(), texture); - latestFrameTimestampHns = timestampHns; - if (!firstFrameWritten.exchange(true)) { - control.cv.notify_all(); - } - }); + 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; + } + } + session.context()->CopyResource(latestFrameTexture.Get(), texture); + legacyLatestFrameTimestampHns = timestampHns; + if (!firstFrameWritten.exchange(true)) { + frameCv.notify_all(); + } + }); + } auto writeVideoFrames = [&]() { const auto frameDuration = std::chrono::duration_cast( @@ -706,6 +735,8 @@ int main(int argc, char* argv[]) { int64_t nextWebcamWriteDueHns = 0; const int64_t nominalWebcamIntervalHns = static_cast(10'000'000ULL / std::max(1, webcamCapture.fps())); + int64_t firstFrameTimestampHns = -1; + int64_t latestFrameTimestampHns = 0; while (!control.stopRequested && !encodeFailed) { Microsoft::WRL::ComPtr videoSample; @@ -713,15 +744,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; @@ -779,10 +870,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; @@ -803,10 +893,10 @@ int main(int argc, char* argv[]) { } if (latestFrameTexture) { // captureVideoSample performs the GPU readback - // (CopyResource/Map) from 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. + // (CopyResource/Map) from latestFrameTexture. 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. hasVideoSample = encoder.captureVideoSample( latestFrameTexture.Get(), frameTimestampHns, @@ -821,17 +911,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(); @@ -974,24 +1064,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(); @@ -1003,12 +1103,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; @@ -1097,16 +1191,6 @@ 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. - 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(); - std::cerr << "[stop-timing] step=wgc-quiesce elapsed_ms=" << stopElapsedMs() - << " drained=" << (wgcDrained ? "true" : "false") << std::endl; beginStopStep("microphone", stepBudgetMs); microphoneCapture.stop(); logStopStep("microphone"); @@ -1124,8 +1208,18 @@ int main(int argc, char* argv[]) { beginStopStep("video-writer-join", stepBudgetMs); stopVideoWriter(); logStopStep("video-writer-join"); - // No frame lock here, and the ordering above is what makes that safe rather - // than incidental: stopVideoWriter() joined the only thread that calls into + // Closing the WGC session only after the writer thread has joined, not + // before: writeVideoFrames is now the only caller of + // session.tryGetNextFrame()/CopyResource, so joining it first guarantees + // nothing is still touching WGC or the D3D device when session.stop() + // tears them down. There is no separate "quiesce the producer" step + // anymore because the writer thread's own exit from its while loop *is* + // the producer stopping -- WGC has no thread of its own left to quiesce. + beginStopStep("wgc-session-close", stepBudgetMs); + session.stop(); + logStopStep("wgc-session-close"); + // The ordering above is what makes finalize() 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 @@ -1173,12 +1267,6 @@ int main(int argc, char* argv[]) { } } - // Releasing the device goes last: by now no thread can still be holding the - // D3D context. - beginStopStep("wgc-session-close", stepBudgetMs); - session.stop(); - logStopStep("wgc-session-close"); - shutdownComplete = true; shutdownWatchdog.join(); diff --git a/electron/native/wgc-capture/src/wgc_session.cpp b/electron/native/wgc-capture/src/wgc_session.cpp index ccab06727..30981b84a 100644 --- a/electron/native/wgc-capture/src/wgc_session.cpp +++ b/electron/native/wgc-capture/src/wgc_session.cpp @@ -230,7 +230,6 @@ bool WgcSession::initialize(HMONITOR monitor, int fps, bool captureCursor) { return false; } - frameArrivedToken_ = framePool_.FrameArrived({this, &WgcSession::onFrameArrived}); return true; } @@ -254,15 +253,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; @@ -275,12 +268,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_); @@ -290,20 +382,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) { @@ -314,13 +407,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(); } @@ -336,70 +454,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..af4c60f84 100644 --- a/electron/native/wgc-capture/src/wgc_session.h +++ b/electron/native/wgc-capture/src/wgc_session.h @@ -10,9 +10,33 @@ #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. This deliberately matches Chromium's +// WgcCaptureSession (modules/desktop_capture/win/wgc_capture_session.cc), +// which does the same thing for the same reason: a FrameArrived handler runs +// on a WGC-owned thread, so any lock a caller takes to synchronize the +// handler with its own pipeline is 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) -- 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. +// +// 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 +49,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 +91,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, mirroring + // Chromium's mapped_texture_ handling: the pool only has 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 9a0c4e4a7c666a20a9f491542c924aca0e0cd030 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/4] 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 | 60 ++++++++++++++----- .../native/wgc-capture/src/wgc_session.cpp | 41 ++++++------- 2 files changed, 65 insertions(+), 36 deletions(-) diff --git a/electron/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index a91634b19..ec9b165aa 100644 --- a/electron/native/wgc-capture/src/main.cpp +++ b/electron/native/wgc-capture/src/main.cpp @@ -893,10 +893,17 @@ int main(int argc, char* argv[]) { } if (latestFrameTexture) { // captureVideoSample performs the GPU readback - // (CopyResource/Map) from latestFrameTexture. 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. + // (CopyResource/Map) 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. hasVideoSample = encoder.captureVideoSample( latestFrameTexture.Get(), frameTimestampHns, @@ -910,6 +917,15 @@ int main(int argc, char* argv[]) { lastEncodedVideoTimestampHns = frameTimestampHns; } } + // 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:: @@ -1208,22 +1224,23 @@ int main(int argc, char* argv[]) { beginStopStep("video-writer-join", stepBudgetMs); stopVideoWriter(); logStopStep("video-writer-join"); - // Closing the WGC session only after the writer thread has joined, not - // before: writeVideoFrames is now the only caller of - // session.tryGetNextFrame()/CopyResource, so joining it first guarantees - // nothing is still touching WGC or the D3D device when session.stop() - // tears them down. There is no separate "quiesce the producer" step - // anymore because the writer thread's own exit from its while loop *is* - // the producer stopping -- WGC has no thread of its own left to quiesce. - beginStopStep("wgc-session-close", stepBudgetMs); - session.stop(); - logStopStep("wgc-session-close"); - // The ordering above is what makes finalize() safe rather than + // 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 does not by itself invalidate what finalize() + // uses -- COM reference counting keeps the underlying device alive until + // MFEncoder releases its own reference. Finalizing first regardless, + // rather than relying on that, because it removes the dependency + // entirely instead of documenting it: a future change to MFEncoder (e.g. + // taking a raw, non-owning pointer) would silently reintroduce a + // use-after-free that this ordering makes structurally impossible. + // + // The ordering below it is what makes finalize() 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 - // the staging texture -- do not reorder these. + // the staging texture -- do not reorder those. beginStopStep("encoder-finalize", shutdownBudgetMs); const bool screenFinalized = encoder.finalize(); logStopStep("encoder-finalize"); @@ -1267,6 +1284,17 @@ int main(int argc, char* argv[]) { } } + // Closing the WGC session only now, after every encoder that might still + // hold a reference to WgcSession's device has released it via finalize() + // above. There is no separate "quiesce the producer" step: writeVideoFrames + // (already joined by stopVideoWriter() above) was the only caller of + // session.tryGetNextFrame()/CopyResource, so its own exit from the while + // loop *is* the producer stopping -- WGC has no thread of its own left to + // quiesce. + beginStopStep("wgc-session-close", stepBudgetMs); + session.stop(); + logStopStep("wgc-session-close"); + shutdownComplete = true; shutdownWatchdog.join(); diff --git a/electron/native/wgc-capture/src/wgc_session.cpp b/electron/native/wgc-capture/src/wgc_session.cpp index 30981b84a..f7cf066a1 100644 --- a/electron/native/wgc-capture/src/wgc_session.cpp +++ b/electron/native/wgc-capture/src/wgc_session.cpp @@ -331,34 +331,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 7b83113d5e3dd8ea904b01099232d2a88cba3c2d 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/4] 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 f7cf066a1..3db27946f 100644 --- a/electron/native/wgc-capture/src/wgc_session.cpp +++ b/electron/native/wgc-capture/src/wgc_session.cpp @@ -318,28 +318,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() { @@ -347,17 +331,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 f21b0a9207b83778a51b47c7d7109674c45e9c05 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/4] 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 3db27946f..1a578b80a 100644 --- a/electron/native/wgc-capture/src/wgc_session.cpp +++ b/electron/native/wgc-capture/src/wgc_session.cpp @@ -339,16 +339,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_}; @@ -367,9 +374,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(); }