fix(cua-driver-rs)(windows): apply PMv2 DPI manifest (RT_MANIFEST type 24) and remove double scaling in capture paths - #1883
Conversation
…4 and drop double scaling in capture paths The .rc script declared the manifest type as the identifier RT_MANIFEST, which the resource compiler emits as a custom string-typed resource the Windows loader never applies - the daemon kept starting DPI-unaware. Use the numeric type 24 so the Per-Monitor V2 manifest actually loads. With PMv2 active, GetWindowRect/GetSystemMetrics return physical pixels, so remove the DPI/96 scaling in screenshot_via_screen_region, screenshot_window_bytes_with_occlusion_unsafe and screenshot_display_bytes that would otherwise shift/oversize captures by the scale factor. Fixes trycua#1879
|
@tobitege is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR fixes Per-Monitor V2 DPI awareness embedding and eliminates double-scaling in Windows screenshot capture. It corrects the manifest resource compiler syntax, removes compensating DPI scaling from three capture paths now that Windows APIs return physical coordinates directly, and updates documentation to reflect physical-pixel semantics. ChangesPer-Monitor V2 DPI Awareness and Capture Coordinate Alignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs (1)
3886-3894:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix coordinate-space wording mismatch in
get_screen_sizeoutput.The updated comment correctly says these are physical pixels, but the returned message still says
points(Line 3894). That makes the tool output contradictory and can mislead callers about click/screenshot coordinate space.Suggested patch
- ToolResult::text(format!("✅ Main display: {w}x{h} points @ {scale}x")) + ToolResult::text(format!("✅ Main display: {w}x{h} physical pixels @ {scale}x"))🤖 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 `@libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs` around lines 3886 - 3894, The tool prints a misleading unit: the code computes physical pixel dimensions (GetSystemMetrics) but the message says "points"; update the output in the block around GetSystemMetrics/GetDpiForSystem (the get_screen_size logic that returns ToolResult::text(...)) to use "pixels" instead of "points" (e.g., change the formatted string emitted by ToolResult::text to "Main display: {w}x{h} pixels @ {scale}x") so the wording matches the comment and actual coordinate space.
🤖 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.
Outside diff comments:
In `@libs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs`:
- Around line 3886-3894: The tool prints a misleading unit: the code computes
physical pixel dimensions (GetSystemMetrics) but the message says "points";
update the output in the block around GetSystemMetrics/GetDpiForSystem (the
get_screen_size logic that returns ToolResult::text(...)) to use "pixels"
instead of "points" (e.g., change the formatted string emitted by
ToolResult::text to "Main display: {w}x{h} pixels @ {scale}x") so the wording
matches the comment and actual coordinate space.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5889ffb-fec2-4eb0-a012-3a0b3397b4f9
📒 Files selected for processing (4)
libs/cua-driver/rust/crates/cua-driver/build.rslibs/cua-driver/rust/crates/cua-driver/cua-driver.rclibs/cua-driver/rust/crates/platform-windows/src/capture.rslibs/cua-driver/rust/crates/platform-windows/src/tools/impl_.rs
… not points The values are physical pixels under PMv2; the tool text and description still said points. Update the wording and the parity example parser.
|
Merged — thanks @tobitege, excellent investigation. Root cause was spot on: |
Summary
cua-driver.exekept starting DPI-unaware. The .rc script now uses the numeric resource type24(RT_MANIFEST) with ID1, which the loader actually picks up.DPI / 96scaling in the three Windows capture paths (screenshot_via_screen_region,screenshot_window_bytes_with_occlusion_unsafe,screenshot_display_bytes). Under PMv2,GetWindowRect/GetSystemMetricsreturn physical pixels - the same unitBitBlt/PrintWindowoperate in - so scaling them again would shift and oversize captures by the display scale factor once the manifest loads.build.rsand theget_screen_sizetool that claimed these APIs return logical pixels under PMv2.All changes are confined to Windows-only code: the
platform-windowscrate is only compiled on Windows (cfg-gated dependency), and the .rc resource is only embedded inside the#[cfg(target_os = windows)]build step. macOS and Linux are untouched.Verification (Windows 11, 3840x2160 @ 125%)
24, ID1; running process reportsGetProcessDpiAwareness = 2(PER_MONITOR). The installed 0.5.2 binary as a negative control: string-typed resource only, awareness0.get_screen_sizenow returns3840x2160 @ 1.25x(was3072x1728 @ 1xvirtualized).cargo check/cargo buildclean; unit tests pass. The 6 failingmcp_protocol_testcases fail identically on unpatched main in this environment (pre-existing, unrelated to this change).Summary by CodeRabbit