From 12f61b1fda041c7429b771f8c9adbb12131e50bd Mon Sep 17 00:00:00 2001 From: fufesou Date: Tue, 22 Sep 2026 15:18:10 +0800 Subject: [PATCH] Fix/drm rotated cursor (#16242) * 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 --- .github/workflows/flutter-build.yml | 38 ++ build.py | 50 +- libs/scrap/Cargo.toml | 2 +- libs/scrap/src/common/drm_reader.rs | 567 +++++++++++++++++-- libs/scrap/src/common/drmtap_dl.rs | 25 + src/ipc.rs | 59 ++ src/ipc/drm.rs | 4 + src/server/drm_capturer.rs | 815 +++++++++++++++++++++++++++- src/server/drm_cursor_theme.rs | 48 ++ 9 files changed, 1546 insertions(+), 62 deletions(-) create mode 100644 src/server/drm_cursor_theme.rs diff --git a/.github/workflows/flutter-build.yml b/.github/workflows/flutter-build.yml index 779971f2d..4199ae201 100644 --- a/.github/workflows/flutter-build.yml +++ b/.github/workflows/flutter-build.yml @@ -2046,6 +2046,44 @@ jobs: *) echo "::error::build.py returned no '$want' feature: $FEATURES"; exit 1 ;; esac done + # Run the drm-gated unit tests HERE, because nowhere else does. The ordinary linux CI + # runs the suite, but `drm` is opt-in, so every test behind that feature -- the cursor + # rotation and hotspot ones especially -- has no continuous coverage at all otherwise. + # Debug rather than --release: these are pure logic tests and the release build is + # already the slow part of this job. + # A MINIMAL feature set, not the production $FEATURES: these are pure logic tests of + # the drm paths, and pulling hwcodec in makes the test binary fail to link at all + # (undefined reference to fcntl64 in this container), so the tests never run. + # + # Two packages, because `cargo test --lib` at the root builds the ROOT package only: + # the provenance tests live in libs/scrap and would never be selected from here. The + # ordinary linux CI runs --workspace but without the drm feature, so these are not + # covered there either. + # + # awk sums the counts rather than bc, which this container does not install. + # + # A cargo test FILTER that matches nothing exits 0, so a rename would turn this into a + # green no-op. Assert each command actually ran something. + run_drm_tests() { + local desc="$1"; shift + local out + out=$("$@" 2>&1) || { echo "$out"; echo "::error::drm tests failed: $desc"; exit 1; } + local ran + ran=$(echo "$out" | awk '/^running [0-9]+ tests?$/ { n += $2 } END { print n+0 }') + echo "$desc ran $ran tests" + if [ "$ran" -lt 1 ]; then + echo "::error::$desc selected no tests; it was renamed or removed" + exit 1 + fi + } + run_drm_tests "rustdesk drm_capturer_tests" \ + cargo test --locked --lib --features drm drm_capturer_tests + run_drm_tests "rustdesk ipc::test" \ + cargo test --locked --lib --features drm ipc::test + run_drm_tests "scrap hotspot_provenance_tests" \ + cargo test --locked -p scrap --features drm hotspot_provenance_tests + run_drm_tests "scrap hotspot_guess_tests" \ + cargo test --locked -p scrap --features drm hotspot_guess_tests cargo build --locked --lib --features "$FEATURES" --release rm -rf target/release/deps target/release/build rm -rf ~/.cargo diff --git a/build.py b/build.py index 4f1953662..a32e5fccc 100755 --- a/build.py +++ b/build.py @@ -390,9 +390,9 @@ def ffi_bindgen_function_refactor(): # The commit is fetched directly by sha, so no branch or tag name takes part in the build: see # build_libdrmtap_so(). This is the SINGLE source of truth for the pin, deliberately not duplicated in # any workflow, so a bump is one edit here (plus the informational version comment in -# libs/scrap/Cargo.toml). This commit is libdrmtap v0.5.4. +# libs/scrap/Cargo.toml). This commit is libdrmtap v0.5.6. LIBDRMTAP_REPO_PINNED = 'https://github.com/rustdesk-org/libdrmtap' -LIBDRMTAP_SHA_PINNED = '5da68a3a368db569716d0d0f11cefacbb11b2290' +LIBDRMTAP_SHA_PINNED = '49b204f275af1a2d6dfead94effb4036c7d50a3a' LIBDRMTAP_REPO = os.environ.get('DRMTAP_REPO', LIBDRMTAP_REPO_PINNED) LIBDRMTAP_SHA = os.environ.get('DRMTAP_SHA', LIBDRMTAP_SHA_PINNED) # Every way of getting a different .so than the pin needs the same explicit opt-in. Otherwise the @@ -489,8 +489,10 @@ def build_libdrmtap_so(): # override of the three -- no fetch, no sha verification, an object built by something this # script cannot see -- so it is the likeliest to hand over a CPU-only build, and skipping the # assertion on exactly this path would leave the check guarding only the case that was - # already trustworthy. + # already trustworthy. The helper check rides along for the same reason, and it matters + # more here than anywhere: this is the branch the packaging workflow takes. _assert_so_has_egl(so) + _assert_so_has_no_helper(so) return so # Fetch the pinned source if it is not already present. third_party/libdrmtap is not a submodule # anymore; it is git-ignored. The commit is fetched BY SHA rather than by cloning a branch: @@ -521,8 +523,22 @@ def build_libdrmtap_so(): f'libdrmtap at {src} is {got_sha}, expected {LIBDRMTAP_SHA} ' f'(stale checkout from a different pin; removed, re-run to re-fetch)') build_dir = os.path.join(src, 'build-pkg') + # Configure a fresh dir, or RE-configure one an earlier build left behind. Without the second + # branch a build-pkg created before this option was added keeps its old configuration, meson + # skips setup, and the compile produces a helper-enabled .so. The assertion below then fails + # the build rather than shipping it, so this is a developer-build correctness fix and not a + # security one -- but it turns a confusing failure into no failure at all. if not os.path.exists(os.path.join(build_dir, 'build.ninja')): - system2(f'meson setup "{build_dir}" "{src}" --buildtype=release') + # -Dhelper=disabled: rustdesk never uses the privileged helper. The capture context is + # opened only in the root service (every drmtap_open lives in src/ipc/drm.rs), which + # already has CAP_SYS_ADMIN, and the unprivileged side opens a render node instead. Without + # this the library still carries the fallback: a fork/exec that walks six hardcoded paths, + # two of them under /usr/local, and execs the first that passes access(X_OK) with no check + # of its owner or mode -- inside the ROOT process. The option compiles that path out + # entirely. Requested by the maintainer on rustdesk#16242. + system2(f'meson setup "{build_dir}" "{src}" --buildtype=release -Dhelper=disabled') + else: + system2(f'meson configure "{build_dir}" -Dhelper=disabled') # Build only the shared library, not the bundled helper binary or the static archive. Since # libdrmtap 0.4.11 the project is `both_libraries` (a version-scripted .so + a static .a), so the # bare `drmtap` target is ambiguous ("drmtap:shared_library" vs "drmtap:static_library"); ask for @@ -533,9 +549,31 @@ def build_libdrmtap_so(): # require exactly one so a stale object from an earlier build is never silently picked. so = _single_real_so(sos, f'the libdrmtap meson build dir {build_dir}') _assert_so_has_egl(so) + _assert_so_has_no_helper(so) return so +def _assert_so_has_no_helper(so_path): + # Asserted on the ARTIFACT for the same reason the EGL check is: a build flag cannot notice a + # stale object left by an earlier build, or one substituted by hand, and `-Dhelper=disabled` is + # silently accepted by meson versions that predate the option. What the option removes is the + # whole fork/exec path, so the produced .so imports no socketpair and carries none of the + # helper search paths; both go to zero, measured on libdrmtap 0.5.6 (4 -> 0 and 3 -> 0). + # + # This matters because the object is loaded by the ROOT service. With the path compiled in, a + # caller that ever lost CAP_SYS_ADMIN would exec whatever sits at the first of six hardcoded + # locations that passes access(X_OK), without checking its owner or its mode. + with open(so_path, 'rb') as f: + blob = f.read() + found = [m.decode() for m in (b'socketpair', b'/usr/libexec/drmtap-helper') if m in blob] + if found: + raise Exception( + f'{so_path} still carries the privileged-helper fallback ({", ".join(found)}). ' + f'It must be built with -Dhelper=disabled: rustdesk captures as root and never uses ' + f'the helper, so this is a fork/exec path in the privileged process that nothing ' + f'reaches. Delete third_party/libdrmtap/build-pkg and rebuild.') + + def _assert_so_has_egl(so_path): # libdrmtap treats egl/glesv2 as OPTIONAL dependencies: without their headers and pkg-config # files, meson silently builds a CPU-only stub. That stub still exports every symbol the loader @@ -863,8 +901,10 @@ def build_deb_from_folder(version, binary_folder, want_drm=False): # takes the .so straight out of a bundle somebody else produced, so it has the same # exposure as DRMTAP_PREBUILT_DIR (see the comment on that branch). A CPU-only stub # would ship, the loader would accept it, and capture would degrade to PipeWire - # without a word. + # without a word. A bundle produced before the helper was disabled is caught here too, + # which is the point: it must not be repackaged into a deb. _assert_so_has_egl(so) + _assert_so_has_no_helper(so) stage_libdrmtap_into_deb(so) system2(f'rm -f "{so}"') system2('rm -f tmpdeb/usr/share/rustdesk/libdrmtap.so tmpdeb/usr/share/rustdesk/libdrmtap.so.0') diff --git a/libs/scrap/Cargo.toml b/libs/scrap/Cargo.toml index 1cc1ff619..709e2bf21 100644 --- a/libs/scrap/Cargo.toml +++ b/libs/scrap/Cargo.toml @@ -14,7 +14,7 @@ wayland = ["gstreamer", "gstreamer-app", "gstreamer-video", "dbus", "tracing", " # `drm` is a pure runtime-dlopen backend: rustdesk loads `libdrmtap.so.0` at runtime (`drmtap_dl.rs`) # and NEVER link-time links it, so the graceful PipeWire fallback when the .so or EGL is absent is # preserved and the drm build pulls in no libdrm/seccomp/cap/EGL link-time deps. The .so is pinned by -# `DRMTAP_SHA` in build.py, which fetches that exact commit (libdrmtap v0.5.4). We deliberately do +# `DRMTAP_SHA` in build.py, which fetches that exact commit (libdrmtap v0.5.6). We deliberately do # NOT depend on the `libdrmtap-sys` crate: its build.rs statically compiles the whole libdrmtap C tree # and a CAP_SYS_ADMIN helper and emits `-ldrm -lseccomp -lcap`, which would defeat the dlopen model. # Depends on `wayland`: the three drm modules live inside the `#[cfg(feature = "wayland")]` arm of diff --git a/libs/scrap/src/common/drm_reader.rs b/libs/scrap/src/common/drm_reader.rs index 3d19c6c41..a269ffe89 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -8,6 +8,7 @@ use hbb_common::log; use std::ffi::CString; use std::io; use std::os::fd::{FromRawFd, OwnedFd}; +use std::os::raw::c_int; // Trust-boundary limits and formats `drm_render` (the unprivileged converter) imports: two copies that drift apart would weaken one side. // 16384 covers 8K+ with headroom; anything larger is rejected as a bogus/hostile geometry. @@ -29,9 +30,274 @@ 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, } +/// Fold everything that makes a cursor a DIFFERENT cursor into its id. +/// +/// The producer dedupes by comparing this against the last one it sent, so anything left out is +/// something a change in which the consumer will never be told about. Geometry and the hotspot are +/// the obvious ones: identical pixels at a new size or hotspot are a new shape. +/// +/// `hot_measured` belongs here too, and that is less obvious. It is not metadata about the cursor, +/// it selects what the consumer DOES with it: false means rotate the bitmap and re-infer a +/// hotspot, true means map the supplied point. So a sample whose pixels and `(0, 0)` hotspot are +/// unchanged but whose provenance flipped is a different cursor as far as the client is concerned, +/// and without this it would be deduped away and never sent. +fn cursor_id(hash: u64, width: u32, height: u32, hotx: i32, hoty: i32, hot_measured: bool) -> u64 { + let mut id = hash; + for v in [ + width as u64, + height as u64, + hotx as u32 as u64, + hoty as u32 as u64, + hot_measured as u64, + ] { + id ^= v; + id = id.wrapping_mul(1099511628211); + } + id +} + +/// Encodes a provenance answer for the one-shot log below. -1 is "nothing reported yet". +fn provenance_code(answered: Option) -> i8 { + match answered { + None => 0, + Some(false) => 1, + Some(true) => 2, + } +} + +/// Whether this sample's provenance is worth a log line, given what was last reported. +/// +/// Its own function because the obvious version of this is a line per cursor read, and a cursor is +/// read far more often than it changes. A per-sample log here was a real defect in libdrmtap for +/// the same reason. So: the first sample of a run, and thereafter only a genuine change of state. +fn provenance_is_news(last: i8, now: i8) -> bool { + last != now +} + +impl DrmReader { + /// Say once, and again only if it changes, where the hotspot is coming from. Without this the + /// three states are indistinguishable on a running box, which makes the decision unverifiable + /// anywhere but a unit test. + fn note_hotspot_provenance(&mut self, answered: Option, hot_measured: bool) { + let now = provenance_code(answered); + let last = std::mem::replace(&mut self.last_provenance, now); + if !provenance_is_news(last, now) { + return; + } + match answered { + Some(true) => log::info!( + "drm: cursor hotspot provenance: the driver published HOTSPOT_X/Y, using its value" + ), + Some(false) => log::info!( + "drm: cursor hotspot provenance: the plane exposes no HOTSPOT_X/Y, inferring the \ + hotspot from the bitmap" + ), + None => log::info!( + "drm: cursor hotspot provenance: unavailable (libdrmtap older than 0.5.6, or a \ + pre-0.5.6 privileged helper); falling back to the coordinate heuristic, which says \ + hot_measured={hot_measured}" + ), + } + } +} + +/// Whether `hot_x`/`hot_y` are the driver's answer rather than a guess. +/// +/// `answered` is what `drmtap_cursor_hotspot_valid` said: `Some(v)` when it answered, `None` when +/// the symbol is absent (a library older than 0.5.6) or it reported that nothing recorded an +/// answer for this sample. +/// +/// When it answered, that is the answer, full stop - including `Some(true)` on a `(0, 0)` hotspot, +/// which is the whole reason the entry point exists: a para-virtualized driver really can put the +/// hotspot at the top-left corner, and the coordinates alone cannot tell that apart from a plane +/// that exposes no HOTSPOT_X/Y at all. When it could not answer, fall back to the old test. That +/// test is wrong in both directions, but it is what this code did before, and it keeps a deployed +/// older `.so` behaving exactly as it used to instead of changing under it. +fn hot_measured_from(answered: Option, hot_x: i32, hot_y: i32) -> bool { + match answered { + Some(measured) => measured, + None => hot_x != 0 || hot_y != 0, + } +} + +/// A mirror has to agree on this many of every 100 pixels of the union to count as symmetry. +/// Swept against master over two theme versions: below 86 the rule starts moving shapes whose +/// corner guess was already right (`grabbing`), and from 92 up the directional cursors of the +/// older theme stop being recognised and regress. 86 to 91 is the interval that holds on both, +/// so this sits in the middle of it. +const MIRROR_AGREEMENT_PERCENT: u64 = 88; + +/// Guess the click point of a cursor bitmap, for the drivers that publish no hotspot. +/// +/// Three facts drive it, in order of how much of the shape they read. +/// +/// A shape that is mirror symmetric about one of the box's mid-lines has no distinguished end on +/// that axis: a crosshair, an I-beam, a two-headed resize arrow. On such an axis the click point +/// is the middle. +/// +/// A shape that mirrors onto itself only under a half turn or about a DIAGONAL is a two-headed or +/// corner resize cursor. Those are a pointer plus the marker of the edge being resized, and the +/// theme puts the click point on that marker, which is the heavier end. The heavier end is also +/// the only thing that separates `e-resize` from `w-resize`: they are the same picture reflected. +/// +/// Everything else keeps the top-left corner of the opaque box, because cursors are DRAWN +/// pointing up and to the left, so that corner is where an arrow's tip is. That is a fact about +/// the sprite in its own upright frame, and the reason this guess is only meaningful when taken +/// there rather than on a sprite the compositor pre-rotated. +/// +/// Measured against the hotspots the theme declares, which is the value a compositor programs +/// into HOTSPOT_X/Y where the property exists, and against what master delivers over the angles +/// master handles (0, 90, 270). adwaita-icon-theme 50 at 24 px: mean error per shape 12.08 px +/// down to 4.20, better on 83 of the 105 (shape, angle) cases and worse on 4. The same holds at +/// 48 px and on adwaita-icon-theme 46. +/// +/// Two shapes stay further out than master and no bitmap rule reaches them: `help`, whose click +/// point the theme puts on the dot of its question mark, and `alias` by half a pixel. That is why +/// a hotspot the driver publishes is always preferred to this, and why rustdesk#16122 recovers it +/// exactly from the injected pointer instead of guessing. +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); + let (cx, cy) = ((minx + maxx) / 2, (miny + maxy) / 2); + // An elongated box keeps its centre whichever way it lies (rustdesk#16242): a bar has no + // corner to speak of. Kept ahead of the mirror test so it still covers an elongated shape + // that is NOT symmetric, which the theme happens not to contain but a custom cursor can. + if bh > bw * 2 || bw > bh * 2 { + return (cx, cy); + } + let opaque = |x: i32, y: i32| { + rgba.get((y as usize * w + x as usize) * 4 + 3) + .is_some_and(|&a| a >= 128) + }; + let agrees = |i: u64, u: u64| u == 0 || i * 100 >= u * MIRROR_AGREEMENT_PERCENT; + // One pass over the opaque box, comparing it with its own three mirrors, and the mass of each + // half while we are here. + let (mut ix, mut ux) = (0u64, 0u64); + let (mut iy, mut uy) = (0u64, 0u64); + let (mut ih, mut uh) = (0u64, 0u64); + let (mut left, mut right, mut top, mut bottom) = (0u64, 0u64, 0u64, 0u64); + let (half_w, half_h) = (bw / 2, bh / 2); + for y in miny..=maxy { + for x in minx..=maxx { + let (fx, fy) = (maxx - (x - minx), maxy - (y - miny)); + let here = opaque(x, y); + for (other, (i, u)) in [ + (opaque(fx, y), (&mut ix, &mut ux)), + (opaque(x, fy), (&mut iy, &mut uy)), + (opaque(fx, fy), (&mut ih, &mut uh)), + ] { + *i += u64::from(here && other); + *u += u64::from(here || other); + } + if here { + if x - minx < half_w { + left += 1; + } else if maxx - x < half_w { + right += 1; + } + if y - miny < half_h { + top += 1; + } else if maxy - y < half_h { + bottom += 1; + } + } + } + } + // Symmetric under a half turn and the centre is the only point the shape cannot tell from + // itself, whichever the per-axis answers are: a two-headed diagonal arrow mirrors to the + // other diagonal on each axis alone, so neither axis reads as symmetric. + if agrees(ih, uh) { + return (cx, cy); + } + let diagonal = mirrors_about_a_diagonal(&opaque, minx, miny, bw, bh, &agrees); + // An axis is DIRECTIONAL when the shape is a pointer plus an edge marker rather than a plain + // arrow: the perpendicular axis mirrors (the n/e/s/w resize cursors), or the shape mirrors + // about a diagonal (the corner ones). The marker is the end the click point belongs to, and + // it is the heavier end, which is the only thing that tells `e-resize` from `w-resize` - they + // are the same picture reflected. Everything else keeps the up-left corner. + let side = |heavier_is_max: bool, lo: i32, hi: i32| if heavier_is_max { hi } else { lo }; + ( + if agrees(ix, ux) { + cx + } else if agrees(iy, uy) || diagonal { + side(right > left, minx, maxx) + } else { + minx + }, + if agrees(iy, uy) { + cy + } else if agrees(ix, ux) || diagonal { + side(bottom > top, miny, maxy) + } else { + miny + }, + ) +} + +/// Whether the opaque box mirrors onto itself about either of its diagonals. +/// +/// A diagonal reflection of a `bw x bh` box is a `bh x bw` box, so the two can only be compared +/// inside a square, and where the box is not square the answer depends on where in that square it +/// is placed: the corner resize cursors are drawn against one corner, and padding away from that +/// corner moves the shape off the diagonal it is symmetric about. `ne-resize` scores 0.62 anchored +/// top-left and 0.98 anchored bottom-right; `nw-resize` is the other way round. So both anchorings +/// are tried and the best answer wins, which is what "the shape is symmetric" means once the +/// reflection is allowed to be shifted back into place. +fn mirrors_about_a_diagonal( + opaque: &impl Fn(i32, i32) -> bool, + minx: i32, + miny: i32, + bw: i32, + bh: i32, + agrees: &impl Fn(u64, u64) -> bool, +) -> bool { + let k = bw.max(bh); + for (dx, dy) in [(0, 0), (k - bw, k - bh)] { + // The box placed at (dx, dy) inside a k x k square, empty everywhere else. + let cell = |r: i32, c: i32| { + let (x, y) = (c - dx, r - dy); + (0..bw).contains(&x) && (0..bh).contains(&y) && opaque(minx + x, miny + y) + }; + let (mut id, mut ud, mut ia, mut ua) = (0u64, 0u64, 0u64, 0u64); + for r in 0..k { + for c in 0..k { + let here = cell(r, c); + for (other, (i, u)) in [ + (cell(c, r), (&mut id, &mut ud)), + (cell(k - 1 - c, k - 1 - r), (&mut ia, &mut ua)), + ] { + *i += u64::from(here && other); + *u += u64::from(here || other); + } + } + } + if agrees(id, ud) || agrees(ia, ua) { + return true; + } + } + false +} + /// 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, @@ -101,6 +367,13 @@ pub struct DrmReader { lib: &'static DrmtapLib, ctx: *mut drmtap_ctx, buf: Vec, + /// Last provenance reported by THIS reader, for the one-shot log. Per reader and not global: + /// several readers run at once, one per captured display, and their provenance can legitimately + /// differ and be stable -- a virtio-gpu output publishes HOTSPOT_X/Y while an i915 one on the + /// same host does not. A single global would read those two steady streams as an endless + /// alternation and log on every sample, which is the per-frame logging this was written to + /// avoid. -1 is "nothing reported yet". + last_provenance: i8, } impl DrmReader { @@ -134,6 +407,7 @@ impl DrmReader { return None; } Some(DrmReader { + last_provenance: -1, lib, ctx, buf: Vec::new(), @@ -363,6 +637,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,53 +651,45 @@ 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 { + // Ask the library where the hotspot came from instead of guessing from the + // coordinates. `hot_x/hot_y == (0, 0)` means two opposite things - the plane + // exposes no HOTSPOT_X/Y (every bare-metal driver), or it exposes them and the + // driver's answer IS the top-left corner - and the old test below cannot separate + // them: it overrides a real (0, 0) measurement with a guess from the bitmap, and + // on a driver without the properties it would trust a hotspot nobody published. + // Asked BEFORE cursor_release, which is what owns this sample. + let answered = self.lib.cursor_hotspot_valid.and_then(|f| { + let mut valid: c_int = 0; + // 0 = answered; -ENOTSUP = nothing recorded an answer for this sample, which + // is what a cursor read through a pre-0.5.6 privileged helper produces. + (f(&c, &mut valid) == 0).then(|| valid != 0) + }); + let hot_measured = hot_measured_from(answered, c.hot_x, c.hot_y); + self.note_hotspot_provenance(answered, hot_measured); + 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 // update (it dedupes by id) and the client keeps rendering the stale cursor. - let mut id = hash; - for v in [cw as u32 as u64, ch as u32 as u64, hotx as u32 as u64, hoty as u32 as u64] { - id ^= v; - id = id.wrapping_mul(1099511628211); - } + let id = cursor_id(hash, cw as u32, ch as u32, hotx, hoty, hot_measured); Some(CursorSnapshot { id, width: cw as u32, height: ch as u32, hotx, hoty, + hot_measured, colors, }) } else { @@ -475,3 +742,239 @@ impl Drop for DrmReader { } } } + +#[cfg(test)] +mod hotspot_provenance_tests { + use super::{cursor_id, hot_measured_from, provenance_code, provenance_is_news}; + + /// The log this makes observable is one line per CHANGE, not one per cursor read. A cursor is + /// read far more often than its provenance changes, and a per-sample line here would be the + /// same defect libdrmtap already had once. + #[test] + fn provenance_is_logged_once_and_then_only_on_a_change() { + let (none, guessed, measured) = ( + provenance_code(None), + provenance_code(Some(false)), + provenance_code(Some(true)), + ); + assert_ne!(none, guessed); + assert_ne!(guessed, measured); + assert_ne!(none, measured); + // First sample of a run: -1 is the unset sentinel, so anything is news. + assert!(provenance_is_news(-1, measured)); + // The same state again is not. + assert!(!provenance_is_news(measured, measured)); + assert!(!provenance_is_news(guessed, guessed)); + // A real transition is. + assert!(provenance_is_news(guessed, measured)); + assert!(provenance_is_news(measured, none)); + } + + /// The dedup key has to move when the provenance does, or the consumer is never told that the + /// same picture now means something different. This is the transition that produces it: the + /// properties become readable, the driver's answer is the corner, and the bitmap is unchanged. + #[test] + fn provenance_changes_the_cursor_identity() { + let guessed = cursor_id(0xabc, 24, 24, 0, 0, false); + let measured = cursor_id(0xabc, 24, 24, 0, 0, true); + assert_ne!( + guessed, measured, + "same pixels, same (0, 0), different meaning: the producer must not dedupe this away" + ); + // And the pre-existing parts still count, so this did not trade one blind spot for another. + assert_ne!(cursor_id(0xabc, 24, 24, 0, 0, true), cursor_id(0xabc, 32, 24, 0, 0, true)); + assert_ne!(cursor_id(0xabc, 24, 24, 0, 0, true), cursor_id(0xabc, 24, 24, 1, 0, true)); + assert_ne!(cursor_id(0xabc, 24, 24, 0, 0, true), cursor_id(0xdef, 24, 24, 0, 0, true)); + } + + /// The case the whole entry point exists for, and the one the old heuristic got backwards: a + /// para-virtualized driver that really publishes the hotspot at the image's top-left corner. + /// `(0, 0)` measured is still measured, and re-inferring one from the bitmap would move a + /// cursor the driver had already placed. + #[test] + fn a_measured_zero_hotspot_is_measured() { + assert!(hot_measured_from(Some(true), 0, 0)); + } + + /// And the mirror: when the library says it was NOT measured, that is the answer even if the + /// coordinates happen to be non-zero. The old test would have trusted them. + #[test] + fn the_librarys_no_beats_the_coordinates() { + assert!(!hot_measured_from(Some(false), 12, 11)); + assert!(!hot_measured_from(Some(false), 0, 0)); + } + + /// No answer available: either the symbol is absent (a library older than 0.5.6) or nothing + /// recorded one for this sample (a cursor read through an older privileged helper). Neither + /// is "it was a guess", so the pre-existing behaviour is kept rather than resolved either way + /// -- an older deployed .so must keep behaving as it used to. + #[test] + fn without_an_answer_the_old_heuristic_stands() { + assert!(!hot_measured_from(None, 0, 0)); + assert!(hot_measured_from(None, 12, 11)); + assert!(hot_measured_from(None, 0, 5)); + } +} + +#[cfg(test)] +mod hotspot_guess_tests { + use super::infer_hotspot; + + /// A sprite from an ASCII picture: `#` is opaque, anything else transparent. Written this way + /// because every case here is about a SHAPE, and a shape is unreadable as a byte array. + fn sprite(rows: &[&str]) -> (Vec, usize, usize) { + let (h, w) = (rows.len(), rows[0].len()); + let mut px = vec![0u8; w * h * 4]; + for (y, row) in rows.iter().enumerate() { + assert_eq!(row.len(), w, "ragged picture"); + for (x, c) in row.bytes().enumerate() { + if c == b'#' { + px[(y * w + x) * 4 + 3] = 255; + } + } + } + (px, w, h) + } + + /// An arrow keeps the top-left corner of its box. Cursors are drawn pointing up and left, so + /// that corner IS the tip - and it is a fact about the sprite upright, which is why the + /// consumer re-takes this guess after turning a rotated sprite back. + #[test] + fn an_arrow_points_at_the_corner_of_its_box() { + let (px, w, h) = sprite(&[ + ".........", + ".#.......", + ".##......", + ".###.....", + ".####....", + ".#####...", + ".###.....", + ".#..##...", + "......##.", + ]); + assert_eq!(infer_hotspot(&px, w, h), (1, 1)); + } + + /// A shape that mirrors onto itself about the box's mid-lines has no tip to point with, so + /// the corner names nothing and the middle is the only answer the shape supports. This is the + /// crosshair/cell/resize family, 17 of the installed theme's 35 shapes. + #[test] + fn a_symmetric_shape_is_held_by_its_middle() { + let (px, w, h) = sprite(&[ + "....#....", + "....#....", + "....#....", + "....#....", + "#########", + "....#....", + "....#....", + "....#....", + "....#....", + ]); + assert_eq!(infer_hotspot(&px, w, h), (4, 4)); + } + + /// Each axis is decided by its OWN mirror. This shape is symmetric left-to-right and not + /// top-to-bottom, which is the single-direction resize family: the click point is centred + /// across the axis that mirrors and stays at the edge on the axis that does not. + #[test] + fn one_symmetric_axis_centres_that_axis_only() { + let (px, w, h) = sprite(&[ + "....#....", + "...###...", + "..#####..", + ".#######.", + "....#....", + "....#....", + "....#....", + "....#....", + "....#....", + ]); + assert_eq!(infer_hotspot(&px, w, h), (4, 0)); + } + + /// A two-headed DIAGONAL arrow mirrors onto the other diagonal about either axis alone, so + /// neither axis reads as symmetric - and it is still a shape with no tip. The half turn is + /// what catches it. + #[test] + fn a_diagonal_two_headed_arrow_is_held_by_its_middle_too() { + let (px, w, h) = sprite(&[ + "###......", + "##.......", + "#.#......", + "...#.....", + "....#....", + ".....#...", + "......#.#", + ".......##", + "......###", + ]); + let (hx, hy) = infer_hotspot(&px, w, h); + assert_eq!((hx, hy), (4, 4), "a half turn maps this shape onto itself"); + } + + /// A single-direction resize cursor is an arrow plus the bar of the edge it resizes, and the + /// theme puts the click point on the BAR. The bar is the heavy end, and it is the only thing + /// that separates `e-resize` from `w-resize`: they are the same picture reflected, so a rule + /// that always took the up-left corner had to be wrong for one of the two. + #[test] + fn a_directional_resize_cursor_is_held_by_its_bar() { + let (px, w, h) = sprite(&[ + "......####", + "..##..####", + ".###..####", + "####..####", + ".###..####", + "..##..####", + "......####", + ]); + assert_eq!(infer_hotspot(&px, w, h), (9, 3)); + // The mirror image has to answer the other side, or the rule is not reading the shape. + let (px, w, h) = sprite(&[ + "####......", + "####..##..", + "####..###.", + "####..####", + "####..###.", + "####..##..", + "####......", + ]); + assert_eq!(infer_hotspot(&px, w, h), (0, 3)); + } + + /// A corner resize cursor is a bracket hugging one corner, and it mirrors onto itself about a + /// DIAGONAL rather than about either axis, so neither per-axis test sees it. The click point + /// is the inner corner of the bracket, which is the heavy end on both axes. + /// + /// A real one's box is usually not square, and a diagonal reflection only lines up inside a + /// square, so which corner of that square the box is placed against 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 therefore tried. That case cannot be built as a small + /// picture - a non-square box can never be exactly diagonal-symmetric - so it is covered by + /// the theme corpus instead, where dropping the second anchoring sends `ne-resize` back past + /// master. + #[test] + fn a_corner_bracket_is_held_by_its_inner_corner() { + let (px, w, h) = sprite(&[ + ".....##", ".....##", ".....##", ".....##", ".....##", "#######", "#######", + ]); + assert_eq!(infer_hotspot(&px, w, h), (6, 6)); + } + + /// The elongation rule stays ahead of the mirror test, so a bar that is NOT symmetric still + /// keeps its centre. Nothing in the installed theme is shaped like this - its two I-beams are + /// both symmetric - but a custom cursor can be, and this is the rustdesk#16242 round-one fix. + #[test] + fn an_elongated_shape_keeps_its_centre_even_when_it_is_lopsided() { + let (px, w, h) = sprite(&["##########.", "#####......", "##########."]); + assert_eq!(infer_hotspot(&px, w, h), (4, 1)); + } + + /// Nothing opaque is not a shape, and must not read as a box at the origin with a hotspot in + /// the middle of it. + #[test] + fn a_blank_sprite_has_no_hotspot_to_guess() { + let (px, w, h) = sprite(&["...", "...", "..."]); + assert_eq!(infer_hotspot(&px, w, h), (0, 0)); + } +} diff --git a/libs/scrap/src/common/drmtap_dl.rs b/libs/scrap/src/common/drmtap_dl.rs index 0312c75bb..ddaaa6071 100644 --- a/libs/scrap/src/common/drmtap_dl.rs +++ b/libs/scrap/src/common/drmtap_dl.rs @@ -127,6 +127,12 @@ type FnGrabMapped = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_frame_info type FnFrameRelease = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_frame_info); type FnGetCursor = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_cursor_info) -> c_int; type FnCursorRelease = unsafe extern "C" fn(*mut drmtap_ctx, *mut drmtap_cursor_info); +/// `drmtap_cursor_hotspot_valid`, added in libdrmtap 0.5.6. Answers, for the sample in hand, +/// whether `hot_x`/`hot_y` were read from the driver's HOTSPOT_X/Y plane properties: 0 with +/// `*valid` set, `-EINVAL` on a null argument, and `-ENOTSUP` when nothing recorded an answer, +/// which is what a cursor read through an older privileged helper produces. +type FnCursorHotspotValid = + unsafe extern "C" fn(*const drmtap_cursor_info, *mut c_int) -> c_int; // Split-capture entry points (libdrmtap >= 0.4.10), required: `grab_desc` runs on the privileged // export side, `open_render`/`convert_dmabuf` on the unprivileged converter side. type FnGrabDesc = @@ -148,6 +154,10 @@ pub struct DrmtapLib { pub frame_release: FnFrameRelease, pub get_cursor: FnGetCursor, pub cursor_release: FnCursorRelease, + /// Optional: it only exists from libdrmtap 0.5.6. Absent means the library cannot say where a + /// hotspot came from, NOT that it was a guess; `drm_reader` keeps the old heuristic for that + /// case and nothing else changes. + pub cursor_hotspot_valid: Option, pub grab_desc: FnGrabDesc, pub open_render: FnOpenRender, pub convert_dmabuf: FnConvertDmabuf, @@ -278,6 +288,8 @@ impl DrmtapLib { }; let render_node: Option = lib.get(b"drmtap_render_node").ok().map(|s| *s); + let cursor_hotspot_valid: Option = + lib.get(b"drmtap_cursor_hotspot_valid").ok().map(|s| *s); // Log the load only now that every required symbol resolved: this fn still returns None on a missing one. let loaded_from = real .as_ref() @@ -309,6 +321,18 @@ impl DrmtapLib { points at and remove any leftover libdrmtap.so.0* beside it. {effect}" ); } + // Same shape as the check above, and for the same reason: a library that REPORTS a + // version which exports the symbol and then does not have it is a stale or + // hand-substituted object, and saying so beats silently taking the legacy path. + if (minor, patch) >= (5, 6) && cursor_hotspot_valid.is_none() { + log::warn!( + "libdrmtap at {loaded_from} reports v{major}.{minor}.{patch} but is missing \ + drmtap_cursor_hotspot_valid: it is a stale or pre-release build. Check what \ + the soname symlink points at. Cursor hotspot provenance falls back to \ + guessing from the coordinates, which cannot tell an absent HOTSPOT_X/Y from \ + a driver that really puts the hotspot at (0, 0)." + ); + } Some(DrmtapLib { _lib: lib, open, @@ -319,6 +343,7 @@ impl DrmtapLib { frame_release, get_cursor, cursor_release, + cursor_hotspot_valid, grab_desc, open_render, convert_dmabuf, diff --git a/src/ipc.rs b/src/ipc.rs index b750607be..cc894b287 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -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::(modern).expect("modern DrmCursor must deserialize") { + Data::DrmCursor { hot_measured, .. } => assert!(!hot_measured), + other => panic!("expected DrmCursor, got {other:?}"), + } + } } 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..00ad02f93 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -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, 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 { 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::, 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)> = 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 +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, 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> = 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, 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, 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, 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, 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, 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::(&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::(&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)> { + 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)> = 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 = 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 { + let bytes: Vec = (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::() + / 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 + ); + } + } } diff --git a/src/server/drm_cursor_theme.rs b/src/server/drm_cursor_theme.rs new file mode 100644 index 000000000..8f8826f4f --- /dev/null +++ b/src/server/drm_cursor_theme.rs @@ -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"), +];