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.
This commit is contained in:
Mariano Abad authored and GitHub committed 2026-09-28 15:54:55 +08:00
1 parent be9b96cf65
commit 0d49ead0c3
8 files changed
+499 -58

No files matched your search

+2
View File
@@ -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
+2 -2
View File
@@ -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
+1 -1
View File
@@ -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
+117
View File
@@ -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<u32>,
}
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<u32> {
self.last_plane_rotation
}
fn read_plane_rotation(&mut self) -> Option<u32> {
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<u32> {
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");
}
}
+72
View File
@@ -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<FnRenderNode>,
/// 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<FnPlaneRotation>,
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<FnCursorHotspotValid> =
lib.get(b"drmtap_cursor_hotspot_valid").ok().map(|s| *s);
let plane_rotation: Option<FnPlaneRotation> =
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),
})
}
+43
View File
@@ -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<u32>,
/// 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::<Data>(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::<Data>(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);
+33 -9
View File
@@ -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<u32>,
/// 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<u32>,
/// 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
}
));
+229 -46
View File
@@ -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<u8>)>,
// 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<u32>, Vec<u8>)>,
// 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<Vec<u8>>; 2],
@@ -35,11 +36,18 @@ struct FrameSlot {
}
impl FrameSlot {
fn publish(&mut self, w: usize, h: usize, fmt: Pixfmt, buf: Vec<u8>) {
fn publish(
&mut self,
w: usize,
h: usize,
fmt: Pixfmt,
plane_rotation: Option<u32>,
buf: Vec<u8>,
) {
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<u8>) {
@@ -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<u32>) -> 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<u32>) {
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<u32>,
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),