feat(wgc): report which GPU capture landed on, and let the tool test audio - #332
Conversation
…audio Neither #252 nor #327 has been reproduced on hardware we control, and the last two reports arrived with no OS, no version and no trace. Two additions, neither of which changes recording behaviour, so that the next report answers a question instead of opening another round of guessing. **Which adapter.** WgcSession creates its device with D3D11CreateDevice(nullptr, ...) -- the default adapter -- and nothing anywhere asks whether that is the adapter driving the display being captured. On one GPU the question does not arise. On a hybrid laptop, a discrete card, or a machine with virtual display adapters, the two can differ, and then every WGC frame has crossed an adapter boundary before the caller touches it: driver work on both GPUs, and the most plausible remaining candidate for these hangs. The helper now emits {"event":"capture-adapter","deviceAdapter":"...","monitorAdapter":"...", "sameAdapter":false} comparing by LUID rather than by description, since two cards of the same model report the same string. monitorAdapter is null when no DXGI output claims the monitor, which is itself worth seeing: on the #252 reporter's machine that would mean a virtual display adapter. Every failure inside the report is silent -- a diagnostic that can end a recording is worse than no diagnostic. **Audio in the diagnostic tool.** captureSystemAudio and captureMic were hardcoded false, so the standalone tool could not exercise the configuration that fails most consistently in #252. Audio and video writes contend for the same sink-writer lock, so a run without audio has nothing to contend with and passes on machines where the app hangs every time -- which is why no clean trace of that case exists. Now --system-audio and --mic, with the reasoning in both the tool's help and the README so a reporter knows to re-run with them. Verified: --system-audio produces an AAC 48 kHz stereo track alongside the video and stops in 152 ms on this machine. The C++ half is compile-verified in CI only; this machine has a single adapter, so it can only ever print sameAdapter=true and cannot exercise the interesting branch.
…e it The enumeration ended on DXGI_ERROR_NOT_FOUND and treated every other failure as success. Neither EnumAdapters1 nor EnumOutputs fills its out-pointer when it fails, so any other error -- DXGI_ERROR_NOT_CURRENTLY_AVAILABLE, which is what EnumOutputs returns to a process in session 0, or an invalid call -- left a null ComPtr to be dereferenced on the next line. That is a crash, inside the one function here whose comment promises that its failures are silent because a diagnostic which can end a recording is worse than no diagnostic. Testing FAILED() instead is both correct and shorter: NOT_FOUND is itself a failure code, so one test covers the normal end of the enumeration and every other way it can stop. The null checks are belt and braces on top. Also names --mic alongside --system-audio in the tool's help and the README. The guidance told a reporter to re-run with system audio whatever their failing recording actually used, which would have sent anyone hitting this with a microphone down a path that reproduces nothing.
|
Warning Review limit reached
Next review available in: 27 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe WGC capture path reports adapter details for the captured monitor. The diagnostic tool supports optional system-audio and microphone capture, passes those settings to the helper, logs them, and documents audio-related stop-hang reproduction. ChangesDiagnostic capture controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WGCSession
participant reportCaptureAdapters
participant DXGI
participant DiagnosticEvent
WGCSession->>reportCaptureAdapters: initialize with captured monitor
reportCaptureAdapters->>DXGI: query WGC adapter and enumerate monitor outputs
DXGI-->>reportCaptureAdapters: adapter names and LUIDs
reportCaptureAdapters->>DiagnosticEvent: emit capture-adapter JSON
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 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 242-254: Update reportCaptureAdapters to capture and validate the
HRESULT from EnumAdapters1 and EnumOutputs before dereferencing their COM
pointers: break only on DXGI_ERROR_NOT_FOUND, return immediately on any other
failure, and continue processing successful enumerations. Preserve the existing
adapter/output traversal and run the required Windows smoke tests for display
and window capture.
In `@scripts/diagnostic-tool/diagnostic.mjs`:
- Around line 101-104: Update the user-facing guidance in
scripts/diagnostic-tool/diagnostic.mjs lines 101-104 and
scripts/diagnostic-tool/README.md lines 35-38 to instruct users to rerun
audio-related hangs with --system-audio for system audio, --mic for microphone
audio, or both for mixed-source tests; keep the guidance consistent in both
locations.
🪄 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: 675e50e4-d25d-400a-8eb3-eb94236e6638
📒 Files selected for processing (3)
electron/native/wgc-capture/src/main.cppscripts/diagnostic-tool/README.mdscripts/diagnostic-tool/diagnostic.mjs
1 participant
Neither #252 nor #327 has been reproduced on hardware we control. The two most recent reports arrived with no OS, no version and no trace, and the standalone diagnostic tool cannot exercise the configuration that fails most consistently. This PR changes no recording behaviour; it makes the next report answer a question.
Which GPU the capture landed on
WgcSession::createD3DDevicecallsD3D11CreateDevice(nullptr, ...)— the default adapter — and nothing anywhere asks whether that is the adapter driving the display being captured. There is noEnumAdaptersorEnumOutputsin the helper at all.On a single-GPU machine the question does not arise. On a hybrid laptop, a machine with a discrete card, or one with virtual display adapters, the two can differ — and then every frame WGC delivers has crossed an adapter boundary before the caller touches it. That crossing is driver work on both GPUs, and it is the most plausible remaining candidate for these hangs.
It also fits every data point we have:
The helper now emits, right after the session initialises:
{"event":"capture-adapter","schemaVersion":2,"deviceAdapter":"AMD Radeon(TM) Graphics","monitorAdapter":"NVIDIA GeForce RTX 2050","sameAdapter":false}Compared by LUID, not by description, because two cards of the same model report the same string.
monitorAdapterisnullwhen no DXGI output claims the monitor, which is worth seeing in its own right — on the #252 machine that would point at the virtual display adapters. Window capture resolves its monitor withMonitorFromWindow.Every failure inside the reporting path returns silently. A diagnostic that can end a recording is worse than no diagnostic.
Audio in the diagnostic tool
captureSystemAudioandcaptureMicwere hardcodedfalseindiagnostic.mjs, so the tool could not test the system-audio case — the one that fails consistently for the #252 reporter, and the reason no clean trace of it exists.Audio and video writes take the same sink-writer lock, so a run without audio has nothing to contend with and passes on a machine where the app hangs every time. That is a trap for anyone told "run the diagnostic tool and attach the JSON": it comes back green and proves nothing.
--system-audioand--micnow exist, with the reasoning in the tool's--helpand in the README so a reporter knows when to re-run.Verified
--system-audioproduces an AAC 48 kHz stereo track alongside the video, stop in 152 ms, exit 0.sameAdapter=true— the interesting branch cannot be exercised here, which is rather the point of shipping it.Does not close anything. It is the instrument, not the fix.
Summary by CodeRabbit
New Features
Documentation