mirror of
https://github.com/rustdesk/rustdesk.git
synced 2026-09-28 16:26:08 +00:00
fufesouandMariano Abad
12f61b1fda
* 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 of9acb37a9d, 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 of7dc6817plus the CI failure fufesou reported on401c2f7. The drm test step was broken three ways and none of them showed up as a red test: it reused the production $FEATURES, so the test binary failed to LINK (undefined reference to fcntl64 from hwcodec) and the tests never ran; it summed the counts with bc, which the ubuntu18.04 container does not install; and `cargo test --lib` at the repo root builds the ROOT package only, so the provenance tests in libs/scrap were never selected -- the "at least one test ran" guard passed on the root filters and hid it. Now: --features drm rather than the production set, awk instead of bc, and an explicit `-p scrap` run with the same assertion. Locally 36, 9 and 5 tests, and a bogus filter still fails the step. Provenance logging state was process-global on both sides. DRM runs one reader per captured display, and two outputs with different but STABLE provenance -- one publishing HOTSPOT_X/Y and one not, which is an ordinary multi-GPU host -- would overwrite each other and make every message look like a change, i.e. exactly the per-frame logging the one-shot was written to avoid. The producer's state moves into DrmReader, the consumer's is keyed by display. The hidden-cursor sentinel is no longer recorded: it carries hot_measured=false because it has no hotspot, so counting it turned every hide and show into a provenance transition. The 180 test is renamed to say what it pins -- i915 advertising rotate-180 with mutter -- and its comment says plainly that it is not a general contract, that Plasma is still wrong there, and that changing it means re-measuring. Verified by mutation: restoring the global slot fails the new per-display test, and so does recording the hidden sentinel. * fix(drm): centre the guessed hotspot on the axes a cursor mirrors about The guess put the click point at the top-left corner of the opaque box unless the box was more than twice as long as it is wide. That corner is where an arrow's tip is, because cursors are drawn pointing up and to the left, but a shape that mirrors onto itself has no tip to point with: a crosshair, a two-headed resize arrow, a cell marker. Those got a corner where the theme puts the middle, and the error is about half the glyph. So: centre the axis the shape is symmetric about, and keep the corner everywhere else. Measured against the hotspots the theme declares, which is the value a compositor programs into HOTSPOT_X/Y where the property exists: adwaita-icon-theme 50, 35 shapes at six nominal sizes, 210 cases mean error 0.43 -> 0.27 of the cursor size, 17 shapes improve, none worse adwaita-icon-theme 46, 34 shapes at five sizes, 170 more cases better at every size, none worse The threshold is not fitted: from 90 to 98 percent agreement nothing regresses, and below 90 three shapes do. The corpus is carried as fixtures so the expected value comes from the theme author rather than from our own heuristic, and two tests read it: the published hotspot must not move when the output turns, with the old flow's 15 px average movement as the control, and the guess must stay inside the measured bounds with the old rule frozen as the baseline. The elongation test stays ahead of the mirror test so an elongated shape that is not symmetric still keeps its centre. * fix(drm): put a directional cursor's hotspot on the edge it resizes The mirror rule centred the axes a shape is symmetric about and left the other axis on the top-left corner. That is right for an arrow, whose tip is that corner, and wrong for the resize cursors: those are a pointer plus the marker of the edge being resized, the theme puts the click point on the marker, and the marker is at whichever end the cursor names. Always taking the corner therefore had to be wrong for one of every mirrored pair, since e-resize and w-resize are the same picture reflected. So an axis is read as directional when the perpendicular axis mirrors, which is the n/e/s/w family, or when the shape mirrors about a diagonal, which is the corner family. A directional axis takes the heavier end. A diagonal reflection only lines up inside a square, and these boxes are not square, so where the box sits in that square decides the answer: ne-resize scores 0.62 anchored one way and 0.98 the other, and nw-resize is the reverse. Both anchorings are tried and the better answer wins. Measured against MASTER now, not against this PR's previous revision, which is what the maintainer asked for. Over the angles master handles (0, 90, 270), adwaita-icon-theme 50 at 24 px: mean error per shape 12.08 px -> 4.20 (shape, angle) cases 83 better, 18 the same, 4 worse of 105 shapes worse than master help (+4.73) and alias (+0.51) Both of those are semantic: the theme puts help's click point on the dot of its question mark. The same holds at 48 px and on adwaita-icon-theme 46, where ne-resize is also 1.34 px worse because its glyph is drawn differently there. The test baseline was wrong and is fixed here: it froze this PR's earlier symmetric aspect test rather than master's tall-only one, so it could only show that the new rule does not regress the old rule. It now freezes master's guess AND master's delivery, and names the two shapes it excuses with the measured size of each. The mirror threshold moves from 95 to 88, the middle of the 86 to 91 interval that regresses nothing on either theme version. --------- Co-authored-by: Mariano Abad <weimaraner@gmail.com>