fix(wgc): make the GPU encode path opt-in until it earns the default - #337
Merged
Conversation
It has one confirmed success and one confirmed regression. On the machine in #252 it took display and window capture from a 13-second hang to a 105 ms stop. On the machine in #336 -- the same person who reported #327 -- it ended recording outright, where the CPU path had been producing video. Its fallbacks cover every check made inside initialize(). Nothing covers a failure that only shows up once frames are flowing, which is exactly what #336 is: the pipeline comes up, the sink writer is configured for NV12, and then a per-frame call fails with nowhere left to fall back to. Defaulting it on makes every user carry that risk so that the few who reproduce #252 might not have to, and #252 is not confirmed fixed by anyone. So: off, and OPENSCREEN_WGC_ENABLE_DXGI_INPUT=1 turns it on, which is what the people in #252 and #327 should be given to test with. Nothing is reverted. The path, its six review fixes, the writerMutex_ lock-order split, the stop breadcrumbs and the adapter reporting all stay -- they are either independent of this default or exist to diagnose the bug it was written for. The one line is the whole change, which is the point: the code was fine, the confidence behind switching it on for everyone was not. Earning the default back means one of two things: a failure mid-encode degrades to the CPU path instead of ending the recording, or someone on the affected hardware confirms it fixes #252.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWindows WGC DXGI input is now disabled by default and enabled with ChangesWindows DXGI input configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 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.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@technical-documentation/architecture/recording.md`:
- Line 72: Update the architecture description around the video writer paths to
state that GPU input is runtime-opt-in via OPENSCREEN_WGC_ENABLE_DXGI_INPUT=1,
while hardware determines whether initialization selects DXGI or CPU. Replace
the claim that GPU input falls back at every step with wording that limits
fallback to initialization; preserve that captureDxgiSample() failures after
encoding starts stop the recording.
🪄 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: 80267acf-413a-45aa-a05b-5effb8af77f7
📒 Files selected for processing (3)
electron/native/README.mdelectron/native/wgc-capture/src/main.cpptechnical-documentation/architecture/recording.md
Two clauses survived the default flip and both said the opposite of the change. "which one runs is a per-machine outcome, not a setting" was true before the env var gated it. "degrades to the CPU one on its own at every step" was never true after the first frame -- which is the entire reason the default moved -- and it sat two sentences from a note saying so. Now says it plainly: the fallback is initialization-only, it ends when frames start, and that is the gap #336 fell into.
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One line, plus the docs that described the old default.
Why
The GPU path has one confirmed success and one confirmed regression:
Its fallbacks cover every check inside
initialize(). Nothing covers a failure that only appears once frames are flowing — which is what #336 is: the pipeline comes up, the sink writer is configured for NV12, and then a per-frame call fails with nowhere left to fall back to.Defaulting it on makes every user carry that risk so the few who reproduce #252 might not have to — and nobody has confirmed it fixes #252.
What this is not
Not a revert. The path, its six review fixes, the
writerMutex_lock-order split, the stop breadcrumbs and the adapter reporting all stay. They are either independent of this default, or they exist to diagnose the bug the path was written for. The one line is the whole change: the code was fine, the confidence behind switching it on for everyone was not.Earning the default back
Either of:
OPENSCREEN_WGC_ENABLE_DXGI_INPUT=1Needs cherry-picking onto
release/v1.9.4before the next RC.Summary by CodeRabbit
OPENSCREEN_WGC_ENABLE_DXGI_INPUT=1.