fix(wgc): GPU DXGI encode path for Windows capture, with a fallback at every step - #305
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Windows recorder now supports DXGI/NV12 GPU encoder input with CPU fallback. It configures GPU conversion and hardware VBR, reports the selected input path, handles temporary bridge contention as dropped frames, and documents fallback and opt-out conditions. ChangesWindows DXGI encoder input
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WGCSession
participant MFEncoder
participant VideoProcessor
participant SinkWriter
WGCSession->>MFEncoder: provide WGC texture
MFEncoder->>VideoProcessor: convert BGRA texture to NV12
VideoProcessor-->>MFEncoder: return converted frame or contention
MFEncoder->>SinkWriter: submit timestamped DXGI sample
SinkWriter-->>MFEncoder: return encoding result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/native/wgc-capture/src/wgc_session.cpp (1)
66-111: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not make
D3D11_CREATE_DEVICE_VIDEO_SUPPORTa hard requirement of capture.
createD3DDevicenow requestsD3D11_CREATE_DEVICE_VIDEO_SUPPORTunconditionally. If an adapter or driver rejects that flag,D3D11CreateDevicefails and the whole recording fails, including the CPU readback path that never needed video support. Only the_DEBUGbranch retries with a reduced flag set.The GPU path does not depend on this flag for the capture device:
initializeDxgiEncodingDevicecreates its own encoder device withD3D11_CREATE_DEVICE_VIDEO_SUPPORT(electron/native/wgc-capture/src/mf_encoder.cpplines 760-782), and the capture device only needs to create the shared keyed-mutex bridge texture. Retry without the flag so a machine that lacks video support still records on the CPU path.🛡️ Proposed retry
if (!succeeded(hr, "D3D11CreateDevice")) { - return false; + // Video support is only useful to the GPU encode path, which has its + // own device. Never let it cost the recording. + flags &= ~D3D11_CREATE_DEVICE_VIDEO_SUPPORT; + hr = D3D11CreateDevice( + nullptr, + D3D_DRIVER_TYPE_HARDWARE, + nullptr, + flags, + featureLevels, + ARRAYSIZE(featureLevels), + D3D11_SDK_VERSION, + &d3dDevice_, + &featureLevel, + &d3dContext_); + if (!succeeded(hr, "D3D11CreateDevice(no video support)")) { + return false; + } }Verify this on real Windows hardware before merge: CI runs only on Linux, so native capture changes need a manual smoke test. Based on coding guidelines: "Native capture changes require a manual smoke test on real macOS or Windows, because CI runs only on Linux."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/wgc-capture/src/wgc_session.cpp` around lines 66 - 111, Update WgcSession::createD3DDevice to retry D3D11CreateDevice without D3D11_CREATE_DEVICE_VIDEO_SUPPORT when the initial creation fails, while retaining D3D11_CREATE_DEVICE_DEBUG handling in debug builds. Preserve the existing failure check and ensure devices without video support can continue through the CPU readback path. Manually smoke-test native capture on real Windows hardware.Source: Coding guidelines
🧹 Nitpick comments (2)
electron/native/wgc-capture/src/mf_encoder.cpp (1)
235-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the hardware-transform attribute failure.
Every other failure branch in
createSinkWriterFromUrlprints the label and the HRESULT. This branch returns silently, so a failure to setMF_READWRITE_ENABLE_HARDWARE_TRANSFORMSproduces no diagnostic and is then reported under theConfigureDxgiManagerstage, which names a different step.♻️ Proposed change
hr = attributes->SetUINT32(MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS, TRUE); if (FAILED(hr)) { + std::cerr << "ERROR: Set MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS(TRUE) failed (hr=0x" + << std::hex << hr << std::dec << ")" << std::endl; failedStage = SinkWriterCreateStage::ConfigureDxgiManager; return hr; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/wgc-capture/src/mf_encoder.cpp` around lines 235 - 239, Update the hardware-transform attribute failure branch in createSinkWriterFromUrl to log the failure label and HRESULT before returning, consistent with the other failure branches. Use the correct stage label for setting MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS instead of reporting ConfigureDxgiManager.electron/native/wgc-capture/src/mf_encoder.h (1)
76-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the "success with no sample" result of
captureDxgiSample.
captureDxgiSamplereturnstrueand leavesoutSampleempty when the keyed-mutex bridge is contended (seemf_encoder.cpplines 1098-1101). A caller that only checks the return value writes nothing and does not know why. The neighbouringcaptureVideoSamplehas a detailed contract comment; state this one too, so the skip semantics stay discoverable from the header.📝 Proposed comment
+ // Returns false only on a real failure. A momentarily contended GPU + // bridge returns true with `outSample` empty: the caller must treat that + // as a skipped frame, not as a sample. bool captureDxgiSample( ID3D11Texture2D* texture, int64_t timestampHns, Microsoft::WRL::ComPtr<IMFSample>& outSample);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/wgc-capture/src/mf_encoder.h` around lines 76 - 79, Add a contract comment immediately above captureDxgiSample documenting that it may return true with outSample empty when the keyed-mutex bridge is contended, and that callers must handle this as a skipped capture rather than a produced sample. Match the detail and style of the neighboring captureVideoSample documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/native/README.md`:
- Line 88: The documentation incorrectly claims shared keyed-mutex texture
creation falls back to CPU encoding, although
MFEncoder::convertBgraTextureToNv12 creates it after initialization and failure
stops recording. Update electron/native/README.md:88-88,
technical-documentation/architecture/recording.md:72-72, and
electron/native/wgc-capture/src/mf_encoder.h:32-37 to remove that fallback claim
or explicitly state that bridge creation failure stops recording; no code change
is requested.
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 610-618: Update the encoderOptions.useDxgiInput condition to use
the resolved config.webcamEnabled value instead of webcamActive, while
preserving writeSeparateWebcam and the software-encoder and environment-variable
checks. Keep inline webcam PiP on the CPU path when webcamEnabled is true and no
separate webcam output is configured; verify with a real Windows webcam
recording.
In `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 729-748: Update MFEncoder::finalize() to call
releaseDxgiPipeline() before MFShutdown(), then reset captureContext_ and
captureDevice_ before completing teardown. Preserve the existing
stagingTexture_, context_, and device_ cleanup, and verify the destruction order
with a real Windows hardware smoke test.
- Around line 941-994: Move the shared bridge-texture setup currently guarded by
captureBridgeTexture_ into initializeDxgiPipeline(), using width_, height_, and
the validated BGRA format to construct its descriptor without a WGC frame.
Ensure every creation, mutex, shared-resource, encoder-open, and input-view
failure causes initialization to select the existing CPU fallback rather than
returning Nv12ConvertResult::Failed from captureDxgiSample; keep per-frame
processing limited to using the already-initialized bridge resources.
- Around line 996-1001: Update the AcquireSync result handling in the
capture-side mutex path to return Nv12ConvertResult::Contended only when the
result is WAIT_TIMEOUT. Propagate or classify all other failure results,
including WAIT_ABANDONED and device errors, as non-recoverable using the
existing error-handling contract.
---
Outside diff comments:
In `@electron/native/wgc-capture/src/wgc_session.cpp`:
- Around line 66-111: Update WgcSession::createD3DDevice to retry
D3D11CreateDevice without D3D11_CREATE_DEVICE_VIDEO_SUPPORT when the initial
creation fails, while retaining D3D11_CREATE_DEVICE_DEBUG handling in debug
builds. Preserve the existing failure check and ensure devices without video
support can continue through the CPU readback path. Manually smoke-test native
capture on real Windows hardware.
---
Nitpick comments:
In `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 235-239: Update the hardware-transform attribute failure branch in
createSinkWriterFromUrl to log the failure label and HRESULT before returning,
consistent with the other failure branches. Use the correct stage label for
setting MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS instead of reporting
ConfigureDxgiManager.
In `@electron/native/wgc-capture/src/mf_encoder.h`:
- Around line 76-79: Add a contract comment immediately above captureDxgiSample
documenting that it may return true with outSample empty when the keyed-mutex
bridge is contended, and that callers must handle this as a skipped capture
rather than a produced sample. Match the detail and style of the neighboring
captureVideoSample documentation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 76007985-6f1d-43a8-b657-19d79c83e829
📒 Files selected for processing (6)
electron/native/README.mdelectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/mf_encoder.cppelectron/native/wgc-capture/src/mf_encoder.helectron/native/wgc-capture/src/wgc_session.cpptechnical-documentation/architecture/recording.md
|
I tested the Windows x64 diagnostic helper from this PR artifact on the machine that reproduces #252 (artifact run 31257864130). Results:
System-audio capture still reproduces the failure on this machine. I reproduced it with both a 10-second run (15-second stop budget) and a 5-second run (8-second stop budget). The shorter run produced: The first system-audio run showed the same |
|
Update after testing the exact PR #305 helper copied into the standalone 1.9+3.5 package:
This exact helper is now packaged locally for further testing. The intermittent failure remains on the affected machine even without system audio; system-audio runs also reproduce it consistently. |
|
Thanks, that is exactly the data needed. Display and window fixed, two failures left, and both your traces say the same thing: Pushed two changes. The system-audio one is a lock-order defect, and it predates the GPU path. The intermittent one I cannot diagnose from here, so it is instrumented rather than guessed at. The encoder keeps a breadcrumb of the call it is inside and the watchdog prints it:
I reproduced your failure shape locally by forcing a stall under the frame lock: Could you re-run on the affected machine? Two things would settle it:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 1271-1278: Move the video `encodeStage_ = "write-sample"`
assignment in the video write path to after `writerMutex_` is acquired, keeping
the existing idle cleanup. In the audio write method, set an audio-specific
stage immediately after locking `writerMutex_` and clear it after the audio
`WriteSample` call, including the early-return path, so the shutdown watchdog
reports the operation that currently owns the writer.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e439471-82fa-4fd8-87de-3e7bc6c07e86
📒 Files selected for processing (3)
electron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/mf_encoder.cppelectron/native/wgc-capture/src/mf_encoder.h
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/native/wgc-capture/src/main.cpp
The DXGI path is the right shape for issue #252: it removes the Map/Unmap readback the reporter's driver wedges inside. What it must not do is become a requirement, because it fixes one machine and every other one still has to record. So every step of it now falls back rather than returning: the encoding device, the NV12 video processor, the shared bridge texture, the sample allocator and the hardware sink writer each drop the whole pipeline and retry the exact chain a machine without a GPU path would have taken. Without that, `useDxgiInput` being the default made the software H.264 fallback unreachable for every recording, and a machine with no hardware encoder went from recording in software to not recording at all. `releaseDxgiPipeline()` puts device_/context_ back on the capture device, because the CPU path's staging texture has to live where the WGC frames do. The choice is no longer knowable from the outside, so callers ask `usesDxgiInput()` and the `encoder-selection` event reports `videoInput`. The bridge acquire was a 5s wait taken on the video-writer thread while it holds the frame lock -- the lock issue #252 is about, measured against an 8s watchdog step budget. It is now a few frame intervals, and a timeout skips the frame instead of ending the recording; the timestamp is stamped after the conversion so a skipped frame no longer stretches the timeline. Frames lost that way are counted and reported once at stop. Measured on a working machine, GPU path against CPU path: - 16.9 Mbps against 1.95 for the same desktop, because the D3D manager switches the sink writer onto a hardware MFT and those default to CBR, spending the full 18 Mbps budget on a static screen. Asking for VBR through ICodecAPI brings it to 2.2. MF_LOW_LATENCY was measured and made no difference, so it is gone. - Colour matches: raw luma 13/222.5/239 against 13/224.3/242, mean rendered RGB 245,240,245 against 246,242,246. The video processor is told full-range BGRA in, studio BT.709 out, and the media types carry the matching tags -- untagged, the driver default is BT.601 and a player reads 1080p as BT.709. - Stop latency 107ms, 0 contended frames over repeated runs, software fallback and preferSoftwareEncoder still land on the CPU path. Co-authored-by: Seb1900 <1712315938@qq.com>
#252's reporter confirmed the GPU path fixes display and window capture on the machine that reproduces it, and found two failures left: one consistent with system audio, one intermittent without it. Both report `wgc-quiesce drained=false` then `video-writer-join phase=abandoned`, which means the video writer is stuck while holding the frame lock and every WGC callback is queued behind it. The system-audio one is a lock-order defect, and it predates the GPU path. `writerMutex_` is held across IMFSinkWriter::WriteSample by both submitVideoSample and writeAudio -- a synchronous encode -- while the capture* entry points took that same mutex just to stamp a sample, from inside main.cpp's frame lock. So an audio write on the mixer thread stalls the video writer, the writer stalls the WGC callbacks, and stop finds nothing drainable. The sample clock moves to a `timestampMutex_` of its own, which no blocking call is ever held across, and the sinkWriter_/finalized_ check goes away with it: submitVideoSample already makes that check before writing, so a sample built for a writer that has gone is discarded one step later instead of costing a lock. The intermittent one is not diagnosable from here, so instrument it rather than guess. The encoder now keeps a breadcrumb of the call it is inside, and the shutdown watchdog prints it: `phase=abandoned encode_stage=bridge-copy` says which driver call wedged, where `encode_stage=idle` says the writer never got into the encoder at all. That is the same move that made #252 legible in the first place. Verified by forcing the failure shape locally with OPENSCREEN_WGC_TEST_STALL_READBACK_MS: wgc-quiesce drained=false at 5001ms, video-writer-join abandoned at 8021ms, exit 3, and the breadcrumb correctly reads `idle` for a stall that is outside the encoder. Display, window, system audio, the software fallback knob and the CPU kill switch all still pass.
|
@Seb1900 gentle ping — the two data points from the last push are still what this is waiting on:
The second one matters more than it did on Friday: #306 argues the wedge is |
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.
The last of the six review findings, and the one that contradicted the title of this PR. Every other step of the GPU path drops the pipeline and retries the CPU chain when it fails. The shared keyed-mutex bridge did not: it was created lazily on the first frame, by which point initialize() had already configured the sink writer for NV12 and there was no chain left to retry. A driver that refuses D3D11_RESOURCE_MISC_SHARED_KEYEDMUTEX therefore failed the recording outright -- on exactly the class of machine that motivated #252 -- while three documents claimed it would record as if the path had never existed. Bridge creation moves into initializeDxgiPipeline(), where returning false already means "warn, releaseDxgiPipeline(), useDxgiInput_ = false". Those three statements are now true rather than aspirational, so none of them needed rewording. The descriptor is built from width_/height_ instead of a captured frame's, which is equivalent rather than a weakening: captureDxgiSample already rejects any texture whose dimensions or format differ from those same values before convertBgraTextureToNv12 is ever reached, so no frame that could disagree with the descriptor can arrive at the bridge. That guard is what makes pre-sizing safe; without it a mismatch would make CopyResource silently no-op and the recording would come out black instead of failing. convertBgraTextureToNv12 is now only the per-frame path, which is what its comment always claimed it was. Compile-verified in CI only; no hardware smoke test on this machine.
433cf75 to
976d44d
Compare
An adversarial review of 4d1a0cc caught this, and three independent passes landed on it separately: naming the audio write was right, putting it in encodeStage_ was not. encodeStage_ is a single slot. Almost all of the video thread's stages -- the whole DXGI bridge sequence in convertBgraTextureToNv12 and captureDxgiSample -- are set with no MFEncoder lock held; only submitVideoSample's stamp sits under writerMutex_. So the previous commit's claim that "both stamps are serialized by the mutex they sit under" was true of the two WriteSample stamps and of nothing else. The consequence ran backwards from the intent. writeAudio runs on the audio-mixer thread, which emits roughly every 10 ms and ends each call by storing "idle". A video thread wedged in bridge-copy therefore had its breadcrumb erased within milliseconds, and the watchdog line -- the single piece of evidence this instrumentation exists to produce -- would have printed encode_stage=idle for precisely the hang it was added to identify. Worse on the system-audio configuration than anywhere else, which is the configuration reproducing #252 most consistently. One slot per writing thread. encodeStage_ is single-writer again (the video thread), audioStage_ belongs to the mixer, and the abandoned-step line prints both. A report showing audio_stage=write-audio next to a video stage stuck on a bridge call now says the two are contending for writerMutex_, which is a thing no shared slot could have expressed. Verified at runtime on the previous commit: a 10-minute DXGI recording (16,177 frames, gpu_bridge_contended=0) and a 15-second one both stopped cleanly, so the surrounding path this touches is exercised, not just compiled.
|
One correction to the ping above, @Seb1900: the helper you packaged locally is now out of date, so please re-download rather than re-running the copy you have. This branch has been rebased onto Fresh artifact, built from the rebased branch: Same two questions as above — whether system audio still reproduces, and the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
electron/native/wgc-capture/src/mf_encoder.cpp (1)
235-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the hardware-transform attribute failure.
The
MF_READWRITE_ENABLE_HARDWARE_TRANSFORMSfailure returns without a message. The two neighbouring failures each log their own HRESULT. The reported stage is alsoConfigureDxgiManager, which names the next step instead of this one.♻️ Proposed change
hr = attributes->SetUINT32(MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS, TRUE); if (FAILED(hr)) { + std::cerr << "ERROR: Set MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS(TRUE) failed (hr=0x" + << std::hex << hr << std::dec << ")" << std::endl; failedStage = SinkWriterCreateStage::ConfigureDxgiManager; return hr; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/wgc-capture/src/mf_encoder.cpp` around lines 235 - 239, Update the MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS failure branch in the encoder setup to log its HRESULT before returning, matching the neighboring attribute-failure handling, and set failedStage to the stage representing hardware-transform configuration rather than ConfigureDxgiManager.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 1140-1161: Reset encodeStage_ to "idle" on every failure return in
captureDxgiSample after the stage is set, including GetBufferByIndex, buffer.As,
GetResource, GetMaxLength, and SetCurrentLength; a scope guard is acceptable if
it reliably clears the stage on exit. Apply the same cleanup to the failure
returns in convertBgraTextureToNv12 that currently leave
"bridge-release-capture" or "output-view" active, while preserving
successful-stage behavior.
---
Nitpick comments:
In `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 235-239: Update the MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS
failure branch in the encoder setup to log its HRESULT before returning,
matching the neighboring attribute-failure handling, and set failedStage to the
stage representing hardware-transform configuration rather than
ConfigureDxgiManager.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9931407-81ef-49b3-8831-cdf0c4da0102
📒 Files selected for processing (2)
electron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/mf_encoder.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/native/wgc-capture/src/main.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/native/wgc-capture/src/mf_encoder.cpp (1)
227-247: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReport the hardware-transform failure distinctly.
Line 237 maps a failed
MF_READWRITE_ENABLE_HARDWARE_TRANSFORMSwrite toConfigureDxgiManager, and the branch prints nothing. TheMF_SINK_WRITER_D3D_MANAGERfailure on line 244 uses the same stage. The reported stage then cannot identify which attribute write failed, and the first failure leaves no log line at all.Add the error line, and use a stage value that names the attribute.
🩹 Proposed change
hr = attributes->SetUINT32(MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS, TRUE); if (FAILED(hr)) { - failedStage = SinkWriterCreateStage::ConfigureDxgiManager; + std::cerr << "ERROR: Set MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS(TRUE) failed (hr=0x" + << std::hex << hr << std::dec << ")" << std::endl; + failedStage = SinkWriterCreateStage::EnableHardwareTransforms; return hr; }
SinkWriterCreateStage::EnableHardwareTransformsneeds a new enumerator and a name mapping next to the existing stages.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/wgc-capture/src/mf_encoder.cpp` around lines 227 - 247, Update the DXGI attribute setup in the sink-writer creation flow to log the HRESULT when MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS fails, and record that failure as a distinct SinkWriterCreateStage::EnableHardwareTransforms value rather than ConfigureDxgiManager. Add the new enumerator and its corresponding stage-name mapping alongside the existing SinkWriterCreateStage definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 227-247: Update the DXGI attribute setup in the sink-writer
creation flow to log the HRESULT when MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS
fails, and record that failure as a distinct
SinkWriterCreateStage::EnableHardwareTransforms value rather than
ConfigureDxgiManager. Add the new enumerator and its corresponding stage-name
mapping alongside the existing SinkWriterCreateStage definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b44c7287-f36d-438f-ac29-290d3ac17aa9
📒 Files selected for processing (2)
electron/native/wgc-capture/src/mf_encoder.cppelectron/native/wgc-capture/src/mf_encoder.h
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/native/wgc-capture/src/mf_encoder.h
Two remaining review findings, both about the same thing: a diagnostic is only worth having if it cannot lie. encodeStage_ was cleared by hand on each return, and the paths that forgot -- GetBufferByIndex, buffer.As, GetResource in captureDxgiSample; the bridge-release-capture and output-view returns in convertBgraTextureToNv12 -- left it naming a call the writer had already left. The watchdog would then report a stage the process was not in, which is worse than reporting nothing, because the next #252 report would be read as evidence. A StageGuard clears it on scope exit instead, in both functions, and the six manual resets it subsumes are gone. submitVideoSample keeps its pair: after the lock there is no early return between setting the stage and clearing it. Also logs the HRESULT when MF_READWRITE_ENABLE_HARDWARE_TRANSFORMS(TRUE) fails. Its two neighbours already log theirs, and this one sits on the path a machine takes when the GPU pipeline is being set up -- exactly the machines whose logs are currently all we have to go on. Not doing the other half of that finding: the stage it reports is ConfigureDxgiManager rather than a value naming hardware transforms. Nothing reads the enum except the CreateSinkWriter comparison at mf_encoder.cpp:550, so a new enumerator would be ceremony. The log line carries the attribute name and the HRESULT, which is what a reader actually needs.
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.
2 participants
Builds on @Seb1900's prototype in #304, rebased onto
main(that branch was cut fromrelease/v1.9.0and conflicts). Their commit is kept as-is; the second commit is the hardening.What #304 got right
The screen path encodes from a CPU readback:
MFVideoFormat_RGB32sink writer input, staging texture,Map(D3D11_MAP_READ), memcpy,Unmap— all on the same D3D11 device/context as WGC, under the shared frame lock. On the reporter's machine (Windows 10, WDDM 2.7, RTX 5070 Ti + AMD iGPU, two virtual display adapters)Unmapnever returns, so the writer thread holds the frame lock,wgc-quiescereportsdrained=false, andvideo-writer-joinis abandoned by the watchdog beforeencoder-finalize— an empty MP4. Their trace and ours agree on the step.The DXGI path removes that call entirely. It is the right fix.
What this PR changes
The GPU path is now a preference, never a requirement. In #304 every DXGI setup failure was a
return false, including a hard error placed between the default sink-writer attempt and the software H.264 retry. SinceuseDxgiInputis on by default for any recording without inline PiP, that made the software fallback unreachable: a machine with no hardware H.264 encoder (VM, RDP session, older iGPU) went from records in software to native recording fails. Now the encoding device, the NV12 video processor, the bridge texture, the sample allocator and the hardware sink writer each drop the whole pipeline and retry the exact chain a machine without a GPU path would have taken.releaseDxgiPipeline()restoresdevice_/context_to the capture device, because the CPU path's staging texture has to live where the WGC frames do.OPENSCREEN_WGC_DISABLE_DXGI_INPUT=1forces it off.No multi-second wait under the frame lock. The bridge acquire was
AcquireSync(..., 5000), taken on the video-writer thread while it holds the very lock #252 is about, against an 8s watchdog step budget. It is now a few frame intervals, and a timeout skips the frame rather than ending the recording. The timestamp is stamped after the conversion, so a skipped frame no longer stretches the timeline. Skips are counted and printed once at stop.Which path ran is now observable. It is a per-machine outcome, so callers read
usesDxgiInput()instead of their own request, andencoder-selectioncarriesvideoInput: "dxgi-nv12" | "cpu-rgb32".Also: the injected-sink-writer-failure test knob now disables the GPU path, so it still proves what it was written to prove; per-frame processor rect/colourspace calls and the input view are hoisted out of the frame loop;
MF_LOW_LATENCYis dropped (measured, no effect).Measured
Verified end to end on a working Windows machine by driving the packaged helper directly. GPU path against CPU path, same idle desktop:
dxgi-nv12)cpu-rgb32)The bitrate one was the surprise: the D3D manager switches the sink writer onto a hardware MFT, and hardware MFTs default to CBR, so a static screen spent the full configured 18 Mbps budget — an 8x file.
MF_MT_AVG_BITRATEalone does not move them; asking for VBR throughICodecAPIdoes.Colour was the other risk, since #304 ran
VideoProcessorBltwith no colourspace set. The processor is now told full-range BGRA in, studio BT.709 out, with matching tags on both media types. The two paths measure the same.Also checked:
preferSoftwareEncoder: true→software-preferred+cpu-rgb32; injected sink-writer failure →software-fallback+cpu-rgb32;OPENSCREEN_WGC_DISABLE_DXGI_INPUT=1→cpu-rgb32; two consecutive recordings; 1080p30 and 60 fps.What we cannot verify
We have no hardware that reproduces #252, so none of the above proves the deadlock is gone — only that the GPU path is correct and the fallbacks work where we can run them. @Seb1900, could you confirm on the machine that fails? The
videoInputfield inencoder-selectionand the[frame-drops]line at stop should make it obvious which path ran.Supersedes #304. Closes #252 once confirmed.
Summary by CodeRabbit
New Features
Documentation