TheLanc3/rustdesk
1
0
Fork 0
mirror of https://github.com/rustdesk/rustdesk.git synced 2026-09-08 05:20:59 +03:00
Code Issues Packages Projects Releases Wiki Activity

drm: close the round-7 review findings

Browse Source 
- the renumbering probe in the DrmDisplaysChanged handler now reads the
  pushed list at wire_idx, the slot our monitor held in the service's
  index space, instead of at the index the client chose. the pushed
  list shares the handshake list's construction, so probing the client
  index compared two different index spaces whenever a wake or hotplug
  had renumbered entries - tearing down a healthy stream or missing a
  real renumbering.
- both message-body reads (cpu frame, cursor pixels) now run under a
  deadline. only the header read re-checked `stop`, so a producer dying
  between a header and its body pinned the receive thread forever and
  every rebuild leaked a thread plus its render context.
- the drm cursor cache gets a size ceiling (drm ids are derived from
  the shape's content, so an animated pointer minted a new key per
  shape and the map grew for the life of the service; x11 ids come
  from a small serial set, so the ceiling is gated and the stock build
  is untouched).
- has_non_drm_backed_display reads a two-scalar accessor instead of
  cloning and geometry-augmenting the whole display list on every
  cursor tick.
- the libdrmtap pin validation moved out of import time into
  build_libdrmtap_so(), so leftover DRMTAP_* environment variables or a
  malformed sha cannot fail a stock build that never touches libdrmtap.
- reworded a workflow comment whose literal expression marker broke
  actionlint.
This commit is contained in:
Mariano Abad
2026-07-30 10:39:59 -03:00
parent 2260054820
commit b977d9b010
5 changed files with 129 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
src/server/display_service.rs
View File

