fix(wgc): repair the four things the GPU path got wrong under review

Four defects CodeRabbit surfaced on #305, each verified against the code
before being touched. One of them is worse than it was reported to be.

**AcquireSync was tested with the wrong predicate, in both directions.**
IDXGIKeyedMutex::AcquireSync reports a timeout as WAIT_TIMEOUT (0x102),
a positive HRESULT that passes both SUCCEEDED() and !FAILED(). The capture
side tested FAILED(), so a timeout fell straight through to CopyResource
without ever holding the key, and only hard errors -- device removed,
E_FAIL, WAIT_ABANDONED -- were caught, then misreported as ordinary
contention, which skips the frame and retries forever on a bridge that can
never work again. The encoder side used succeeded(), so a timeout there read
as acquired. Both now test WAIT_TIMEOUT by value.

This also means the "Contended frames: 0" figure in the PR description was
measured with a counter that could not count: a timeout never reached it.
The number says nothing either way and is being remeasured.

**Inline webcam PiP would have silently lost its overlay.** useDxgiInput
read webcamActive, which is only set once webcam capture starts -- long
after the encoder is configured. The condition was therefore dead: always
false, always permitting the GPU path. A webcamEnabled recording with no
separate output would have run on DXGI, which cannot compose the overlay,
and reported success. It now reads config.webcamEnabled, which is final at
that point.

**The stop breadcrumb named the wrong call.** encodeStage_ was stamped
before writerMutex_ was taken, so a video thread queued behind an audio
write reported "write-sample" while it was in fact blocked on the lock --
precisely the case the watchdog exists to distinguish, since an audio write
is the only other thing that takes that mutex. It is now stamped inside the
lock, and writeAudio names its own write instead of being anonymous. Both
stamps are serialized by the mutex they sit under.

**finalize() left Media Foundation objects for the destructor.**
videoSampleAllocator_ and dxgiDeviceManager_ outlived MFShutdown(), and
captureDevice_/captureContext_ kept the WGC device alive past the point
main.cpp believes session.stop() releases it. finalize() now calls
releaseDxgiPipeline() and drops the capture device before MFShutdown().

Not addressed here: bridge-texture creation still happens on the first
frame, so a driver that refuses shared keyed-mutex textures fails the
recording rather than degrading, which contradicts what three documents
claim. That one is a restructure with a real trade-off attached and is
being decided separately.

Compile-verified in CI only; no hardware smoke test on this machine.

Author: EtienneLescot

electron/native/wgc-capture/src/main.cpp

