github/rustdesk
4
0 Fork 0
mirror of https://github.com/rustdesk/rustdesk.git synced 2026-11-04 08:34:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix/drm rotated cursor (#16242)

Browse Source 
* 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)

* test(drm): drive the cursor path, and drop the field nothing reads

The two tests called the helpers, so nothing pinned the production
decision: restore master's `t == 90 || t == 270` in deliver_drm_cursor
and both still pass while the 180 sprite goes back to upside down. The
new test drives deliver_drm_cursor and reads back what it published,
and it fails under exactly that mutation.

The other way to reintroduce the bug was to point the receive thread at
`Shared::transform` again. That field had no readers left once both
gates moved to `cursor_transform` - it was stored and never loaded - so
it is gone, and the doc above TRANSFORM_PENDING now names the field
that actually holds a racing cursor back.

Also four comments that said more than the code does: the sprite is
treated as pre-rotated at every angle because wl_output cannot say
otherwise, not because the compositor provably always does it; the old
flow was off at every angle, not just at one of 90/270; the 180
symptom had its own cause, the clamp, rather than all of it coming out
of the hotspot guess; and the test arrow's opaque box is its left three
columns, not the whole bitmap.

* docs(drm): say what the 180 measurement actually showed

The loop asserts the old mapping was wrong at 180 too, and that is true
of the mapping, but master never ran it there: the clamp sent 180 down
the untouched branch, so guess and sprite turned together and the
hotspot landed on the tip. The bench measured exactly that - (0,0) at
180, with only the sprite upside down. The comment said the old flow was
wrong at every angle without that distinction, which is the kind of gap
between a claim and a measurement worth closing before someone else
finds it.

* fix(drm): centre the guessed hotspot of an I-beam lying either way

The bounding-box guess only centred TALL shapes, so a HORIZONTAL I-beam was
handed the top-left corner of its box. Adwaita's `vertical-text` is exactly that
shape: at 24 px its opaque box is 20x9 and the theme's own hotspot is (12, 11),
its centre. The corner guess lands 11.2 px away; the centre lands 1.4 px away.

It matters more since this branch turns the sprite upright before guessing. Such
a cursor used to reach a quarter-turned output as a TALL bitmap, where the
tall-only rule centred it by accident; guessing on the upright bitmap - correct in
itself, and what fixes arrows at every angle - is what exposed the asymmetry.

Found by fufesou at pixel level on rustdesk#16242, and reproduced here against the
installed theme before changing anything: same declared hotspot, same bounding
box, same two errors. Then checked across ALL 35 shapes of that theme at 24 px:
making the aspect test symmetric moves exactly one of them, `vertical-text`, from
11.2 px to 1.4 px, and leaves the other 34 byte-identical.

The old flow is not the fix. It was right for this one shape and wrong for arrows
at every angle, which is what `a_guessed_hotspot_is_guessed_on_the_upright_sprite`
pins; the fix belongs in the guess, not in the order of operations.

The regression test that missed this used only `arrow()`, as fufesou pointed out.
Three things the first version of the replacement got wrong, all of them in what
it CLAIMED rather than in the fix:

- Its expected point was the centre of the BITMAP, which equals the centre of the
  opaque box only because the fixtures filled their bitmaps symmetrically. Both
  I-beam fixtures are now off-centre in their buffers, the way a sprite sits in a
  64x64 hardware cursor, and the test asserts up front that the two centres differ
  so the fixture cannot quietly stop discriminating.
- It called the arrow's box "elongated by less than the factor of two", when 3x6
  IS the factor of two and the arrow keeps its tip only because the comparison is
  strict. That makes the arrow the control for one side of the threshold, and it
  is now described as such.
- It said the case was driven "through the whole delivery path" while chaining the
  helpers itself. It now calls `deliver_drm_cursor` and reads the published
  hotspot back out of DRM_CURSOR, at 0, 90, 180 and 270, feeding it what the
  producer really sends for a guessed hotspot: the producer's own guess, made on
  the sprite as scanned out.

And the threshold was pinned from one side only. A third fixture carries the real
2.22:1 aspect of the shape that was wrong, so a stricter rule cannot pass on 6:1
bars while silently un-centring the actual cursor.

Mutations, each restored byte-for-byte afterwards: back to tall-only, the bitmap
centre instead of the box centre, the threshold loosened to one and tightened to
three. All four fail the test.

* build(drm): point the pin at the synced mirror and drop the helper fallback

The mirror is synced, so the pin moves from 5da68a3a (libdrmtap 0.5.4) to
49b204f (0.5.6), which is what rustdesk-org/libdrmtap now carries.

-Dhelper=disabled, as the maintainer asked on #16242. rustdesk never uses the
privileged helper: every drmtap_open lives in src/ipc/drm.rs, i.e. the root
service, which already holds CAP_SYS_ADMIN, and the unprivileged side opens a
render node instead. What the library carries without the option is a fallback
that walks six hardcoded paths, two of them under /usr/local, and execs the
first one that passes access(X_OK) -- no check of its owner, no check of its
mode -- from inside the ROOT process. The option compiles that path out.

Asserted on the artifact as well as passed as a flag, for the same reason the
EGL check is: a flag cannot notice a stale build-pkg or an object substituted
by hand, and meson accepts an unknown -D silently on versions predating the
option. Measured on the produced .so: socketpair 4 -> 0, the helper search
paths 3 -> 0, and nm -D shows no fork, execl or socketpair import. The check
fails as it should when pointed at a .so built with the helper.

* fix(drm): use the library's hotspot provenance, and keep legacy IPC meaning

Answers the two blockers in the maintainer's review of 9acb37a9d, plus the
three he marked should-fix.

hot_measured no longer guesses. The pinned libdrmtap 0.5.6 exports
drmtap_cursor_hotspot_valid, and the old test -- hot_x != 0 || hot_y != 0 --
cannot separate the two things a (0, 0) hotspot means: no HOTSPOT_X/Y on the
plane, or a driver whose answer really is the corner. The symbol is resolved
OPTIONALLY, the way drmtap_render_node and drmtap_list_devices already are,
with the same version-aware warning when a library that reports 0.5.6 or newer
does not carry it. An older deployed .so keeps the previous behaviour instead
of the ABI floor moving under it and refusing to load.

The IPC default is corrected. Data is JSON over a unix socket between two
processes upgraded separately, so an old root service can be streaming to a
freshly started --server and its DrmCursor has no hot_measured at all. Read as
bool::default() the new consumer discards a hotspot the producer measured and
re-infers one from the upright bitmap, moving the cursor on a rotated display
for the length of the upgrade. The default is now true, which is not a claim
that the value was measured: it means "no provenance available, so behave as
the old protocol did". A new producer that wants re-inference sends false
explicitly, which serializes, so new-to-new is unchanged.

hot_measured is folded into the cursor id. It is not metadata about the cursor,
it selects what the consumer does with it, so a sample whose pixels and (0, 0)
hotspot are unchanged but whose provenance flipped is a different cursor and
must not be deduped away by the producer.

build.py reconfigures an existing meson build dir with -Dhelper=disabled
instead of only configuring a fresh one; a directory from before the option
kept its old configuration and the artifact assertion then failed the build.

And the drm CI job now runs the drm-gated tests. It built the feature without
ever executing them, so every cursor rotation and hotspot test had no
continuous coverage. A cargo test filter that matches nothing exits 0, so the
step also asserts it actually ran some.

Verified by mutation, each caught by its own test and no other: ignoring the
library's answer fails the two provenance tests; the serde default back to
false fails the legacy-deserialization test; dropping hot_measured from the id
fails the identity test.

* feat(drm): say once where the cursor hotspot came from

