fix(wgc): make the Windows recorder DPI-aware and stop guessing its monitor - #351
Conversation
…onitor `cursor-sampler.exe` opted into per-monitor-v2 DPI awareness in 5efe5e6. `wgc-capture.exe` -- the recorder itself -- never did, and its monitor lookup quietly depended on staying unaware: `findMonitorForCapture` matched the bounds it was handed against the rects from `EnumDisplayMonitors`, which a DPI-unaware process gets *virtualized*, divided by the primary display's scale factor whatever monitor they describe. The bounds on the other side came from `Display.bounds`, which is DIPs. Two coordinate spaces, one comparison. Measured on a 1920x1080 panel at 150%: Electron reports `scaleFactor 1.875` (DIP bounds 1024x576) while Win32 virtualization divides by 1.5 (1280x720). So the two spaces already diverge on a *single* display -- the issue's assumption that one monitor works by accident does not hold here. What kept it working was the overlap heuristic, and that is the real defect: a DIP rect and a physical rect anchored on the primary always overlap, so the fallback always answered, correctly, right up to the arrangement where a non-primary display's two origins drift apart by more than a screen width. Then it answered "the primary", and recorded the wrong screen in silence. The fix has to land as a pair, so both sides move to physical at once: - `dpi_awareness.h` is now the one place that says every native helper runs per-monitor-v2 aware. Both binaries include it; wgc-capture refuses to start if it cannot (after the change below, an unaware process is guaranteed to mismatch, so continuing would ship a known-broken state), cursor-sampler warns and carries on because a misplaced overlay still leaves a usable recording. - `helperCoordinates.ts` is the TypeScript half: one `toHelperRect`, used by the capture config *and* by the cursor telemetry, which previously converted on its own while the recorder did not. The platform guard is load-bearing -- `dipToScreenRect` is `@platform win32` and simply absent elsewhere. macOS and Linux are unaffected: the SCK helper reports points, and the PipeWire helper normalizes against its own stream dimensions. - `findMonitorForCapture` drops the overlap heuristic and the fall-to-primary. It matches within 8 px or refuses, printing both rects. The tolerance is not slack: at 175% (scaleFactor 2.1875) Electron's DIP bounds 878x494 round-trip back as 1921x1081, because Chromium scales with enclosing rects in both directions. An exact compare would reject the correct monitor. - The two non-Electron producers of the same wire format stop sending a hardcoded 1920x1080 fiction and omit the bounds instead, which lands on the primary deterministically rather than by accident. Verified on real hardware at 150% and 175%, single 1920x1080 display: - `GetProcessDpiAwareness` on the live process: 0 (UNAWARE) before, 2 (PER_MONITOR_AWARE) after. - Recorded resolution is 1920x1080 with both the old and the new helper, so `GraphicsCaptureItem::Size()` is physical regardless of awareness and this changes no recording's dimensions. - A helper rebuilt without the awareness call reproduces the mismatch against the app's real bounds: `1024x576` vs `enumerated 1280x720`. - Cursor telemetry: 508/508 samples inside [0,1], cx 0.75000 against 0.750000 computed for the parked pointer. - In the exported MP4 the drawn cursor lands 1.5 px from the computed point; the system pointer's tip is on the target pixel in the raw video. Without the conversion it would be 288 px off. - Forcing the process unaware with an app-compat shim exercises the refusal path. Fixes #346
|
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)
📝 WalkthroughWalkthroughThe change aligns Electron and native Windows capture coordinates in physical pixels. It adds shared conversion and DPI-awareness helpers, updates monitor matching and diagnostics, adds conversion tests, and removes fixed display bounds from diagnostic scripts. ChangesWindows DPI-aware capture coordinates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ElectronIPCHandlers
participant toHelperRect
participant wgc-capture
participant findMonitorForCapture
ElectronIPCHandlers->>toHelperRect: Convert DIP display bounds
toHelperRect-->>ElectronIPCHandlers: Return physical helper bounds
ElectronIPCHandlers->>wgc-capture: Send capture configuration
wgc-capture->>findMonitorForCapture: Match physical bounds
findMonitorForCapture-->>wgc-capture: Return monitor or nullptr
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/ipc/handlers.ts (1)
2308-2369: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd boundary tests for both helper-coordinate call sites.
helperCoordinates.test.tstests the utility only. It does not prove that the capture config and cursor telemetry preserve the physical-coordinate contract.
electron/ipc/handlers.ts#L2308-L2369: Add an IPC-level test that asserts the spawned Windows helper config uses converted bounds in both top-level fields andsource.bounds.electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts#L231-L238: Add tests that sampler-provided bounds remain unchanged and Electron-provided bounds are converted.As per coding guidelines, “Add a test for every new behavior in the same package as the code under test.”
🤖 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/ipc/handlers.ts` around lines 2308 - 2369, Add same-package boundary tests for both affected call sites: in electron/ipc/handlers.ts (2308-2369), add an IPC-level test asserting the spawned Windows helper config uses converted physical bounds in both the top-level display fields and source.bounds; in electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts (231-238), add tests confirming sampler-provided bounds remain unchanged while Electron-provided bounds are converted.Source: Coding guidelines
🤖 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/cursor-sampler.cpp`:
- Around line 421-429: Update the startup flow around
enablePerMonitorV2DpiAwareness() to fail with a nonzero status before emitting
ready when DPI awareness cannot be enabled; do not continue sampling with
virtualized cursor coordinates or merely log a warning. Ensure the parent treats
this failure as disabling or dropping the cursor overlay.
---
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 2308-2369: Add same-package boundary tests for both affected call
sites: in electron/ipc/handlers.ts (2308-2369), add an IPC-level test asserting
the spawned Windows helper config uses converted physical bounds in both the
top-level display fields and source.bounds; in
electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts
(231-238), add tests confirming sampler-provided bounds remain unchanged while
Electron-provided bounds are converted.
🪄 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: d857535b-0276-4255-b7b8-8891f387f831
📒 Files selected for processing (11)
electron/ipc/handlers.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.tselectron/native-bridge/helperCoordinates.test.tselectron/native-bridge/helperCoordinates.tselectron/native/wgc-capture/CMakeLists.txtelectron/native/wgc-capture/src/cursor-sampler.cppelectron/native/wgc-capture/src/dpi_awareness.helectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/monitor_utils.cppscripts/diagnostic-tool/diagnostic.mjsscripts/test-windows-wgc-helper.mjs
Review catch on #351. Warning and carrying on was wrong: for a display capture the caller normalizes the sampler's coordinates against a *physical* rect (`toHelperRect`), so an unaware sampler puts every point at 1/scale of its real offset -- exactly #272, silently reintroduced. The comment claimed "the caller can drop the overlay", but nothing told the caller anything. Exiting non-zero before `ready` does tell it: `start()` rejects on the exit-before-ready path, handlers.ts catches it and clears the session, and the recording proceeds with no cursor data. No overlay beats an overlay in the wrong place. Verified with an app-compat shim forcing the process unaware: exit=1, no `ready` emitted, `ERROR: Could not enable per-monitor-v2 DPI awareness` on stderr. Also adds the call-site test the review asked for. The existing test covers `toHelperRect` itself, which cannot prove this file still calls it -- and the recorder and the cursor telemetry each deciding their own space is what #346 was. It pins both branches: Electron bounds get converted, sampler bounds (GetWindowRect, already physical) pass through. Confirmed to go red when the conversion is removed. Not added: an IPC-level test for the capture config. No test in the repo imports electron/ipc/handlers.ts -- it pulls 36 modules and boots Electron transitively through RECORDINGS_DIR -- so making it importable is a larger change than this fix.
|
Both points actioned — the first one caught a real defect in my reasoning, thanks. Fail before I traced it rather than take it on faith, and the mechanism is exactly as described: for a display capture the caller normalizes the sampler's coordinates against a physical rect ( My comment claimed "the caller can drop the overlay", which was wrong — nothing told the caller anything. Exiting non-zero before Verified with an app-compat shim forcing the process unaware: Boundary tests — one added, one declined with a reason. Added Not added: the IPC-level test for the capture config. No test in the repo imports Full suite after the change: 145 files, 1704 passed. |
Review catch on #351. Warning and carrying on was wrong: for a display capture the caller normalizes the sampler's coordinates against a *physical* rect (`toHelperRect`), so an unaware sampler puts every point at 1/scale of its real offset -- exactly #272, silently reintroduced. The comment claimed "the caller can drop the overlay", but nothing told the caller anything. Exiting non-zero before `ready` does tell it: `start()` rejects on the exit-before-ready path, handlers.ts catches it and clears the session, and the recording proceeds with no cursor data. No overlay beats an overlay in the wrong place. Verified with an app-compat shim forcing the process unaware: exit=1, no `ready` emitted, `ERROR: Could not enable per-monitor-v2 DPI awareness` on stderr. Also adds the call-site test the review asked for. The existing test covers `toHelperRect` itself, which cannot prove this file still calls it -- and the recorder and the cursor telemetry each deciding their own space is what #346 was. It pins both branches: Electron bounds get converted, sampler bounds (GetWindowRect, already physical) pass through. Confirmed to go red when the conversion is removed. Not added: an IPC-level test for the capture config. No test in the repo imports electron/ipc/handlers.ts -- it pulls 36 modules and boots Electron transitively through RECORDINGS_DIR -- so making it importable is a larger change than this fix.
1 participant
Summary
cursor-sampler.exeopted into per-monitor-v2 DPI awareness in 5efe5e6.wgc-capture.exe— the recorder itself — never did, and its monitor lookup quietly depended on staying unaware:findMonitorForCapturematched the bounds it was handed against the rects fromEnumDisplayMonitors, which a DPI-unaware process gets virtualized, divided by the primary display's scale factor whatever monitor they describe. The bounds on the other side came fromDisplay.bounds, which is DIPs. Two coordinate spaces, one comparison.Measured on a 1920×1080 panel at 150 %: Electron reports
scaleFactor 1.875(DIP bounds1024x576) while Win32 virtualization divides by 1.5 (1280x720). The two spaces already diverge on a single display — the issue's assumption that one monitor works by accident does not hold here.What kept it working was the overlap heuristic, and that is the real defect. A DIP rect and a physical rect anchored on the primary always overlap, so the fallback always answered — correctly, right up to the arrangement where a non-primary display's two origins drift apart by more than a screen width. Then it answered "the primary", and recorded the wrong screen in silence.
What changed
The fix lands as a pair, so both sides move to physical at once.
dpi_awareness.his now the one place that states every native helper runs per-monitor-v2 aware. Both binaries include it.wgc-capturerefuses to start if it cannot — after the change below, an unaware process is guaranteed to mismatch, so continuing would ship a known-broken state.cursor-samplerwarns and carries on, because a misplaced overlay still leaves a usable recording.helperCoordinates.tsis the TypeScript half: onetoHelperRect, used by the capture config and by the cursor telemetry, which previously converted on its own while the recorder did not. The platform guard is load-bearing —dipToScreenRectis@platform win32and simply absent elsewhere.findMonitorForCapturedrops the overlap heuristic and the fall-to-primary. It matches within 8 px or refuses, printing both rects. The tolerance is not slack: at 175 % (scaleFactor 2.1875) Electron's DIP bounds878x494round-trip back as1921x1081, because Chromium scales with enclosing rects in both directions. An exact compare would reject the correct monitor.1920x1080fiction and omit the bounds instead, which lands on the primary deterministically rather than by accident.macOS and Linux are unaffected: the ScreenCaptureKit helper reports points, the same space Electron uses, and the PipeWire helper normalizes against its own stream dimensions and is never handed a rect.
Behaviour change worth reviewing
The helper now refuses where it used to guess. In practice that fires when the display topology changes inside the ~1 s between reading
screen.getAllDisplays()and the helper'sEnumDisplayMonitors— plug/unplug, resolution or scale change, docking. The user sees "recording didn't start" and reclicks, instead of getting a video of the wrong screen an hour later. That trade is the point of the PR, but it is a trade.Not verifiable on the hardware I have: mixed-DPI multi-monitor. The 8 px tolerance comes from an arithmetic sweep (origins −6000..8000 × common panel sizes, floor and round anchoring: worst 4 px up to 300 % scaling, 8 px at 450 %), not from hardware.
Related issue
Fixes #346
Type of change
Release impact
Desktop impact
Screenshots / video
The green box is drawn at the target point (physical
1440,538), zoom ×5 on raw pixels.Frame geometry detected automatically in the export: content rect
x 192..1727, y 108..971(1536×864, ratio 1.7778). Expected cursor(1344, 538.4); measured stem at x 1345.5 (1.5 px off) and glyph centre y 535 (3 px). Without the conversion it would be at x 1632 — 288 px to the right.Testing
Real hardware, single 1920×1080 display, driven through the app's HUD at 150 % and 175 % scaling.
The awareness flip
GetProcessDpiAwarenesson the live process: 0 (UNAWARE) with the old binary, 2 (PER_MONITOR_AWARE) with the new one.GraphicsCaptureItem::Size()is physical regardless of awareness — this changes no recording's dimensions. That was the highest-risk unknown; it is now measured, not assumed./MTinvariant from the 1.9.1 Store rejection holds).The bug, reproduced
requested 0,0 1024x576vsenumerated 0,0 1280x720.1920x1080.End to end through the app
cx 0.75000against0.750000computed for the parked pointer.Refusal paths
ERROR: Could not enable per-monitor-v2 DPI awareness.878x494,1024x576, a phantom monitor at5760,0): refused, both rects in the message.1921x1081at 175 %): accepted, three consecutive clean runs.Suite
npx tsc --noEmit,npx tsc -p tsconfig.test.json --noEmit,npm run lint,node scripts/check-docs.mjs: clean.npm run test: 144 files, 1702 passed, 4 skipped.npm run build:native:win(no CI job compiles them on a PR).Summary by CodeRabbit
Bug Fixes
Tests