@@ -612,9 +612,17 @@ int main(int argc, char* argv[]) {
     // produce. The env var is the escape hatch for a machine where the GPU path
     // misbehaves in a way the encoder's own probes do not catch -- a support
     // answer instead of a hotfix.
+    //
+    // config.webcamEnabled, not webcamActive: the latter is only set once the
+    // webcam capture has started, which happens well after this. Reading it
+    // here made the PiP condition dead code -- always false, so always
+    // permitting the GPU path -- and an inline-PiP recording would have run on
+    // DXGI and silently dropped the overlay, reporting success either way.
+    // config.webcamEnabled is already cleared above when webcam init fails,
+    // and writeSeparateWebcam is assigned there too, so both are final here.
     encoderOptions.useDxgiInput =
         !config.preferSoftwareEncoder &&
-        (!webcamActive || writeSeparateWebcam) &&
+        (!config.webcamEnabled || writeSeparateWebcam) &&
         readEnvInt("OPENSCREEN_WGC_DISABLE_DXGI_INPUT", 0) == 0;
 
     MFEncoder encoder;

electron/native/wgc-capture/src/mf_encoder.cpp

@@ -1029,11 +1029,25 @@ MFEncoder::Nv12ConvertResult MFEncoder::convertBgraTextureToNv12(
     // Key 0 is the capture side's, key 1 the encoder's. Timing out here leaves
     // key 0 exactly where it was, so the next frame simply tries again; that
     // is the whole reason this one is recoverable and the one below is not.
+    //
+    // Tested by value, not with FAILED(): AcquireSync reports a timeout as
+    // WAIT_TIMEOUT (0x102), which is a *positive* HRESULT and therefore passes
+    // both SUCCEEDED() and !FAILED(). Testing FAILED() alone got this backwards
+    // in both directions -- a timeout fell through to the CopyResource below
+    // without ever holding the key, and a hard error (DXGI_ERROR_DEVICE_REMOVED,
+    // E_FAIL, WAIT_ABANDONED) was reported as ordinary contention, which skips
+    // the frame and retries forever on a bridge that can never work again. The
+    // recording then ends successfully with almost no frames in it.
     encodeStage_ = "bridge-acquire-capture";
-    if (FAILED(captureBridgeMutex_->AcquireSync(0, acquireTimeoutMs))) {
+    const HRESULT captureAcquireHr = captureBridgeMutex_->AcquireSync(0, acquireTimeoutMs);
+    if (captureAcquireHr == static_cast<HRESULT>(WAIT_TIMEOUT)) {
         encodeStage_ = "idle";
         return Nv12ConvertResult::Contended;
     }
+    if (!succeeded(captureAcquireHr, "Acquire capture bridge")) {
+        encodeStage_ = "idle";
+        return Nv12ConvertResult::Failed;
+    }
     encodeStage_ = "bridge-copy";
     captureContext_->CopyResource(captureBridgeTexture_.Get(), texture);
     encodeStage_ = "bridge-release-capture";
@@ -1043,8 +1057,17 @@ MFEncoder::Nv12ConvertResult MFEncoder::convertBgraTextureToNv12(
     encodeStage_ = "bridge-acquire-encoder";
     // Key 1 was just handed over by this same thread and nothing else in the
     // process can hold it, so a failure here means the bridge is broken rather
-    // than busy, and no later frame could recover it.
-    if (!succeeded(encoderBridgeMutex_->AcquireSync(1, acquireTimeoutMs), "Acquire encoder bridge")) {
+    // than busy, and no later frame could recover it. A timeout counts as broken
+    // for that same reason, and needs the same by-value test as above, since
+    // WAIT_TIMEOUT passes SUCCEEDED() and would otherwise be read as acquired.
+    const HRESULT encoderAcquireHr = encoderBridgeMutex_->AcquireSync(1, acquireTimeoutMs);
+    if (encoderAcquireHr == static_cast<HRESULT>(WAIT_TIMEOUT)) {
+        std::cerr << "ERROR: Acquire encoder bridge timed out" << std::endl;
+        encodeStage_ = "idle";
+        return Nv12ConvertResult::Failed;
+    }
+    if (!succeeded(encoderAcquireHr, "Acquire encoder bridge")) {
+        encodeStage_ = "idle";
         return Nv12ConvertResult::Failed;
     }
     const auto releaseEncoderBridge = [&]() {
@@ -1268,12 +1291,16 @@ bool MFEncoder::submitVideoSample(IMFSample* sample) {
     // encode synchronously on the calling thread. Callers must NOT hold any
     // lock shared with a thread that needs to make timely progress (e.g. a
     // stop-request check) across this call.
-    encodeStage_ = "write-sample";
+    // Stamped after the lock, not before it. The breadcrumb is meant to name the
+    // call the writer is *inside*; setting it first made "write-sample" also mean
+    // "queued behind writeAudio, which is inside WriteSample" -- the one case the
+    // watchdog most needs to tell apart, since an audio write is the only other
+    // thing that takes this mutex.
     std::scoped_lock writerLock(writerMutex_);
     if (!sinkWriter_ || finalized_) {
-        encodeStage_ = "idle";
         return false;
     }
+    encodeStage_ = "write-sample";
     const bool written = succeeded(sinkWriter_->WriteSample(videoStreamIndex_, sample), "WriteSample");
     encodeStage_ = "idle";
     return written;
@@ -1317,7 +1344,14 @@ bool MFEncoder::writeAudio(const BYTE* data, DWORD byteCount, int64_t timestampH
     sample->SetSampleTime(std::max<int64_t>(0, timestampHns));
     sample->SetSampleDuration(durationHns);
 
-    return succeeded(sinkWriter_->WriteSample(audioStreamIndex_, sample.Get()), "WriteSample(audio)");
+    // Named too, for the same reason the video write is: this is a synchronous
+    // encode holding writerMutex_, so it is a place the process can be stuck,
+    // and a watchdog report that only ever names video writes cannot say so.
+    encodeStage_ = "write-audio";
+    const bool written =
+        succeeded(sinkWriter_->WriteSample(audioStreamIndex_, sample.Get()), "WriteSample(audio)");
+    encodeStage_ = "idle";
+    return written;
 }
 
 bool MFEncoder::finalize() {
@@ -1333,6 +1367,17 @@ bool MFEncoder::finalize() {
         sinkWriter_.Reset();
     }
     stagingTexture_.Reset();
+    // Before MFShutdown(), not left to the destructor. Two of the objects
+    // releaseDxgiPipeline() drops -- videoSampleAllocator_ and
+    // dxgiDeviceManager_ -- are Media Foundation objects, and releasing those
+    // after MFShutdown() has run is not something the platform promises
+    // anything about. The rest matters to the caller rather than to MF:
+    // captureDevice_/captureContext_ hold the WGC D3D11 device, so leaving them
+    // set means session.stop() in main.cpp is no longer dropping the last
+    // reference to the device it thinks it owns.
+    releaseDxgiPipeline();
+    captureContext_.Reset();
+    captureDevice_.Reset();
     context_.Reset();
     device_.Reset();
     MFShutdown();