diff --git a/electron/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index a9e21d45c..be8145d6f 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,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, - // 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. + // 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( @@ -1112,18 +1217,27 @@ 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 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 +1397,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 +1436,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 +1538,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 +1584,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 @@ -1486,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 @@ -1537,8 +1676,13 @@ int main(int argc, char* argv[]) { } } - // Releasing the device goes last: by now no thread can still be holding the - // D3D context. + // 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 76649a990..b75d0819e 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,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_); @@ -297,20 +406,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 +431,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 +478,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; 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) {