-
Notifications
You must be signed in to change notification settings - Fork 81
fix(wgc): make the Windows recorder DPI-aware and stop guessing its monitor #351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
113 changes: 113 additions & 0 deletions
113
electron/native-bridge/cursor/recording/windowsNativeRecordingSession.test.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<string, unknown>) => { | ||
| 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); | ||
| }); | ||
| }); |
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(); | ||
| }); | ||
| }); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| #pragma once | ||
|
|
||
| #include <Windows.h> | ||
|
|
||
| // 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); | ||
| } |
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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.