From 0d49ead0c37756095572eec74bc7ee7988fc58ea Mon Sep 17 00:00:00 2001 From: Mariano Abad Date: Mon, 28 Sep 2026 04:54:55 -0300 Subject: [PATCH] fix(drm): turn a captured frame only when the plane did not rotate it (#16345) * fix(drm): turn a captured frame only when the plane did not rotate it A compositor rotates an output either in hardware, setting the primary plane's `rotation` property, or in software, drawing the scanout already turned. `wl_output` cannot tell the two apart, and `frame_transform` guessed: 90/270 turned, 180 left alone, which is right for i915 + mutter (rotate-180 in hardware, scanout upright) and wrong wherever the compositor drew the scanout turned, where 180 comes out upside down. Measured before changing it. virtio-gpu (no rotation property, mutter 46): the scanout dump at 180 is the desktop upside down, at 90 and 270 the logical desktop turned inside the native mode framebuffer. i915 + mutter (GNOME 50) at 180: the plane reports rotate-180 and the dump is upright, identical to the unrotated one. amdgpu + KWin 6 at 180: the plane stays at rotate-0 and the dump is upside down. The producer reads `drmtap_plane_rotation()` (libdrmtap 0.5.8, optional symbol) right after each grab and stamps it on the frame, in the dma-buf descriptor and in the CPU frame header, both `serde(default)` so an older producer still parses. A plane that rotated scans out the logical desktop whatever the output transform says: the compositor programs it from the CRTC transform, which also folds in the connector's panel orientation that `wl_output` never carries, so the consumer leaves such a frame alone. A plane at rotate-0 scanned out what the compositor drew, and the frame is turned by the whole output transform. `None` (a library before 0.5.8, or nothing it could read) keeps the previous rule, so nothing changes until the library says otherwise. The session is still sized from the output transform: a plane that rotated 90 in hardware hands over the portrait scanout those dims already name. * build: pin libdrmtap 0.5.8 (rustdesk-org/libdrmtap 95d4d7454) The rotation fix reads drmtap_plane_rotation(), added in libdrmtap 0.5.8. The mirror carries that commit since 2026-09-25, so the pin moves there. * drm: pin the decoding of a frame from a producer older than the plane rotation The root service and the --server are upgraded separately, so a frame header or a dma-buf descriptor without `plane_rotation` must still decode, as None, which keeps the previous rule. The tests built both messages with the field set to None; this one decodes payloads that omit it, and one that carries it (greptile on #16345). Serde already reads a missing Option as None, so what the test pins is the wire name and the None on absence, for both messages. Mutation: renaming the field on the wire, in either message, turns it red. * drm: take the plane rotation right after the grab, not after the copies `drmtap_plane_rotation()` reads the rotation property of the plane the last grab came from at the moment it is called. The cpu path asked only after `DrmReader::grab()` had copied the frame into its buffer and the producer had copied it again into the message, so on a large frame the compositor had time to change the plane and the frame went out with the rotation of the next one; `frame_transform` turns that frame by it with nothing to absorb the skew (zhou, review of 26-sep). `DrmReader` now reads the rotation right after a successful `drmtap_grab_mapped` or `drmtap_grab_desc`, before any copy or release, and `plane_rotation()` returns that snapshot, so both paths stamp a frame with the rotation its own grab saw. Test: a_frame_keeps_the_rotation_its_plane_had_when_it_was_grabbed, in `plane_rotation_tests` (which CI already runs), drives the reader through a fake library whose frame release turns the plane: the rotation read at the grab survives on the cpu grab and on the dma-buf export. Mutations: no snapshot on the cpu grab, none on the export, and the cpu snapshot taken after the release, each turn it red. --- .github/workflows/flutter-build.yml | 2 + build.py | 4 +- libs/scrap/Cargo.toml | 2 +- libs/scrap/src/common/drm_reader.rs | 117 ++++++++++++ libs/scrap/src/common/drmtap_dl.rs | 72 ++++++++ src/ipc.rs | 43 +++++ src/ipc/drm.rs | 42 ++++- src/server/drm_capturer.rs | 275 +++++++++++++++++++++++----- 8 files changed, 499 insertions(+), 58 deletions(-) diff --git a/.github/workflows/flutter-build.yml b/.github/workflows/flutter-build.yml index 57b898321..85291d637 100644 --- a/.github/workflows/flutter-build.yml +++ b/.github/workflows/flutter-build.yml @@ -2090,6 +2090,8 @@ jobs: 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 + run_drm_tests "scrap plane_rotation_tests" \ + cargo test --locked -p scrap --features drm plane_rotation_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 a32e5fccc..7df00f341 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.6. +# libs/scrap/Cargo.toml). This commit is libdrmtap v0.5.8. LIBDRMTAP_REPO_PINNED = 'https://github.com/rustdesk-org/libdrmtap' -LIBDRMTAP_SHA_PINNED = '49b204f275af1a2d6dfead94effb4036c7d50a3a' +LIBDRMTAP_SHA_PINNED = '95d4d74549631aa5c39461300acfd2e106583cc9' 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 diff --git a/libs/scrap/Cargo.toml b/libs/scrap/Cargo.toml index 135391312..c29647ad1 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.6). We deliberately do +# `DRMTAP_SHA` in build.py, which fetches that exact commit (libdrmtap v0.5.8). 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 296059905..845c03d7e 100644 --- a/libs/scrap/src/common/drm_reader.rs +++ b/libs/scrap/src/common/drm_reader.rs @@ -377,6 +377,9 @@ pub struct DrmReader { /// 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, + /// The plane rotation read right after the last successful grab, before any copy: libdrmtap + /// reads the property when asked, so a read after the copies could describe the next frame. + last_plane_rotation: Option, } impl DrmReader { @@ -414,6 +417,7 @@ impl DrmReader { lib, ctx, buf: Vec::new(), + last_plane_rotation: None, }) } @@ -437,6 +441,7 @@ impl DrmReader { format!("drmtap_grab_mapped failed: errno {errno}"), )); } + self.last_plane_rotation = self.read_plane_rotation(); if frame.data.is_null() || frame.width == 0 || frame.height == 0 { (self.lib.frame_release)(self.ctx, &mut frame); return Err(io::ErrorKind::WouldBlock.into()); @@ -531,6 +536,23 @@ impl DrmReader { .map(|s| s.to_owned()) } + /// The DRM `rotation` bitmask of the plane the last successful grab read from, as it was right + /// after that grab. A plane without the property answers rotate-0: it cannot have turned + /// anything, so the compositor drew the scanout already turned and the whole output transform + /// is still to be undone. `None` only when the library cannot say: it predates the call + /// (0.5.8), no plane is bound, or the property set could not be read. + pub fn plane_rotation(&self) -> Option { + self.last_plane_rotation + } + + fn read_plane_rotation(&mut self) -> Option { + let f = self.lib.plane_rotation?; + let mut rotation: u32 = 0; + // SAFETY: self.ctx is a live context and `rotation` outlives the call. + let rc = unsafe { f(self.ctx, &mut rotation) }; + plane_rotation_answer(rc, rotation) + } + /// Zero-copy EXPORT grab: fills a `drmtap_dmabuf_desc` (dma-buf fd, plane layout, HDR metadata) WITHOUT mapping, detiling or copying pixels, so on this /// path the root process never loads libEGL/libGLESv2. The exported fd is READ-ONLY (libdrmtap drops `DRM_RDWR` and `dup` shares that open file /// description), so the `--server` that receives it can map the scanout but never write the live framebuffer. Validation here is METADATA ONLY. @@ -562,6 +584,7 @@ impl DrmReader { format!("drmtap_grab_desc failed: errno {errno}"), )); } + self.last_plane_rotation = self.read_plane_rotation(); // `desc.dma_buf_fd` is the canonical fd (what split_capture.c sends); `frame` owns it too and `frame_release` closes the library's copy. let raw_fd = if desc.dma_buf_fd >= 0 { desc.dma_buf_fd @@ -985,3 +1008,97 @@ mod hotspot_guess_tests { assert_eq!(infer_hotspot(&px, w, h), (0, 0)); } } + +/// DRM_MODE_ROTATE_0: the `rotation` bitmask of a plane that turned nothing. +const DRM_MODE_ROTATE_0: u32 = 1 << 0; + +/// What a `drmtap_plane_rotation` return means to the consumer. Success is the mask. `-ENOTSUP` +/// (the plane has no `rotation` property) is rotate-0, not "unknown": such a plane cannot have +/// turned the scanout, so the frame arrives turned by the whole output transform, which is the +/// virtio-gpu and vmwgfx case this exists for. Anything else (`-ENOENT` with no plane bound, a +/// failed property read) is `None`, and the consumer keeps its pre-0.5.8 rule. +fn plane_rotation_answer(rc: c_int, rotation: u32) -> Option { + if rc == 0 { + Some(rotation) + } else if rc == -hbb_common::libc::ENOTSUP { + Some(DRM_MODE_ROTATE_0) + } else { + None + } +} + +#[cfg(test)] +mod plane_rotation_tests { + use super::*; + use std::os::fd::AsRawFd; + use std::sync::atomic::{AtomicI32, AtomicU32, Ordering}; + + #[test] + fn a_plane_without_the_property_turned_nothing() { + assert_eq!(plane_rotation_answer(0, 0x4), Some(0x4)); + assert_eq!(plane_rotation_answer(0, 0x1), Some(0x1)); + assert_eq!(plane_rotation_answer(-hbb_common::libc::ENOTSUP, 0), Some(0x1)); + assert_eq!(plane_rotation_answer(-hbb_common::libc::ENOENT, 0), None); + assert_eq!(plane_rotation_answer(-hbb_common::libc::EINVAL, 0), None); + assert_eq!(plane_rotation_answer(-hbb_common::libc::EIO, 7), None); + } + + // What the plane answers whenever it is asked. The fake release turns it, the way a compositor + // may once the frame is out, so a read taken after the release or outside the grab sees 0x1. + static PLANE: AtomicU32 = AtomicU32::new(0x1); + static FD: AtomicI32 = AtomicI32::new(-1); + static PIXELS: [u8; 16] = [0; 16]; + + unsafe extern "C" fn plane_rotation(_: *mut drmtap_ctx, rotation: *mut u32) -> c_int { + *rotation = PLANE.load(Ordering::SeqCst); + 0 + } + unsafe extern "C" fn frame_release(_: *mut drmtap_ctx, _: *mut drmtap_frame_info) { + PLANE.store(0x1, Ordering::SeqCst); + } + unsafe extern "C" fn grab_mapped(_: *mut drmtap_ctx, f: *mut drmtap_frame_info) -> c_int { + (*f).data = PIXELS.as_ptr() as *mut _; + (*f).width = 2; + (*f).height = 2; + (*f).stride = 8; + (*f).format = DRM_FORMAT_XRGB8888; + 0 + } + unsafe extern "C" fn grab_desc( + _: *mut drmtap_ctx, + d: *mut drmtap_dmabuf_desc, + _: *mut drmtap_frame_info, + ) -> c_int { + (*d).dma_buf_fd = FD.load(Ordering::SeqCst); + (*d).width = 2; + (*d).height = 2; + (*d).num_planes = 1; + (*d).pitches[0] = 8; + 0 + } + + #[test] + fn a_frame_keeps_the_rotation_its_plane_had_when_it_was_grabbed() { + let lib: &'static DrmtapLib = Box::leak(Box::new(DrmtapLib::fake( + grab_mapped, + frame_release, + grab_desc, + plane_rotation, + ))); + let mut r = DrmReader { + lib, + ctx: std::ptr::null_mut(), + buf: Vec::new(), + last_provenance: -1, + last_plane_rotation: None, + }; + PLANE.store(0x4, Ordering::SeqCst); + r.grab().expect("the fake grab succeeds"); + assert_eq!(r.plane_rotation(), Some(0x4), "cpu grab"); + let (fd, _peer) = std::os::unix::net::UnixStream::pair().unwrap(); + FD.store(fd.as_raw_fd(), Ordering::SeqCst); + PLANE.store(0x8, Ordering::SeqCst); + r.grab_desc().expect("the fake export succeeds"); + assert_eq!(r.plane_rotation(), Some(0x8), "dma-buf export"); + } +} diff --git a/libs/scrap/src/common/drmtap_dl.rs b/libs/scrap/src/common/drmtap_dl.rs index ddaaa6071..6bb74eee6 100644 --- a/libs/scrap/src/common/drmtap_dl.rs +++ b/libs/scrap/src/common/drmtap_dl.rs @@ -140,6 +140,12 @@ type FnGrabDesc = type FnOpenRender = unsafe extern "C" fn(*const c_char) -> *mut drmtap_ctx; // libdrmtap >= 0.4.15; returns a ctx-owned string, or NULL if it has none. type FnRenderNode = unsafe extern "C" fn(*mut drmtap_ctx) -> *const c_char; +/// `drmtap_plane_rotation`, added in libdrmtap 0.5.8: the DRM `rotation` bitmask the primary +/// plane scans out with, read now (0x1 = 0, 0x2 = 90, 0x4 = 180, 0x8 = 270, plus 0x10/0x20 for +/// a reflection). 0 with `*rotation` set; `-ENOTSUP` when the plane has no such property, which +/// means the compositor can only have rotated in software; `-ENOENT` with no plane bound; +/// `-EINVAL` on a null argument. +type FnPlaneRotation = unsafe extern "C" fn(*mut drmtap_ctx, *mut u32) -> c_int; type FnConvertDmabuf = unsafe extern "C" fn(*mut drmtap_ctx, *const drmtap_dmabuf_desc, *mut drmtap_frame_info) -> c_int; @@ -162,9 +168,64 @@ pub struct DrmtapLib { pub open_render: FnOpenRender, pub convert_dmabuf: FnConvertDmabuf, pub render_node: Option, + /// Optional: it only exists from libdrmtap 0.5.8. Absent means the library cannot say whether + /// the plane rotated the scanout, and the consumer keeps the pre-0.5.8 rule for that case. + pub plane_rotation: Option, pub version: (c_int, c_int, c_int), } +/// A library whose capture entry points are the given fakes and whose others do nothing, for the +/// tests of the reader. +#[cfg(test)] +impl DrmtapLib { + pub(crate) fn fake( + grab_mapped: FnGrabMapped, + frame_release: FnFrameRelease, + grab_desc: FnGrabDesc, + plane_rotation: FnPlaneRotation, + ) -> Self { + unsafe extern "C" fn open(_: *const drmtap_config) -> *mut drmtap_ctx { + std::ptr::null_mut() + } + unsafe extern "C" fn close(_: *mut drmtap_ctx) {} + unsafe extern "C" fn list_displays(_: *mut drmtap_ctx, _: *mut drmtap_display, _: c_int) -> c_int { + 0 + } + unsafe extern "C" fn get_cursor(_: *mut drmtap_ctx, _: *mut drmtap_cursor_info) -> c_int { + -1 + } + unsafe extern "C" fn cursor_release(_: *mut drmtap_ctx, _: *mut drmtap_cursor_info) {} + unsafe extern "C" fn open_render(_: *const c_char) -> *mut drmtap_ctx { + std::ptr::null_mut() + } + unsafe extern "C" fn convert_dmabuf( + _: *mut drmtap_ctx, + _: *const drmtap_dmabuf_desc, + _: *mut drmtap_frame_info, + ) -> c_int { + -1 + } + DrmtapLib { + _lib: hbb_common::libloading::os::unix::Library::this().into(), + open, + close, + list_displays, + list_devices: None, + grab_mapped, + frame_release, + get_cursor, + cursor_release, + cursor_hotspot_valid: None, + grab_desc, + open_render, + convert_dmabuf, + render_node: None, + plane_rotation: Some(plane_rotation), + version: (0, 5, 8), + } + } +} + // SAFETY: the resolved fn pointers are plain C entry points with no interior mutability; // libdrmtap contexts are used single-threaded by the caller. The Library handle is never moved out. unsafe impl Send for DrmtapLib {} @@ -290,6 +351,8 @@ impl DrmtapLib { 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); + let plane_rotation: Option = + lib.get(b"drmtap_plane_rotation").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() @@ -333,6 +396,14 @@ impl DrmtapLib { a driver that really puts the hotspot at (0, 0)." ); } + if (minor, patch) >= (5, 8) && plane_rotation.is_none() { + log::warn!( + "libdrmtap at {loaded_from} reports v{major}.{minor}.{patch} but is missing \ + drmtap_plane_rotation: it is a stale or pre-release build. Check what the \ + soname symlink points at. A 180-degree output rotated by the compositor in \ + software will be captured upside down." + ); + } Some(DrmtapLib { _lib: lib, open, @@ -348,6 +419,7 @@ impl DrmtapLib { open_render, convert_dmabuf, render_node, + plane_rotation, version: (major, minor, patch), }) } diff --git a/src/ipc.rs b/src/ipc.rs index 0887485a5..fdb1e1478 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -543,6 +543,9 @@ pub enum Data { DrmFrame { width: u32, height: u32, + /// See `DmabufDesc::plane_rotation`; absent from a producer older than this field. + #[serde(default)] + plane_rotation: Option, /// See `DmabufDesc::cursor_pos`: the cursor plane position read right after this frame, /// or `None` when the cursor is hidden, the read failed or the producer predates the field. #[serde(default)] @@ -2377,6 +2380,45 @@ mod test { } } + /// A frame from a producer that predates the plane rotation reads as `None`, in the cpu frame + /// header and in the dma-buf descriptor: the root service and the `--server` are upgraded + /// separately, and `None` keeps the previous rule. + #[cfg(all(target_os = "linux", feature = "drm"))] + #[test] + fn a_frame_without_a_plane_rotation_reads_as_none() { + let legacy = r#"{"t":"DrmFrame","c":{"width":8,"height":8}}"#; + match serde_json::from_str::(legacy).expect("legacy DrmFrame must deserialize") { + Data::DrmFrame { plane_rotation, .. } => assert_eq!(plane_rotation, None), + other => panic!("expected DrmFrame, got {other:?}"), + } + let modern = r#"{"t":"DrmFrame","c":{"width":8,"height":8,"plane_rotation":4}}"#; + match serde_json::from_str::(modern).expect("modern DrmFrame must deserialize") { + Data::DrmFrame { plane_rotation, .. } => assert_eq!(plane_rotation, Some(4)), + other => panic!("expected DrmFrame, got {other:?}"), + } + let desc = DmabufDesc { + buffer_id: 0, + width: 4, + height: 4, + format: 0, + modifier: 0, + fb_id: 0, + num_planes: 1, + offsets: [0; 4], + pitches: [16, 0, 0, 0], + hdr_eotf: 0, + hdr_max_nits: 0, + has_fd: false, + plane_rotation: Some(4), + cursor_pos: None, + }; + let mut value = serde_json::to_value(&desc).unwrap(); + assert!(value.as_object_mut().unwrap().remove("plane_rotation").is_some()); + let legacy: DmabufDesc = + serde_json::from_value(value).expect("legacy DmabufDesc must deserialize"); + assert_eq!(legacy.plane_rotation, None); + } + /// A frame header from a producer that predates the cursor plane position reads as `None`; /// a producer that sends one is read back exactly. A pin, not a gate: the consumer only ever /// measures against `Some`. @@ -2389,6 +2431,7 @@ mod test { width, height, cursor_pos, + .. } => { assert_eq!((width, height), (8, 8)); assert_eq!(cursor_pos, None); diff --git a/src/ipc/drm.rs b/src/ipc/drm.rs index a6963aed6..b2a81e18e 100644 --- a/src/ipc/drm.rs +++ b/src/ipc/drm.rs @@ -45,6 +45,14 @@ pub struct DmabufDesc { pub hdr_max_nits: u32, /// True: the fd rides this message's SCM_RIGHTS cmsg. False: import-once cache hit for `fb_id`. pub has_fd: bool, + /// The DRM `rotation` bitmask the primary plane scanned this frame out with, from + /// `drmtap_plane_rotation()` (libdrmtap 0.5.8); a plane without the property is reported as + /// rotate-0. `None` when the library cannot say (older than 0.5.8, nothing bound, or the + /// property set unreadable). A frame from a plane that rotated in hardware is already + /// upright; one from a plane that did not is turned by the output transform. See + /// `frame_transform`. + #[serde(default)] + pub plane_rotation: Option, /// Cursor plane position read right after this frame was grabbed, in scanout pixels of the /// display this stream shows, so a few ms newer than the frame. `None` means the cursor is /// hidden, the read failed or was rejected, or the producer predates this field. It rides @@ -107,6 +115,8 @@ enum DrmProducerMsg { width: u32, height: u32, data: Bytes, + /// See `DmabufDesc::plane_rotation`. + plane_rotation: Option, /// See `DmabufDesc::cursor_pos`. The dmabuf path carries it inside the descriptor; this /// one has no descriptor, so it travels beside the pixels. cursor_pos: Option<(i32, i32)>, @@ -151,6 +161,7 @@ mod cursor_pos_tests { hdr_eotf: 0, hdr_max_nits: 0, has_fd: false, + plane_rotation: None, cursor_pos: None, } } @@ -184,6 +195,7 @@ mod cursor_pos_tests { width: 4, height: 4, data: Bytes::new(), + plane_rotation: None, cursor_pos: None, }; stamp_cursor_pos(&mut cpu, Some(&visible)); @@ -1069,12 +1081,14 @@ async fn handle_drm_conn(stream: Connection) -> ResultType<()> { width, height, data, + plane_rotation, cursor_pos, }) => { conn.send_msg( &Data::DrmFrame { width, height, + plane_rotation, cursor_pos, }, None, @@ -1154,6 +1168,7 @@ fn drm_capture_worker( // state itself (CREDIT_STALL) since our watchdog cannot advance. None } else if use_dmabuf { + // Read right after the grab: the library answers for the plane that grab read from. Some(match reader.grab_desc() { Ok((fd, d)) => Ok(DrmProducerMsg::Frame { desc: DmabufDesc { @@ -1169,6 +1184,7 @@ fn drm_capture_worker( hdr_eotf: d.hdr_eotf, hdr_max_nits: d.hdr_max_nits, has_fd: true, // every exported frame carries its fd; see the send below + plane_rotation: reader.plane_rotation(), cursor_pos: None, // stamped below, from the one cursor read of this tick }, fd: Some(fd), @@ -1176,15 +1192,18 @@ fn drm_capture_worker( Err(err) => Err(err), }) } else { - Some(match reader.grab() { - Ok((buf, w, h)) => Ok(DrmProducerMsg::FrameCpu { - width: w as u32, - height: h as u32, - data: Bytes::copy_from_slice(buf), - cursor_pos: None, - }), - Err(err) => Err(err), - }) + // The mapped buffer borrows the reader: copy it out, then take the rotation the grab + // recorded. + let copied = reader + .grab() + .map(|(buf, w, h)| (Bytes::copy_from_slice(buf), w as u32, h as u32)); + Some(copied.map(|(data, width, height)| DrmProducerMsg::FrameCpu { + width, + height, + data, + plane_rotation: reader.plane_rotation(), + cursor_pos: None, + })) }; // ONE cursor read per tick, and none on a stalled or failed grab: the plane position rides // the frame it was read next to, and the shape ships when it changes. The position is a @@ -1673,6 +1692,7 @@ mod drm_conn_tests { &Data::DrmFrame { width: 1920, height: 1080, + plane_rotation: None, cursor_pos: None, }, None, @@ -1685,6 +1705,7 @@ mod drm_conn_tests { Data::DrmFrame { width: 1920, height: 1080, + plane_rotation: None, cursor_pos: None } )); @@ -1701,6 +1722,7 @@ mod drm_conn_tests { &Data::DrmFrame { width: 4, height: 4, + plane_rotation: None, cursor_pos: None, }, Some(rd.as_fd()), @@ -1818,6 +1840,7 @@ mod drm_conn_tests { let payload = serde_json::to_vec(&Data::DrmFrame { width: 8, height: 8, + plane_rotation: None, cursor_pos: None, }) .unwrap(); @@ -1831,6 +1854,7 @@ mod drm_conn_tests { Data::DrmFrame { width: 8, height: 8, + plane_rotation: None, cursor_pos: None } )); diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index e489193bb..73fa3e6d9 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -26,8 +26,9 @@ const HANDSHAKE_WAIT_MS: u64 = DRM_CONNECT_TIMEOUT_MS + DISPLAY_LIST_TIMEOUT_MS const BODY_READ_TIMEOUT: Duration = Duration::from_secs(5); struct FrameSlot { - // Row stride is `pixels.len() / height`, possibly padded; the format is per frame. - latest: Option<(usize, usize, Pixfmt, Vec)>, + // Row stride is `pixels.len() / height`, possibly padded; the format and the plane rotation + // the producer read for this frame (`DmabufDesc::plane_rotation`) are per frame. + latest: Option<(usize, usize, Pixfmt, Option, Vec)>, // TWO slots: two buffers can be idle at once -- the receive path takes one and publishes in two // SEPARATE acquisitions, so the encoder can hand its borrow back in between. free: [Option>; 2], @@ -35,11 +36,18 @@ struct FrameSlot { } impl FrameSlot { - fn publish(&mut self, w: usize, h: usize, fmt: Pixfmt, buf: Vec) { + fn publish( + &mut self, + w: usize, + h: usize, + fmt: Pixfmt, + plane_rotation: Option, + buf: Vec, + ) { if let Some((.., old)) = self.latest.take() { self.recycle(old); } - self.latest = Some((w, h, fmt, buf)); + self.latest = Some((w, h, fmt, plane_rotation, buf)); } fn recycle(&mut self, buf: Vec) { @@ -65,9 +73,9 @@ struct Shared { // 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. + // sprite upside down). The frame's own angle is not shared: `frame()` computes it per frame + // from the output transform and the plane rotation stamped on the frame. The receive thread + // defers any cursor that races the store. cursor_transform: std::sync::atomic::AtomicI32, // The cursor calibration geometry, from the same snapshot as the transform and stored before // it: a receive thread that has seen the transform sees this too. `None` means this stream @@ -83,8 +91,9 @@ pub struct IpcDrmCapturer { // What the encoder was sized from: CapturerInfo{width,height} is read once, at build time. // With a rotated output these are the ROTATED dimensions, matching the frames delivered. session_size: Option<(usize, usize)>, - // Output rotation in degrees: a rotated scanout holds the desktop drawn sideways, so frames - // are turned back before delivery. Fixed per session; a rotation rebuilds the capturer. + // Output transform in degrees, from the wayland snapshot; each frame is turned back by it + // unless the plane rotated the scanout (`frame_transform`). Fixed per session; a rotation + // rebuilds the capturer. transform: i32, // The wayland snapshot generation this session was built from: a later invalidation means // the layout (a rotation included) may have changed, and frame() asks for a rebuild. @@ -193,15 +202,34 @@ 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 +/// The DRM `rotation` value of a plane that did not rotate or reflect anything. +const PLANE_ROTATE_0: u32 = 0x1; + +/// The angle the FRAME has to be turned back by. +/// +/// A compositor rotates an output either in hardware, setting the primary plane's `rotation` +/// (i915 + mutter at 180, measured: the scanout is upright and the plane reports rotate-180), or +/// in software, drawing the scanout already turned (virtio-gpu and vmwgfx have no such property, +/// KWin leaves the amdgpu plane at rotate-0; measured: 180 comes out upside down and 90/270 +/// sideways inside the native mode). `wl_output` cannot tell the two apart; `plane_rotation` can. +/// A plane that rotated scans out the logical desktop whatever the output transform says: the +/// compositor programs it from the CRTC transform, which also folds in the connector's panel +/// orientation that `wl_output` never carries, so the two are not subtracted. A plane at rotate-0 +/// (a plane without the property arrives as rotate-0, see `DrmReader::plane_rotation`) scanned +/// out what the compositor drew, and the frame is turned by the whole transform. `None` (a +/// libdrmtap before 0.5.8, or nothing the library could read) keeps the pre-0.5.8 rule: 90/270 +/// turned, 180 left alone. The cursor does not go through this: see `Shared::cursor_transform`. +fn frame_transform(wl_transform: i32, plane_rotation: Option) -> i32 { + match plane_rotation { + Some(mask) if mask != PLANE_ROTATE_0 => 0, + Some(_) => wl_transform, + None => { + if wl_transform == 90 || wl_transform == 270 { + wl_transform + } else { + 0 + } + } } } @@ -369,7 +397,6 @@ impl IpcDrmCapturer { let snapshot_gen = scrap::wayland::display::wayland_snapshot_generation(); let wl = scrap::wayland::display::get_displays(); let (wl_transform, origin) = transform_and_origin(&displays, wire_idx, &wl); - let transform = frame_transform(wl_transform); // Same snapshot as the transform, stored BEFORE the transform's Release store below. let cal = calibration_context(&displays, wire_idx, &wl, snapshot_gen); log::info!( @@ -391,8 +418,8 @@ impl IpcDrmCapturer { connector: displays.get(wire_idx).map(connector_key), session_size: displays .get(wire_idx) - .map(|d| rotated_dims(transform, d.width as usize, d.height as usize)), - transform, + .map(|d| rotated_dims(wl_transform, d.width as usize, d.height as usize)), + transform: wl_transform, snapshot_gen, cur: Vec::new(), cur_w: 0, @@ -458,7 +485,7 @@ impl TraitCapturer for IpcDrmCapturer { self.shared.cv.wait_timeout(slot, deadline - now).unwrap(); slot = guard; } - if let Some((w, h, fmt, buf)) = slot.latest.take() { + if let Some((w, h, fmt, plane_rotation, buf)) = slot.latest.take() { drop(slot); // A layout change bumps the generation and is otherwise invisible here (mode // and framebuffer keep their size). Rebuild for the new transform; not counted @@ -470,10 +497,16 @@ impl TraitCapturer for IpcDrmCapturer { format!("drm: display {} layout changed; rebuilding", self.display), )); } - // Frames arrive in scanout orientation, the session was sized rotated, so the - // guard compares rotated dims. convert_to_yuv only refuses a LARGER source (a - // smaller one leaves stale edges); first frame: CRTC mode vs scanout fb. - let (fw, fh) = rotated_dims(self.transform, w, h); + // The angle this frame still has to be turned by: the output transform, unless + // the plane rotated the scanout itself (per frame, since only the producer can + // see the plane). Frames arrive in scanout orientation, the session was + // sized rotated, so the guard compares rotated dims: a plane that rotated 90 in + // hardware hands over a portrait scanout and `t` is 0, a plane that did not hands + // over the landscape mode and `t` is 90; both land on the session size. + // convert_to_yuv only refuses a LARGER source (a smaller one leaves stale edges); + // first frame: CRTC mode vs scanout fb. + let t = frame_transform(self.transform, plane_rotation); + let (fw, fh) = rotated_dims(t, w, h); if self.session_size.is_some_and(|(sw, sh)| (fw, fh) != (sw, sh)) { self.shared.slot.lock().unwrap().recycle(buf); if !self.got_frame { @@ -493,7 +526,7 @@ impl TraitCapturer for IpcDrmCapturer { ), )); } - if self.transform == 0 { + if t == 0 { let previous = std::mem::replace(&mut self.cur, buf); self.shared.slot.lock().unwrap().recycle(previous); } else if !matches!(fmt, Pixfmt::BGRA | Pixfmt::RGBA) { @@ -512,7 +545,7 @@ impl TraitCapturer for IpcDrmCapturer { ), )); } else { - unrotate_bgra(&buf, w, h, self.transform, &mut self.cur); + unrotate_bgra(&buf, w, h, t, &mut self.cur); self.shared.slot.lock().unwrap().recycle(buf); } self.cur_w = fw; @@ -737,7 +770,7 @@ async fn recv_thread( buf.clear(); buf.extend_from_slice(data); let mut slot = shared.slot.lock().unwrap(); - slot.publish(w as usize, h as usize, fmt, buf); + slot.publish(w as usize, h as usize, fmt, desc.plane_rotation, buf); shared.cv.notify_one(); } Err(err) if err.kind() == io::ErrorKind::WouldBlock => {} @@ -756,6 +789,7 @@ async fn recv_thread( Data::DrmFrame { width, height, + plane_rotation, cursor_pos, } => { if cal.is_some() { @@ -790,7 +824,13 @@ async fn recv_thread( ); } let mut slot = shared.slot.lock().unwrap(); - slot.publish(width as usize, height as usize, Pixfmt::BGRA, buf); + slot.publish( + width as usize, + height as usize, + Pixfmt::BGRA, + plane_rotation, + buf, + ); shared.cv.notify_one(); } Ok(Err(err)) => break format!("frame body: {err}"), @@ -1379,8 +1419,7 @@ fn deliver_drm_cursor( // 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. + // say which of the two happened, 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); @@ -2747,19 +2786,17 @@ mod drm_capturer_tests { 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. + /// The pre-0.5.8 rule, kept for `None` (a library that cannot say): it pins what was MEASURED + /// on i915 advertising rotate-180 with mutter, the primary plane scanning out upright while + /// the compositor pre-rotates the cursor sprite, so the frame is left alone and the sprite is + /// turned. With a plane answer the rule below decides the frame; the sprite rule stays. #[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); + assert_eq!(frame_transform(180, None), 0); + assert_eq!(frame_transform(90, None), 90); + assert_eq!(frame_transform(270, None), 270); + // The plane says so: rotate-180 in hardware, the scanout is already upright. + assert_eq!(frame_transform(180, Some(0x4)), 0); let (up, w, h) = arrow(); let (scan, sw, sh) = as_scanned_out(&up, w, h, 180); let mut turned = Vec::new(); @@ -2791,6 +2828,129 @@ mod drm_capturer_tests { } } + /// Measured on virtio-gpu (no `rotation` property, mutter draws the scanout turned): the + /// dump at 180 is upside down and the dumps at 90/270 are the logical desktop turned inside + /// the native mode. A plane that reports rotate-0 did nothing, so the frame is turned by the + /// whole output transform, 180 included; a plane that reports a rotation did all of it. + #[test] + fn the_frame_is_turned_unless_the_plane_rotated_it() { + assert_eq!(frame_transform(180, Some(0x1)), 180); + assert_eq!(frame_transform(90, Some(0x1)), 90); + assert_eq!(frame_transform(270, Some(0x1)), 270); + assert_eq!(frame_transform(0, Some(0x1)), 0); + assert_eq!(frame_transform(90, Some(0x2)), 0); + assert_eq!(frame_transform(180, Some(0x4)), 0); + assert_eq!(frame_transform(270, Some(0x8)), 0); + // A reflection is the plane's work too: a reflected scanout is left alone. + assert_eq!(frame_transform(180, Some(0x1 | 0x10)), 0); + assert_eq!(frame_transform(180, Some(0x4 | 0x20)), 0); + // No rotate bit at all is not a kernel value and gets no special case. + assert_eq!(frame_transform(180, Some(0)), 0); + assert_eq!(frame_transform(90, Some(0)), 0); + } + + /// The compositor programs the plane from the CRTC transform, which is the logical transform + /// composed with the connector's panel orientation (mutter: meta_output_logical_to_crtc_transform); + /// `wl_output` carries the logical part only. A panel mounted upside down at logical 0 has + /// the plane at rotate-180 and an upright framebuffer: subtracting would turn it over, and + /// master delivered it upright. So the plane answer is not measured against `wl_output`. + #[test] + fn a_plane_that_rotated_holds_the_logical_desktop_whatever_wl_output_says() { + assert_eq!(frame_transform(0, Some(0x4)), 0); + assert_eq!(frame_transform(90, Some(0x4)), 0); + assert_eq!(frame_transform(180, Some(0x2)), 0); + assert_eq!(frame_transform(90, Some(0x8)), 0); + } + + #[test] + fn a_180_frame_is_turned_only_when_the_plane_did_not_turn_it() { + use scrap::TraitPixelBuffer; + let (src, w, h) = px_frame(&[&[1, 2, 3], &[4, 5, 6]], 0); + let mut c = capturer_with(Some((w, h))); + c.transform = 180; + // Software rotation (virtio-gpu): the scanout is upside down and comes back upright. + put_frame_with(&c, w, h, Some(0x1), &src); + match c.frame(Duration::from_millis(50)) { + Ok(Frame::PixelBuffer(pb)) => { + assert_eq!((pb.width(), pb.height()), (w, h)); + assert_eq!(labels_of(pb.data(), w, h), vec![vec![6, 5, 4], vec![3, 2, 1]]); + } + Ok(_) => panic!("expected a pixel-buffer frame"), + Err(err) => panic!("expected a delivered frame, got {err}"), + } + // Hardware rotation (i915 + mutter): the scanout is already upright and is left alone. + put_frame_with(&c, w, h, Some(0x4), &src); + match c.frame(Duration::from_millis(50)) { + Ok(Frame::PixelBuffer(pb)) => { + assert_eq!(labels_of(pb.data(), w, h), vec![vec![1, 2, 3], vec![4, 5, 6]]); + } + Ok(_) => panic!("expected a pixel-buffer frame"), + Err(err) => panic!("expected a delivered frame, got {err}"), + } + // A producer that cannot say (pre-0.5.8 library) keeps the old rule: 180 left alone. + put_frame_with(&c, w, h, None, &src); + match c.frame(Duration::from_millis(50)) { + Ok(Frame::PixelBuffer(pb)) => { + assert_eq!(labels_of(pb.data(), w, h), vec![vec![1, 2, 3], vec![4, 5, 6]]); + } + Ok(_) => panic!("expected a pixel-buffer frame"), + Err(err) => panic!("expected a delivered frame, got {err}"), + } + } + + #[test] + fn a_plane_that_rotated_90_in_hardware_hands_over_a_portrait_scanout() { + use scrap::TraitPixelBuffer; + let mut c = capturer_with(Some((32, 64))); // rotated session of a 64x32 mode + c.transform = 90; + // The plane did the 90: the scanout is already 32x64 and upright, nothing to turn. + put_frame_rot(&c, 32, 64, Some(0x2)); + match c.frame(Duration::from_millis(50)) { + Ok(Frame::PixelBuffer(pb)) => assert_eq!((pb.width(), pb.height()), (32, 64)), + Ok(_) => panic!("expected a pixel-buffer frame"), + Err(err) => panic!("expected a delivered frame, got {err}"), + } + // The plane did nothing: the landscape mode arrives and is turned to 32x64. + put_frame_rot(&c, 64, 32, Some(0x1)); + match c.frame(Duration::from_millis(50)) { + Ok(Frame::PixelBuffer(pb)) => assert_eq!((pb.width(), pb.height()), (32, 64)), + Ok(_) => panic!("expected a pixel-buffer frame"), + Err(err) => panic!("expected a delivered frame, got {err}"), + } + } + + #[test] + fn a_portrait_scanout_from_a_plane_that_rotated_90_is_left_alone() { + use scrap::TraitPixelBuffer; + // A 2x3 portrait scanout: the plane did the 90, so the pixels are already upright. + let (portrait, pw, ph) = px_frame(&[&[1, 2], &[3, 4], &[5, 6]], 0); + let mut c = capturer_with(Some((pw, ph))); + c.transform = 90; + put_frame_with(&c, pw, ph, Some(0x2), &portrait); + match c.frame(Duration::from_millis(50)) { + Ok(Frame::PixelBuffer(pb)) => { + assert_eq!((pb.width(), pb.height()), (pw, ph)); + assert_eq!(labels_of(pb.data(), pw, ph), vec![vec![1, 2], vec![3, 4], vec![5, 6]]); + } + Ok(_) => panic!("expected a pixel-buffer frame"), + Err(err) => panic!("expected a delivered frame, got {err}"), + } + // The same output with a plane that did nothing: the 3x2 landscape mode arrives and is + // turned exactly as `unrotate_bgra` turns a 90 frame. + let (landscape, lw, lh) = px_frame(&[&[1, 2, 3], &[4, 5, 6]], 0); + let mut expected = Vec::new(); + unrotate_bgra(&landscape, lw, lh, 90, &mut expected); + put_frame_with(&c, lw, lh, Some(0x1), &landscape); + match c.frame(Duration::from_millis(50)) { + Ok(Frame::PixelBuffer(pb)) => { + assert_eq!((pb.width(), pb.height()), (pw, ph)); + assert_eq!(pb.data(), &expected[..]); + } + Ok(_) => panic!("expected a pixel-buffer frame"), + Err(err) => panic!("expected a delivered frame, got {err}"), + } + } + #[test] fn unrotate_180_reverses_both_axes() { let (src, w, h) = px_frame(&[&[1, 2, 3], &[4, 5, 6]], 0); @@ -2845,13 +3005,35 @@ mod drm_capturer_tests { } fn put_frame(c: &IpcDrmCapturer, w: usize, h: usize) { + put_frame_rot(c, w, h, None); + } + + fn put_frame_rot(c: &IpcDrmCapturer, w: usize, h: usize, plane_rotation: Option) { let mut buf = c.shared.slot.lock().unwrap().take_free().unwrap_or_default(); buf.clear(); buf.resize(w * h * 4, 0); let mut slot = c.shared.slot.lock().unwrap(); - slot.publish(w, h, Pixfmt::BGRA, buf); + slot.publish(w, h, Pixfmt::BGRA, plane_rotation, buf); + c.shared.cv.notify_one(); } + fn put_frame_with( + c: &IpcDrmCapturer, + w: usize, + h: usize, + plane_rotation: Option, + pixels: &[u8], + ) { + let mut buf = c.shared.slot.lock().unwrap().take_free().unwrap_or_default(); + buf.clear(); + buf.extend_from_slice(pixels); + assert_eq!(buf.len(), w * h * 4); + let mut slot = c.shared.slot.lock().unwrap(); + slot.publish(w, h, Pixfmt::BGRA, plane_rotation, buf); + c.shared.cv.notify_one(); + } + + #[test] fn a_delivered_frame_clears_the_streak_but_keeps_the_cadence_and_the_convert_verdict() { let key = "test:frame-keeps-cadence"; @@ -3188,8 +3370,9 @@ mod drm_capturer_tests { for t in [90, 180, 270] { assert!(calibration_context(&drm, 0, &one(t), 7).is_err(), "transform {t}"); } - // 180 is folded to 0 for the FRAME; that fold must never reach the calibration. - assert_eq!(frame_transform(transform_and_origin(&drm, 0, &one(180)).0), 0); + // A plane that rotated folds 180 to 0 for the FRAME; that fold must never reach the + // calibration. + assert_eq!(frame_transform(transform_and_origin(&drm, 0, &one(180)).0, Some(0x4)), 0); // Partial snapshot: two connectors, one output, even one with a matching name. let two = [ drm_display("HDMI-A-1", 1920, 1080),