From ba7bc0c698e754ed16fd5ed729f4a691dadd26e4 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Tue, 30 Jun 2020 13:40:56 -0500 Subject: [PATCH 01/11] Adds an idle loop to the x11 backend, and uses it for repainting. --- druid-shell/Cargo.toml | 3 +- druid-shell/src/platform/x11/application.rs | 123 +++++++++++++- druid-shell/src/platform/x11/window.rs | 178 ++++++++++++++------ 3 files changed, 247 insertions(+), 57 deletions(-) diff --git a/druid-shell/Cargo.toml b/druid-shell/Cargo.toml index 00dad9e744..850a9bc5b7 100644 --- a/druid-shell/Cargo.toml +++ b/druid-shell/Cargo.toml @@ -14,7 +14,7 @@ rustdoc-args = ["--cfg", "docsrs"] default-target = "x86_64-pc-windows-msvc" [features] -x11 = ["x11rb", "cairo-sys-rs"] +x11 = ["x11rb", "nix", "cairo-sys-rs"] [dependencies] # NOTE: When changing the piet or kurbo versions, ensure that @@ -40,6 +40,7 @@ gtk = { version = "0.8.1", optional = true } glib = { version = "0.9.3", optional = true } glib-sys = { version = "0.9.1", optional = true } gtk-sys = { version = "0.9.2", optional = true } +nix = { version = "0.17.0", optional = true } x11rb = { version = "0.6.0", features = ["allow-unsafe-code", "present", "randr", "xfixes"], optional = true } [target.'cfg(target_os="windows")'.dependencies] diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 9b9e26497d..aabc8c4fd0 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -16,8 +16,10 @@ use std::cell::RefCell; use std::collections::HashMap; -use std::convert::TryInto; +use std::convert::{TryFrom, TryInto}; +use std::os::unix::io::RawFd; use std::rc::Rc; +use std::time::{Duration, Instant}; use anyhow::{anyhow, Context, Error}; use x11rb::connection::Connection; @@ -30,6 +32,7 @@ use x11rb::xcb_ffi::XCBConnection; use crate::application::AppHandler; use super::clipboard::Clipboard; +use super::util; use super::window::Window; #[derive(Clone)] @@ -62,6 +65,10 @@ pub(crate) struct Application { window_id: u32, /// The mutable `Application` state. state: Rc>, + /// The read end of the "idle pipe", a pipe that allows the event loop to be woken up. + idle_read: RawFd, + /// The write end of the "idle pipe", a pipe that allows the event loop to be woken up. + idle_write: RawFd, /// The major opcode of the Present extension, if it is supported. present_opcode: Option, } @@ -92,6 +99,8 @@ impl Application { windows: HashMap::new(), })); + let (idle_read, idle_write) = nix::unistd::pipe2(nix::fcntl::OFlag::O_NONBLOCK)?; + let present_opcode = if std::env::var_os("DRUID_SHELL_DISABLE_X11_PRESENT").is_some() { // Allow disabling Present with an environment variable. None @@ -110,6 +119,8 @@ impl Application { screen_num: screen_num as i32, window_id, state, + idle_read, + idle_write, present_opcode, }) } @@ -350,13 +361,22 @@ impl Application { Ok(false) } - pub fn run(self, _handler: Option>) { + fn run_inner(self) -> Result<(), Error> { + // Try to figure out the refresh rate of the current screen. We run the idle loop at that + // rate. + let refresh_rate = util::refresh_rate(self.connection(), self.window_id).unwrap_or(60.0); + let timeout = Duration::from_millis((1000.0 / refresh_rate) as u64); + let mut last_idle_time = Instant::now(); loop { - if let Ok(ev) = self.connection.wait_for_event() { + let next_idle_time = last_idle_time + timeout; + poll_with_timeout(&self.connection, self.idle_read, next_idle_time) + .context("Error while waiting for X11 connection")?; + + while let Some(ev) = self.connection.poll_for_event()? { match self.handle_event(&ev) { Ok(quit) => { if quit { - break; + return Ok(()); } } Err(e) => { @@ -364,6 +384,48 @@ impl Application { } } } + + let now = Instant::now(); + if now >= next_idle_time { + last_idle_time = now; + // Clear out our idle pipe. Each write to it adds one byte; it's unlikely that there + // will be much in it, but read it 16 bytes at a time just in case. + let mut read_buf = [0u8; 16]; + loop { + match nix::unistd::read(self.idle_read, &mut read_buf[..]) { + Err(nix::Error::Sys(nix::errno::Errno::EINTR)) => {} + // According to write(2), this is the outcome of reading an empty, O_NONBLOCK + // pipe. + Err(nix::Error::Sys(nix::errno::Errno::EAGAIN)) => { + break; + } + Err(e) => { + return Err(e).context("Failed to read from idle pipe"); + } + // According to write(2), this is the outcome of reading an O_NONBLOCK pipe + // when the other end has been closed. This shouldn't happen to us because we + // own both ends, but just in case. + Ok(0) => { + break; + } + Ok(_) => {} + } + } + + if let Ok(state) = self.state.try_borrow() { + for w in state.windows.values() { + w.run_idle(); + } + } else { + log::error!("In idle loop, application state already borrowed"); + } + } + } + } + + pub fn run(self, _handler: Option>) { + if let Err(e) = self.run_inner() { + log::error!("{}", e); } } @@ -404,4 +466,57 @@ impl Application { log::warn!("Application::get_locale is currently unimplemented for X11 platforms. (defaulting to en-US)"); "en-US".into() } + + pub(crate) fn idle_pipe(&self) -> RawFd { + self.idle_write + } +} + +/// Returns when there is an event ready to read from `conn`, or there is data ready to read from +/// `idle` and the `timeout` has passed. +// This was taken, with minor modifications, from the xclock_utc example in the x11rb crate. +// https://raw.githubusercontent.com/psychon/x11rb/a6bd1453fd8e931394b9b1f2185fad48b7cca5fe/examples/xclock_utc.rs +fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> Result<(), Error> { + use nix::poll::{poll, PollFd, PollFlags}; + use std::os::raw::c_int; + use std::os::unix::io::AsRawFd; + + let fd = conn.as_raw_fd(); + let mut poll_fds = [ + PollFd::new(fd, PollFlags::POLLIN), + PollFd::new(idle, PollFlags::POLLIN), + ]; + + // We start with no timeout in the poll call. If we get something from the idle handler, we'll + // start setting one. + let mut poll_timeout = -1; + loop { + let readable = |p: &PollFd| { + p.revents() + .unwrap_or_else(PollFlags::empty) + .contains(PollFlags::POLLIN) + }; + + match poll(&mut poll_fds, poll_timeout) { + Ok(_) => { + if readable(&poll_fds[0]) { + break; + } + if readable(&poll_fds[1]) { + let now = Instant::now(); + if now >= timeout { + break; + } else { + poll_timeout = c_int::try_from(timeout.duration_since(now).as_millis()) + .unwrap_or(c_int::max_value()) + } + } + } + + Err(nix::Error::Sys(nix::errno::Errno::EINTR)) => {} + Err(e) => return Err(e.into()), + } + } + + Ok(()) } diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 426d18afad..06c8ef9cc0 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -17,7 +17,9 @@ use std::any::Any; use std::cell::RefCell; use std::convert::TryInto; +use std::os::unix::io::RawFd; use std::rc::{Rc, Weak}; +use std::sync::{Arc, Mutex}; use anyhow::{anyhow, Context, Error}; use cairo::{XCBConnection as CairoXCBConnection, XCBDrawable, XCBSurface, XCBVisualType}; @@ -26,15 +28,16 @@ use x11rb::connection::Connection; use x11rb::protocol::present::{CompleteNotifyEvent, ConnectionExt as _, IdleNotifyEvent}; use x11rb::protocol::xfixes::{ConnectionExt as _, Region}; use x11rb::protocol::xproto::{ - self, AtomEnum, ConfigureNotifyEvent, ConnectionExt, CreateGCAux, EventMask, ExposeEvent, - Gcontext, Pixmap, PropMode, Rectangle, Visualtype, WindowClass, + self, AtomEnum, ConfigureNotifyEvent, ConnectionExt, CreateGCAux, EventMask, Gcontext, Pixmap, + PropMode, Rectangle, Visualtype, WindowClass, }; use x11rb::wrapper::ConnectionExt as _; use x11rb::xcb_ffi::XCBConnection; +use crate::common_util::IdleCallback; use crate::dialog::{FileDialogOptions, FileInfo}; use crate::error::Error as ShellError; -use crate::keyboard::{KeyState, KeyEvent, Modifiers}; +use crate::keyboard::{KeyEvent, KeyState, Modifiers}; use crate::kurbo::{Point, Rect, Size, Vec2}; use crate::mouse::{Cursor, MouseButton, MouseButtons, MouseEvent}; use crate::piet::{Piet, RenderContext}; @@ -246,8 +249,6 @@ impl WindowBuilder { // TODO(x11/errors): Should do proper cleanup (window destruction etc) in case of error let atoms = self.atoms(id)?; let cairo_surface = RefCell::new(self.create_cairo_surface(id, &visual_type)?); - // Figure out the refresh rate of the current screen - let refresh_rate = util::refresh_rate(conn, id); let state = RefCell::new(WindowState { size: self.size, invalid: Rect::ZERO, @@ -279,9 +280,10 @@ impl WindowBuilder { app: self.app.clone(), handler, cairo_surface, - refresh_rate, atoms, state, + idle_queue: Arc::new(Mutex::new(Vec::new())), + idle_pipe: self.app.idle_pipe(), present_data: RefCell::new(present_data), buffers, }); @@ -332,9 +334,11 @@ pub(crate) struct Window { app: Application, handler: RefCell>, cairo_surface: RefCell, - refresh_rate: Option, atoms: WindowAtoms, state: RefCell, + idle_queue: Arc>>, + // Writing to this wakes up the event loop, so that it can run idle handlers. + idle_pipe: RawFd, /// When this is `Some(_)`, we use the X11 Present extension to present windows. This syncs all /// presentation to vblank and it appears to prevent tearing (subject to various caveats @@ -352,10 +356,10 @@ pub(crate) struct Window { /// this case, we will render the next frame immediately unless we're already waiting for a /// completion notification. (If we are waiting for a completion notification, we just make /// a note to schedule a new frame once we get it.) - /// 3) Someone calls `invalidate` or `invalidate_rect` on us. We send ourselves an expose event - /// and end up in state 2. This is better than rendering straight away, because for example - /// they might have called `invalidate` from their paint callback, and then we'd end up - /// painting re-entrantively. + /// 3) Someone calls `invalidate` or `invalidate_rect` on us. We schedule ourselves to repaint + /// in the idle loop. This is better than rendering straight away, because for example they + /// might have called `invalidate` from their paint callback, and then we'd end up painting + /// re-entrantively. /// /// This is probably not the best (or at least, not the lowest-latency) scheme we can come up /// with, because invalidations that happen shortly after a vblank might need to wait 2 frames @@ -585,16 +589,11 @@ impl Window { self.app .connection() .copy_area(pixmap, self.id, self.gc, x, y, x, y, w, h)?; - // We aren't using the present extension, so fall back to sleeping for scheduling - // redraws. Sleeping is a terrible way to schedule redraws, but hopefully we don't - // have to fall back to this very often. - // TODO: once we have an idle handler, we should use that. Sleeping causes lots of - // problems when windows are dragged to resize. - if anim && self.refresh_rate.is_some() { - let sleep_amount_ms = (1000.0 / self.refresh_rate.unwrap()) as u64; - std::thread::sleep(std::time::Duration::from_millis(sleep_amount_ms)); - - self.invalidate_rect(size.to_rect()); + // We aren't using the present extension, so use the idle loop to schedule redraws. + // Note that the idle loop's wakeup times are roughly tied to the refresh rate, so this + // shouldn't draw too often. + if anim { + self.invalidate(); } } self.app.connection().flush()?; @@ -649,6 +648,28 @@ impl Window { Ok(()) } + /// Redraw more-or-less now. + /// + /// "More-or-less" because if we're already waiting on a present, we defer the drawing until it + /// completes. + fn redraw_now(&self) -> Result<(), Error> { + if self.waiting_on_present()? { + self.set_needs_present(true)?; + } else { + self.render()?; + } + Ok(()) + } + + /// Schedule a redraw on the idle loop. + fn redraw_soon(&self) { + let idle = IdleHandle { + queue: Arc::clone(&self.idle_queue), + pipe: self.idle_pipe, + }; + idle.schedule_redraw(); + } + fn invalidate(&self) { match self.size() { Ok(size) => self.invalidate_rect(size.to_rect()), @@ -657,28 +678,11 @@ impl Window { } fn invalidate_rect(&self, rect: Rect) { - if let Err(err) = self.enlarge_invalid_rect(rect) { + if let Err(err) = self.enlarge_invalid_rect(rect) { log::error!("Window::invalidate_rect - failed to enlarge rect: {}", err); } - // See: http://rtbo.github.io/rust-xcb/xcb/ffi/xproto/struct.xcb_expose_event_t.html - let expose_event = ExposeEvent { - window: self.id, - x: rect.x0 as u16, - y: rect.y0 as u16, - width: rect.width() as u16, - height: rect.height() as u16, - count: 0, - response_type: x11rb::protocol::xproto::EXPOSE_EVENT, - sequence: 0, - }; - log_x11!(self.app.connection().send_event( - false, - self.id, - EventMask::Exposure, - expose_event, - )); - log_x11!(self.app.connection().flush()); + self.redraw_soon(); } fn set_title(&self, title: &str) { @@ -869,13 +873,13 @@ impl Window { // Check whether we missed presenting on any frames. if let Some(last_msc) = present.last_msc { if last_msc.wrapping_add(1) != event.msc { - log::info!( + log::debug!( "missed a present: msc went from {} to {}", last_msc, event.msc ); if let Some(last_ust) = present.last_ust { - log::info!("ust went from {} to {}", last_ust, event.ust); + log::debug!("ust went from {} to {}", last_ust, event.ust); } } } @@ -923,6 +927,36 @@ impl Window { .map(|p| p.needs_present) .unwrap_or(false)) } + + pub(crate) fn run_idle(&self) { + let mut queue = Vec::new(); + std::mem::swap(&mut *self.idle_queue.lock().unwrap(), &mut queue); + + let mut needs_redraw = false; + if let Ok(mut handler) = self.handler.try_borrow_mut() { + for callback in queue { + match callback { + IdleKind::Callback(f) => { + f.call(handler.as_any()); + } + IdleKind::Token(tok) => { + handler.idle(tok); + } + IdleKind::Redraw => { + needs_redraw = true; + } + } + } + } else { + log::error!("Not running idle callbacks because the handler is borrowed"); + } + + if needs_redraw { + if let Err(e) = self.redraw_now() { + log::error!("Error redrawing: {}", e); + } + } + } } impl Buffers { @@ -1117,21 +1151,56 @@ fn key_mods(mods: u16) -> Modifiers { ret } +/// A handle that can get used to schedule an idle handler. Note that +/// this handle is thread safe. #[derive(Clone)] -pub(crate) struct IdleHandle; +pub struct IdleHandle { + queue: Arc>>, + pipe: RawFd, +} + +pub(crate) enum IdleKind { + Callback(Box), + Token(IdleToken), + Redraw, +} impl IdleHandle { - pub fn add_idle_callback(&self, _callback: F) + fn wake(&self) { + loop { + match nix::unistd::write(self.pipe, &[0]) { + Err(nix::Error::Sys(nix::errno::Errno::EINTR)) => {} + Err(nix::Error::Sys(nix::errno::Errno::EAGAIN)) => {} + Err(e) => { + log::error!("Failed to write to idle pipe: {}", e); + break; + } + Ok(_) => { + break; + } + } + } + } + + pub(crate) fn schedule_redraw(&self) { + self.queue.lock().unwrap().push(IdleKind::Redraw); + self.wake(); + } + + pub fn add_idle_callback(&self, callback: F) where F: FnOnce(&dyn Any) + Send + 'static, { - // TODO(x11/idle_handles): implement IdleHandle::add_idle_callback - log::warn!("IdleHandle::add_idle_callback is currently unimplemented for X11 platforms."); + self.queue + .lock() + .unwrap() + .push(IdleKind::Callback(Box::new(callback))); + self.wake(); } - pub fn add_idle_token(&self, _token: IdleToken) { - // TODO(x11/idle_handles): implement IdleHandle::add_idle_token - log::warn!("IdleHandle::add_idle_token is currently unimplemented for X11 platforms."); + pub fn add_idle_token(&self, token: IdleToken) { + self.queue.lock().unwrap().push(IdleKind::Token(token)); + self.wake(); } } @@ -1253,9 +1322,14 @@ impl WindowHandle { } pub fn get_idle_handle(&self) -> Option { - // TODO(x11/idle_handles): implement WindowHandle::get_idle_handle - //log::warn!("WindowHandle::get_idle_handle is currently unimplemented for X11 platforms."); - Some(IdleHandle) + if let Some(w) = self.window.upgrade() { + Some(IdleHandle { + queue: Arc::clone(&w.idle_queue), + pipe: w.idle_pipe, + }) + } else { + None + } } pub fn get_scale(&self) -> Result { From 8f644ddf40ba49a04e53d878df00360cb18ea873 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Tue, 30 Jun 2020 13:55:06 -0500 Subject: [PATCH 02/11] Add changelog. --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8166304d7f..0fe2e6a285 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ You can find its changes [documented below](#060---2020-06-01). - GTK: Don't crash when receiving an external command while a file dialog is visible. ([#1043] by [@jneem]) - Fix derive `Data` when type param bounds are defined ([#1058] by [@chris-zen]) - Ensure that `update` is called after all commands. ([#1062] by [@jneem]) +- X11: Support idle callbacks. ([#1072] by [@jneem]) - GTK: Don't interrupt `KeyEvent.repeat` when releasing another key. ([#1081] by [@raphlinus]) ### Visual @@ -352,6 +353,7 @@ Last release without a changelog :( [#1054]: https://github.com/linebender/druid/pull/1054 [#1058]: https://github.com/linebender/druid/pull/1058 [#1062]: https://github.com/linebender/druid/pull/1062 +[#1072]: https://github.com/linebender/druid/pull/1072 [#1081]: https://github.com/linebender/druid/pull/1081 [Unreleased]: https://github.com/linebender/druid/compare/v0.6.0...master From c1bc7a2bf880d6dff7d24f2c6117a93ebfc74fad Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Thu, 2 Jul 2020 12:05:52 -0500 Subject: [PATCH 03/11] Address review comments. --- druid-shell/src/platform/x11/application.rs | 92 +++++++++++++-------- druid-shell/src/platform/x11/window.rs | 7 +- 2 files changed, 60 insertions(+), 39 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index aabc8c4fd0..03e00b7dce 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -44,6 +44,10 @@ pub(crate) struct Application { /// /// A display is a collection of screens. connection: Rc, + /// An `XCBConnection` is *technically* safe to use from other threads, but there are lots of + /// subtleties; let's just avoid the issue altogether. As far as public API is concerned, this + /// causes `druid_shell::WindowHandle` to be `!Send` and `!Sync`. + marker: std::marker::PhantomData<*mut XCBConnection>, /// The default screen of the connected display. /// /// The connected display may also have additional screens. @@ -122,6 +126,7 @@ impl Application { idle_read, idle_write, present_opcode, + marker: std::marker::PhantomData, }) } @@ -369,10 +374,19 @@ impl Application { let mut last_idle_time = Instant::now(); loop { let next_idle_time = last_idle_time + timeout; - poll_with_timeout(&self.connection, self.idle_read, next_idle_time) - .context("Error while waiting for X11 connection")?; + self.connection.flush()?; - while let Some(ev) = self.connection.poll_for_event()? { + // Before we poll on the connection's file descriptor, check whether there are any + // events ready. It could be that XCB has some events in its internal buffers because + // of something that happened during the idle loop. + let mut event = self.connection.poll_for_event()?; + + if event.is_none() { + poll_with_timeout(&self.connection, self.idle_read, next_idle_time) + .context("Error while waiting for X11 connection")?; + } + + while let Some(ev) = event { match self.handle_event(&ev) { Ok(quit) => { if quit { @@ -383,34 +397,13 @@ impl Application { log::error!("Error handling event: {:#}", e); } } + event = self.connection.poll_for_event()?; } let now = Instant::now(); if now >= next_idle_time { last_idle_time = now; - // Clear out our idle pipe. Each write to it adds one byte; it's unlikely that there - // will be much in it, but read it 16 bytes at a time just in case. - let mut read_buf = [0u8; 16]; - loop { - match nix::unistd::read(self.idle_read, &mut read_buf[..]) { - Err(nix::Error::Sys(nix::errno::Errno::EINTR)) => {} - // According to write(2), this is the outcome of reading an empty, O_NONBLOCK - // pipe. - Err(nix::Error::Sys(nix::errno::Errno::EAGAIN)) => { - break; - } - Err(e) => { - return Err(e).context("Failed to read from idle pipe"); - } - // According to write(2), this is the outcome of reading an O_NONBLOCK pipe - // when the other end has been closed. This shouldn't happen to us because we - // own both ends, but just in case. - Ok(0) => { - break; - } - Ok(_) => {} - } - } + drain_idle_pipe(self.idle_read)?; if let Ok(state) = self.state.try_borrow() { for w in state.windows.values() { @@ -442,7 +435,6 @@ impl Application { for window in state.windows.values() { window.destroy(); } - log_x11!(self.connection.flush()); } } } else { @@ -452,7 +444,6 @@ impl Application { fn finalize_quit(&self) { log_x11!(self.connection.destroy_window(self.window_id)); - log_x11!(self.connection.flush()); } pub fn clipboard(&self) -> Clipboard { @@ -472,8 +463,37 @@ impl Application { } } -/// Returns when there is an event ready to read from `conn`, or there is data ready to read from -/// `idle` and the `timeout` has passed. +/// Clears out our idle pipe; `idle_read` should be the reading end of a pipe that was opened with +/// O_NONBLOCK. +fn drain_idle_pipe(idle_read: RawFd) -> Result<(), Error> { + // Each write to the idle pipe adds one byte; it's unlikely that there will be much in it, but + // read it 16 bytes at a time just in case. + let mut read_buf = [0u8; 16]; + loop { + match nix::unistd::read(idle_read, &mut read_buf[..]) { + Err(nix::Error::Sys(nix::errno::Errno::EINTR)) => {} + // According to write(2), this is the outcome of reading an empty, O_NONBLOCK + // pipe. + Err(nix::Error::Sys(nix::errno::Errno::EAGAIN)) => { + break; + } + Err(e) => { + return Err(e).context("Failed to read from idle pipe"); + } + // According to write(2), this is the outcome of reading an O_NONBLOCK pipe + // when the other end has been closed. This shouldn't happen to us because we + // own both ends, but just in case. + Ok(0) => { + break; + } + Ok(_) => {} + } + } + Ok(()) +} + +/// Returns when there is an event ready to read from `conn`, or we got signalled by another thread +/// writing into our idle pipe and the `timeout` has passed. // This was taken, with minor modifications, from the xclock_utc example in the x11rb crate. // https://raw.githubusercontent.com/psychon/x11rb/a6bd1453fd8e931394b9b1f2185fad48b7cca5fe/examples/xclock_utc.rs fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> Result<(), Error> { @@ -482,10 +502,12 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> use std::os::unix::io::AsRawFd; let fd = conn.as_raw_fd(); - let mut poll_fds = [ + let mut both_poll_fds = [ PollFd::new(fd, PollFlags::POLLIN), PollFd::new(idle, PollFlags::POLLIN), ]; + let mut just_connection = [PollFd::new(fd, PollFlags::POLLIN)]; + let mut poll_fds = &mut both_poll_fds[..]; // We start with no timeout in the poll call. If we get something from the idle handler, we'll // start setting one. @@ -497,12 +519,16 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> .contains(PollFlags::POLLIN) }; - match poll(&mut poll_fds, poll_timeout) { + match poll(poll_fds, poll_timeout) { Ok(_) => { if readable(&poll_fds[0]) { break; } - if readable(&poll_fds[1]) { + if poll_fds.len() == 1 || readable(&poll_fds[1]) { + // Now that we got signalled, stop polling from the idle pipe and use a timeout + // instead. + poll_fds = &mut just_connection; + let now = Instant::now(); if now >= timeout { break; diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 06c8ef9cc0..663d67df8f 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -474,7 +474,7 @@ impl Window { /// /// ### Connection /// - /// Does not flush the connection. + /// Does not flush the connection (it gets flushed by the event loop). pub fn destroy(&self) { log_x11!(self.app.connection().destroy_window(self.id)); } @@ -596,18 +596,15 @@ impl Window { self.invalidate(); } } - self.app.connection().flush()?; Ok(()) } fn show(&self) { log_x11!(self.app.connection().map_window(self.id)); - log_x11!(self.app.connection().flush()); } fn close(&self) { self.destroy(); - log_x11!(self.app.connection().flush()); } /// Set whether the window should be resizable @@ -633,7 +630,6 @@ impl Window { self.id, xproto::Time::CurrentTime, )); - log_x11!(conn.flush()); } fn enlarge_invalid_rect(&self, rect: Rect) -> Result<(), Error> { @@ -693,7 +689,6 @@ impl Window { AtomEnum::STRING, title.as_bytes(), )); - log_x11!(self.app.connection().flush()); } fn set_menu(&self, _menu: Menu) { From c04ebcdb1dfa0d8f5666e6aabf3f72cb41bda0c4 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Mon, 13 Jul 2020 14:09:03 -0500 Subject: [PATCH 04/11] Some cleanups and comments. --- druid-shell/src/platform/x11/application.rs | 19 ++++++++++++----- druid-shell/src/platform/x11/window.rs | 23 ++++++++++++++------- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 03e00b7dce..5e81b01287 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -44,9 +44,10 @@ pub(crate) struct Application { /// /// A display is a collection of screens. connection: Rc, - /// An `XCBConnection` is *technically* safe to use from other threads, but there are lots of - /// subtleties; let's just avoid the issue altogether. As far as public API is concerned, this - /// causes `druid_shell::WindowHandle` to be `!Send` and `!Sync`. + /// An `XCBConnection` is *technically* safe to use from other threads, but there are + /// subtleties; see https://github.com/psychon/x11rb/blob/master/src/event_loop_integration.rs. + /// Let's just avoid the issue altogether. As far as public API is concerned, this causes + /// `druid_shell::WindowHandle` to be `!Send` and `!Sync`. marker: std::marker::PhantomData<*mut XCBConnection>, /// The default screen of the connected display. /// @@ -69,9 +70,11 @@ pub(crate) struct Application { window_id: u32, /// The mutable `Application` state. state: Rc>, - /// The read end of the "idle pipe", a pipe that allows the event loop to be woken up. + /// The read end of the "idle pipe", a pipe that allows the event loop to be woken up from + /// other threads. idle_read: RawFd, - /// The write end of the "idle pipe", a pipe that allows the event loop to be woken up. + /// The write end of the "idle pipe", a pipe that allows the event loop to be woken up from + /// other threads. idle_write: RawFd, /// The major opcode of the Present extension, if it is supported. present_opcode: Option, @@ -444,6 +447,12 @@ impl Application { fn finalize_quit(&self) { log_x11!(self.connection.destroy_window(self.window_id)); + if let Err(e) = nix::unistd::close(self.idle_read) { + log::error!("Error closing idle_read: {}", e); + } + if let Err(e) = nix::unistd::close(self.idle_write) { + log::error!("Error closing idle_write: {}", e); + } } pub fn clipboard(&self) -> Clipboard { diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 663d67df8f..7beee958f8 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -657,13 +657,20 @@ impl Window { Ok(()) } - /// Schedule a redraw on the idle loop. + /// Schedule a redraw on the idle loop, or if we are waiting on present then schedule it for + /// when the current present finishes. fn redraw_soon(&self) { - let idle = IdleHandle { - queue: Arc::clone(&self.idle_queue), - pipe: self.idle_pipe, - }; - idle.schedule_redraw(); + if matches!(self.waiting_on_present(), Ok(true)) { + if let Err(e) = self.set_needs_present(true) { + log::error!("Window::redraw_soon - failed to schedule present: {}", e); + } + } else { + let idle = IdleHandle { + queue: Arc::clone(&self.idle_queue), + pipe: self.idle_pipe, + }; + idle.schedule_redraw(); + } } fn invalidate(&self) { @@ -712,7 +719,7 @@ impl Window { if self.waiting_on_present()? { self.set_needs_present(true)?; } else if expose.count == 0 { - self.render()?; + self.redraw_soon(); } Ok(()) } @@ -1147,7 +1154,7 @@ fn key_mods(mods: u16) -> Modifiers { } /// A handle that can get used to schedule an idle handler. Note that -/// this handle is thread safe. +/// this handle can be cloned and sent between threads. #[derive(Clone)] pub struct IdleHandle { queue: Arc>>, From b8c2cf5b60d1accbe34b73c9c64e58f756eb4995 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Mon, 13 Jul 2020 14:22:39 -0500 Subject: [PATCH 05/11] Reduce "missed present" spam. --- druid-shell/src/platform/x11/window.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 7beee958f8..d1e5304dad 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -886,7 +886,13 @@ impl Window { } } - present.last_msc = Some(event.msc); + // Only store the last MSC if we're animating (if we aren't animating, missed MSCs + // aren't interesting). + present.last_msc = if present.needs_present { + Some(event.msc) + } else { + None + }; present.last_ust = Some(event.ust); present.waiting_on = None; } From f5f2d5b7fb8d31c192f6ea0526db9f6dc3877841 Mon Sep 17 00:00:00 2001 From: jneem Date: Tue, 14 Jul 2020 15:49:52 -0500 Subject: [PATCH 06/11] Update druid-shell/src/platform/x11/application.rs Co-authored-by: Leopold Luley --- druid-shell/src/platform/x11/application.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 5e81b01287..7477f2a6e6 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -45,7 +45,7 @@ pub(crate) struct Application { /// A display is a collection of screens. connection: Rc, /// An `XCBConnection` is *technically* safe to use from other threads, but there are - /// subtleties; see https://github.com/psychon/x11rb/blob/master/src/event_loop_integration.rs. + /// subtleties; see https://github.com/psychon/x11rb/blob/41ab6610f44f5041e112569684fc58cd6d690e57/src/event_loop_integration.rs. /// Let's just avoid the issue altogether. As far as public API is concerned, this causes /// `druid_shell::WindowHandle` to be `!Send` and `!Sync`. marker: std::marker::PhantomData<*mut XCBConnection>, From e218d8c29c5dc5f73dfae9fce558a2b3a3e3e641 Mon Sep 17 00:00:00 2001 From: jneem Date: Tue, 14 Jul 2020 15:50:05 -0500 Subject: [PATCH 07/11] Update druid-shell/src/platform/x11/application.rs Co-authored-by: Leopold Luley --- druid-shell/src/platform/x11/application.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 7477f2a6e6..12383be20a 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -504,7 +504,7 @@ fn drain_idle_pipe(idle_read: RawFd) -> Result<(), Error> { /// Returns when there is an event ready to read from `conn`, or we got signalled by another thread /// writing into our idle pipe and the `timeout` has passed. // This was taken, with minor modifications, from the xclock_utc example in the x11rb crate. -// https://raw.githubusercontent.com/psychon/x11rb/a6bd1453fd8e931394b9b1f2185fad48b7cca5fe/examples/xclock_utc.rs +// https://github.com/psychon/x11rb/blob/a6bd1453fd8e931394b9b1f2185fad48b7cca5fe/examples/xclock_utc.rs fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> Result<(), Error> { use nix::poll::{poll, PollFd, PollFlags}; use std::os::raw::c_int; From b1ad8815b33d80eb7125b545ccebea5287a08760 Mon Sep 17 00:00:00 2001 From: jneem Date: Tue, 14 Jul 2020 15:50:17 -0500 Subject: [PATCH 08/11] Update druid-shell/src/platform/x11/window.rs Co-authored-by: Leopold Luley --- druid-shell/src/platform/x11/window.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index d1e5304dad..2beb8561a5 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -660,7 +660,7 @@ impl Window { /// Schedule a redraw on the idle loop, or if we are waiting on present then schedule it for /// when the current present finishes. fn redraw_soon(&self) { - if matches!(self.waiting_on_present(), Ok(true)) { + if let Ok(true) = self.waiting_on_present() { if let Err(e) = self.set_needs_present(true) { log::error!("Window::redraw_soon - failed to schedule present: {}", e); } From c62552e2ca0b8737a795cfc4bf075dcdbb03d8af Mon Sep 17 00:00:00 2001 From: jneem Date: Tue, 14 Jul 2020 15:52:39 -0500 Subject: [PATCH 09/11] Update druid-shell/src/platform/x11/application.rs Co-authored-by: Leopold Luley --- druid-shell/src/platform/x11/application.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 12383be20a..699b155ce8 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -522,7 +522,7 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> // start setting one. let mut poll_timeout = -1; loop { - let readable = |p: &PollFd| { + fn readable(p: &PollFd) -> bool { p.revents() .unwrap_or_else(PollFlags::empty) .contains(PollFlags::POLLIN) From 3cbd3598502c27f19b26469dae6d698c0d690790 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Tue, 14 Jul 2020 16:10:38 -0500 Subject: [PATCH 10/11] Comments from the review. --- druid-shell/src/platform/x11/application.rs | 8 +++++++- druid-shell/src/platform/x11/window.rs | 4 ---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 699b155ce8..a4b36a9b27 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -371,7 +371,12 @@ impl Application { fn run_inner(self) -> Result<(), Error> { // Try to figure out the refresh rate of the current screen. We run the idle loop at that - // rate. + // rate. The rate-limiting of the idle loop has two purposes: + // - When the present extension is disabled, we paint in the idle loop. By limiting the + // idle loop to the monitor's refresh rate, we aren't painting unnecessarily. + // - By running idle commands at a limited rate, we limit spurious wake-ups: if the X11 + // connection is otherwise idle, we'll wake up at most once per frame, run *all* the + // pending idle commands, and then go back to sleep. let refresh_rate = util::refresh_rate(self.connection(), self.window_id).unwrap_or(60.0); let timeout = Duration::from_millis((1000.0 / refresh_rate) as u64); let mut last_idle_time = Instant::now(); @@ -531,6 +536,7 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> match poll(poll_fds, poll_timeout) { Ok(_) => { if readable(&poll_fds[0]) { + // There is an X11 event ready to be handled. break; } if poll_fds.len() == 1 || readable(&poll_fds[1]) { diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 2beb8561a5..10e149d437 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -471,10 +471,6 @@ impl Window { } /// Start the destruction of the window. - /// - /// ### Connection - /// - /// Does not flush the connection (it gets flushed by the event loop). pub fn destroy(&self) { log_x11!(self.app.connection().destroy_window(self.id)); } From e52def4dd84da012705bb1fdad6cbe97b212a28b Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Wed, 15 Jul 2020 08:50:47 -0500 Subject: [PATCH 11/11] Shut up, clippy. --- druid-shell/src/platform/x11/application.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index a4b36a9b27..d346c6389b 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -527,7 +527,7 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> // start setting one. let mut poll_timeout = -1; loop { - fn readable(p: &PollFd) -> bool { + fn readable(p: PollFd) -> bool { p.revents() .unwrap_or_else(PollFlags::empty) .contains(PollFlags::POLLIN) @@ -535,11 +535,11 @@ fn poll_with_timeout(conn: &Rc, idle: RawFd, timeout: Instant) -> match poll(poll_fds, poll_timeout) { Ok(_) => { - if readable(&poll_fds[0]) { + if readable(poll_fds[0]) { // There is an X11 event ready to be handled. break; } - if poll_fds.len() == 1 || readable(&poll_fds[1]) { + if poll_fds.len() == 1 || readable(poll_fds[1]) { // Now that we got signalled, stop polling from the idle pipe and use a timeout // instead. poll_fds = &mut just_connection;