The provenance decision was unverifiable anywhere but a unit test: nothing on
a running box distinguishes the three states, so a deploy proved nothing. One
info line does, and with it all three are measured -- Some(false) on i915,
None with a 0.5.4 library swapped in behind the soname, and Some(true) via a
C probe on virtio-gpu, where no rustdesk build runs.

Once per state, not once per sample. A cursor is read on every capture
iteration and its provenance changes approximately never; a per-frame log line
here is the same defect libdrmtap already shipped once. Measured on Sigma-26:
one line across a whole session. The should-we-log decision is its own
function so it cannot quietly become per-sample again.

* test(drm): drive a legacy cursor message from the wire, not from a literal

The upgrade window is the one thing in this round that could not be staged
with two live processes: a --server from one build does not run inside the
other's tree, so the old service respawns it and it dies with exit 127. Tried
on two boxes, with two files swapped and with the whole tree.

This is the honest form of that test and it runs in CI rather than once on a
desk. It takes the exact bytes an old producer puts on the socket -- no
hot_measured key at all, adjacently tagged as Data really is -- deserializes
them the way the receive loop does, drives the real delivery path, and reads
back what was published. The transform is non-zero on purpose: at 0 the path
never consults hot_measured, so a test there could not tell the branches
apart, and the fixture is asserted to make mapping and re-inferring disagree
before anything is concluded from them.

Verified by mutation: restoring #[serde(default)] -- the reported bug -- fails
it, and so does a delivery path that ignores hot_measured.

* fix(drm): run the tests that matter in ci, and keep provenance state per stream

Answers the review of 7dc6817 plus the CI failure fufesou reported on 401c2f7.

The drm test step was broken three ways and none of them showed up as a red
test: it reused the production $FEATURES, so the test binary failed to LINK
(undefined reference to fcntl64 from hwcodec) and the tests never ran; it
summed the counts with bc, which the ubuntu18.04 container does not install;
and `cargo test --lib` at the repo root builds the ROOT package only, so the
provenance tests in libs/scrap were never selected -- the "at least one test
ran" guard passed on the root filters and hid it. Now: --features drm rather
than the production set, awk instead of bc, and an explicit `-p scrap` run
with the same assertion. Locally 36, 9 and 5 tests, and a bogus filter still
fails the step.

Provenance logging state was process-global on both sides. DRM runs one
reader per captured display, and two outputs with different but STABLE
provenance -- one publishing HOTSPOT_X/Y and one not, which is an ordinary
multi-GPU host -- would overwrite each other and make every message look like
a change, i.e. exactly the per-frame logging the one-shot was written to
avoid. The producer's state moves into DrmReader, the consumer's is keyed by
display. The hidden-cursor sentinel is no longer recorded: it carries
hot_measured=false because it has no hotspot, so counting it turned every
hide and show into a provenance transition.

The 180 test is renamed to say what it pins -- i915 advertising rotate-180
with mutter -- and its comment says plainly that it is not a general contract,
that Plasma is still wrong there, and that changing it means re-measuring.

Verified by mutation: restoring the global slot fails the new per-display
test, and so does recording the hidden sentinel.

* fix(drm): centre the guessed hotspot on the axes a cursor mirrors about

The guess put the click point at the top-left corner of the opaque box unless
the box was more than twice as long as it is wide. That corner is where an
arrow's tip is, because cursors are drawn pointing up and to the left, but a
shape that mirrors onto itself has no tip to point with: a crosshair, a
two-headed resize arrow, a cell marker. Those got a corner where the theme puts
the middle, and the error is about half the glyph.

So: centre the axis the shape is symmetric about, and keep the corner
everywhere else. Measured against the hotspots the theme declares, which is the
value a compositor programs into HOTSPOT_X/Y where the property exists:

  adwaita-icon-theme 50, 35 shapes at six nominal sizes, 210 cases
    mean error 0.43 -> 0.27 of the cursor size, 17 shapes improve, none worse
  adwaita-icon-theme 46, 34 shapes at five sizes, 170 more cases
    better at every size, none worse

The threshold is not fitted: from 90 to 98 percent agreement nothing regresses,
and below 90 three shapes do.

The corpus is carried as fixtures so the expected value comes from the theme
author rather than from our own heuristic, and two tests read it: the published
hotspot must not move when the output turns, with the old flow's 15 px average
movement as the control, and the guess must stay inside the measured bounds
with the old rule frozen as the baseline.

The elongation test stays ahead of the mirror test so an elongated shape that
is not symmetric still keeps its centre.

* fix(drm): put a directional cursor's hotspot on the edge it resizes

The mirror rule centred the axes a shape is symmetric about and left the
other axis on the top-left corner. That is right for an arrow, whose tip is
that corner, and wrong for the resize cursors: those are a pointer plus the
marker of the edge being resized, the theme puts the click point on the
marker, and the marker is at whichever end the cursor names. Always taking
the corner therefore had to be wrong for one of every mirrored pair, since
e-resize and w-resize are the same picture reflected.

So an axis is read as directional when the perpendicular axis mirrors, which
is the n/e/s/w family, or when the shape mirrors about a diagonal, which is
the corner family. A directional axis takes the heavier end.

A diagonal reflection only lines up inside a square, and these boxes are not
square, so where the box sits in that square decides the answer: ne-resize
scores 0.62 anchored one way and 0.98 the other, and nw-resize is the reverse.
Both anchorings are tried and the better answer wins.

Measured against MASTER now, not against this PR's previous revision, which
is what the maintainer asked for. Over the angles master handles (0, 90, 270),
adwaita-icon-theme 50 at 24 px:

  mean error per shape   12.08 px -> 4.20
  (shape, angle) cases   83 better, 18 the same, 4 worse of 105
  shapes worse than master   help (+4.73) and alias (+0.51)

Both of those are semantic: the theme puts help's click point on the dot of
its question mark. The same holds at 48 px and on adwaita-icon-theme 46,
where ne-resize is also 1.34 px worse because its glyph is drawn differently
there.

The test baseline was wrong and is fixed here: it froze this PR's earlier
symmetric aspect test rather than master's tall-only one, so it could only
show that the new rule does not regress the old rule. It now freezes master's
guess AND master's delivery, and names the two shapes it excuses with the
measured size of each.

The mirror threshold moves from 95 to 88, the middle of the 86 to 91 interval
that regresses nothing on either theme version.

---------

Co-authored-by: Mariano Abad <weimaraner@gmail.com>
This commit is contained in:
fufesouandMariano Abad authored and GitHub committed 2026-09-22 15:18:10 +08:00
1 parent 6a0eb9a1fd
commit 12f61b1fda
9 files changed
+1546 -62
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

