fix(wgc): report adapter LUIDs, because the descriptions can be identical - #342
Merged
Conversation
…ical
The `capture-adapter` diagnostic compared adapters by LUID and printed only
their descriptions. On the exact configuration it exists to catch, those two
descriptions are the same string.
An IddCx virtual display driver renders through the physical GPU and inherits
its description while being a separate DXGI adapter with its own LUID. Measured
on a rented multi-adapter box:
adapter[0] NVIDIA Quadro RTX 4000 LUID 0:24084 -> \.\DISPLAY1
adapter[1] NVIDIA Quadro RTX 4000 LUID 0:12889146 -> the Parsec IDD
So a machine with the divergence would have reported two identical names beside
`sameAdapter:false` — which reads as a bug in the reporting rather than as the
finding it is, and would most likely have been dismissed as one. The comparison
was never wrong; the output was unreadable at the only moment it matters.
Adds `deviceLuid` and `monitorLuid` to the event, and one `[adapters]` line per
enumerated adapter on stderr with its LUID and its outputs, marking the captured
one. Two adapters sharing a description is something a reader has to see before
they will believe `sameAdapter` over the names, and an adapter with no output at
all is how an inactive virtual display presents -- which is why the rented box
reported `sameAdapter:true` despite having a virtual display driver installed.
Formatted HighPart:LowPart to match QueryDisplayConfig traces, so a LUID from a
bug report can be grepped straight against one.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Windows capture helper now reports capture and monitor adapter LUIDs, distinguishes monitor lookup states, enumerates DXGI adapters and outputs at startup, and documents cross-adapter diagnostics. ChangesGPU adapter diagnostics
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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}
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 `@electron/native/wgc-capture/src/main.cpp`:
- Around line 307-311: Update the DXGI monitor/output enumeration loops in main
so they stop only when the enumeration call returns DXGI_ERROR_NOT_FOUND; treat
DXGI_ERROR_NOT_CURRENTLY_AVAILABLE as an indeterminate enumeration failure and
report that distinct status instead of emitting monitorLuid:null or
outputs=none. Preserve the existing no-output handling for genuinely completed
enumeration, and run the WGC smoke test on a real Windows system.
🪄 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: 472a463e-0f04-4098-8c6a-86f2b70a5cc8
📒 Files selected for processing (2)
electron/native/README.mdelectron/native/wgc-capture/src/main.cpp
DXGI_ERROR_NOT_CURRENTLY_AVAILABLE is not the end of an enumeration. It is what EnumOutputs answers a process in session 0, and it means the outputs were never inspected rather than that the adapter has none. Both were being folded into the same FAILED() test, so a machine we could not look at reported exactly what a machine with an active virtual display reports: monitorAdapter null. That value is one this diagnostic tells its readers to act on -- the briefing sent to an operator said null meant a purely virtual display. It would have sent them down the wrong path, and it is the same class of defect this PR was opened to fix for the descriptions: an ambiguity that reads as a finding. The event now carries monitorLookup: ok, no-output-claims-it, or unavailable, so the absence of a monitor adapter is never reported without saying whether anyone actually looked. The stderr dump says outputs=unavailable rather than outputs=none for the same reason. Unreachable from the app, which spawns the helper from the user's session -- but the helper is run standalone by hand during exactly this investigation, and that is where session 0 gets reached.
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.
The
capture-adapterdiagnostic from #332 compared adapters by LUID and printed only their descriptions. On the exact configuration it exists to catch, those two descriptions are the same string.Measured on a rented multi-adapter VM:
An IddCx virtual display driver renders through the physical GPU and inherits its description string, while being a separate DXGI adapter with its own LUID.
QueryDisplayConfigconfirmed the virtual monitor sits under0:12889146.So a machine that actually has the divergence would have printed:
{"deviceAdapter":"NVIDIA Quadro RTX 4000","monitorAdapter":"NVIDIA Quadro RTX 4000","sameAdapter":false}Two identical names next to
sameAdapter:false— which reads as a bug in the reporting rather than as the finding it is, and would most likely have been dismissed as one. The comparison was never wrong. The output was unreadable at the only moment it matters, and we were days away from asking four reporters to run it.What changed
deviceLuidandmonitorLuidon the event, formattedHighPart:LowPartto matchQueryDisplayConfigtraces so a LUID from a bug report greps straight against one[adapters]line per enumerated adapter on stderr, with its LUID and its outputs, marking the captured oneThe stderr dump matters for the negative case too: an adapter with no output is how an inactive virtual display presents, and that is exactly why the rented box reported
sameAdapter:truedespite having a virtual display driver installed. Having one is not enough — it has to be driving the captured display.Not in scope
Making
createD3DDeviceselect the adapter that owns the captured HMONITOR instead of taking the default. That is a real correctness gap — nothing in DXGI promises adapter 0 drives the display, and nothing in the code checks — but it is a behaviour change and belongs in its own PR. This one only makes the existing diagnostic legible.Refs #252, #292, #327.
Summary by CodeRabbit
New Features
Documentation