diff --git a/build.py b/build.py index 7779f2a7b..43f203258 100755 --- a/build.py +++ b/build.py @@ -509,6 +509,52 @@ def _assert_so_has_egl(so_path): DRM_PACKAGE_NAME = 'rustdesk-unattended-wayland' +def assert_so_satisfies_the_runtime_abi_gate(so_path): + """The .so we are about to ship must be one the RUNTIME will actually accept. + + `abi_accepted` in libs/scrap/src/common/drmtap_dl.rs is the only place the pinned library's + version is ever validated, and it runs at dlopen time on the USER's machine. Nothing in the + build or in CI compared the two, so the pin and the gate could drift apart and every existing + assertion would still pass: the EGL check does not look at the version, the CI symbol contract + does not call drmtap_version(), and the deb-contents regex matches any `libdrmtap.so.0.X.Y`. + A green pipeline could therefore produce a deb in which DRM capture can never start, and the + only symptom on the host is one log line before it falls back to the portal. + + So parse the gate out of the Rust and apply it here, to the object being staged. This is the + same rule, not a copy of the numbers: if someone bumps the constants, this reads the new ones. + """ + m = re.search(r'libdrmtap\.so\.(\d+)\.(\d+)\.(\d+)', os.path.basename(so_path)) + if not m: + # Not a versioned soname (a local dev build, say). The gate cannot be evaluated, and + # inventing a verdict would be worse than saying so. + print(f'[drm] cannot read a version out of {so_path}; skipping the ABI-gate cross-check') + return + so_ver = tuple(int(g) for g in m.groups()) + gate_src = open('libs/scrap/src/common/drmtap_dl.rs').read() + + def _const(name): + mm = re.search(rf'const {name}: c_int = (\d+);', gate_src) + return int(mm.group(1)) if mm else None + + major, minor = _const('DRMTAP_ABI_MAJOR'), _const('DRMTAP_ABI_MINOR') + mm = re.search(r'const DRMTAP_MIN_MINOR_PATCH: \(c_int, c_int\) = \((\d+), (\d+)\);', gate_src) + floor = (int(mm.group(1)), int(mm.group(2))) if mm else None + if major is None or minor is None or floor is None: + raise Exception( + 'could not parse the libdrmtap ABI gate out of drmtap_dl.rs (DRMTAP_ABI_MAJOR / ' + 'DRMTAP_ABI_MINOR / DRMTAP_MIN_MINOR_PATCH). The gate moved and this check did not; ' + 'fix the check rather than removing it, or the pin and the gate can drift silently.') + accepted = so_ver[0] == major and so_ver[1] == minor and (so_ver[1], so_ver[2]) >= floor + if not accepted: + raise Exception( + f'the libdrmtap being packaged is {so_ver[0]}.{so_ver[1]}.{so_ver[2]}, which the ' + f'runtime loader would REFUSE: drmtap_dl.rs accepts exactly major {major}, minor ' + f'{minor}, patch >= {floor[1]}. Shipping it produces a deb whose DRM capture can never ' + 'start. Move the build pin and the gate together, or fix whichever one is wrong.') + print(f'[drm] libdrmtap {so_ver[0]}.{so_ver[1]}.{so_ver[2]} satisfies the runtime ABI gate ' + f'(major {major}, minor {minor}, patch >= {floor[1]})') + + def stage_libdrmtap_into_deb(so_path): # Put the built libdrmtap object plus its soname symlink into the staged deb. Only the soname # symlink is needed: libdrmtap is resolved by ABSOLUTE path (/usr/lib/rustdesk/libdrmtap.so.0) at @@ -516,6 +562,7 @@ def stage_libdrmtap_into_deb(so_path): # system-wide /etc/ld.so.conf.d search path, which would let this private library shadow a system # library for every binary on the host (Debian Policy 10.2 forbids that). No ld.so.conf.d drop-in # and no ldconfig trigger are shipped, so the stock postinst is used unchanged. + assert_so_satisfies_the_runtime_abi_gate(so_path) so_basename = os.path.basename(so_path) system2('mkdir -p tmpdeb/usr/lib/rustdesk') system2(f'cp {so_path} tmpdeb/usr/lib/rustdesk/') @@ -615,9 +662,14 @@ def build_flutter_deb(version, features): DRMTAP_DLOPEN_MARKER = b'/usr/lib/rustdesk/libdrmtap.so.0' +# Present only when `drm-wake` is compiled in: the runtime option constant is itself +# #[cfg(feature = "drm-wake")] (src/ipc/drm.rs). The dlopen marker above cannot stand in for it - +# `--features drm` alone produces a binary that carries the dlopen path and NO wake code, and that +# is exactly the deb this assertion is here to refuse. +DRMTAP_WAKE_MARKER = b'enable-drm-display-wake' -def _carries_drmtap_marker(path): +def _carries_drmtap_marker(path, marker=DRMTAP_DLOPEN_MARKER): # Chunked, with an overlap of len(marker)-1 so the marker cannot be missed at a chunk boundary: # librustdesk.so is ~45 MB and there is no reason to hold it all in memory, and the `with` # closes deterministically instead of relying on refcounting. @@ -627,9 +679,9 @@ def _carries_drmtap_marker(path): chunk = f.read(1 << 20) if not chunk: return False - if DRMTAP_DLOPEN_MARKER in tail + chunk: + if marker in tail + chunk: return True - tail = chunk[-(len(DRMTAP_DLOPEN_MARKER) - 1):] + tail = chunk[-(len(marker) - 1):] def assert_staged_binary_is_drm(): @@ -650,6 +702,18 @@ def assert_staged_binary_is_drm(): f'{DRMTAP_DLOPEN_MARKER.decode()} dlopen path in {binaries or "any staged binary"}); ' 'refusing to package it as the unattended-wayland variant, which conflicts with and ' 'replaces the stock package but could never capture') + # And the WAKE half. `--drm` enables `drm-wake` too (see get_features), and the deb is named and + # documented as the variant that can reach a machine whose screen has gone dark. The dlopen + # marker above does not distinguish them: `--features drm` alone carries it and has no wake code + # at all. Asserting only the first half is how a deb can be named for a feature it does not have. + if not any(_carries_drmtap_marker(p, DRMTAP_WAKE_MARKER) for p in binaries): + raise Exception( + f'--drm was requested but the staged binary has no {DRMTAP_WAKE_MARKER.decode()} ' + f'marker in {binaries or "any staged binary"}, so it was built without `drm-wake`; ' + 'refusing to package it as the unattended-wayland variant, which is named and ' + 'documented as the build that can wake an idle-disabled display. If this fired under ' + '--skip-cargo, the cargo line that produced the bundle is missing the feature: ' + '--features ...,drm,drm-wake') def build_deb_from_folder(version, binary_folder, want_drm=False): diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index 1ba913e59..5922fbc2f 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -144,8 +144,11 @@ fn display_info_of(display: i32) -> Option { /// The three verdicts live together because they are three answers to one question, "can this display /// be captured over DRM right now", and they feed each other: the rebuild cadence and the zero-frame /// streak both end in the same demotion, and the convert verdict is what keeps a multi-GPU display -/// off the dma-buf path so it never gets there. An entry is dropped entirely the moment the display -/// delivers a frame, which is the single reset for all of it. +/// off the dma-buf path so it never gets there. A delivered frame resets the streak verdicts +/// (`zero_frame_streak`, `demotes`) and NOTHING else: it is evidence that a grab succeeded once, not +/// evidence about the rebuild cadence -- which exists for displays that deliver a first frame and +/// then fail -- nor about which GPU exports the scanout. The entry itself is bounded by connector +/// count, so it costs nothing to keep. #[derive(Clone, Copy)] struct DisplayHealth { /// Consecutive capture sessions that ended without ever producing a frame. A display whose @@ -486,10 +489,30 @@ impl TraitCapturer for IpcDrmCapturer { self.cur_fmt = fmt; if !self.got_frame { // First frame of this session: DRM capture works for this display, clear its - // failure streak. + // failure streak. Clear ONLY the streak, not the whole entry. + // + // Dropping the entry here is what a first frame used to do, and it silently + // disarmed the two verdicts that a first frame says nothing about: + // - `last_build`/`rapid_builds` exist precisely for a display that DELIVERS a + // first frame and then fails downstream every cycle. Wiping the cadence on + // that frame meant the flap guard could never reach RAPID_REBUILD_MAX in the + // one case its own doc comment describes -- a guard that cannot fire. + // - `prefer_cpu` is a property of the HOST (which GPU exports this monitor), + // documented as following the monitor for the process run. It is learned by + // a failed dma-buf session and consumed by the next one, so erasing it on + // the first frame it made possible meant every rebuild re-paid that failed + // session. Worse, the set happens on the recv thread and this delete on the + // encoder thread, so a convert failure racing a queued frame could destroy + // the bit inside the very session that learned it. + // Only `drm_clear_prefer_cpu` (on a topology change, where the mapping really + // can have changed) may clear the convert verdict. self.got_frame = true; if let Some(key) = &self.connector { - DRM_DISPLAY_HEALTH.lock().unwrap().remove(key); + if let Some(h) = DRM_DISPLAY_HEALTH.lock().unwrap().get_mut(key) { + h.zero_frame_streak = 0; + h.demotes = 0; + h.since = Instant::now(); + } } } } else { @@ -1596,23 +1619,31 @@ pub(super) fn get_display_infos() -> Option> { } /// Index (into the cached DRM display list) of the compositor's PRIMARY output. DRM connector order -/// is not the compositor's primary, so match the compositor's primary (from the same Wayland source -/// the geometry augmentation uses) to the DRM list by normalized connector name; fall back to 0 when -/// unknown. Without this the first DRM connector is always streamed, which is the wrong initial -/// display whenever the primary is not connector 0. +/// is not the compositor's primary, so find which DRM entry was assigned the compositor's primary +/// output; fall back to 0 when unknown. Without this the first DRM connector is always streamed, +/// which is the wrong initial display whenever the primary is not connector 0. +/// +/// It asks `assign_wayland_outputs` rather than re-matching by name, and that is the point: whatever +/// that function decides IS the geometry advertised to the client for each index, so deriving the +/// primary from the same assignment makes the advertised primary and the advertised geometry agree +/// BY CONSTRUCTION. Matching by name here separately was the second, weaker copy of the same +/// question -- it had neither the unique-resolution step nor the layout-order fallback, so on any +/// compositor whose output names do not normalize to the DRM connector names (or reports none at +/// all, which the Wayland display code has its own "nameless compositor" path for) it silently +/// answered 0 while the geometry augmentation had matched that display to a different output. pub(super) fn get_primary_index() -> usize { let list = match &*DRM_STATE.lock().unwrap() { ProbeState::Available(_, list) => list.clone(), _ => return 0, }; let wl = scrap::wayland::display::get_displays(); - if let Some(pw) = wl.displays.get(wl.primary) { - let pn = normalize_connector(&pw.name); - if let Some(idx) = list.iter().position(|d| normalize_connector(&d.name) == pn) { - return idx; - } + if wl.displays.is_empty() { + return 0; } - 0 + assign_wayland_outputs(&list, &wl.displays) + .iter() + .position(|assigned| *assigned == Some(wl.primary)) + .unwrap_or(0) } /// The DRM enumeration reports every monitor at physical size and origin (0,0) — it deliberately