src/ipc.rs
+59
View File
@@ -555,9 +555,29 @@ pub enum Data {
height: u32,
hotx: i32,
hoty: i32,
/// Whether the hotspot came from the driver, or was inferred from the bitmap.
///
/// Absent means the producer predates the field, and the default must then be TRUE, not
/// `bool::default()`. These messages are JSON over a unix socket between two processes
/// that are upgraded separately: an old root service can be streaming to a freshly
/// started `--server`, and its `{"hotx": 12, "hoty": 11}` carries no provenance. Read as
/// `false`, the new consumer would throw that hotspot away and re-infer one from the
/// upright bitmap, moving the cursor on a rotated display for the length of the upgrade.
/// Defaulting to true means "no provenance available, so behave as the old protocol did"
/// -- use the point the producer sent. It is not a claim that the value was measured; a
/// new producer that wants re-inference says `hot_measured: false` explicitly, which
/// serializes, so new-to-new is unaffected.
#[serde(default = "legacy_hot_measured")]
hot_measured: bool,
},
}
/// The default for a `hot_measured` that is not on the wire at all; see the field's docs.
#[cfg(all(target_os = "linux", feature = "drm"))]
fn legacy_hot_measured() -> bool {
true
}
#[tokio::main(flavor = "current_thread")]
pub async fn start(postfix: &str) -> ResultType<()> {
let mut incoming = new_listener(postfix).await?;
@@ -2310,4 +2330,43 @@ mod test {
fn test_select_server_uid_fails_when_multiple_servers_are_ambiguous() {
assert!(select_server_uid_for_user_main_ipc(&[501, 502], None, false).is_err());
}
/// The upgrade window: an old root service is still streaming when a new `--server` starts,
/// and its DrmCursor carries no `hot_measured` at all. Read as `false` the consumer would
/// discard a hotspot the producer did measure and re-infer one from the bitmap, which moves
/// the cursor on a rotated display until the old process is replaced. `bool::default()` is
/// the wrong default here; the right one preserves what the old protocol meant.
#[cfg(all(target_os = "linux", feature = "drm"))]
#[test]
fn a_drm_cursor_without_provenance_keeps_the_hotspot_the_producer_sent() {
// `Data` is adjacently tagged (`tag = "t", content = "c"`), so this is the real shape on
// the socket, not a simplified one.
let legacy =
r#"{"t":"DrmCursor","c":{"id":7,"width":24,"height":24,"hotx":12,"hoty":11}}"#;
let msg: Data = serde_json::from_str(legacy).expect("legacy DrmCursor must deserialize");
match msg {
Data::DrmCursor {
hotx,
hoty,
hot_measured,
..
} => {
assert_eq!((hotx, hoty), (12, 11));
assert!(
hot_measured,
"a message with no provenance field must be treated as the old protocol did, \
i.e. use the hotspot as sent, not re-infer it"
);
}
other => panic!("expected DrmCursor, got {other:?}"),
}
// And a NEW producer that really wants re-inference says so, which serializes: the
// default must not swallow an explicit false.
let modern = r#"{"t":"DrmCursor","c":{"id":7,"width":24,"height":24,"hotx":0,"hoty":0,"hot_measured":false}}"#;
match serde_json::from_str::<Data>(modern).expect("modern DrmCursor must deserialize") {
Data::DrmCursor { hot_measured, .. } => assert!(!hot_measured),
other => panic!("expected DrmCursor, got {other:?}"),
}
}
}
src/ipc/drm.rs
+4
View File
@@ -107,6 +107,7 @@ enum DrmProducerMsg {
height: u32,
hotx: i32,
hoty: i32,
hot_measured: bool,
colors: Vec<u8>,
},
}
@@ -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()
src/server/drm_capturer.rs
+791 -24
View File
@@ -53,17 +53,22 @@ impl FrameSlot {
}
}
/// `Shared.transform` before new() stores the real value: a cursor arriving this early is held
/// back and replayed once the session transform is in, because the producer will not resend it
/// until the shape changes.
/// `Shared.cursor_transform` before new() stores the real value: a cursor arriving this early is
/// held back and replayed once the session transform is in, because the producer will not resend
/// it until the shape changes.
const TRANSFORM_PENDING: i32 = i32::MIN;
struct Shared {
slot: Mutex<FrameSlot>,
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.
transform: std::sync::atomic::AtomicI32,
// The output's real transform, TRANSFORM_PENDING until new() stores it post-handshake. Only
// the cursor needs it across threads: it differs from the angle the FRAME is turned by on a
// hardware-rotated 180 output, where the primary plane scans out upright but the compositor
// still pre-rotates the sprite (measured on i915 + mutter: the frame arrived upright and the
// sprite upside down). The frame's own angle is not shared - it is fixed for the session and
// lives on the capturer that uses it. The receive thread defers any cursor that races the
// store.
cursor_transform: std::sync::atomic::AtomicI32,
}
pub struct IpcDrmCapturer {
@@ -177,10 +182,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 +189,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<DrmDisplayInfo> {
match &*DRM_STATE.lock().unwrap() {
@@ -325,7 +338,7 @@ impl IpcDrmCapturer {
ended: None,
}),
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::<ResultType<(Vec<DrmDisplayInfo>, usize)>>();
@@ -350,13 +363,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
.transform
.store(transform, std::sync::atomic::Ordering::Release);
.cursor_transform
.store(wl_transform, std::sync::atomic::Ordering::Release);
Ok((
IpcDrmCapturer {
shared,
@@ -634,16 +648,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<u8>)> = None;
let mut pending_cursor: Option<(u64, u32, u32, i32, i32, bool, Vec<u8>)> = 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 +761,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 +780,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 +793,7 @@ async fn recv_thread(
height,
hotx,
hoty,
hot_measured,
raw,
t,
);
@@ -921,17 +939,34 @@ fn deliver_drm_cursor(
height: u32,
hotx: i32,
hoty: i32,
hot_measured: bool,
raw: Vec<u8>,
t: i32,
) {
let (width, height, hotx, hoty, colors) = if t == 90 || t == 270 {
// Every non-zero angle, 180 included. Measured on i915 + mutter with the output at 180: the
// frame arrived upright and the sprite upside down, so the compositor had pre-rotated the
// sprite by the full transform while the plane scanned out already turned. wl_output cannot
// say which of the two happened - the same blindness `frame_transform` defers to - so the
// sprite is treated as pre-rotated at every angle.
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. The old flow was off at every angle; what differs is the size of the error, which
// is why only one of them got reported (see the test below).
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)
};
note_received_provenance(display, id, hot_measured);
let id = fold_cursor_id(id, t);
set_drm_cursor(
display,
@@ -947,6 +982,50 @@ fn deliver_drm_cursor(
);
}
/// Last provenance seen PER DISPLAY. Not one global: several streams run at once and their
/// provenance can differ and still be stable -- one output publishing HOTSPOT_X/Y while another
/// does not is a normal multi-GPU host -- and a single slot would read that as an endless
/// alternation and log on every cursor message.
static LAST_RECEIVED_PROVENANCE: Mutex<BTreeMap<i32, bool>> = Mutex::new(BTreeMap::new());
/// Whether this display's provenance changed, recording the new value. Its own function so the
/// per-display behaviour can be asserted without reading a log.
fn provenance_is_news_for(display: i32, hot_measured: bool) -> bool {
LAST_RECEIVED_PROVENANCE
.lock()
.unwrap()
.insert(display, hot_measured)
!= Some(hot_measured)
}
/// Say once per display, and again only on a change, what the producer told us about the hotspot.
///
/// The counterpart of the producer's own provenance line, and it exists for the case that line
/// cannot cover: a message that arrives with no `hot_measured` field at all, where what happens is
/// decided by the field's serde default and is otherwise invisible on a running box.
///
/// The hidden-cursor sentinel is skipped. It is sent with `hot_measured: false` because there is no
/// hotspot to describe, so recording it would make every hide/show look like a provenance change.
fn note_received_provenance(display: i32, id: u64, hot_measured: bool) {
if id == scrap::drm_reader::HIDDEN_CURSOR_ID {
return;
}
if !provenance_is_news_for(display, hot_measured) {
return;
}
if hot_measured {
log::info!(
"drm: cursor hotspots arrive MEASURED; mapping the point the producer sent. \
(a producer that predates the field reads as measured too, on purpose: that \
preserves what the old protocol meant)"
);
} else {
log::info!(
"drm: cursor hotspots arrive as a GUESS; re-inferring from the upright bitmap"
);
}
}
fn fold_cursor_id(id: u64, t: i32) -> u64 {
if id == scrap::drm_reader::HIDDEN_CURSOR_ID {
id
@@ -1783,7 +1862,7 @@ mod drm_capturer_tests {
ended: None,
}),
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 +2001,362 @@ mod drm_capturer_tests {
assert_eq!(back, src);
}
/// An upright arrow, tip at (0,0): a 4x6 bitmap, every pixel with x <= y/2 opaque, so the
/// opaque box is the left 3 columns. 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<u8>, 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)
}
/// An I-beam lying along x, deliberately NOT centred in its bitmap: 16x8, opaque rows 2..4 and
/// columns 1..13, so the opaque box is (1,2)-(12,3), 12x2 - elongated, which is what Adwaita's
/// `vertical-text` shape is. Its click point is the centre of that BOX, (6,2), while the centre
/// of the bitmap is (7,3). The offset is the point: a real sprite sits somewhere inside a
/// 64x64 hardware buffer (the adwaita box above is (2,6)-(21,14) in a 24x24 one), and a fixture
/// centred in its bitmap would let an implementation that returns the bitmap centre pass a test
/// claiming to check the box centre.
fn ibeam_horizontal() -> (Vec<u8>, usize, usize, (i32, i32)) {
let (w, h) = (16usize, 8usize);
let mut px = vec![0u8; w * h * 4];
for y in 2..4 {
for x in 1..13 {
px[(y * w + x) * 4 + 3] = 255;
}
}
(px, w, h, (6, 2))
}
/// The shape at the aspect ratio that actually matters: a 20x9 opaque box, which is what the
/// installed Adwaita `vertical-text` measures at 24 px. 2.22:1, only just past the rule's
/// factor of two, so it pins the threshold from ABOVE: a stricter rule (say `bw > bh * 3`)
/// leaves the 6:1 bars below centred and silently un-centres the real cursor. Placed
/// off-centre in a 24x24 buffer like the real one, box (2,6)-(21,14), centre (11,10) - the
/// same numbers the theme file gives, against its declared hotspot of (12,11).
fn ibeam_theme_aspect() -> (Vec<u8>, usize, usize, (i32, i32)) {
let (w, h) = (24usize, 24usize);
let mut px = vec![0u8; w * h * 4];
for y in 6..15 {
for x in 2..22 {
px[(y * w + x) * 4 + 3] = 255;
}
}
(px, w, h, (11, 10))
}
/// The same shape standing up and equally off-centre: 8x16, opaque columns 2..4 and rows 1..13,
/// box (2,1)-(3,12), centre (2,6) against a bitmap centre of (3,7). The tall case the guess
/// already handled, kept so the symmetric rule cannot pass by breaking it.
fn ibeam_vertical() -> (Vec<u8>, usize, usize, (i32, i32)) {
let (w, h) = (8usize, 16usize);
let mut px = vec![0u8; w * h * 4];
for y in 1..13 {
for x in 2..4 {
px[(y * w + x) * 4 + 3] = 255;
}
}
(px, w, h, (2, 6))
}
/// 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<u8>, 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. Two causes, not one. The sprite came
// out upside down at 180 because master turned it only at 90 and 270, so a 180 sprite was
// never turned back at all. And at every angle 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 not in that corner - at 90 the
// error is the arrow's WIDTH along x, small enough to pass as fine; at 270 it is its HEIGHT
// along y, which is the one that got reported.
//
// The 180 entry below is about that mapping, not about what was seen: on master the clamp sent
// 180 down the untouched branch, so the guess and the sprite were turned together and the
// hotspot landed on the tip anyway - measured as (0,0) there, with only the sprite wrong. What
// the loop shows is that the guess is not a point that survives being mapped, at any angle.
// This test covers the mapping; the sprite is covered by the two below.
#[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]);
}
// rustdesk#16242, the maintainer's pixel-level finding: the guess only centred TALL shapes, so
// a horizontal I-beam got the corner of its box. Before the rotation rework such a cursor
// reached a quarter-turned output as a tall bitmap and the same rule centred it by accident, so
// turning the sprite upright first - correct in itself - turned a nearly right hotspot into one
// about 11 px out. Measured on the installed Adwaita `vertical-text` at 24 px: theme hotspot
// (12, 11), opaque box 20x9, corner guess (2, 6) = 11.2 px away, centre guess (11, 10) = 1.4 px.
//
// The old flow is not the fix: it was right for this one shape and wrong for arrows at every
// angle, which is what the test above pins. The fix is for the guess to centre an elongated box
// whichever way it lies, and that is what this checks - for both orientations and at the real
// 2.22:1 aspect of the shape that was wrong - by driving `deliver_drm_cursor` and reading the
// published hotspot back, not by chaining the helpers itself.
/// The upgrade window, end to end from the WIRE rather than from a struct literal.
///
/// An old root service can be streaming to a freshly started `--server`, and its `DrmCursor`
/// carries no `hot_measured` at all. This takes the exact bytes such a producer puts on the
/// socket, deserializes them the way the consumer's receive loop does, and drives the real
/// delivery path, then reads back what was published. Staging it with two live processes is
/// not possible with how the package is built - a `--server` from one build will not run
/// inside the other's tree - so this is the honest form of that test, and it runs in CI
/// instead of once on somebody's desk.
///
/// The transform must be non-zero: at 0 the delivery path never consults `hot_measured`, so a
/// test at 0 could not tell the two branches apart.
/// Two streams with different but STABLE provenance must not read as a change on every
/// message. A single global slot did exactly that: display A measured and display B inferred,
/// alternating forever, is a normal multi-GPU host and would have logged per cursor message -
/// the per-frame logging this was written to avoid. And the hidden-cursor sentinel carries
/// `hot_measured: false` because it has no hotspot, so recording it would turn every hide and
/// show into a provenance transition.
#[test]
fn provenance_is_tracked_per_display_and_ignores_the_hidden_cursor() {
let (a, b) = (9_510, 9_511);
LAST_RECEIVED_PROVENANCE.lock().unwrap().remove(&a);
LAST_RECEIVED_PROVENANCE.lock().unwrap().remove(&b);
// First sighting of each is news; the same value again is not.
assert!(provenance_is_news_for(a, true));
assert!(!provenance_is_news_for(a, true));
assert!(provenance_is_news_for(b, false));
assert!(!provenance_is_news_for(b, false));
// The interleaving that a single global slot could not survive.
for _ in 0..4 {
assert!(!provenance_is_news_for(a, true), "display A is stable");
assert!(!provenance_is_news_for(b, false), "display B is stable");
}
// A real change on one display is still reported, and does not disturb the other.
assert!(provenance_is_news_for(a, false));
assert!(!provenance_is_news_for(b, false));
// The hidden sentinel is not recorded at all: it would otherwise look like B flipping.
note_received_provenance(b, scrap::drm_reader::HIDDEN_CURSOR_ID, true);
assert!(
!provenance_is_news_for(b, false),
"the hidden cursor must not count as a provenance change"
);
LAST_RECEIVED_PROVENANCE.lock().unwrap().remove(&a);
LAST_RECEIVED_PROVENANCE.lock().unwrap().remove(&b);
}
#[test]
fn a_legacy_cursor_message_keeps_the_hotspot_the_old_protocol_meant() {
use crate::ipc::Data;
let (up, w, h, _box_centre) = ibeam_theme_aspect();
let t = 90;
let (scan, sw, sh) = as_scanned_out(&up, w, h, t);
// A hotspot the old producer measured and sent as a point on the scanned-out sprite.
let (hotx, hoty) = (3, 5);
// Exactly what an old producer writes: `Data` is adjacently tagged, and there is no
// `hot_measured` key because that build has no such field.
let legacy = format!(
r#"{{"t":"DrmCursor","c":{{"id":11,"width":{sw},"height":{sh},"hotx":{hotx},"hoty":{hoty}}}}}"#
);
let (id, mw, mh, mx, my, measured) =
match serde_json::from_str::<Data>(&legacy).expect("a legacy DrmCursor must parse") {
Data::DrmCursor { id, width, height, hotx, hoty, hot_measured } => {
(id, width, height, hotx, hoty, hot_measured)
}
other => panic!("expected DrmCursor, got {other:?}"),
};
assert!(measured, "no provenance on the wire must mean what the old protocol meant");
// The two branches must disagree here, or the assertion below proves nothing.
let mapped = unrotate_hotspot(t, mw as i32, mh as i32, mx, my);
let mut turned = Vec::new();
unrotate_bgra(&scan, sw, sh, t, &mut turned);
let (dw, dh) = rotated_dims(t, sw, sh);
let reinferred = scrap::drm_reader::infer_hotspot(&turned, dw, dh);
assert_ne!(
mapped, reinferred,
"fixture cannot discriminate: mapping and re-inferring give the same point"
);
let display = 9_400;
deliver_drm_cursor(display, 1, id, mw, mh, mx, my, measured, scan, t);
let published = {
let map = DRM_CURSOR.lock().unwrap();
let (_, c) = map.get(&display).expect("the delivery path published nothing");
(c.hotx, c.hoty)
};
assert_eq!(
published, mapped,
"a legacy message must keep the hotspot the producer sent, transformed as a point; \
re-inferring it would move the cursor for the length of an upgrade"
);
// The control: a NEW producer asking for re-inference says so explicitly, and that must
// not be swallowed by the default.
let modern = format!(
r#"{{"t":"DrmCursor","c":{{"id":12,"width":{sw},"height":{sh},"hotx":{hotx},"hoty":{hoty},"hot_measured":false}}}}"#
);
let Data::DrmCursor { hot_measured, .. } =
serde_json::from_str::<Data>(&modern).expect("a modern DrmCursor must parse")
else {
panic!("expected DrmCursor")
};
assert!(!hot_measured);
let (scan2, _, _) = as_scanned_out(&up, w, h, t);
deliver_drm_cursor(display + 1, 1, 12, sw as u32, sh as u32, hotx, hoty, false, scan2, t);
let published2 = {
let map = DRM_CURSOR.lock().unwrap();
let (_, c) = map.get(&(display + 1)).expect("nothing published for the control");
(c.hotx, c.hoty)
};
assert_eq!(
published2, reinferred,
"an explicit false must still re-infer, or the legacy default has eaten the new \
producer's request"
);
}
#[test]
fn an_ibeam_keeps_its_centre_whichever_way_it_lies() {
use scrap::drm_reader::infer_hotspot;
for (i, (name, (up, w, h, box_centre))) in [
("horizontal", ibeam_horizontal()),
("vertical", ibeam_vertical()),
("theme-aspect", ibeam_theme_aspect()),
]
.into_iter()
.enumerate()
{
// The fixture must not be centred in its own bitmap, or this test cannot tell the
// centre of the opaque BOX - which is what the guess computes and what a cursor's
// click point is - from the centre of the buffer the sprite happens to sit in.
let bitmap_centre = (((w - 1) / 2) as i32, ((h - 1) / 2) as i32);
assert_ne!(
box_centre, bitmap_centre,
"the {name} fixture is centred in its bitmap, so it cannot discriminate"
);
// The upright guess is the centre of the opaque box, not its corner.
let guess = infer_hotspot(&up, w, h);
assert_eq!(guess, box_centre, "the {name} I-beam is centred, not cornered");
// And it is what the delivery path PUBLISHES, not just what the helper computes:
// `deliver_drm_cursor` is where the guessed branch is chosen and where the sprite is
// turned back, so the sprite is handed over pre-rotated exactly as the compositor
// scans it out, and the published hotspot is read back out of DRM_CURSOR.
for (j, t) in [0, 90, 180, 270].into_iter().enumerate() {
let display = 9_300 + (i * 8 + j) as i32;
let (scan, sw, sh) = as_scanned_out(&up, w, h, t);
// What the producer actually sends for a guessed hotspot: its own guess, made on
// the sprite AS SCANNED OUT (drm_reader does exactly that). At 0 the delivery path
// publishes it unchanged, which is why it has to be the real value and not a
// placeholder; at every other angle the path discards it and guesses again.
let (phx, phy) = scrap::drm_reader::infer_hotspot(&scan, sw, sh);
deliver_drm_cursor(display, 1, 7, sw as u32, sh as u32, phx, phy, false, scan, t);
let map = DRM_CURSOR.lock().unwrap();
let (_, c) = map.get(&display).expect("the cursor path published nothing");
assert_eq!(
(c.width as usize, c.height as usize),
(w, h),
"the published {name} sprite is upright-sized at {t}"
);
assert_eq!(c.colors, up, "the published {name} sprite is upright at {t}");
assert_eq!(
(c.hotx, c.hoty),
box_centre,
"the published {name} hotspot is the box centre at {t}"
);
}
}
// The arrow is untouched, and it sits exactly ON the boundary rather than safely inside
// it: its box is 3x6, which is the factor of two itself, so it keeps its tip only because
// the comparison is strict. That makes it the control for the other side of the threshold
// - loosen the rule to `* 1` and this line fails, tighten it past 2.22 and the
// theme-aspect case above fails. The two together pin where the boundary is.
let (arrow_px, aw, ah) = arrow();
assert_eq!(infer_hotspot(&arrow_px, aw, ah), (0, 0));
}
/// NOT a general DRM/Wayland contract, and should not be read as one. This pins the behaviour
/// MEASURED on i915 advertising rotate-180 with mutter: the primary plane scans out already
/// upright while the compositor pre-rotates the cursor sprite, and `wl_output` cannot tell
/// hardware rotation from compositor rotation, so the frame is left alone and the sprite is
/// turned. Arch with KDE Plasma is still wrong at 180, which is exactly why this is scoped to
/// what was measured rather than stated as a rule. The robust fix is to propagate the actual
/// KMS plane rotation instead of inferring both from the `wl_output` transform; until then,
/// changing this test means re-measuring on the compositor in question, not reasoning from it.
#[test]
fn a_180_output_turns_the_cursor_but_not_the_frame_on_i915_plus_mutter() {
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);
}
// The two above exercise the helpers, which cannot see WHICH angle the cursor path is handed
// or whether it turns 180 at all - the line that decides both is `if t != 0` in
// `deliver_drm_cursor`, and reverting it to master's `t == 90 || t == 270` leaves every helper
// assertion above still passing. So this drives the real path and reads back what it published.
#[test]
fn the_cursor_path_publishes_an_upright_sprite_at_every_angle() {
let (up, w, h) = arrow();
for (i, t) in [0, 90, 180, 270].into_iter().enumerate() {
// A display id of its own per angle: DRM_CURSOR is process-wide and keyed by display.
let display = 9_100 + i as i32;
let (scan, sw, sh) = as_scanned_out(&up, w, h, t);
deliver_drm_cursor(display, 1, 7, sw as u32, sh as u32, 0, 0, false, scan, t);
let map = DRM_CURSOR.lock().unwrap();
let (_, c) = map.get(&display).expect("the cursor path published nothing");
assert_eq!(
(c.width as usize, c.height as usize),
(w, h),
"the published sprite is upright-sized at {t}"
);
assert_eq!(c.colors, up, "the published sprite is upright at {t}");
assert_eq!((c.hotx, c.hoty), (0, 0), "the tip is the hotspot at {t}");
}
}
#[test]
fn unrotate_180_reverses_both_axes() {
let (src, w, h) = px_frame(&[&[1, 2, 3], &[4, 5, 6]], 0);
@@ -2390,4 +2825,336 @@ mod drm_capturer_tests {
assert!(demote_cooldown(1) + burn < Duration::from_secs(40));
assert!(demote_cooldown(5) + burn > Duration::from_secs(8 * 60));
}
// ---------------------------------------------------------------------------------------
// rustdesk#16242 round 2: the measurement behind the inference model.
//
// The question a synthetic fixture cannot answer: when the driver publishes no hotspot and
// the sprite has to be turned upright before guessing, how far from the THEME's real click
// point does each flow land? Both flows are driven here, per shape and per angle, against
// the hotspot the theme author wrote into the cursor file. It reads the installed Adwaita
// theme, so it prints instead of asserting and is ignored by default:
//
// cargo test --features drm --lib drm_capturer_tests::adwaita -- --ignored --nocapture
// ---------------------------------------------------------------------------------------
/// Minimal Xcursor reader. The format is a 16-byte header, a table of contents of 12-byte
/// entries, and one chunk per (nominal size, animation frame); an image chunk is 36 bytes of
/// header followed by width*height little-endian ARGB words, so byte 3 of each pixel is the
/// alpha `infer_hotspot` reads. Only the FIRST frame of each nominal size is taken.
fn xcursor_images(path: &std::path::Path) -> Vec<(u32, usize, usize, i32, i32, Vec<u8>)> {
let b = match std::fs::read(path) {
Ok(b) => b,
Err(_) => return Vec::new(),
};
let u32at = |o: usize| -> u32 {
if o + 4 > b.len() {
0
} else {
u32::from_le_bytes([b[o], b[o + 1], b[o + 2], b[o + 3]])
}
};
if b.len() < 16 || &b[0..4] != b"Xcur" {
return Vec::new();
}
let (hdr, ntoc) = (u32at(4) as usize, u32at(12) as usize);
let mut out: Vec<(u32, usize, usize, i32, i32, Vec<u8>)> = Vec::new();
for i in 0..ntoc {
let e = hdr + i * 12;
if u32at(e) != 0xfffd_0002 {
continue;
}
let nominal = u32at(e + 4);
if out.iter().any(|(n, ..)| *n == nominal) {
continue;
}
let p = u32at(e + 8) as usize;
let (w, h) = (u32at(p + 16) as usize, u32at(p + 20) as usize);
let (xhot, yhot) = (u32at(p + 24) as i32, u32at(p + 28) as i32);
let px = p + 36;
if w == 0 || h == 0 || px.saturating_add(w * h * 4) > b.len() {
continue;
}
out.push((nominal, w, h, xhot, yhot, b[px..px + w * h * 4].to_vec()));
}
out
}
fn dist(a: (i32, i32), b: (i32, i32)) -> f64 {
let (dx, dy) = ((a.0 - b.0) as f64, (a.1 - b.1) as f64);
(dx * dx + dy * dy).sqrt()
}
#[test]
#[ignore]
fn adwaita_hotspot_survey() {
let dir = std::env::var("SURVEY_DIR")
.unwrap_or_else(|_| "/usr/share/icons/Adwaita/cursors".to_owned());
let dir = std::path::Path::new(&dir);
let mut names: Vec<String> = std::fs::read_dir(dir)
.expect("no installed Adwaita cursors to measure")
.filter_map(|e| e.ok())
.filter(|e| e.file_type().map(|t| t.is_file()).unwrap_or(false))
.map(|e| e.file_name().to_string_lossy().into_owned())
.collect();
names.sort();
let want: u32 = std::env::var("SURVEY_SIZE")
.ok()
.and_then(|s| s.parse().ok())
.unwrap_or(24);
let mut display = 20_000;
println!("SURVEY size={want}");
println!("shape,size,w,h,truth_x,truth_y,box,t,sent_x,sent_y,base_x,base_y,base_err,head_x,head_y,head_err,master_x,master_y,master_err");
for name in &names {
let imgs = xcursor_images(&dir.join(name));
let Some((n, w, h, tx, ty, up)) = imgs.into_iter().find(|(n, ..)| *n == want) else {
println!("# {name}: no {want} px image");
continue;
};
// The opaque box, to tell the shapes apart afterwards.
let (mut minx, mut miny, mut maxx, mut maxy) = (w as i32, h as i32, -1i32, -1i32);
for (i, px) in up.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);
}
}
let bx = format!("{}x{}+{}+{}", maxx - minx + 1, maxy - miny + 1, minx, miny);
for t in [0, 90, 180, 270] {
let (scan, sw, sh) = as_scanned_out(&up, w, h, t);
// What the producer puts on the wire: the guess taken on the sprite as the plane
// holds it, with hot_measured false.
let sent = scrap::drm_reader::infer_hotspot(&scan, sw, sh);
// The flow before this PR: map that point back as if it were a pixel.
let base = unrotate_hotspot(t, sw as i32, sh as i32, sent.0, sent.1);
// The flow in this PR, read back from the real delivery path.
display += 1;
deliver_drm_cursor(display, 1, 1, sw as u32, sh as u32, sent.0, sent.1, false, scan, t);
let head = {
let map = DRM_CURSOR.lock().unwrap();
let (_, c) = map.get(&display).expect("nothing published");
(c.hotx, c.hoty)
};
let m = master_delivered(&up, w, h, t);
println!(
"{name},{n},{w},{h},{tx},{ty},{bx},{t},{},{},{},{},{:.2},{},{},{:.2},{},{},{:.2}",
sent.0, sent.1, base.0, base.1, dist(base, (tx, ty)),
head.0, head.1, dist(head, (tx, ty)),
m.0, m.1, dist(m, (tx, ty))
);
}
}
}
// -----------------------------------------------------------------------------------------
// rustdesk#16242 round 2: the guess measured against the hotspots a THEME declares.
//
// Every test above builds its own sprite, so all of them compare the guess with the guess.
// That can pin a mapping but it cannot say whether the guess is any good. These two read the
// expected value from the cursor theme instead - the click point its author wrote into the
// file, which is exactly what a compositor programs into HOTSPOT_X/Y when it has the
// property to program.
// -----------------------------------------------------------------------------------------
include!("drm_cursor_theme.rs");
/// Expand a fixture's packed alpha mask into the sprite the delivery path takes. Only alpha
/// is set: the guess reads nothing else.
fn theme_sprite(mask_hex: &str, w: usize, h: usize) -> Vec<u8> {
let bytes: Vec<u8> = (0..mask_hex.len() / 2)
.map(|i| u8::from_str_radix(&mask_hex[i * 2..i * 2 + 2], 16).expect("fixture hex"))
.collect();
let mut px = vec![0u8; w * h * 4];
for i in 0..w * h {
if bytes[i / 8] & (0x80 >> (i % 8)) != 0 {
px[i * 4 + 3] = 255;
}
}
px
}
/// The guess MASTER ships, frozen. Only a TALL box is centred there: the symmetric aspect
/// test came later, in this PR, so freezing that instead would measure this PR against its own
/// earlier revision rather than against what is merged.
fn master_guess(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)
}
}
/// What master DELIVERS for an upright sprite on an output turned by `t`: it guesses on the
/// sprite as the plane holds it and then maps that point as if it were a pixel, and it does
/// that only at 90 and 270 - 0 and 180 pass through untouched.
fn master_delivered(up: &[u8], w: usize, h: usize, t: i32) -> (i32, i32) {
let (scan, sw, sh) = as_scanned_out(up, w, h, t);
let (gx, gy) = master_guess(&scan, sw, sh);
if t == 90 || t == 270 {
unrotate_hotspot(t, sw as i32, sh as i32, gx, gy)
} else {
(gx, gy)
}
}
fn away(a: (i32, i32), b: (i32, i32)) -> f64 {
(((a.0 - b.0) as f64).powi(2) + ((a.1 - b.1) as f64).powi(2)).sqrt()
}
/// A hotspot names a point on the SHAPE, so the same sprite has to get the same click point
/// whatever angle the output happens to sit at. This drives the real delivery path for every
/// shape in the theme at all four angles and demands exactly that.
///
/// The maintainer's objection on this PR was a table of four shapes where the old flow landed
/// closer than the new one. It did, at some angles - and 16 to 19 px out at the others, which
/// is what the second half of this test measures: with the old flow not one of the 35 shapes
/// keeps its hotspot across the four angles, and the point moves by 15 px on average. That
/// number is also the control. Without it the first assertion would still pass on a revert,
/// since a flow that is wrong the same way at every angle is consistent too.
#[test]
fn a_published_hotspot_does_not_move_when_the_output_turns() {
let mut display = 21_000;
let (mut spread_sum, mut spread_max, mut spread_zero) = (0f64, 0f64, 0usize);
for &(name, w, h, _, _, mask) in THEME_SHAPES {
let (w, h) = (w as usize, h as usize);
let up = theme_sprite(mask, w, h);
let (mut published, mut mapped) = (Vec::new(), Vec::new());
for t in [0, 90, 180, 270] {
let (scan, sw, sh) = as_scanned_out(&up, w, h, t);
// What the producer puts on the wire: the guess taken on the sprite as the plane
// holds it, with no provenance behind it.
let sent = scrap::drm_reader::infer_hotspot(&scan, sw, sh);
mapped.push(unrotate_hotspot(t, sw as i32, sh as i32, sent.0, sent.1));
display += 1;
deliver_drm_cursor(
display, 1, 1, sw as u32, sh as u32, sent.0, sent.1, false, scan, t,
);
let map = DRM_CURSOR.lock().unwrap();
let (_, c) = map.get(&display).expect("the delivery path published nothing");
published.push((c.hotx, c.hoty));
}
assert!(
published.windows(2).all(|p| p[0] == p[1]),
"{name}: the published hotspot follows the output angle: {published:?}"
);
let spread = mapped
.iter()
.flat_map(|a| mapped.iter().map(move |b| away(*a, *b)))
.fold(0f64, f64::max);
spread_sum += spread;
spread_max = spread_max.max(spread);
if spread == 0.0 {
spread_zero += 1;
}
}
// The control, and it has to be read with the guess this PR now makes. A box centre and a
// heavier-half extreme both map like a pixel, so for the 16 shapes that land on one the
// old flow happens to be angle-independent too. The other 19 move, by up to 26.87 px on a
// 24 px sprite. Without this the assertion above would also pass on a revert: a flow that
// is wrong the same way at every angle is consistent too.
let n = THEME_SHAPES.len();
assert!(
n - spread_zero >= 15,
"the control has gone vacuous: the old flow now keeps {spread_zero} of {n} shapes \
stable, so the assertion above no longer separates the two flows"
);
assert!(
spread_sum / n as f64 > 5.0 && spread_max > 20.0,
"the control is weak: old-flow spread mean {:.2} max {:.2} over {n} shapes",
spread_sum / n as f64,
spread_max
);
}
/// How far the delivered hotspot lands from the click point the theme declares, per shape,
/// measured against MASTER and not against an earlier revision of this PR.
///
/// Master handles 0, 90 and 270 for the cursor (180 passes through untouched there), so the
/// comparison is over those three angles, which is the set master actually covers.
///
/// Two shapes stay further out than master and they are named rather than averaged away.
/// `help` is the honest limit of any bitmap rule: the theme puts its click point on the dot of
/// its question mark, which is not recoverable from alpha. `alias` is 0.51 px. Everything else
/// is equal or better, and the mean over the corpus falls from 12.08 px to 4.20.
#[test]
fn no_shape_lands_further_from_the_theme_than_master_except_the_two_no_bitmap_can_reach() {
const NOT_INFERABLE: [&str; 2] = ["help", "alias"];
let (mut sum_master, mut sum_now) = (0f64, 0f64);
let mut regressed = Vec::new();
for &(name, w, h, hx, hy, mask) in THEME_SHAPES {
let (w, h) = (w as usize, h as usize);
let up = theme_sprite(mask, w, h);
let truth = (hx, hy);
let angles = [0, 90, 270];
let mut e_master = 0f64;
let mut e_now = 0f64;
for t in angles {
e_master += away(master_delivered(&up, w, h, t), truth);
// The upright inference answers the same at every angle by construction; the
// angle-independence test above is what pins that.
e_now += away(scrap::drm_reader::infer_hotspot(&up, w, h), truth);
}
let (e_master, e_now) = (e_master / angles.len() as f64, e_now / angles.len() as f64);
sum_master += e_master;
sum_now += e_now;
if e_now > e_master + 0.005 && !NOT_INFERABLE.contains(&name) {
regressed.push((name, e_master, e_now));
}
}
let n = THEME_SHAPES.len() as f64;
assert!(
regressed.is_empty(),
"further from the theme hotspot than master, averaged over 0/90/270: {regressed:?}"
);
assert!(
sum_now / n <= 4.5 && sum_master / n >= 11.0,
"mean error over {n} shapes: master {:.2}, this pr {:.2} (measured 12.08 and 4.20)",
sum_master / n,
sum_now / n
);
}
/// The two shapes the test above excuses are excused for a measured reason, not because they
/// were in the way: `alias` is a rounding-sized 0.11 px and `help` is the bitmap limit. If
/// either ever moves, the allow-list has to be re-argued rather than quietly widened.
#[test]
fn the_two_excused_shapes_are_excused_by_how_much() {
for (name, bound) in [("alias", 0.75f64), ("help", 5.5f64)] {
let &(_, w, h, hx, hy, mask) = THEME_SHAPES
.iter()
.find(|s| s.0 == name)
.expect("the corpus must carry the excused shapes");
let (w, h) = (w as usize, h as usize);
let up = theme_sprite(mask, w, h);
let truth = (hx, hy);
let e_master: f64 = [0, 90, 270]
.iter()
.map(|&t| away(master_delivered(&up, w, h, t), truth))
.sum::<f64>()
/ 3.0;
let e_now = away(scrap::drm_reader::infer_hotspot(&up, w, h), truth);
assert!(
e_now - e_master <= bound,
"{name} is now {:.2} px worse than master, over the {bound} px this excuses",
e_now - e_master
);
}
}
}
src/server/drm_cursor_theme.rs
+48
View File
@@ -0,0 +1,48 @@
// Generated from the installed cursor theme by `adwaita_hotspot_survey` (the ignored test in
// `drm_capturer.rs`); see its doc comment for the command. adwaita-icon-theme 50.0-1, every shape
// the theme ships as a real file, at its 24 px nominal size - the GNOME default cursor size.
//
// Each entry is the shape name, the hotspot the THEME declares, and the sprite's alpha mask: one
// bit per pixel, row-major, most significant bit first, set where alpha >= 128. The mask is all
// the hotspot guess reads, and rotating a mask is exact, so it is the whole fixture.
//
// The point of carrying these is that the expected value comes from the theme author, not from
// our own heuristic: a test that compares the guess against the guess cannot say whether the
// guess is right.
const THEME_SHAPES: &[(&str, u32, u32, i32, i32, &str)] = &[
("X_cursor", 24, 24, 11, 12, "00000000000000000000000000000001830003c78007ffc007ffc007ffc003ff8001ff0001ff8003ff8007ffc007ffc003ef8003c780000000000000000000000000000000000000"),
("alias", 24, 24, 18, 5, "00000000000000000001ffc000ffc0007fc0003fc0007fc0007fc000ffc000ffc001fdc001f8c001f04001e00001e00001e00000e00000e000007000003000000000000000000000"),
("all-resize", 24, 24, 12, 11, "001800003c00007e0000ff0001ff8003ffc007ffe00f3cf01f3cf83ffffc7ffffe7ffffe3ffffc1f3cf80f3cf007ffe003ffc001ff8000ff00007e00003c00001800000000000000"),
("all-scroll", 24, 24, 11, 11, "00000000300000780000fc0001fe0003ff0007ff800e01c01e79e03efdf07efdf87efdf83efdf01e79e00e01c007ff8003ff0001fe0000fc00007800003000000000000000000000"),
("cell", 24, 24, 11, 11, "0000000078000078000078000078000078000078000078000030007f03f87f87f87f87f87f03f8003000007800007800007800007800007800007800007800000000000000000000"),
("col-resize", 24, 24, 12, 12, "0000000038000038000038000038000139000339800739c00f39e01ffff03ffff87ffffc3ffff83ffff80f39f00739e00739c0013900003800003800003800003800000000000000"),
("context-menu", 24, 24, 3, 1, "0000000000001000001800001c00001e00001f00001f80001fc0001fe7fe1ff7fe1ffffe1ffffe1ffffe1fe7fe1fe7fe1ff7fe1df7fe10fffe00fffe007ffe007800000000000000"),
("copy", 24, 24, 3, 1, "0000000000000000001000001800001c00001e00001f00001f80001fc0001fe3c01ffff01ffff81ffff81ffffc1f3ffc1e3ffc1c3ffc183ffc101ff8000ff00007e0000000000000"),
("crosshair", 24, 24, 11, 11, "0000000078000078000078000078000078000078000078000078007ffff87ffff87ffff87ffff8007800007800007800007800007800007800007800007800000000000000000000"),
("default", 24, 24, 3, 1, "0000000000001000001800001c00001e00001f00001f80001fc0001fe0001ff0001ff8001ffc001ffe001fe0001fe0001ff0001df00010f80000f800007800007800000000000000"),
("e-resize", 24, 24, 19, 13, "000000000000000000000000000078001078001878001c78001e78001f781ffff81ffff81ffff81ffff81ffff8001ff8001e78001c78001878001078000078000000000000000000"),
("ew-resize", 24, 24, 12, 12, "0000000000000000000000000000000081000181800381c00781e00ffff01ffff83ffffc1ffff81ffff80781f00381e00381c0008100000000000000000000000000000000000000"),
("grab", 24, 24, 11, 2, "000000000000001800003c0000ff0001ff8001ffe001fff001fff001fff001fff001fff00dfff01ffff01ffff00ffff00ffff007fff007fff003fff001fff000ffe0000000000000"),
("grabbing", 24, 24, 9, 5, "000000000000000000000000000000001f0000ff8001ffe001fff007fff00ffff00ffff00ffff00ffff007fff007fff003fff003fff001ffe0000000000000000000000000000000"),
("help", 24, 24, 12, 21, "000000007e0001ff8003ffc007ffc007ffe00fc7e00fc7e00fc7e0000fe0003fc0003f80007f00007e00007c00007c00000000007c00007c00007c00007c00007c00000000000000"),
("n-resize", 24, 24, 13, 6, "0000000000000000000ffff80ffff80ffff80ffff8007f00007f0000ff8001ffc003ffe007fff0003e00003e00003e00003e00003e00003e00003e00003e00000000000000000000"),
("ne-resize", 24, 24, 15, 10, "0000000000000000000000001ffff01ffff01ffff01ffff00000f00ffef007fef003fef001fef001fef003fef007fef00ffef01fcef01f86f01f02f01e00f00c0000000000000000"),
("nesw-resize", 24, 24, 11, 11, "00000000000000000000ffc0007fc0003fc0003fc0003fc0107fc018ffc01dffc01ff9c01ff0401fe0001fe0001fe0001ff0001ff800000000000000000000000000000000000000"),
("no-drop", 24, 24, 3, 1, "0000000000000000001000001800001c00001e00001f00001f80001fc0001fe3c01ffff01ffff81ffff81ffffc1f3ffc1e3ffc1c3ffc183ffc101ff8000ff00007e0000000000000"),
("not-allowed", 24, 24, 12, 12, "000000000000000000007e0001ff8003ffc007ffe00ffff00ffff01ffff81ffff81ffff81ffff81ffff81ffff80ffff00ffff007ffe003ffc001ff8000ff00000000000000000000"),
("ns-resize", 24, 24, 12, 13, "000000000000001000003800007c0000fe0001ff0003ff8007ffc0007c00007c00007c00007c00007c00007c0007ffc003ff8001ff8001ff0000fe00007c00003800000000000000"),
("nw-resize", 24, 24, 10, 10, "0000000000000000000000000ffff80ffff80ffff80ffff80f00000f7ff00f7fe00f7fc00f7f800f7f800f7fc00f7fe00f7ff00f73f80f61f80f40f80f0078000030000000000000"),
("nwse-resize", 24, 24, 11, 11, "0000000000000000001ff8001ff0001ff0001fe0001fe0001ff0401ff8c01ffdc01cffc0107fc0003fc0003fc0003fc0007fc000ffc0000000000000000000000000000000000000"),
("pointer", 24, 24, 7, 5, "00000000000000000000000000c00001e00001e00001e00001e00001ff0001ffe001fff001fff01dfff03ffff03ffff01ffff00ffff007fff003fff001fff000ffe0000000000000"),
("progress", 24, 24, 3, 1, "0000000000001000001800001c00001e00001f3e001fff801fffc01fffc01fffe01fffe01fffe01fffe01fffe01fffc01fffc01cff80103f00000000000000000000000000000000"),
("row-resize", 24, 24, 12, 13, "000000001000003800007c0000fe0001ff0003ff8007ffc0007c00007c007ffffc7ffffc7ffffc007c00007c0007ffc003ff8001ff8000ff00007e00007c00003800000000000000"),
("s-resize", 24, 24, 13, 18, "000000000000003e00003e00003e00003e00003e00003e00003e00003e0007fff003ffe001ffc000ff80007f00003e000ffff80ffff80ffff80ffff8000000000000000000000000"),
("se-resize", 24, 24, 15, 15, "0000000000000000001e00f01f02f01f86f01fcef00ffef007fef003fef001fef001fef003fef007fef00ffef00000f01ffff01ffff01ffff01ffff0000000000000000000000000"),
("sw-resize", 24, 24, 10, 15, "0000000000000000000f00780f40f80f61f80f73f80f7ff00f7fe00f7fc00f7f800f7f800f7fc00f7fe00f7ff00f00000ffff80ffff80ffff80ffff8000000000000000000000000"),
("text", 24, 24, 11, 12, "00000000000003de0003fe0003fe0000700000700000700000700000700000700000700000700000700000700000700000700000700000700003fe0003fe0003fe00000000000000"),
("vertical-text", 24, 24, 12, 11, "00000000000000000000000000000000000038001c38001c38001c3ffffc3ffffc3ffffc38001c38001c38001c000000000000000000000000000000000000000000000000000000"),
("w-resize", 24, 24, 6, 13, "0000000000000000000000001e00001e08001e18001e38001e78001ef8001ffff81ffff81ffff81ffff81ffff81ff8001e78001e38001e18001e08001e0000000000000000000000"),
("wait", 24, 24, 11, 11, "000000000000000000007e0001ff8003ffc007ffe00ffff00ffff01ffff81ffff81ffff81ffff81ffff81ffff80ffff00ffff007ffe003ffc001ff8000ff00000000000000000000"),
("zoom-in", 24, 24, 11, 10, "00000000000000000000fc0003ff0007ff800fffc00fffc01fffe01fffe01fffe01fffe01fffe01fffe00fffc00fffc007ffe003ffe000fff00000f80000f8000078000000000000"),
("zoom-out", 24, 24, 11, 10, "000000000000000000007e0001ff8003ffc007ffe007ffe00ffff00ffff00ffff00ffff00ffff00ffff007ffe007ffe003fff001fff0007ff800007c00007c00003c000000000000"),
];