@@ -462,16 +462,17 @@ pub(super) fn get_display_info(idx: usize) -> Option<DisplayInfo> {
// list shorter than the synced list means at least one advertised display is served by PipeWire.
#[cfg(all(target_os = "linux", feature = "drm"))]
pub fn has_non_drm_backed_display() -> bool {
match super::drm_capturer::get_display_infos() {
// A display served by PipeWire is either ABSENT from the DRM list (a shorter list, e.g. a
match super::drm_capturer::display_count_and_any_demoted() {
// A display served by PipeWire is either ABSENT from the DRM list (a shorter count, e.g. a
// pure-portal display) or PRESENT-BUT-DEMOTED (kept in place at the same index and marked
// offline so the index space stays aligned -- see get_display_infos). The length check alone
// misses the demotion case (same length), so a display that is not online-DRM (`!online`) is
// treated as non-DRM-backed too. This is what gates the hidden-cursor sentinel: it stays
// authoritative only in a pure-DRM session.
Some(drm) => {
drm.len() < SYNC_DISPLAYS.lock().unwrap().displays.len()
|| drm.iter().any(|d| !d.online)
// offline so the index space stays aligned -- see get_display_infos). The count check alone
// misses the demotion case (same count), so a demoted display is treated as non-DRM-backed
// too. This is what gates the hidden-cursor sentinel: it stays authoritative only in a
// pure-DRM session. The scalar accessor is deliberate: this is polled every cursor tick
// while the sentinel is active, and cloning + geometry-augmenting the whole list per tick
// (what get_display_infos does) answered the same two facts.
Some((count, any_demoted)) => {
count < SYNC_DISPLAYS.lock().unwrap().displays.len() || any_demoted
}
None => false,
}

73
src/server/drm_capturer.rs
View File

@@ -51,6 +51,11 @@ const DISPLAY_LIST_TIMEOUT_MS: u64 = HANDSHAKE_TIMEOUT_MS + 4000;
/// the first byte, once for the body). Derived from those parts rather than written as a constant,
/// so a change to either one cannot silently invert the relationship again.
const HANDSHAKE_WAIT_MS: u64 = DRM_CONNECT_TIMEOUT_MS + DISPLAY_LIST_TIMEOUT_MS * 2 + 500;
/// Deadline for a message BODY once its header has arrived (cpu frame, cursor pixels). Bodies
/// follow their header immediately on a local socket, so this is generous by orders of magnitude;
/// it exists so a producer that dies mid-message cannot pin the receive thread forever (only the
/// header read re-checks `stop`).
const BODY_READ_TIMEOUT: Duration = Duration::from_secs(5);
struct FrameSlot {
// (width, height, pixel format, packed pixels) of the newest frame not yet consumed by
@@ -752,9 +757,16 @@ async fn recv_thread(
.saturating_mul(4);
// Read the body straight into a recycled frame buffer and publish that same buffer:
// the pixels are copied once, by the kernel, on their way out of the socket.
// Deadlined: only the HEADER read re-checks `stop` (the 200ms poll at the loop top),
// so a producer that dies between a header and its body would otherwise pin this
// thread forever -- Drop sets `stop`, nobody observes it, and every rebuild leaks a
// thread plus its render context. The body follows its header immediately on a local
// socket (a 20MB 2880x1800 frame arrives in single-digit ms), so a whole
// BODY_READ_TIMEOUT of silence is a dead producer, not a slow one.
let mut buf = shared.slot.lock().unwrap().free.take().unwrap_or_default();
match conn.next_raw_into(&mut buf).await {
Ok(()) => {
match tokio::time::timeout(BODY_READ_TIMEOUT, conn.next_raw_into(&mut buf)).await {
Err(_) => break "cpu frame body read timed out".to_owned(),
Ok(Ok(())) => {
if buf.len() < need {
break format!(
"cpu frame: body {} bytes < {need} for {width}x{height}",
@@ -765,7 +777,7 @@ async fn recv_thread(
slot.publish(width as usize, height as usize, Pixfmt::BGRA, buf);
shared.cv.notify_one();
}
Err(err) => break format!("frame body: {err}"),
Ok(Err(err)) => break format!("frame body: {err}"),
}
// Ack this CPU frame too (flow control; see the dma-buf arm above).
if let Err(err) = conn.send_frame_ack().await {
@@ -788,9 +800,11 @@ async fn recv_thread(
.saturating_mul(4);
// A cursor is tiny and changes rarely, so this one keeps its own buffer (the frame
// recycler is for scanout-sized bodies) and hands it straight to the cursor cache.
// Deadlined for the same reason as the cpu-frame body above.
let mut raw = Vec::new();
match conn.next_raw_into(&mut raw).await {
Ok(()) => {
match tokio::time::timeout(BODY_READ_TIMEOUT, conn.next_raw_into(&mut raw)).await {
Err(_) => break "cursor body read timed out".to_owned(),
Ok(Ok(())) => {
if raw.len() < need {
break format!(
"cursor body {} bytes < {need} for {width}x{height}",
@@ -810,7 +824,7 @@ async fn recv_thread(
},
);
}
Err(err) => break format!("cursor body: {err}"),
Ok(Err(err)) => break format!("cursor body: {err}"),
}
}
// Live hotplug: the service pushed a fresh display list after a connector-topology change.
@@ -818,15 +832,23 @@ async fn recv_thread(
// this never trips the wayland::clear() re-probe restart loop). A subsequent
// get_display_infos()/get_primary_index() then reports the fresh geometry.
Data::DrmDisplaysChanged(list) => {
// Did this stream's index just come to mean a different monitor? Compare against what
// Did this stream's slot just come to mean a different monitor? Compare against what
// the service actually bound us to. If it moved, keeping the stream alive would send
// monitor A's pixels under monitor B's advertised geometry, and route injected input
// by B's rect, until something else happened to fail. End it instead: the video
// service rebuilds against the fresh list, which is the same path a resolution change
// already takes. Checked BEFORE the list is swapped in, so the comparison is against
// the topology this stream was started on.
//
// The probe uses wire_idx, not `display`: this pushed list is in the SERVICE'S index
// space (the same fresh-enumeration construction as the handshake list), and wire_idx
// is where our monitor sat in that space when the stream was bound. `display` is a
// position in the list the CLIENT chose from, which is exactly the index space that
// can disagree with the service's whenever a wake or hotplug renumbered entries --
// probing it here would pit slot `display` against slot wire_idx and either tear down
// a healthy stream or miss a genuine renumbering.
let now_at_our_index = list
.get(display.max(0) as usize)
.get(wire_idx)
.map(|d| (d.device.clone(), d.crtc_id));
if bound_to.is_some() && now_at_our_index != bound_to {
swap_available_displays(list);
@@ -1461,6 +1483,41 @@ pub(super) async fn refresh_displays_for_login() {
}
}
/// The advertised DRM display count plus whether any display is demoted to PipeWire, as two
/// scalars. `None` until probed/available. This exists for the cursor path, which polls
/// `display_service::has_non_drm_backed_display` on every tick while the hidden-cursor sentinel is
/// active (the steady state whenever the pointer is off a captured CRTC) and only ever needed these
/// two facts -- `get_display_infos` would clone the whole list and run the wayland geometry
/// augmentation per tick just to read `len()` and `online`.
///
/// Mirrors get_display_infos' demotion semantics exactly: only a MULTI-display host advertises a
/// demoted display (on a single-display host the whole-desktop PipeWire stream IS that display, so
/// it stays online and served).
pub(super) fn display_count_and_any_demoted() -> Option<(usize, bool)> {
// Snapshot the identity keys under DRM_STATE, then consult health with DRM_STATE released --
// same order as get_display_infos, and the same reason: never hold DRM_STATE while taking one
// of the per-display maps.
let (len, keys): (usize, Vec<String>) = match &*DRM_STATE.lock().unwrap() {
ProbeState::Available(_, list) => (
list.len(),
if list.len() > 1 {
list.iter().map(connector_key).collect()
} else {
Vec::new()
},
),
_ => return None,
};
let any_demoted = if len > 1 {
let health = DRM_DISPLAY_HEALTH.lock().unwrap();
keys.iter()
.any(|k| health.get(k).is_some_and(|h| h.demoted()))
} else {
false
};
Some((len, any_demoted))
}
/// The cached DRM displays as protobuf `DisplayInfo`, augmented with the compositor's logical layout
/// (per-monitor position + scale). `None` until probed/available.
pub(super) fn get_display_infos() -> Option<Vec<DisplayInfo>> {

15
src/server/input_service.rs
View File

@@ -426,6 +426,21 @@ fn run_cursor(sp: MouseCursorService, state: &mut StateCursor) -> ResultType<()>
let mut tmp = Message::new();
tmp.set_cursor_data(data);
msg = Arc::new(tmp);
// A DRM cursor id is derived from the shape's pixels plus geometry, so an animated
// pointer mints a new id on every shape change and this map would grow for the life
// of the service, each entry pinning a compressed cursor message. (Upstream's X11
// ids come from a small set of XFixes serials, so the map is effectively bounded
// there -- which is why the ceiling is gated and the stock build stays untouched.)
// Past the ceiling, drop the map and start over: the next request for any evicted
// shape just recompresses it, and the ceiling comfortably covers every static shape
// plus a generous animation window.
#[cfg(all(target_os = "linux", feature = "drm"))]
{
const CURSOR_CACHE_MAX: usize = 64;
if state.cached_cursor_data.len() >= CURSOR_CACHE_MAX {
state.cached_cursor_data.clear();
}
}
state.cached_cursor_data.insert(cache_key, msg.clone());
super::log::trace!("Cursor data updated, hcursor: {}", cache_key);
}