From 2881e861ece9ab18dd02d37736d043dab4aea808 Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Wed, 12 Aug 2026 14:48:11 +0200 Subject: [PATCH 1/2] fix(wgc): make the Windows recorder DPI-aware and stop guessing its monitor `cursor-sampler.exe` opted into per-monitor-v2 DPI awareness in 5efe5e62. `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 --- electron/ipc/handlers.ts | 21 +++-- .../windowsNativeRecordingSession.ts | 10 +-- .../native-bridge/helperCoordinates.test.ts | 50 ++++++++++++ electron/native-bridge/helperCoordinates.ts | 36 +++++++++ electron/native/wgc-capture/CMakeLists.txt | 2 + .../native/wgc-capture/src/cursor-sampler.cpp | 12 ++- .../native/wgc-capture/src/dpi_awareness.h | 35 +++++++++ electron/native/wgc-capture/src/main.cpp | 13 ++++ .../native/wgc-capture/src/monitor_utils.cpp | 76 ++++++++++++------- scripts/diagnostic-tool/diagnostic.mjs | 11 +-- scripts/test-windows-wgc-helper.mjs | 10 +-- 11 files changed, 225 insertions(+), 51 deletions(-) create mode 100644 electron/native-bridge/helperCoordinates.test.ts create mode 100644 electron/native-bridge/helperCoordinates.ts create mode 100644 electron/native/wgc-capture/src/dpi_awareness.h diff --git a/electron/ipc/handlers.ts b/electron/ipc/handlers.ts index 7d63b934b..14262b11a 100644 --- a/electron/ipc/handlers.ts +++ b/electron/ipc/handlers.ts @@ -71,6 +71,7 @@ import { createCursorRecordingSession } from "../native-bridge/cursor/recording/ import { requestMacCursorAccessibilityAccess } from "../native-bridge/cursor/recording/macNativeCursorRecordingSession"; import { findPipeWireCursorHelperPath } from "../native-bridge/cursor/recording/pipeWireCursorRecordingSession"; import type { CursorRecordingSession } from "../native-bridge/cursor/recording/session"; +import { toHelperRect } from "../native-bridge/helperCoordinates"; import { terminateNativeWindowsCapture, waitForNativeWindowsCaptureStop, @@ -2304,6 +2305,11 @@ export function registerIpcHandlers( null) : getSelectedDisplay(); const bounds = sourceDisplay?.bounds ?? getSelectedSourceBounds(); + // `bounds` is DIPs; the helper matches it against physical monitor rects + // (getopenscreen/openscreen#346). Converted here, at the wire, and not in + // `getSelectedSourceBounds` — the cursor session shares that getter and + // converts on its own side. + const helperBounds = toHelperRect(bounds); const displayId = typeof request.source.displayId === "number" && Number.isFinite(request.source.displayId) ? request.source.displayId @@ -2332,10 +2338,10 @@ export function registerIpcHandlers( fps: request.video.fps, videoWidth: request.video.width, videoHeight: request.video.height, - displayX: bounds.x, - displayY: bounds.y, - displayW: bounds.width, - displayH: bounds.height, + displayX: helperBounds.x, + displayY: helperBounds.y, + displayW: helperBounds.width, + displayH: helperBounds.height, hasDisplayBounds: true, captureSystemAudio: request.audio.system.enabled, captureMic: request.audio.microphone.enabled, @@ -2360,7 +2366,7 @@ export function registerIpcHandlers( sourceId: request.source.sourceId, displayId: Number.isFinite(displayId) ? displayId : null, windowHandle: request.source.windowHandle ?? null, - bounds, + bounds: helperBounds, }, video: request.video, audio: request.audio, @@ -2377,7 +2383,10 @@ export function registerIpcHandlers( webcam: request.webcam, encoder: { preferSoftwareEncoder }, cursor: { mode: cursorCaptureMode }, - bounds, + // Both spaces, deliberately: the helper's own errors quote the physical + // rect, and a report that only carried the DIP one would be read against + // numbers it never saw (getopenscreen/openscreen#346). + bounds: { dip: bounds, helper: helperBounds }, sourceId: selectedSource?.id ?? null, usedDisplayMatch: Boolean(sourceDisplay), outputPath, diff --git a/electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts b/electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts index d2d2f0cc8..87be87d68 100644 --- a/electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts +++ b/electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts @@ -9,6 +9,7 @@ import type { CursorRecordingSample, NativeCursorAsset, } from "../../../../src/native/contracts"; +import { toHelperRect } from "../../helperCoordinates"; import type { CursorRecordingSession } from "./session"; import type { WindowsCursorEvent, @@ -232,12 +233,9 @@ export class WindowsNativeRecordingSession implements CursorRecordingSession { // The cursor-sampler reports raw x/y in physical screen pixels (Win32 // GetCursorInfo). `payload.bounds` from the sampler's GetWindowRect is also // physical, so use it as-is. Bounds from Electron's `screen` API (or the - // fallback for display captures) are in DIPs — convert to physical screen - // coordinates via `dipToScreenRect`, which correctly handles the virtual-screen - // origin across multi-monitor and mixed-DPI setups (a naive - // `bounds.x * scaleFactor` would misplace the origin on non-primary - // displays). - const physicalBounds = payload.bounds != null ? bounds : screen.dipToScreenRect(null, bounds); + // fallback for display captures) are in DIPs, so they go through the shared + // conversion the capture helper's bounds also use. + const physicalBounds = payload.bounds != null ? bounds : toHelperRect(bounds); const physicalX = physicalBounds.x; const physicalY = physicalBounds.y; const width = Math.max(1, physicalBounds.width); diff --git a/electron/native-bridge/helperCoordinates.test.ts b/electron/native-bridge/helperCoordinates.test.ts new file mode 100644 index 000000000..490134fac --- /dev/null +++ b/electron/native-bridge/helperCoordinates.test.ts @@ -0,0 +1,50 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { toHelperRect } from "./helperCoordinates"; + +const dipToScreenRect = vi.fn(); + +vi.mock("electron", () => ({ + screen: { + get dipToScreenRect() { + return dipToScreenRect; + }, + }, +})); + +/** + * The bug this pins is invisible at 100% scaling, which is what every dev machine + * and all of CI runs at — so the assertion cannot be "the numbers come out right", + * it has to be "the conversion is reached at all, and only where it exists". + * `dipToScreenRect` is `@platform win32` in Electron: on darwin it is not a + * function, so calling it unconditionally would throw rather than mis-convert. + */ +describe("toHelperRect", () => { + const REAL_PLATFORM = process.platform; + const setPlatform = (value: NodeJS.Platform) => + Object.defineProperty(process, "platform", { value, configurable: true }); + + afterEach(() => { + setPlatform(REAL_PLATFORM); + dipToScreenRect.mockReset(); + }); + + const DIP = { x: 1920, y: 0, width: 2560, height: 1440 }; + + it("converts DIP bounds to physical screen pixels on Windows", () => { + const physical = { x: 3840, y: 0, width: 5120, height: 2880 }; + dipToScreenRect.mockReturnValue(physical); + setPlatform("win32"); + + expect(toHelperRect(DIP)).toEqual(physical); + // `null` as the window: scale relative to the display nearest the rect, + // which is the part that holds up on a mixed-DPI desktop. + expect(dipToScreenRect).toHaveBeenCalledWith(null, DIP); + }); + + it.each(["darwin", "linux"] as const)("passes the rect through on %s", (platform) => { + setPlatform(platform); + + expect(toHelperRect(DIP)).toBe(DIP); + expect(dipToScreenRect).not.toHaveBeenCalled(); + }); +}); diff --git a/electron/native-bridge/helperCoordinates.ts b/electron/native-bridge/helperCoordinates.ts new file mode 100644 index 000000000..5cda92e4c --- /dev/null +++ b/electron/native-bridge/helperCoordinates.ts @@ -0,0 +1,36 @@ +import { type Rectangle, screen } from "electron"; + +/** + * Converts a rect from Electron's coordinate space into the one OpenScreen's + * native capture and cursor helpers speak. This is the single place that knows + * the difference; every rect crossing into a helper goes through it. + * + * Electron's `screen` module reports display geometry in **DIPs**. The Windows + * helpers (`wgc-capture.exe`, `cursor-sampler.exe`) opt into per-monitor-v2 DPI + * awareness — see `electron/native/wgc-capture/src/dpi_awareness.h` — so Win32 + * hands them unvirtualized **physical pixels**. The two spaces are equal at 100% + * scaling and only there, which is how both bugs of this class shipped: #272 + * (cursor drawn short of its real position) and #346 (`findMonitorForCapture` + * comparing DIP bounds against virtualized monitor rects, and silently recording + * the primary display instead of the chosen one). + * + * macOS and Linux need no conversion. The ScreenCaptureKit helper reports its + * capture frame in points, the same space Electron's `screen` uses, and the + * PipeWire helper normalizes the cursor against its own stream dimensions and is + * never handed a rect at all. The platform check is not cosmetic: Electron marks + * `dipToScreenRect` `@platform win32`, so it is simply absent from `screen` + * elsewhere. + * + * @param dipBounds a rect in Electron DIPs, e.g. `Display.bounds`. + */ +export function toHelperRect(dipBounds: Rectangle): Rectangle { + if (process.platform !== "win32") { + return dipBounds; + } + + // Not `x * scaleFactor`: that misplaces the origin of every non-primary + // display. `dipToScreenRect(null, rect)` scales relative to the display + // nearest the rect, which is what makes it correct on mixed-DPI desktops — + // the arrangement #346 is about. + return screen.dipToScreenRect(null, dipBounds); +} diff --git a/electron/native/wgc-capture/CMakeLists.txt b/electron/native/wgc-capture/CMakeLists.txt index d1f55066b..c2947df77 100644 --- a/electron/native/wgc-capture/CMakeLists.txt +++ b/electron/native/wgc-capture/CMakeLists.txt @@ -38,6 +38,7 @@ set(CMAKE_CXX_EXTENSIONS OFF) add_executable(wgc-capture src/audio_sample_utils.cpp src/audio_sample_utils.h + src/dpi_awareness.h src/dshow_webcam_capture.cpp src/dshow_webcam_capture.h src/main.cpp @@ -74,6 +75,7 @@ target_link_libraries(wgc-capture PRIVATE add_executable(cursor-sampler src/cursor-sampler.cpp + src/dpi_awareness.h ) target_compile_definitions(cursor-sampler PRIVATE diff --git a/electron/native/wgc-capture/src/cursor-sampler.cpp b/electron/native/wgc-capture/src/cursor-sampler.cpp index fc68ebf28..17981045f 100644 --- a/electron/native/wgc-capture/src/cursor-sampler.cpp +++ b/electron/native/wgc-capture/src/cursor-sampler.cpp @@ -2,6 +2,8 @@ #include #include +#include "dpi_awareness.h" + #include #include #include @@ -416,7 +418,15 @@ int main(int argc, char* argv[]) { // capture and the consumer both work in physical pixels. On any scaled // display the cursor then lands short of its real position, by more the // further it is from the origin (getopenscreen/openscreen#272). - SetProcessDpiAwarenessContext(DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2); + // + // Unlike wgc-capture, a failure here is not worth aborting for: a cursor + // overlay drawn at the wrong offset still leaves a usable recording, and the + // caller can drop the overlay. Say so and carry on. + if (!enablePerMonitorV2DpiAwareness()) { + std::cerr << "WARNING: Could not enable per-monitor-v2 DPI awareness; " + "cursor positions will be wrong on scaled displays" + << std::endl; + } if (argc < 2) { std::cerr << "Usage: cursor-sampler [windowHandle]" << std::endl; diff --git a/electron/native/wgc-capture/src/dpi_awareness.h b/electron/native/wgc-capture/src/dpi_awareness.h new file mode 100644 index 000000000..b470df014 --- /dev/null +++ b/electron/native/wgc-capture/src/dpi_awareness.h @@ -0,0 +1,35 @@ +#pragma once + +#include + +// Every OpenScreen native helper runs per-monitor-v2 DPI aware, and this is the +// one place that says so. +// +// A DPI-unaware process does not get real coordinates from Win32: every rect and +// point it reads back is *virtualized*, divided by the PRIMARY display's scale +// factor whatever monitor it actually describes. The helper and its TypeScript +// caller then live in two different coordinate spaces that happen to coincide at +// 100% scaling -- which is exactly why both bugs of this class shipped unnoticed +// (getopenscreen/openscreen#272, cursor offset; #346, wrong monitor recorded). +// +// The TypeScript side converts Electron's DIP rects before handing them over, in +// electron/native-bridge/helperCoordinates.ts. The contract both sides implement +// is: helpers speak physical screen pixels, always. +// +// Returns false when the process could not be put in that state. In practice +// that only happens when something outside our control -- an app-compat +// "override high DPI scaling behaviour" setting on the .exe -- already pinned a +// different context. Callers decide what that means for them; nothing here can +// safely *guess* which space the numbers are in. +inline bool enablePerMonitorV2DpiAwareness() { + if (SetProcessDpiAwarenessContext(DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2)) { + return true; + } + + // ERROR_ACCESS_DENIED means an awareness context was already selected for + // this process. That is fine when it is the one we wanted, and fatal when it + // is not, so check rather than assuming either way. + return GetLastError() == ERROR_ACCESS_DENIED && + AreDpiAwarenessContextsEqual(GetThreadDpiAwarenessContext(), + DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2); +} diff --git a/electron/native/wgc-capture/src/main.cpp b/electron/native/wgc-capture/src/main.cpp index a17926966..1479bc842 100644 --- a/electron/native/wgc-capture/src/main.cpp +++ b/electron/native/wgc-capture/src/main.cpp @@ -1,4 +1,5 @@ #include "audio_sample_utils.h" +#include "dpi_awareness.h" #include "mf_encoder.h" #include "monitor_utils.h" #include "wasapi_loopback_capture.h" @@ -641,6 +642,18 @@ void readCaptureCommands(CaptureControl& control, const std::function -#include +#include #include namespace { @@ -25,22 +24,31 @@ std::vector enumerateMonitors() { return monitors; } -bool rectMatchesBounds(const RECT& rect, const MonitorBounds& bounds) { - return rect.left == bounds.x && - rect.top == bounds.y && - (rect.right - rect.left) == bounds.width && - (rect.bottom - rect.top) == bounds.height; -} +// The bounds arrive as physical pixels recovered from Electron's DIP rect, and +// that round trip is lossy: Chromium scales with *enclosing* rectangles in both +// directions, so it rounds outward twice. Measured on a 1920x1080 panel at 175% +// (scaleFactor 2.1875): DIP 878x494 comes back as 1921x1081, one pixel proud in +// each dimension. An exact compare would reject the right monitor on an entirely +// ordinary setup. +// +// So match with a tolerance. Scanning every integer origin against the common +// panel sizes puts the worst round-trip error at 4 px up to 300% scaling and +// 8 px at 450%, so 8 covers the range Windows actually offers rather than +// leaving margin on top of it. It stays far below any real coordinate-space +// divergence: the smallest this code guards against is a 1920x1080 display at +// 150%, where the two spaces are 256 px apart, and #346's multi-monitor case is +// off by a full screen width. A tolerance large enough to absorb *that* would +// put the bug back, so this number is a ceiling, not a dial to turn up. +constexpr int64_t kBoundsTolerancePx = 8; -int64_t overlapArea(const RECT& rect, const MonitorBounds& bounds) { - const LONG left = std::max(rect.left, bounds.x); - const LONG top = std::max(rect.top, bounds.y); - const LONG right = std::min(rect.right, bounds.x + bounds.width); - const LONG bottom = std::min(rect.bottom, bounds.y + bounds.height); - if (right <= left || bottom <= top) { - return 0; - } - return static_cast(right - left) * static_cast(bottom - top); +bool rectMatchesBounds(const RECT& rect, const MonitorBounds& bounds) { + // int64_t, not LONG: `bounds` is parsed straight out of the JSON config with + // no range check, and a subtraction of two hostile ints would overflow. + const auto close = [](int64_t a, int64_t b) { return (a > b ? a - b : b - a) <= kBoundsTolerancePx; }; + return close(rect.left, bounds.x) && + close(rect.top, bounds.y) && + close(rect.right - rect.left, bounds.width) && + close(rect.bottom - rect.top, bounds.height); } } // namespace @@ -53,7 +61,12 @@ HMONITOR findMonitorForCapture(int64_t displayId, const MonitorBounds* bounds) { // Electron's display_id is not stable across all Windows capture backends. // Bounds are the most reliable contract because they come from Electron's - // selected display and match the WGC monitor coordinate space. + // selected display. + // + // They only match these rects because the caller converts them out of DIPs + // first and this process is per-monitor-v2 aware (see dpi_awareness.h). Both + // halves are load-bearing: drop either and no monitor matches at all + // (getopenscreen/openscreen#346). if (bounds && bounds->width > 0 && bounds->height > 0) { for (const auto& candidate : monitors) { if (rectMatchesBounds(candidate.rect, *bounds)) { @@ -61,18 +74,25 @@ HMONITOR findMonitorForCapture(int64_t displayId, const MonitorBounds* bounds) { } } - HMONITOR bestMonitor = nullptr; - int64_t bestArea = 0; + // No guessing from here. This used to fall back to "whichever monitor + // overlaps the requested rect most", which sounds harmless and is how + // #346 stayed hidden for so long: on the primary, the two spaces share + // the origin and differ only by a scale, so one rect always contains the + // other and the heuristic always answered. It kept answering, correctly, + // right up to the arrangement where a non-primary display's two origins + // drift apart by more than a screen width -- and then it silently + // answered "the primary". A guess that is usually right is worse than a + // refusal, because nobody ever finds out. + std::cerr << "ERROR: No monitor matches the requested bounds " + << bounds->x << "," << bounds->y << " " << bounds->width << "x" << bounds->height + << "; enumerated:"; for (const auto& candidate : monitors) { - const int64_t area = overlapArea(candidate.rect, *bounds); - if (area > bestArea) { - bestArea = area; - bestMonitor = candidate.monitor; - } - } - if (bestMonitor) { - return bestMonitor; + std::cerr << " " << candidate.rect.left << "," << candidate.rect.top << " " + << (candidate.rect.right - candidate.rect.left) << "x" + << (candidate.rect.bottom - candidate.rect.top); } + std::cerr << std::endl; + return nullptr; } // Best-effort fallback for helpers invoked without bounds. Some callers pass diff --git a/scripts/diagnostic-tool/diagnostic.mjs b/scripts/diagnostic-tool/diagnostic.mjs index 373c8f4b2..40338b1a7 100644 --- a/scripts/diagnostic-tool/diagnostic.mjs +++ b/scripts/diagnostic-tool/diagnostic.mjs @@ -144,11 +144,12 @@ function buildConfig(opts) { fps: 30, videoWidth: 1280, videoHeight: 720, - displayX: 0, - displayY: 0, - displayW: 1920, - displayH: 1080, - hasDisplayBounds: true, + // No Electron here, so no real display rect to send. The helper reads these + // as physical pixels, and a hardcoded 1920x1080 only happens to hit on an + // unscaled 1080p primary. Omitting them skips the bounds match entirely and + // lands on the primary monitor deterministically, which is what this tool + // wanted all along (#346). + hasDisplayBounds: false, captureSystemAudio: opts.systemAudio, captureMic: opts.mic, captureCursor: false, diff --git a/scripts/test-windows-wgc-helper.mjs b/scripts/test-windows-wgc-helper.mjs index ba7bb3259..e1dc48148 100644 --- a/scripts/test-windows-wgc-helper.mjs +++ b/scripts/test-windows-wgc-helper.mjs @@ -387,11 +387,11 @@ const config = { fps: 30, videoWidth: 1280, videoHeight: 720, - displayX: 0, - displayY: 0, - displayW: 1920, - displayH: 1080, - hasDisplayBounds: true, + // Same reasoning as scripts/diagnostic-tool/diagnostic.mjs: without Electron + // there is no honest display rect to send, and the helper reads these as + // physical pixels. Omitting them lands on the primary monitor + // deterministically instead of by accident (#346). + hasDisplayBounds: false, captureSystemAudio: WITH_SYSTEM_AUDIO, captureMic: WITH_MICROPHONE, captureCursor: CAPTURE_CURSOR, From cd08855eb12a014c69200cdb2933c01c36f6e397 Mon Sep 17 00:00:00 2001 From: Etienne Lescot Date: Wed, 12 Aug 2026 15:17:42 +0200 Subject: [PATCH 2/2] fix(cursor): fail the sampler when DPI awareness cannot be set 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. --- .../windowsNativeRecordingSession.test.ts | 113 ++++++++++++++++++ .../native/wgc-capture/src/cursor-sampler.cpp | 14 ++- 2 files changed, 121 insertions(+), 6 deletions(-) create mode 100644 electron/native-bridge/cursor/recording/windowsNativeRecordingSession.test.ts diff --git a/electron/native-bridge/cursor/recording/windowsNativeRecordingSession.test.ts b/electron/native-bridge/cursor/recording/windowsNativeRecordingSession.test.ts new file mode 100644 index 000000000..61541f55c --- /dev/null +++ b/electron/native-bridge/cursor/recording/windowsNativeRecordingSession.test.ts @@ -0,0 +1,113 @@ +import { EventEmitter } from "node:events"; +import { Readable } from "node:stream"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const spawn = vi.fn(); +const dipToScreenRect = vi.fn(); + +vi.mock("node:child_process", () => ({ spawn: (...args: unknown[]) => spawn(...args) })); +vi.mock("electron", () => ({ + app: { getAppPath: () => "C:\\app", isPackaged: false }, + screen: { + dipToScreenRect: (...args: unknown[]) => dipToScreenRect(...args), + screenToDipPoint: (p: unknown) => p, + getDisplayNearestPoint: () => ({ scaleFactor: 1, bounds: DIP_BOUNDS }), + getPrimaryDisplay: () => ({ bounds: DIP_BOUNDS }), + }, +})); + +import { WindowsNativeRecordingSession } from "./windowsNativeRecordingSession"; + +/** A 1920x1080 panel at 187.5%, the arrangement measured while fixing #346. */ +const DIP_BOUNDS = { x: 0, y: 0, width: 1024, height: 576 }; +const PHYSICAL_BOUNDS = { x: 0, y: 0, width: 1920, height: 1080 }; + +function fakeHelper() { + // Push-only streams: the test feeds them, so `_read` has nothing to do. + const noPull = () => undefined; + const stdout = new Readable({ read: noPull }); + const stderr = new Readable({ read: noPull }); + const child = Object.assign(new EventEmitter(), { stdout, stderr, pid: 4242, kill: vi.fn() }); + return { child, stdout }; +} + +/** + * The unit test on `toHelperRect` proves the conversion works; it cannot prove + * this file still calls it. That is the half that broke in #346 — the recorder + * and the cursor telemetry each decided for themselves which space they were in + * — so pin the two branches that decision has. + */ +describe("WindowsNativeRecordingSession bounds handling", () => { + const REAL_PLATFORM = process.platform; + const setPlatform = (value: NodeJS.Platform) => + Object.defineProperty(process, "platform", { value, configurable: true }); + + beforeEach(() => { + setPlatform("win32"); + // First candidate in the lookup, so nothing has to exist on disk but this. + process.env.OPENSCREEN_CURSOR_SAMPLER_EXE = __filename; + dipToScreenRect.mockReturnValue(PHYSICAL_BOUNDS); + vi.spyOn(console, "error").mockImplementation(() => undefined); + vi.spyOn(console, "info").mockImplementation(() => undefined); + }); + + afterEach(() => { + setPlatform(REAL_PLATFORM); + process.env.OPENSCREEN_CURSOR_SAMPLER_EXE = undefined; + spawn.mockReset(); + dipToScreenRect.mockReset(); + vi.restoreAllMocks(); + }); + + const run = async (sample: Record) => { + const { child, stdout } = fakeHelper(); + spawn.mockReturnValue(child); + const session = new WindowsNativeRecordingSession({ + getDisplayBounds: () => DIP_BOUNDS, + maxSamples: 10, + sampleIntervalMs: 33, + startTimeMs: 1000, + }); + const started = session.start(); + stdout.push(`${JSON.stringify({ type: "ready", timestampMs: 1000 })}\n`); + await started; + stdout.push(`${JSON.stringify(sample)}\n`); + await new Promise((resolve) => setImmediate(resolve)); + return (await session.stop()).samples; + }; + + it("normalizes a display capture against the physical rect, not the DIP one", async () => { + const samples = await run({ + type: "sample", + timestampMs: 1100, + x: 1440, + y: 538, + visible: true, + handle: null, + asset: null, + }); + + expect(dipToScreenRect).toHaveBeenCalledWith(null, DIP_BOUNDS); + // 1440/1920, not 1440/1024 — the latter is 1.40625 and lands off-frame. + expect(samples[0].cx).toBeCloseTo(0.75, 5); + expect(samples[0].cy).toBeCloseTo(538 / 1080, 5); + }); + + it("uses the sampler's own bounds as-is for a window capture", async () => { + const samples = await run({ + type: "sample", + timestampMs: 1100, + x: 900, + y: 500, + visible: true, + handle: "0x10003", + bounds: { x: 100, y: 100, width: 800, height: 600 }, + asset: null, + }); + + // GetWindowRect is already physical, so converting it again would be wrong. + expect(dipToScreenRect).not.toHaveBeenCalled(); + expect(samples[0].cx).toBeCloseTo((900 - 100) / 800, 5); + expect(samples[0].cy).toBeCloseTo((500 - 100) / 600, 5); + }); +}); diff --git a/electron/native/wgc-capture/src/cursor-sampler.cpp b/electron/native/wgc-capture/src/cursor-sampler.cpp index 17981045f..aa58a1cd7 100644 --- a/electron/native/wgc-capture/src/cursor-sampler.cpp +++ b/electron/native/wgc-capture/src/cursor-sampler.cpp @@ -419,13 +419,15 @@ int main(int argc, char* argv[]) { // display the cursor then lands short of its real position, by more the // further it is from the origin (getopenscreen/openscreen#272). // - // Unlike wgc-capture, a failure here is not worth aborting for: a cursor - // overlay drawn at the wrong offset still leaves a usable recording, and the - // caller can drop the overlay. Say so and carry on. + // Bail out rather than sample, and bail out before `ready`: for a display + // capture the caller normalizes these coordinates against a *physical* rect, + // so an unaware sampler puts every point at 1/scale of its real offset. The + // caller turns an exit-before-ready into "record without cursor data" + // (handlers.ts catches the failed start and drops the session), which is the + // outcome we want -- no overlay beats an overlay in the wrong place. if (!enablePerMonitorV2DpiAwareness()) { - std::cerr << "WARNING: Could not enable per-monitor-v2 DPI awareness; " - "cursor positions will be wrong on scaled displays" - << std::endl; + std::cerr << "ERROR: Could not enable per-monitor-v2 DPI awareness" << std::endl; + return 1; } if (argc < 2) {