From 02832d8d9c2aa25f4c8bb16b3c601402f76302d8 Mon Sep 17 00:00:00 2001 From: jtnunley Date: Thu, 22 Dec 2022 16:06:12 -0800 Subject: [PATCH 1/5] Implement shared memory transfer for X11 --- Cargo.toml | 4 ++-- src/x11.rs | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 380f9ca..7fe9d77 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,7 +16,7 @@ rust-version = "1.60.0" default = ["x11", "wayland", "wayland-dlopen"] wayland = ["wayland-backend", "wayland-client", "nix", "fastrand"] wayland-dlopen = ["wayland-sys/dlopen"] -x11 = ["bytemuck", "x11rb", "x11-dl"] +x11 = ["bytemuck", "nix", "x11rb", "x11-dl"] [dependencies] thiserror = "1.0.30" @@ -30,7 +30,7 @@ wayland-client = { version = "0.30.0", optional = true } wayland-sys = "0.30.0" bytemuck = { version = "1.12.3", optional = true } x11-dl = { version = "2.19.1", optional = true } -x11rb = { version = "0.11.0", features = ["allow-unsafe-code", "dl-libxcb"], optional = true } +x11rb = { version = "0.11.0", features = ["allow-unsafe-code", "dl-libxcb", "shm"], optional = true } [target.'cfg(all(unix, not(any(target_vendor = "apple", target_os = "android", target_os = "redox", target_os = "linux", target_os = "freebsd"))))'.dependencies] fastrand = { version = "1.8.0", optional = true } diff --git a/src/x11.rs b/src/x11.rs index b745294..9b3db82 100644 --- a/src/x11.rs +++ b/src/x11.rs @@ -5,13 +5,16 @@ //! addition, we may also want to blit to a pixmap instead of a window. use crate::SoftBufferError; +use nix::libc::{shmget, shmat, shmdt, shmctl, IPC_PRIVATE, IPC_RMID}; use raw_window_handle::{XcbDisplayHandle, XcbWindowHandle, XlibDisplayHandle, XlibWindowHandle}; -use std::fmt; +use std::{fmt, io}; +use std::ptr::{NonNull, null_mut}; use x11_dl::xlib::Display; use x11_dl::xlib_xcb::Xlib_xcb; use x11rb::connection::Connection; +use x11rb::protocol::shm::{self, ConnectionExt as _}; use x11rb::protocol::xproto::{self, ConnectionExt as _, Gcontext, Window}; use x11rb::xcb_ffi::XCBConnection; @@ -28,6 +31,13 @@ pub struct X11Impl { /// The depth (bits per pixel) of the drawing context. depth: u8, + + /// Information about SHM, if it is available. + shm: Option +} + +struct ShmInfo { + } impl X11Impl { @@ -151,6 +161,49 @@ impl X11Impl { } } +struct ShmSegment { + id: i32, + ptr: NonNull, + size: usize, +} + +impl ShmSegment { + /// Create a new `ShmSegment` with the given size. + fn new(size: usize) -> io::Result { + unsafe { + // Create the shared memory segment. + let id = shmget(IPC_PRIVATE, size, 0o600); + if id == -1 { + return Err(io::Error::last_os_error()); + } + + // Get the pointer it maps to. + let ptr = shmat(id, null_mut(), 0); + let ptr = match NonNull::new(ptr as *mut i8) { + Some(ptr) => ptr, + None => { + shmctl(id, IPC_RMID, null_mut()); + return Err(io::Error::last_os_error()); + } + }; + + Ok(Self { id, ptr, size }) + } + } +} + +impl Drop for ShmSegment { + fn drop(&mut self) { + unsafe { + // Detach the shared memory segment. + shmdt(self.ptr.as_ptr() as _); + + // Delete the shared memory segment. + shmctl(self.id, IPC_RMID, null_mut()); + } + } +} + impl Drop for X11Impl { fn drop(&mut self) { // Close the graphics context that we created. From a2ef94f1705014c580ece77981f0fdb22ca2c234 Mon Sep 17 00:00:00 2001 From: jtnunley Date: Thu, 5 Jan 2023 07:02:56 -0800 Subject: [PATCH 2/5] Change to x11rb --- src/x11.rs | 277 +++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 250 insertions(+), 27 deletions(-) diff --git a/src/x11.rs b/src/x11.rs index 9b3db82..e38e9f2 100644 --- a/src/x11.rs +++ b/src/x11.rs @@ -1,21 +1,23 @@ //! Implementation of software buffering for X11. //! //! This module converts the input buffer into an XImage and then sends it over the wire to be -//! drawn. A more effective implementation would use shared memory instead of the wire. In -//! addition, we may also want to blit to a pixmap instead of a window. +//! drawn by the X server. The SHM extension is used if available. + +#![allow(clippy::uninlined_format_args)] use crate::SoftBufferError; -use nix::libc::{shmget, shmat, shmdt, shmctl, IPC_PRIVATE, IPC_RMID}; +use nix::libc::{shmat, shmctl, shmdt, shmget, IPC_PRIVATE, IPC_RMID}; use raw_window_handle::{XcbDisplayHandle, XcbWindowHandle, XlibDisplayHandle, XlibWindowHandle}; +use std::ptr::{null_mut, NonNull}; use std::{fmt, io}; -use std::ptr::{NonNull, null_mut}; use x11_dl::xlib::Display; use x11_dl::xlib_xcb::Xlib_xcb; use x11rb::connection::Connection; +use x11rb::errors::{ConnectionError, ReplyError, ReplyOrIdError}; use x11rb::protocol::shm::{self, ConnectionExt as _}; -use x11rb::protocol::xproto::{self, ConnectionExt as _, Gcontext, Window}; +use x11rb::protocol::xproto::{self, ConnectionExt as _}; use x11rb::xcb_ffi::XCBConnection; /// The handle to an X11 drawing context. @@ -24,20 +26,21 @@ pub struct X11Impl { connection: XCBConnection, /// The window to draw to. - window: Window, + window: xproto::Window, /// The graphics context to use when drawing. - gc: Gcontext, + gc: xproto::Gcontext, /// The depth (bits per pixel) of the drawing context. depth: u8, /// Information about SHM, if it is available. - shm: Option + shm: Option, } struct ShmInfo { - + /// The shared memory segment, paired with its ID. + seg: Option<(ShmSegment, shm::Seg)>, } impl X11Impl { @@ -107,10 +110,13 @@ impl X11Impl { let window = window_handle.window; - // Start getting the depth of the window. + // Run in parallel: start getting the window depth and the SHM extension. let geometry_token = connection .get_geometry(window) .swbuf_err("Failed to send geometry request")?; + let query_token = connection + .query_extension(shm::X11_EXTENSION_NAME.as_bytes()) + .swbuf_err("Failed to send SHM query request")?; // Create a new graphics context to draw to. let gc = connection @@ -131,33 +137,170 @@ impl X11Impl { .reply() .swbuf_err("Failed to get geometry reply")?; + // See if SHM is available. + let shm_info = { + let present = query_token + .reply() + .swbuf_err("Failed to get SHM query reply")? + .present; + + if present { + // SHM is available. + Some(ShmInfo { seg: None }) + } else { + None + } + }; + Ok(Self { connection, window, gc, depth: geometry_reply.depth, + shm: shm_info, }) } pub(crate) unsafe fn set_buffer(&mut self, buffer: &[u32], width: u16, height: u16) { // Draw the image to the buffer. - let result = self.connection.put_image( - xproto::ImageFormat::Z_PIXMAP, - self.window, - self.gc, - width, - height, - 0, - 0, - 0, - self.depth, - bytemuck::cast_slice(buffer), - ); - - match result { - Err(e) => log::error!("Failed to draw image to window: {}", e), - Ok(token) => token.ignore_error(), + let result = unsafe { self.set_buffer_shm(buffer, width, height) }.and_then(|had_shm| { + if had_shm { + Ok(()) + } else { + log::debug!("Falling back to non-SHM method"); + self.set_buffer_fallback(buffer, width, height) + } + }); + + if let Err(e) = result { + log::error!("Failed to draw image to window: {}", e); + } + } + + /// Put the given buffer into the window using the SHM method. + /// + /// Returns `false` if SHM is not available. + /// + /// # Safety + /// + /// The buffer's length must be `width * height`. + unsafe fn set_buffer_shm( + &mut self, + buffer: &[u32], + width: u16, + height: u16, + ) -> Result { + let shm_info = match self.shm { + Some(ref mut info) => info, + None => return Ok(false), + }; + + // Get the SHM segment to use. + let necessary_size = (width as usize) * (height as usize) * 4; + let (segment, segment_id) = shm_info.segment(&self.connection, necessary_size)?; + + // Copy the buffer into the segment. + // SAFETY: The buffer is properly sized. + unsafe { + segment.copy(buffer); } + + // Put the image into the window. + self.connection + .shm_put_image( + self.window, + self.gc, + width, + height, + 0, + 0, + width, + height, + 0, + 0, + self.depth, + xproto::ImageFormat::Z_PIXMAP.into(), + false, + segment_id, + 0, + )? + .ignore_error(); + + Ok(true) + } + + /// Put the given buffer into the window using the fallback wire transfer method. + fn set_buffer_fallback( + &mut self, + buffer: &[u32], + width: u16, + height: u16, + ) -> Result<(), PushBufferError> { + self.connection + .put_image( + xproto::ImageFormat::Z_PIXMAP, + self.window, + self.gc, + width, + height, + 0, + 0, + 0, + self.depth, + bytemuck::cast_slice(buffer), + )? + .ignore_error(); + + Ok(()) + } +} + +impl ShmInfo { + /// Allocate a new `ShmSegment` of the given size. + fn segment( + &mut self, + conn: &impl Connection, + size: usize, + ) -> Result<(&mut ShmSegment, shm::Seg), PushBufferError> { + // Get the size of the segment currently in use. + let needs_realloc = match self.seg { + Some((ref seg, _)) => seg.size() < size, + None => true, + }; + + // Reallocate if necessary. + if needs_realloc { + let new_seg = ShmSegment::new(size)?; + self.associate(conn, new_seg)?; + } + + // Get the segment and ID. + Ok(self + .seg + .as_mut() + .map(|(ref mut seg, id)| (seg, *id)) + .unwrap()) + } + + /// Associate an SHM segment with the server. + fn associate( + &mut self, + conn: &impl Connection, + seg: ShmSegment, + ) -> Result<(), PushBufferError> { + // Register the guard. + let new_id = conn.generate_id()?; + conn.shm_attach(new_id, seg.id(), true)?.ignore_error(); + + // Take out the old one and detach it. + if let Some((old_seg, old_id)) = self.seg.replace((seg, new_id)) { + conn.shm_detach(old_id)?.ignore_error(); + + // Drop the old segment. + drop(old_seg); + } + + Ok(()) } } @@ -190,6 +333,33 @@ impl ShmSegment { Ok(Self { id, ptr, size }) } } + + /// Copy data into this shared memory segment. + /// + /// # Safety + /// + /// This function assumes that the size of `self`'s buffer is larger than or equal to `data.len()`. + unsafe fn copy(&mut self, data: &[impl bytemuck::NoUninit]) { + let incoming_data = bytemuck::cast_slice::<_, u8>(data); + + unsafe { + std::ptr::copy_nonoverlapping( + incoming_data.as_ptr(), + self.ptr.as_ptr() as *mut u8, + incoming_data.len(), + ) + } + } + + /// Get the size of this shared memory segment. + fn size(&self) -> usize { + self.size + } + + /// Get the shared memory ID. + fn id(&self) -> u32 { + self.id as _ + } } impl Drop for ShmSegment { @@ -213,12 +383,65 @@ impl Drop for X11Impl { } } +/// An error that can occur when pushing a buffer to the window. +#[derive(Debug)] +enum PushBufferError { + /// We encountered an X11 error. + X11(ReplyError), + + /// We exhausted the XID space. + XidExhausted, + + /// A system error occurred while creating the shared memory segment. + System(io::Error), +} + +impl fmt::Display for PushBufferError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::X11(e) => write!(f, "X11 error: {}", e), + Self::XidExhausted => write!(f, "XID space exhausted"), + Self::System(e) => write!(f, "System error: {}", e), + } + } +} + +impl std::error::Error for PushBufferError {} + +impl From for PushBufferError { + fn from(e: ConnectionError) -> Self { + Self::X11(ReplyError::ConnectionError(e)) + } +} + +impl From for PushBufferError { + fn from(e: ReplyError) -> Self { + Self::X11(e) + } +} + +impl From for PushBufferError { + fn from(e: ReplyOrIdError) -> Self { + match e { + ReplyOrIdError::ConnectionError(e) => Self::X11(ReplyError::ConnectionError(e)), + ReplyOrIdError::X11Error(e) => Self::X11(ReplyError::X11Error(e)), + ReplyOrIdError::IdsExhausted => Self::XidExhausted, + } + } +} + +impl From for PushBufferError { + fn from(e: io::Error) -> Self { + Self::System(e) + } +} + /// Convenient wrapper to cast errors into SoftBufferError. trait ResultExt { fn swbuf_err(self, msg: impl Into) -> Result; } -impl ResultExt for Result { +impl ResultExt for Result { fn swbuf_err(self, msg: impl Into) -> Result { self.map_err(|e| { SoftBufferError::PlatformError(Some(msg.into()), Some(Box::new(LibraryError(e)))) From 950008a607b649c770e63082768812ec98fe6ec4 Mon Sep 17 00:00:00 2001 From: jtnunley Date: Thu, 5 Jan 2023 09:40:02 -0800 Subject: [PATCH 3/5] Fix leak + review from psychon --- src/x11.rs | 55 ++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 8 deletions(-) diff --git a/src/x11.rs b/src/x11.rs index e38e9f2..de4aa20 100644 --- a/src/x11.rs +++ b/src/x11.rs @@ -14,7 +14,7 @@ use std::{fmt, io}; use x11_dl::xlib::Display; use x11_dl::xlib_xcb::Xlib_xcb; -use x11rb::connection::Connection; +use x11rb::connection::{Connection, RequestConnection}; use x11rb::errors::{ConnectionError, ReplyError, ReplyOrIdError}; use x11rb::protocol::shm::{self, ConnectionExt as _}; use x11rb::protocol::xproto::{self, ConnectionExt as _}; @@ -114,8 +114,8 @@ impl X11Impl { let geometry_token = connection .get_geometry(window) .swbuf_err("Failed to send geometry request")?; - let query_token = connection - .query_extension(shm::X11_EXTENSION_NAME.as_bytes()) + connection + .prefetch_extension_information(shm::X11_EXTENSION_NAME) .swbuf_err("Failed to send SHM query request")?; // Create a new graphics context to draw to. @@ -139,10 +139,7 @@ impl X11Impl { // See if SHM is available. let shm_info = { - let present = query_token - .reply() - .swbuf_err("Failed to get SHM query reply")? - .present; + let present = is_shm_available(&connection); if present { // SHM is available. @@ -339,7 +336,8 @@ impl ShmSegment { /// # Safety /// /// This function assumes that the size of `self`'s buffer is larger than or equal to `data.len()`. - unsafe fn copy(&mut self, data: &[impl bytemuck::NoUninit]) { + unsafe fn copy(&mut self, data: &[T]) { + debug_assert!(data.len() * std::mem::size_of::() <= self.size,); let incoming_data = bytemuck::cast_slice::<_, u8>(data); unsafe { @@ -376,6 +374,19 @@ impl Drop for ShmSegment { impl Drop for X11Impl { fn drop(&mut self) { + // If we used SHM, make sure it's detached from the server. + if let Some(ShmInfo { + seg: Some((segment, seg_id)), + }) = self.shm.take() + { + if let Ok(token) = self.connection.shm_detach(seg_id) { + token.ignore_error(); + } + + // Drop the segment. + drop(segment); + } + // Close the graphics context that we created. if let Ok(token) = self.connection.free_gc(self.gc) { token.ignore_error(); @@ -383,6 +394,34 @@ impl Drop for X11Impl { } } +/// Test to see if SHM is available. +fn is_shm_available(c: &impl Connection) -> bool { + // Create a small SHM segment. + let seg = match ShmSegment::new(0x1000) { + Ok(seg) => seg, + Err(_) => return false, + }; + + // Attach and detach it. + let seg_id = match c.generate_id() { + Ok(id) => id, + Err(_) => return false, + }; + + let (attach, detach) = { + let attach = c.shm_attach(seg_id, seg.id(), false); + let detach = c.shm_detach(seg_id); + + match (attach, detach) { + (Ok(attach), Ok(detach)) => (attach, detach), + _ => return false, + } + }; + + // Check the replies. + matches!((attach.check(), detach.check()), (Ok(()), Ok(()))) +} + /// An error that can occur when pushing a buffer to the window. #[derive(Debug)] enum PushBufferError { From e46cb671e4af1b17621109bf1efc9c6d088880e3 Mon Sep 17 00:00:00 2001 From: jtnunley Date: Thu, 5 Jan 2023 12:13:31 -0800 Subject: [PATCH 4/5] Add waits to prevent illegal writes to SHM --- src/x11.rs | 70 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 12 deletions(-) diff --git a/src/x11.rs b/src/x11.rs index de4aa20..6ac5861 100644 --- a/src/x11.rs +++ b/src/x11.rs @@ -14,7 +14,8 @@ use std::{fmt, io}; use x11_dl::xlib::Display; use x11_dl::xlib_xcb::Xlib_xcb; -use x11rb::connection::{Connection, RequestConnection}; +use x11rb::connection::{Connection, RequestConnection, SequenceNumber}; +use x11rb::cookie::Cookie; use x11rb::errors::{ConnectionError, ReplyError, ReplyOrIdError}; use x11rb::protocol::shm::{self, ConnectionExt as _}; use x11rb::protocol::xproto::{self, ConnectionExt as _}; @@ -41,6 +42,20 @@ pub struct X11Impl { struct ShmInfo { /// The shared memory segment, paired with its ID. seg: Option<(ShmSegment, shm::Seg)>, + + /// A cookie indicating that the shared memory segment is ready to be used. + /// + /// We can't soundly read from or write to the SHM segment until the X server is done processing the + /// `shm::PutImage` request. However, the X server handles requests in order, which means that, if + /// we send a very small request after the `shm::PutImage` request, then the X server will have to + /// process that request before it can process the `shm::PutImage` request. Therefore, we can use + /// the reply to that small request to determine when the `shm::PutImage` request is done. + /// + /// In this case, we use `GetInputFocus` since it is a very small request. + /// + /// We store the sequence number instead of the `Cookie` since we cannot hold a self-referential + /// reference to the `connection` field. + done_processing: Option, } impl X11Impl { @@ -143,7 +158,10 @@ impl X11Impl { if present { // SHM is available. - Some(ShmInfo { seg: None }) + Some(ShmInfo { + seg: None, + done_processing: None, + }) } else { None } @@ -192,12 +210,15 @@ impl X11Impl { None => return Ok(false), }; + // If the X server is still processing the last image, wait for it to finish. + shm_info.finish_wait(&self.connection)?; + // Get the SHM segment to use. let necessary_size = (width as usize) * (height as usize) * 4; let (segment, segment_id) = shm_info.segment(&self.connection, necessary_size)?; // Copy the buffer into the segment. - // SAFETY: The buffer is properly sized. + // SAFETY: The buffer is properly sized and we've ensured that the X server isn't reading from it. unsafe { segment.copy(buffer); } @@ -223,6 +244,9 @@ impl X11Impl { )? .ignore_error(); + // Send a short request to act as a notification for when the X server is done processing the image. + shm_info.begin_wait(&self.connection)?; + Ok(true) } @@ -299,6 +323,25 @@ impl ShmInfo { Ok(()) } + + /// Begin waiting for the SHM processing to finish. + fn begin_wait(&mut self, c: &impl Connection) -> Result<(), PushBufferError> { + let cookie = c.get_input_focus()?.sequence_number(); + let old_cookie = self.done_processing.replace(cookie); + debug_assert!(old_cookie.is_none()); + Ok(()) + } + + /// Wait for the SHM processing to finish. + fn finish_wait(&mut self, c: &impl Connection) -> Result<(), PushBufferError> { + if let Some(done_processing) = self.done_processing.take() { + // Cast to a cookie and wait on it. + let cookie = Cookie::<_, xproto::GetInputFocusReply>::new(c, done_processing); + cookie.reply()?; + } + + Ok(()) + } } struct ShmSegment { @@ -336,6 +379,7 @@ impl ShmSegment { /// # Safety /// /// This function assumes that the size of `self`'s buffer is larger than or equal to `data.len()`. + /// In addition, no other processes should be reading from or writing to this memory. unsafe fn copy(&mut self, data: &[T]) { debug_assert!(data.len() * std::mem::size_of::() <= self.size,); let incoming_data = bytemuck::cast_slice::<_, u8>(data); @@ -375,16 +419,18 @@ impl Drop for ShmSegment { impl Drop for X11Impl { fn drop(&mut self) { // If we used SHM, make sure it's detached from the server. - if let Some(ShmInfo { - seg: Some((segment, seg_id)), - }) = self.shm.take() - { - if let Ok(token) = self.connection.shm_detach(seg_id) { - token.ignore_error(); - } + if let Some(mut shm) = self.shm.take() { + // If we were in the middle of processing a buffer, wait for it to finish. + shm.finish_wait(&self.connection).ok(); + + if let Some((segment, seg_id)) = shm.seg.take() { + if let Ok(token) = self.connection.shm_detach(seg_id) { + token.ignore_error(); + } - // Drop the segment. - drop(segment); + // Drop the segment. + drop(segment); + } } // Close the graphics context that we created. From 5db2d4924bfc420121f49f10cf4da68ed0eeded1 Mon Sep 17 00:00:00 2001 From: jtnunley Date: Thu, 5 Jan 2023 12:22:09 -0800 Subject: [PATCH 5/5] @ids1024 code review --- src/x11.rs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/x11.rs b/src/x11.rs index 6ac5861..f83a32a 100644 --- a/src/x11.rs +++ b/src/x11.rs @@ -283,6 +283,9 @@ impl ShmInfo { conn: &impl Connection, size: usize, ) -> Result<(&mut ShmSegment, shm::Seg), PushBufferError> { + // Round the size up to the next power of two to prevent frequent reallocations. + let size = size.next_power_of_two(); + // Get the size of the segment currently in use. let needs_realloc = match self.seg { Some((ref seg, _)) => seg.size() < size, @@ -360,13 +363,15 @@ impl ShmSegment { return Err(io::Error::last_os_error()); } - // Get the pointer it maps to. - let ptr = shmat(id, null_mut(), 0); - let ptr = match NonNull::new(ptr as *mut i8) { - Some(ptr) => ptr, - None => { - shmctl(id, IPC_RMID, null_mut()); - return Err(io::Error::last_os_error()); + // Map the SHM to our memory space. + let ptr = { + let ptr = shmat(id, null_mut(), 0); + match NonNull::new(ptr as *mut i8) { + Some(ptr) => ptr, + None => { + shmctl(id, IPC_RMID, null_mut()); + return Err(io::Error::last_os_error()); + } } };