diff --git a/electron/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index 63cbcbba7..ec9b165aa 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,17 @@ 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. 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, @@ -820,18 +917,27 @@ 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 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 +1080,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 +1119,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 +1207,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,12 +1224,23 @@ 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 + // 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"); @@ -1173,8 +1284,13 @@ int main(int argc, char* argv[]) { } } - // Releasing the device goes last: by now no thread can still be holding the - // D3D context. + // 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"); diff --git a/electron/native/wgc-capture/src/wgc_session.cpp b/electron/native/wgc-capture/src/wgc_session.cpp index ccab06727..1a578b80a 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,128 @@ 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&) { + // 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 + // 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() { + counter -= 1; + } + }; + + // 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. + // + // 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. + 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; + } + + // 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(); +} + +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 +399,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 +424,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 +471,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;