From 4d78f916ce23ce525c4a7bae44cee10397ea0ac9 Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Fri, 4 Sep 2026 00:26:43 -0300 Subject: [PATCH] fix(drm): turn the cursor by the output's real angle, and guess its hotspot upright Two things were wrong with a rotated output's cursor, and the maintainer's physical test found both. The bare-metal hotspot is a guess - the top-left of the sprite's opaque box, since HOTSPOT_X/Y only exist on para-virtualised drivers - and it was guessed on the sprite as scanned out, then mapped as if it were a point on it. A turned arrow's tip is never in that corner: at 90 the guess is off by the arrow's width along x, small enough to pass as fine, and at 270 by its height along y, which is what was noticed. Measured on an i915 box with a 64x64 Adwaita arrow, hotspot against tip: +11,0 at 90 and 0,+18 at 270 before, 0,0 at every angle after. A guessed hotspot is now guessed on the upright sprite, in the consumer, and the wire says whether the driver measured it so a real one still maps as a point. And 180 was clamped to 0 for the whole session because a hardware-rotated primary plane scans out upright. That holds for the frame and not for the cursor: the compositor pre-rotates the sprite in software either way, so it arrived upside down. The frame keeps the clamp; the cursor gets the real angle. (cherry picked from commit 6a73100b4783fbf9c8f43e0dc7651b7568db9ea9) --- libs/scrap/src/common/drm_reader.rs | 67 +++++++------ src/ipc.rs | 2 + src/ipc/drm.rs | 4 + src/server/drm_capturer.rs | 140 ++++++++++++++++++++++++---- 4 files changed, 170 insertions(+), 43 deletions(-) diff --git a/libs/scrap/src/common/drm_reader.rs b/libs/scrap/src/common/drm_reader.rs index 3d19c6c41..108909529 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -29,9 +29,39 @@ pub struct CursorSnapshot { pub height: u32, pub hotx: i32, pub hoty: i32, + /// Whether the hotspot came from the plane's HOTSPOT_X/Y properties. Those exist on + /// para-virtualised drivers only; on bare metal it is `infer_hotspot`'s guess, made on the + /// bitmap as scanned out - which on a rotated output is the compositor's pre-rotated sprite, + /// so the consumer must re-guess on the upright one. + pub hot_measured: bool, pub colors: Vec, } +/// Where a cursor bitmap's hotspot most likely is, from its opaque pixels alone: the top-left of +/// the opaque bounding box for an arrow, its centre for a tall shape (an I-beam). Only meaningful +/// on an UPRIGHT sprite - a rotated arrow's tip is some other corner of its box. +pub fn infer_hotspot(rgba: &[u8], w: usize, h: usize) -> (i32, i32) { + let (mut minx, mut miny, mut maxx, mut maxy) = (w as i32, h as i32, -1i32, -1i32); + for (i, px) in rgba.chunks_exact(4).take(w * h).enumerate() { + if px[3] >= 128 { + let (x, y) = ((i % w) as i32, (i / w) as i32); + minx = minx.min(x); + maxx = maxx.max(x); + miny = miny.min(y); + maxy = maxy.max(y); + } + } + if maxx < minx || maxy < miny { + return (0, 0); + } + let (bw, bh) = (maxx - minx + 1, maxy - miny + 1); + if bh > bw * 2 { + ((minx + maxx) / 2, (miny + maxy) / 2) + } else { + (minx, miny) + } +} + /// One enumerated DRM display, physical geometry only (the server overlays the Wayland logical origin/scale where it can match one). pub struct DisplaySnapshot { pub name: String, @@ -363,6 +393,7 @@ impl DrmReader { height: 1, hotx: 0, hoty: 0, + hot_measured: false, colors: vec![0, 0, 0, 0], }) } else if !c.pixels.is_null() @@ -376,38 +407,19 @@ impl DrmReader { let src = std::slice::from_raw_parts(c.pixels, n); let mut hash: u64 = 1469598103934665603; let mut colors = Vec::with_capacity(n * 4); - let (mut minx, mut miny, mut maxx, mut maxy) = (cw, ch, -1i32, -1i32); - for (i, &p) in src.iter().enumerate() { - let a = ((p >> 24) & 0xff) as u8; - let r = ((p >> 16) & 0xff) as u8; - let g = ((p >> 8) & 0xff) as u8; - let b = (p & 0xff) as u8; - colors.push(r); - colors.push(g); - colors.push(b); - colors.push(a); + for &p in src.iter() { + colors.push(((p >> 16) & 0xff) as u8); + colors.push(((p >> 8) & 0xff) as u8); + colors.push((p & 0xff) as u8); + colors.push(((p >> 24) & 0xff) as u8); hash ^= p as u64; hash = hash.wrapping_mul(1099511628211); - if a >= 128 { - let x = (i as i32) % cw; - let y = (i as i32) / cw; - if x < minx { minx = x; } - if x > maxx { maxx = x; } - if y < miny { miny = y; } - if y > maxy { maxy = y; } - } } - let (hotx, hoty) = if c.hot_x != 0 || c.hot_y != 0 { + let hot_measured = c.hot_x != 0 || c.hot_y != 0; + let (hotx, hoty) = if hot_measured { (c.hot_x, c.hot_y) - } else if maxx >= minx && maxy >= miny { - let (bw, bh) = (maxx - minx + 1, maxy - miny + 1); - if bh > bw * 2 { - ((minx + maxx) / 2, (miny + maxy) / 2) - } else { - (minx, miny) - } } else { - (0, 0) + infer_hotspot(&colors, cw as usize, ch as usize) }; // Fold geometry + hotspot into the id: identical pixels with a changed size or // hotspot must count as a new shape, otherwise drm_capture_worker suppresses the @@ -423,6 +435,7 @@ impl DrmReader { height: ch as u32, hotx, hoty, + hot_measured, colors, }) } else { diff --git a/src/ipc.rs b/src/ipc.rs index f4dc4f3f2..42600eb89 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -555,6 +555,8 @@ pub enum Data { height: u32, hotx: i32, hoty: i32, + #[serde(default)] + hot_measured: bool, }, } diff --git a/src/ipc/drm.rs b/src/ipc/drm.rs index 15e500e60..f0b5dcecb 100644 --- a/src/ipc/drm.rs +++ b/src/ipc/drm.rs @@ -107,6 +107,7 @@ enum DrmProducerMsg { height: u32, hotx: i32, hoty: i32, + hot_measured: bool, colors: Vec, }, } @@ -915,6 +916,7 @@ async fn handle_drm_conn(stream: Connection) -> ResultType<()> { height, hotx, hoty, + hot_measured, colors, } => { conn.send_msg( @@ -924,6 +926,7 @@ async fn handle_drm_conn(stream: Connection) -> ResultType<()> { height, hotx, hoty, + hot_measured, }, None, ) @@ -1115,6 +1118,7 @@ fn drm_capture_worker( height: c.height, hotx: c.hotx, hoty: c.hoty, + hot_measured: c.hot_measured, colors: c.colors, }) .is_err() diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index 23d7a6720..c6fead269 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -61,9 +61,15 @@ const TRANSFORM_PENDING: i32 = i32::MIN; struct Shared { slot: Mutex, cv: Condvar, - // Session transform, TRANSFORM_PENDING until new() stores it post-handshake; the receive - // thread turns cursor bitmaps with it and defers any cursor that races the store. + // Frame transform, TRANSFORM_PENDING until new() stores it post-handshake. transform: std::sync::atomic::AtomicI32, + // The output's real transform, for the cursor. It differs from `transform` on a + // hardware-rotated 180 output: the primary plane scans out upright there, but the compositor + // still pre-rotates the cursor sprite in software (measured on i915 + mutter: the frame + // arrived upright and the sprite upside down), so the cursor is turned back by the real + // angle even when the frame must not be. The receive thread defers any cursor that races + // the store. + cursor_transform: std::sync::atomic::AtomicI32, } pub struct IpcDrmCapturer { @@ -177,10 +183,6 @@ fn transform_and_origin( .copied() .flatten() .map(|j| wl.displays[j].transform) - // Hardware-rotated 180 scans out already upright (i915 advertises rotate-180 and - // mutter uses it), and wl_output cannot tell hardware from software rotation, so 180 - // keeps master behavior until the plane rotation property travels the wire. - .map(|t| if t == 90 || t == 270 { t } else { 0 }) .unwrap_or(0); let origin = augment_with_wayland_geometry_from(drm, wl, &assignment) .get(wire_idx) @@ -188,6 +190,18 @@ fn transform_and_origin( (transform, origin) } +/// The angle the FRAME has to be turned back by. Hardware-rotated 180 scans out already upright +/// (i915 advertises rotate-180 and mutter uses it), and wl_output cannot tell hardware from +/// software rotation, so 180 keeps master behavior until the plane rotation property travels +/// the wire. The cursor does not go through this: see `Shared::cursor_transform`. +fn frame_transform(wl_transform: i32) -> i32 { + if wl_transform == 90 || wl_transform == 270 { + wl_transform + } else { + 0 + } +} + /// Takes DRM_STATE: never call it while holding one of the per-display maps below. fn display_info_of(display: i32) -> Option { match &*DRM_STATE.lock().unwrap() { @@ -326,6 +340,7 @@ impl IpcDrmCapturer { }), cv: Condvar::new(), transform: std::sync::atomic::AtomicI32::new(TRANSFORM_PENDING), + cursor_transform: std::sync::atomic::AtomicI32::new(TRANSFORM_PENDING), }); let stop = Arc::new(AtomicBool::new(false)); let (tx, rx) = std::sync::mpsc::channel::, usize)>>(); @@ -350,10 +365,14 @@ impl IpcDrmCapturer { // clear racing the build rebuilds once instead of running a session on stale geometry. let snapshot_gen = scrap::wayland::display::wayland_snapshot_generation(); let wl = scrap::wayland::display::get_displays(); - let (transform, origin) = transform_and_origin(&displays, wire_idx, &wl); + let (wl_transform, origin) = transform_and_origin(&displays, wire_idx, &wl); + let transform = frame_transform(wl_transform); // This capturer now shows that layout. If the session init's own wayland query failed it // saved an empty baseline, so this is the only record of what the stream is built on. super::display_service::note_capturer_layout(&wl.displays, snapshot_gen); + shared + .cursor_transform + .store(wl_transform, std::sync::atomic::Ordering::Release); shared .transform .store(transform, std::sync::atomic::Ordering::Release); @@ -634,16 +653,18 @@ async fn recv_thread( // A cursor that arrived before new() stored the session transform, held for replay. Only the // newest matters; the 200 ms recv timeout guarantees this is retried even on an idle wire. - let mut pending_cursor: Option<(u64, u32, u32, i32, i32, Vec)> = None; + let mut pending_cursor: Option<(u64, u32, u32, i32, i32, bool, Vec)> = None; let end_reason = loop { if stop.load(Ordering::SeqCst) { break "stopped".to_owned(); } if pending_cursor.is_some() { - let t = shared.transform.load(std::sync::atomic::Ordering::Acquire); + let t = shared.cursor_transform.load(std::sync::atomic::Ordering::Acquire); if t != TRANSFORM_PENDING { - if let Some((id, width, height, hotx, hoty, raw)) = pending_cursor.take() { - deliver_drm_cursor(display, cursor_epoch, id, width, height, hotx, hoty, raw, t); + if let Some((id, width, height, hotx, hoty, hot_measured, raw)) = pending_cursor.take() { + deliver_drm_cursor( + display, cursor_epoch, id, width, height, hotx, hoty, hot_measured, raw, t, + ); } } } @@ -745,6 +766,7 @@ async fn recv_thread( height, hotx, hoty, + hot_measured, } => { // get_cursor_data() hands `colors` straight to the client, which renders // width*height*4 RGBA bytes: a short body would make it READ PAST THE BUFFER. A @@ -763,9 +785,9 @@ async fn recv_thread( raw.len() ); } - let t = shared.transform.load(std::sync::atomic::Ordering::Acquire); + let t = shared.cursor_transform.load(std::sync::atomic::Ordering::Acquire); if t == TRANSFORM_PENDING { - pending_cursor = Some((id, width, height, hotx, hoty, raw)); + pending_cursor = Some((id, width, height, hotx, hoty, hot_measured, raw)); } else { pending_cursor = None; deliver_drm_cursor( @@ -776,6 +798,7 @@ async fn recv_thread( height, hotx, hoty, + hot_measured, raw, t, ); @@ -921,14 +944,27 @@ fn deliver_drm_cursor( height: u32, hotx: i32, hoty: i32, + hot_measured: bool, raw: Vec, t: i32, ) { - let (width, height, hotx, hoty, colors) = if t == 90 || t == 270 { + // Every non-zero angle, 180 included: the compositor pre-rotates the sprite by the output's + // full transform whether or not the primary plane is hardware-rotated. + let (width, height, hotx, hoty, colors) = if t != 0 { let mut turned = Vec::new(); unrotate_bgra(&raw, width as usize, height as usize, t, &mut turned); - let (hx, hy) = unrotate_hotspot(t, width as i32, height as i32, hotx, hoty); - (height as i32, width as i32, hx, hy, turned) + let (dw, dh) = rotated_dims(t, width as usize, height as usize); + // A hotspot the driver measured is a point on the scanout sprite and maps like a pixel. + // A guessed one was guessed on the ROTATED sprite - top-left of an arrow's box - and + // the tip of a turned arrow is some other corner, so it is guessed again on the upright + // one. That is the difference between 90 and 270: on one the tip happens to stay in + // the guessed corner, on the other it does not. + let (hx, hy) = if hot_measured { + unrotate_hotspot(t, width as i32, height as i32, hotx, hoty) + } else { + scrap::drm_reader::infer_hotspot(&turned, dw, dh) + }; + (dw as i32, dh as i32, hx, hy, turned) } else { (width as i32, height as i32, hotx, hoty, raw) }; @@ -1784,6 +1820,7 @@ mod drm_capturer_tests { }), cv: Condvar::new(), transform: std::sync::atomic::AtomicI32::new(0), + cursor_transform: std::sync::atomic::AtomicI32::new(0), }), stop: Arc::new(AtomicBool::new(false)), display: 0, @@ -1922,6 +1959,77 @@ mod drm_capturer_tests { assert_eq!(back, src); } + /// An upright arrow, tip at (0,0): 4 wide, 6 tall, every pixel with x <= y/2 opaque. Its + /// opaque box is the whole bitmap, and its top-left corner IS the tip - which is exactly why + /// the bare-metal guess works when the sprite is upright, and only then. + fn arrow() -> (Vec, usize, usize) { + let (w, h) = (4usize, 6usize); + let mut px = vec![0u8; w * h * 4]; + for y in 0..h { + for x in 0..w { + if x <= y / 2 { + px[(y * w + x) * 4 + 3] = 255; + } + } + } + (px, w, h) + } + + /// What the compositor puts in the cursor plane on an output rotated by `t`: the upright sprite + /// rotated forward by t, which is what turning it back by (360 - t) degrees produces. + fn as_scanned_out(upright: &[u8], w: usize, h: usize, t: i32) -> (Vec, usize, usize) { + let mut out = Vec::new(); + unrotate_bgra(upright, w, h, (360 - t) % 360, &mut out); + let (dw, dh) = rotated_dims(t, w, h); + (out, dw, dh) + } + + // rustdesk#15886, the maintainer's physical test: the cursor looked right at 0 and 90, drawn + // above the click point at 270, and upside down at 180. All of it comes out of one line: the + // hotspot was GUESSED on the sprite as scanned out, and only afterwards mapped as if it were a + // point on it. The guess picks the top-left of the opaque box, and a turned arrow's tip is + // never in that corner - at 90 the error is the arrow's WIDTH along x, small enough to pass + // as fine; at 270 it is the arrow's HEIGHT along y, which is what was noticed. + #[test] + fn a_guessed_hotspot_is_guessed_on_the_upright_sprite() { + use scrap::drm_reader::infer_hotspot; + let (up, w, h) = arrow(); + let tip = infer_hotspot(&up, w, h); + assert_eq!(tip, (0, 0)); + let mut old_wrong_at = Vec::new(); + for t in [90, 180, 270] { + let (scan, sw, sh) = as_scanned_out(&up, w, h, t); + // The old flow: guess on the scanned-out sprite, then map the point. + let (gx, gy) = infer_hotspot(&scan, sw, sh); + let old = unrotate_hotspot(t, sw as i32, sh as i32, gx, gy); + // The new flow: turn the sprite back first, then guess. + let mut turned = Vec::new(); + unrotate_bgra(&scan, sw, sh, t, &mut turned); + let (dw, dh) = rotated_dims(t, sw, sh); + assert_eq!(turned, up, "the sprite turns back to upright at {t}"); + assert_eq!(infer_hotspot(&turned, dw, dh), tip, "the tip is found again at {t}"); + if old != tip { + old_wrong_at.push(t); + } + } + // The old flow is wrong at every angle, not only the one that was noticed. + assert_eq!(old_wrong_at, vec![90, 180, 270]); + } + + // The frame keeps master's 180 behaviour (hardware-rotated 180 scans out upright), but the + // cursor plane is pre-rotated by the compositor regardless, so the cursor path turns 180. + #[test] + fn a_180_output_turns_the_cursor_but_not_the_frame() { + assert_eq!(frame_transform(180), 0); + assert_eq!(frame_transform(90), 90); + assert_eq!(frame_transform(270), 270); + let (up, w, h) = arrow(); + let (scan, sw, sh) = as_scanned_out(&up, w, h, 180); + let mut turned = Vec::new(); + unrotate_bgra(&scan, sw, sh, 180, &mut turned); + assert_eq!(turned, up); + } + #[test] fn unrotate_180_reverses_both_axes() { let (src, w, h) = px_frame(&[&[1, 2, 3], &[4, 5, 6]], 0);