mirror of
https://github.com/rustdesk/rustdesk.git
synced 2026-10-02 01:58:41 +00:00
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.
This commit is contained in:
Mariano Abad
@@ -2041,6 +2041,23 @@ 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 cargo test FILTER that matches nothing exits 0, so a rename would turn this step
|
||||
# into a green no-op. Assert it actually ran something.
|
||||
for filter in drm_capturer_tests ipc::test; do
|
||||
out=$(cargo test --locked --lib --features "$FEATURES" "$filter" 2>&1) || {
|
||||
echo "$out"; echo "::error::drm tests failed for filter '$filter'"; exit 1; }
|
||||
ran=$(echo "$out" | sed -nE 's/^running ([0-9]+) tests?$/\1/p' | paste -sd+ | bc)
|
||||
echo "$filter ran ${ran:-0} tests"
|
||||
if [ "${ran:-0}" -lt 1 ]; then
|
||||
echo "::error::filter '$filter' selected no tests; it was renamed or removed"
|
||||
exit 1
|
||||
fi
|
||||
done
|
||||
cargo build --locked --lib --features "$FEATURES" --release
|
||||
rm -rf target/release/deps target/release/build
|
||||
rm -rf ~/.cargo
|
||||
|
||||
@@ -523,6 +523,11 @@ 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')):
|
||||
# -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
|
||||
@@ -532,6 +537,8 @@ def build_libdrmtap_so():
|
||||
# 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
|
||||
|
||||
@@ -6,6 +6,7 @@ use super::drmtap_dl::{
|
||||
};
|
||||
use hbb_common::log;
|
||||
use std::ffi::CString;
|
||||
use std::os::raw::c_int;
|
||||
use std::io;
|
||||
use std::os::fd::{FromRawFd, OwnedFd};
|
||||
|
||||
@@ -48,6 +49,51 @@ pub struct CursorSnapshot {
|
||||
/// (2, 6), which is 11.2 px from the truth; the centre is 1.4 px away. Checked against all 35
|
||||
/// shapes of the installed theme at 24 px: making the test symmetric moves exactly that one shape
|
||||
/// and leaves the other 34 byte-identical, arrows included.
|
||||
/// 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
|
||||
}
|
||||
|
||||
/// 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<bool>, hot_x: i32, hot_y: i32) -> bool {
|
||||
match answered {
|
||||
Some(measured) => measured,
|
||||
None => hot_x != 0 || hot_y != 0,
|
||||
}
|
||||
}
|
||||
|
||||
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() {
|
||||
@@ -423,7 +469,20 @@ impl DrmReader {
|
||||
hash ^= p as u64;
|
||||
hash = hash.wrapping_mul(1099511628211);
|
||||
}
|
||||
let hot_measured = 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);
|
||||
let (hotx, hoty) = if hot_measured {
|
||||
(c.hot_x, c.hot_y)
|
||||
} else {
|
||||
@@ -432,11 +491,7 @@ impl DrmReader {
|
||||
// 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,
|
||||
@@ -496,3 +551,53 @@ impl Drop for DrmReader {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod hotspot_provenance_tests {
|
||||
use super::{cursor_id, hot_measured_from};
|
||||
|
||||
/// 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));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<FnCursorHotspotValid>,
|
||||
pub grab_desc: FnGrabDesc,
|
||||
pub open_render: FnOpenRender,
|
||||
pub convert_dmabuf: FnConvertDmabuf,
|
||||
@@ -278,6 +288,8 @@ impl DrmtapLib {
|
||||
};
|
||||
let render_node: Option<FnRenderNode> =
|
||||
lib.get(b"drmtap_render_node").ok().map(|s| *s);
|
||||
let cursor_hotspot_valid: Option<FnCursorHotspotValid> =
|
||||
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,
|
||||
|
||||
+58
-1
@@ -555,11 +555,29 @@ pub enum Data {
|
||||
height: u32,
|
||||
hotx: i32,
|
||||
hoty: i32,
|
||||
#[serde(default)]
|
||||
/// 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?;
|
||||
@@ -2306,4 +2324,43 @@ mod test {
|
||||
fn test_select_server_uid_fails_when_multiple_servers_are_ambiguous() {
|
||||
assert!(select_server_uid_for_user_main_ipc(&[501, 502], None, false).is_err());
|
||||
}
|
||||
|
||||
/// The upgrade window: an old root service is still streaming when a new `--server` starts,
|
||||
/// and its DrmCursor carries no `hot_measured` at all. Read as `false` the consumer would
|
||||
/// discard a hotspot the producer did measure and re-infer one from the bitmap, which moves
|
||||
/// the cursor on a rotated display until the old process is replaced. `bool::default()` is
|
||||
/// the wrong default here; the right one preserves what the old protocol meant.
|
||||
#[cfg(all(target_os = "linux", feature = "drm"))]
|
||||
#[test]
|
||||
fn a_drm_cursor_without_provenance_keeps_the_hotspot_the_producer_sent() {
|
||||
// `Data` is adjacently tagged (`tag = "t", content = "c"`), so this is the real shape on
|
||||
// the socket, not a simplified one.
|
||||
let legacy =
|
||||
r#"{"t":"DrmCursor","c":{"id":7,"width":24,"height":24,"hotx":12,"hoty":11}}"#;
|
||||
let msg: Data = serde_json::from_str(legacy).expect("legacy DrmCursor must deserialize");
|
||||
match msg {
|
||||
Data::DrmCursor {
|
||||
hotx,
|
||||
hoty,
|
||||
hot_measured,
|
||||
..
|
||||
} => {
|
||||
assert_eq!((hotx, hoty), (12, 11));
|
||||
assert!(
|
||||
hot_measured,
|
||||
"a message with no provenance field must be treated as the old protocol did, \
|
||||
i.e. use the hotspot as sent, not re-infer it"
|
||||
);
|
||||
}
|
||||
other => panic!("expected DrmCursor, got {other:?}"),
|
||||
}
|
||||
|
||||
// And a NEW producer that really wants re-inference says so, which serializes: the
|
||||
// default must not swallow an explicit false.
|
||||
let modern = r#"{"t":"DrmCursor","c":{"id":7,"width":24,"height":24,"hotx":0,"hoty":0,"hot_measured":false}}"#;
|
||||
match serde_json::from_str::<Data>(modern).expect("modern DrmCursor must deserialize") {
|
||||
Data::DrmCursor { hot_measured, .. } => assert!(!hot_measured),
|
||||
other => panic!("expected DrmCursor, got {other:?}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user