From ad3d37168cda3bda2f2f979bd00acb0ec4ab2e29 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Sat, 20 Jun 2020 10:31:30 -0500 Subject: [PATCH 01/13] Start supporting Present extension. --- druid-shell/Cargo.toml | 3 +- druid-shell/examples/perftest.rs | 1 + druid-shell/src/platform/x11/application.rs | 82 +++- druid-shell/src/platform/x11/error.rs | 24 +- druid-shell/src/platform/x11/window.rs | 438 +++++++++++++++++--- 5 files changed, 490 insertions(+), 58 deletions(-) diff --git a/druid-shell/Cargo.toml b/druid-shell/Cargo.toml index b827a52779..b47e96f42f 100644 --- a/druid-shell/Cargo.toml +++ b/druid-shell/Cargo.toml @@ -39,7 +39,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 } -x11rb = { version = "0.6.0", features = ["allow-unsafe-code", "randr"], optional = true } +x11rb = { version = "0.6.0", features = ["allow-unsafe-code", "present", "randr", "xfixes"], optional = true } [target.'cfg(target_os="windows")'.dependencies] wio = "0.2.2" @@ -78,3 +78,4 @@ features = ["Window", "MouseEvent", "CssStyleDeclaration", "WheelEvent", "KeyEve [dev-dependencies] piet-common = { version = "0.1.1", features = ["png"] } +env_logger = "0.7.1" diff --git a/druid-shell/examples/perftest.rs b/druid-shell/examples/perftest.rs index 03fa39cf65..d5b8d8d7ce 100644 --- a/druid-shell/examples/perftest.rs +++ b/druid-shell/examples/perftest.rs @@ -133,6 +133,7 @@ impl WinHandler for PerfTest { } fn main() { + env_logger::init(); let app = Application::new().unwrap(); let mut builder = WindowBuilder::new(app.clone()); let perf_test = PerfTest { diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index a7e30d78c0..37e6951555 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -21,6 +21,8 @@ use std::rc::Rc; use anyhow::{anyhow, Context, Error}; use x11rb::connection::Connection; +use x11rb::protocol::present::ConnectionExt as _; +use x11rb::protocol::xfixes::ConnectionExt as _; use x11rb::protocol::xproto::{ConnectionExt, CreateWindowAux, EventMask, WindowClass}; use x11rb::protocol::Event; use x11rb::xcb_ffi::XCBConnection; @@ -60,6 +62,8 @@ pub(crate) struct Application { window_id: u32, /// The mutable `Application` state. state: Rc>, + /// The major opcode of the Present extension, if it is supported. + present_opcode: Option, } /// The mutable `Application` state. @@ -87,14 +91,69 @@ impl Application { quitting: false, windows: HashMap::new(), })); + let present_opcode = match Application::query_present_opcode(&connection) { + Ok(p) => p, + Err(e) => { + log::info!("failed to find Present extension: {}", e); + None + } + }; Ok(Application { connection, screen_num: screen_num as i32, window_id, state, + present_opcode, }) } + // Check if the Present extension is supported, returning its opcode if it is. + fn query_present_opcode(conn: &Rc) -> Result, Error> { + let query = conn + .query_extension(b"Present")? + .reply() + .context("query Present extension")?; + + if !query.present { + return Ok(None); + } + + let opcode = Some(query.major_opcode); + + // If Present is there at all, version 1.0 should be supported. This code + // shouldn't have a real effect; it's just a sanity check. + let version = conn + .present_query_version(1, 0)? + .reply() + .context("query Present version")?; + log::info!( + "X server supports Present version {}.{}", + version.major_version, + version.minor_version, + ); + + // We need the XFIXES extension to use regions. This code looks like it's just doing a + // sanity check but it is *necessary*: XFIXES doesn't work until we've done version + // negotiation + // (https://www.x.org/releases/X11R7.7/doc/fixesproto/fixesproto.txt) + let version = conn + .xfixes_query_version(5, 0)? + .reply() + .context("query XFIXES version")?; + log::info!( + "X server supports XFIXES version {}.{}", + version.major_version, + version.minor_version, + ); + + Ok(opcode) + } + + #[inline] + pub(crate) fn present_opcode(&self) -> Option { + self.present_opcode + } + fn create_event_window(conn: &Rc, screen_num: i32) -> Result { let id = conn.generate_id()?; let setup = conn.setup(); @@ -249,6 +308,27 @@ impl Application { self.finalize_quit(); } } + Event::PresentCompleteNotify(ev) => { + let w = self + .window(ev.window) + .context("COMPLETE_NOTIFY - failed to get window")?; + w.handle_complete_notify(ev) + .context("COMPLETE_NOTIFY - failed to handle")?; + } + Event::PresentIdleNotify(ev) => { + let w = self + .window(ev.window) + .context("IDLE_NOTIFY - failed to get window")?; + w.handle_idle_notify(ev) + .context("IDLE_NOTIFY - failed to handle")?; + } + // In XCB, errors are reported asynchronously by default, by sending them to the event + // loop. You can opt-out of this by using ..._checked variants of a function, and then + // getting the error synchronously. We use this in window initialization, but otherwise + // we take the async route. + Event::Error(e) => { + return Err(x11rb::errors::ReplyError::from(e.clone()).into()); + } _ => {} } Ok(false) @@ -264,7 +344,7 @@ impl Application { } } Err(e) => { - log::error!("Error handling event: {}", e); + log::error!("Error handling event: {:#}", e); } } } diff --git a/druid-shell/src/platform/x11/error.rs b/druid-shell/src/platform/x11/error.rs index dce51664f8..b8b2d46883 100644 --- a/druid-shell/src/platform/x11/error.rs +++ b/druid-shell/src/platform/x11/error.rs @@ -15,16 +15,30 @@ //! Errors at the application shell level. use std::fmt; +use std::sync::Arc; -/// The X11 backend doesn't currently define any platform-specific errors; -/// it uses the `crate::error::Other` variant instead. #[derive(Debug, Clone)] -pub enum Error {} +pub enum Error { + XError(Arc), +} impl fmt::Display for Error { - fn fmt(&self, _: &mut fmt::Formatter) -> Result<(), fmt::Error> { - Ok(()) + fn fmt(&self, f: &mut fmt::Formatter) -> Result<(), fmt::Error> { + let Error::XError(e) = self; + e.fmt(f) } } impl std::error::Error for Error {} + +impl From for Error { + fn from(err: x11rb::protocol::Error) -> Error { + Error::XError(Arc::new(x11rb::errors::ReplyError::X11Error(err))) + } +} + +impl From for Error { + fn from(err: x11rb::errors::ReplyError) -> Error { + Error::XError(Arc::new(err)) + } +} diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 24aa67fdc7..0399c88c78 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -20,13 +20,17 @@ use std::convert::TryInto; use std::rc::{Rc, Weak}; use anyhow::{anyhow, Context, Error}; -use cairo::{XCBConnection, XCBDrawable, XCBSurface, XCBVisualType}; +use cairo::{XCBConnection as CairoXCBConnection, XCBDrawable, XCBSurface, XCBVisualType}; use x11rb::atom_manager; 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, ConnectionExt, EventMask, ExposeEvent, PropMode, Visualtype, WindowClass, + self, AtomEnum, ConnectionExt, EventMask, ExposeEvent, Pixmap, PropMode, Rectangle, Visualtype, + WindowClass, }; -use x11rb::wrapper::ConnectionExt as WrapperConnectionExt; +use x11rb::wrapper::ConnectionExt as _; +use x11rb::xcb_ffi::XCBConnection; use crate::dialog::{FileDialogOptions, FileInfo}; use crate::error::Error as ShellError; @@ -135,14 +139,14 @@ impl WindowBuilder { // Replace the window's WM_PROTOCOLS with the following. let protocols = [atoms.WM_DELETE_WINDOW]; - // TODO(x11/errors): Check the response for errors? conn.change_property32( PropMode::Replace, window_id, atoms.WM_PROTOCOLS, AtomEnum::ATOM, &protocols, - )?; + )? + .check()?; Ok(atoms) } @@ -155,7 +159,7 @@ impl WindowBuilder { ) -> Result, Error> { let conn = self.app.connection(); let cairo_xcb_connection = unsafe { - XCBConnection::from_raw_none( + CairoXCBConnection::from_raw_none( conn.get_raw_xcb_connection() as *mut cairo_sys::xcb_connection_t ) }; @@ -184,9 +188,12 @@ impl WindowBuilder { let screen_num = self.app.screen_num(); let id = conn.generate_id()?; let setup = conn.setup(); - // TODO(x11/errors): Don't unwrap for screen or visual_type? - let screen = setup.roots.get(screen_num as usize).unwrap(); - let visual_type = util::get_visual_from_screen(&screen).unwrap(); + let screen = setup + .roots + .get(screen_num as usize) + .ok_or_else(|| anyhow!("Invalid screen num: {}", screen_num))?; + let visual_type = util::get_visual_from_screen(&screen) + .ok_or_else(|| anyhow!("Couldn't get visual from screen"))?; let visual_id = visual_type.visual_id; let cw_values = xproto::CreateWindowAux::new() @@ -236,7 +243,17 @@ impl WindowBuilder { let cairo_context = self.create_cairo_context(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 }); + let state = RefCell::new(WindowState { + size: self.size, + invalid: Rect::ZERO, + }); + let present_data = match self.initialize_present_data(id, screen.root_depth) { + Ok(p) => Some(p), + Err(e) => { + log::info!("Failed to initialize present extension: {}", e); + None + } + }; let handler = RefCell::new(self.handler.unwrap()); let window = Rc::new(Window { @@ -247,6 +264,7 @@ impl WindowBuilder { refresh_rate, atoms, state, + present_data: RefCell::new(present_data), }); window.set_title(&self.title); @@ -257,6 +275,40 @@ impl WindowBuilder { Ok(handle) } + + fn initialize_present_data(&self, window_id: u32, depth: u8) -> Result { + if self.app.present_opcode().is_some() { + let conn = self.app.connection(); + + // We use the COMPLETE_NOTIFY events to schedule the next frame, and the IDLE_NOTIFY + // events to manage our buffers. + let id = conn.generate_id()?; + use x11rb::protocol::present::EventMask::*; + conn.present_select_input(id, window_id, CompleteNotify | IdleNotify)? + .check() + .context("set present event mask")?; + + let region_id = conn.generate_id()?; + conn.xfixes_create_region(region_id, &[]) + .context("create region")?; + + let mut ret = PresentData { + serial: 0, + idle_pixmaps: Vec::new(), + all_pixmaps: Vec::new(), + region: region_id, + waiting_on: None, + needs_present: false, + width: self.size.width as u16, + height: self.size.height as u16, + depth, + }; + ret.create_pixmaps(&conn, window_id)?; + Ok(ret) + } else { + Err(anyhow!("no present opcode")) + } + } } /// An X11 window. @@ -268,6 +320,36 @@ pub(crate) struct Window { refresh_rate: Option, atoms: WindowAtoms, state: RefCell, + + /// 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 as long as compositing is enabled. + /// + /// The Present extension works roughly like this: we submit a pixmap for presentation. It will + /// get drawn at the next vblank, and some time shortly after that we'll get a notification + /// that the drawing was completed. + /// + /// There are three ways that rendering can get triggered: + /// 1) We render a frame, and it signals to us that an animation is requested. In this case, we + /// will render the next frame as soon as we get a notification that the just-presented + /// frame completed. In other words, we use `CompleteNotifyEvent` to schedule rendering. + /// 2) We get an expose event telling us that a region got invalidated. In + /// 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. + /// + /// 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 + /// before they appear. If we're getting lots of invalidations, it might be better to render more + /// than once per frame. Note that if we do, it will require some changes to part 1) above, + /// because if we render twice in a frame then we will get two completion notifications in a + /// row, so we don't want to present on both of them. The `msc` field of the completion + /// notification might be useful here, because it allows us to check how many frames have + /// actually been presented. + present_data: RefCell>, } // This creates a `struct WindowAtoms` containing the specified atoms as members (along with some @@ -299,6 +381,42 @@ atom_manager! { /// The mutable state of the window. struct WindowState { size: Size, + /// The region that was invalidated since the last time we rendered. + invalid: Rect, +} + +/// The state involved in using X's Present extension. +#[derive(Debug)] +struct PresentData { + /// A monotonically increasing request counter. + serial: u32, + /// A list of idle pixmaps. We take a pixmap from here for rendering and presenting. + /// + /// When we submit a pixmap to present, we're not allowed to touch it again until we get a + /// corresponding IDLE_NOTIFY event. In my limited experiments this happens shortly after + /// vsync, meaning that we may want to start rendering the next pixmap before we get the old + /// one back. Therefore, we keep a list of pixmaps. We pop one each time we render, and push + /// one when we get IDLE_NOTIFY. + /// + /// Since the current code only renders at most once per vsync, two pixmaps seems to always be + /// enough. Nevertheless, we will allocate more on the fly if we need them. Note that rendering + /// more than once per vsync can only improve latency, because only the most recently-presented + /// pixmap will get rendered. + idle_pixmaps: Vec, + /// A list of all the allocated pixmaps (including the idle ones). + all_pixmaps: Vec, + /// The region that we use for telling X what to present. + region: Region, + /// Did we submit a present that hasn't completed yet? If so, this is its serial number. + waiting_on: Option, + /// We need to render another frame as soon as the current one is done presenting. + needs_present: bool, + /// The width of the currently allocated pixmaps. + width: u16, + /// The height of the currently allocated pixmaps. + height: u16, + /// The depth of the currently allocated pixmaps. + depth: u8, } impl Window { @@ -331,18 +449,18 @@ impl Window { Ok(borrow!(self.state)?.size) } - fn set_size(&self, size: Size) -> Result<(), Error> { + fn set_size_and_depth(&self, size: Size, depth: u8) -> Result<(), Error> { // TODO(x11/dpi_scaling): detect DPI and scale size let new_size = { let mut state = borrow_mut!(self.state)?; if size != state.size { state.size = size; - Some(size) + true } else { - None + false } }; - if let Some(size) = new_size { + if new_size { self.cairo_surface()? .set_size(size.width as i32, size.height as i32) .map_err(|status| { @@ -354,41 +472,47 @@ impl Window { })?; borrow_mut!(self.handler)?.size(size); } + if let Some(present_data) = borrow_mut!(self.present_data)?.as_mut() { + present_data.resize( + self.app.connection(), + self.id, + size.width as u16, + size.height as u16, + depth, + ); + } Ok(()) } - /// Tell the X server to mark the specified `rect` as needing redraw. - /// - /// ### Connection - /// - /// Does not flush the connection. - fn request_redraw(&self, rect: Rect) { - // 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, - )); + // Ensure that our cairo context is targeting the right drawable, allocating one + // if necessary. + fn update_cairo_context(&self) -> Result<(), Error> { + if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { + let pixmap = if let Some(p) = present.idle_pixmaps.last() { + *p + } else { + present.create_pixmap(self.app.connection(), self.id)? + }; + + let cairo = self.cairo_surface()?; + let drawable = XCBDrawable(pixmap); + let state = borrow!(self.state)?; + cairo + .set_drawable(&drawable, state.size.width as i32, state.size.height as i32) + .map_err(|e| anyhow!("Failed to update cairo drawable: {}", e))?; + } + Ok(()) } fn render(&self, invalid_rect: Rect) -> Result<(), Error> { // Figure out the window's current size + // TODO: Store the size and depth instead of requiring a round-trip every frame. let reply = self.app.connection().get_geometry(self.id)?.reply()?; let size = Size::new(reply.width as f64, reply.height as f64); - self.set_size(size)?; + self.set_size_and_depth(size, reply.depth)?; let mut anim = false; + self.update_cairo_context()?; { let mut cairo_ctx = borrow_mut!(self.cairo_context)?; let mut piet_ctx = Piet::new(&mut cairo_ctx); @@ -418,18 +542,25 @@ impl Window { err?; } - if anim && self.refresh_rate.is_some() { - // TODO(x11/render_improvements): Sleeping is a terrible way to schedule redraws. - // I think I'll end up having to write a redraw scheduler or something. :| - // Doing it this way for now to proof-of-concept it. - // - // Eventually we also need to make sure we respect V-Sync timings. - // A druid-shell test utility should probably be written to verify that. - // Inspiration can be taken from: https://www.vsynctester.com/we - let sleep_amount_ms = (1000.0 / self.refresh_rate.unwrap()) as u64; - std::thread::sleep(std::time::Duration::from_millis(sleep_amount_ms)); + borrow_mut!(self.state)?.invalid = Rect::ZERO; + self.set_needs_present(false)?; - self.request_redraw(size.to_rect()); + if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { + present.present(self.app.connection(), self.id, invalid_rect)?; + if anim { + self.enlarge_invalid_rect(size.to_rect())?; + present.needs_present = true; + } + } else { + // 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. + 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()); + } } self.app.connection().flush()?; Ok(()) @@ -471,6 +602,18 @@ impl Window { log_x11!(conn.flush()); } + fn enlarge_invalid_rect(&self, rect: Rect) -> Result<(), Error> { + let invalid = &mut borrow_mut!(self.state)?.invalid; + // This is basically just a rectangle union, but we need to be careful because + // `Rect::union` doesn't do what we want when one rect is empty. + if invalid.area() == 0.0 { + *invalid = rect; + } else if rect.area() > 0.0 { + *invalid = invalid.union(rect); + } + Ok(()) + } + fn invalidate(&self) { match self.size() { Ok(size) => self.invalidate_rect(size.to_rect()), @@ -479,7 +622,30 @@ impl Window { } fn invalidate_rect(&self, rect: Rect) { - self.request_redraw(rect); + match self.enlarge_invalid_rect(rect) { + Ok(rect) => rect, + Err(err) => { + 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()); } @@ -510,7 +676,14 @@ impl Window { (expose.x as f64, expose.y as f64), (expose.width as f64, expose.height as f64), ); - Ok(self.render(rect)?) + + if self.waiting_on_present()? { + self.enlarge_invalid_rect(rect)?; + self.set_needs_present(true)?; + } else { + self.render(rect)?; + } + Ok(()) } pub fn handle_key_press(&self, key_press: &xproto::KeyPressEvent) -> Result<(), Error> { @@ -637,6 +810,169 @@ impl Window { borrow_mut!(self.handler)?.destroy(); Ok(()) } + + pub fn handle_complete_notify(&self, event: &CompleteNotifyEvent) -> Result<(), Error> { + if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { + // A little sanity check (which isn't worth an early return): we should only have + // one present request in flight, so we should only get notified about the request + // that we're waiting for. + if present.waiting_on != Some(event.serial) { + log::warn!( + "Got a notify for serial {}, but waiting on {:?}", + event.serial, + present.waiting_on + ); + } + + present.waiting_on = None; + } + + if self.needs_present()? { + let invalid = borrow!(self.state)?.invalid; + self.render(invalid)?; + } + Ok(()) + } + + pub fn handle_idle_notify(&self, event: &IdleNotifyEvent) -> Result<(), Error> { + if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { + present.idle_pixmaps.push(event.pixmap); + } + Ok(()) + } + + fn waiting_on_present(&self) -> Result { + Ok(borrow!(self.present_data)? + .as_ref() + .map(|p| p.waiting_on.is_some()) + .unwrap_or(false)) + } + + fn set_needs_present(&self, val: bool) -> Result<(), Error> { + if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { + present.needs_present = val; + } + Ok(()) + } + + fn needs_present(&self) -> Result { + Ok(borrow!(self.present_data)? + .as_ref() + .map(|p| p.needs_present) + .unwrap_or(false)) + } + + /// Frees all resources used for the Present extension, and falls back to just using EXPOSE. + // FIXME: figure out if we need this + #[allow(dead_code)] + pub fn disable_present(&self) -> Result<(), Error> { + if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { + present.destroy_x_resources(self.app.connection()); + } + *borrow_mut!(self.present_data)? = None; + Ok(()) + } +} + +impl PresentData { + /// Frees all the X resources that we hold. Note the potential for memory leaks, because this + /// is not called automatically on drop. (Maybe we should add an `Rc` to + /// `PresentData` so that we can do it on drop?) + fn destroy_x_resources(&mut self, conn: &Rc) { + self.free_pixmaps(conn); + log_x11!(conn.xfixes_destroy_region(self.region)); + } + + /// Frees all the X pixmaps that we hold. + fn free_pixmaps(&mut self, conn: &Rc) { + for &p in &self.all_pixmaps { + log_x11!(conn.free_pixmap(p)); + } + self.all_pixmaps.clear(); + self.idle_pixmaps.clear(); + } + + fn resize( + &mut self, + conn: &Rc, + window_id: u32, + width: u16, + height: u16, + depth: u8, + ) { + // TODO: we can avoid reallocating so frequently by allowing "oversize" pixmaps. + if (width, height, depth) != (self.width, self.height, self.depth) { + self.free_pixmaps(conn); + self.width = width; + self.height = height; + self.depth = depth; + log_x11!(self.create_pixmaps(conn, window_id)); + } + } + + /// Creates a new pixmap for rendering to. The new pixmap will be first in line for rendering. + fn create_pixmap(&mut self, conn: &Rc, window_id: u32) -> Result { + let pixmap_id = conn.generate_id()?; + conn.create_pixmap(self.depth, pixmap_id, window_id, self.width, self.height)?; + self.all_pixmaps.push(pixmap_id); + self.idle_pixmaps.push(pixmap_id); + Ok(pixmap_id) + } + + fn create_pixmaps(&mut self, conn: &Rc, window_id: u32) -> Result<(), Error> { + if !self.all_pixmaps.is_empty() { + log::error!("BUG: need to free the pixmaps before creating new ones"); + self.free_pixmaps(conn); + } + + // Two buffers is probably enough. We'll create more on demand. + self.create_pixmap(conn, window_id)?; + self.create_pixmap(conn, window_id)?; + Ok(()) + } + + // We have already rendered into the active pixmap buffer. Present it to the + // X server, and then rotate the buffers. + fn present( + &mut self, + conn: &Rc, + window_id: u32, + rect: Rect, + ) -> Result<(), Error> { + let x_rect = Rectangle { + x: rect.x0 as i16, + y: rect.y0 as i16, + width: rect.width() as u16, + height: rect.height() as u16, + }; + + let pixmap = self + .idle_pixmaps + .pop() + .ok_or_else(|| anyhow!("PresentData::present didn't have a pixmap available"))?; + + log_x11!(conn.xfixes_set_region(self.region, &[x_rect])); + log_x11!(conn.present_pixmap( + window_id, + pixmap, + self.serial, + self.region, + self.region, + 0, + 0, + x11rb::NONE, + x11rb::NONE, + x11rb::NONE, + x11rb::protocol::present::Option::None.into(), + 0, + 1, + 0, + &[], + )); + self.waiting_on = Some(self.serial); + self.serial += 1; + Ok(()) + } } // Converts from, e.g., the `details` field of `xcb::xproto::ButtonPressEvent` From 9b08ae2358074635ce741eed60a083af68f499d9 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Sat, 20 Jun 2020 10:52:12 -0500 Subject: [PATCH 02/13] Reinstate the present-disabling fallback. --- druid-shell/Cargo.toml | 2 +- druid-shell/examples/perftest.rs | 2 +- druid-shell/src/platform/x11/application.rs | 7 +++++++ druid-shell/src/platform/x11/window.rs | 2 -- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/druid-shell/Cargo.toml b/druid-shell/Cargo.toml index b47e96f42f..12629a41cd 100644 --- a/druid-shell/Cargo.toml +++ b/druid-shell/Cargo.toml @@ -78,4 +78,4 @@ features = ["Window", "MouseEvent", "CssStyleDeclaration", "WheelEvent", "KeyEve [dev-dependencies] piet-common = { version = "0.1.1", features = ["png"] } -env_logger = "0.7.1" +simple_logger = { version = "1.6.0", default-features = false } diff --git a/druid-shell/examples/perftest.rs b/druid-shell/examples/perftest.rs index d5b8d8d7ce..5159e0c2f1 100644 --- a/druid-shell/examples/perftest.rs +++ b/druid-shell/examples/perftest.rs @@ -133,7 +133,7 @@ impl WinHandler for PerfTest { } fn main() { - env_logger::init(); + simple_logger::init().expect("Failed to init simple logger"); let app = Application::new().unwrap(); let mut builder = WindowBuilder::new(app.clone()); let perf_test = PerfTest { diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 37e6951555..bc9066f308 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -327,6 +327,13 @@ impl Application { // getting the error synchronously. We use this in window initialization, but otherwise // we take the async route. Event::Error(e) => { + if let x11rb::protocol::Error::Request(req) = e { + if self.present_opcode == Some(req.major_opcode) { + for window in borrow!(self.state)?.windows.values() { + window.disable_present()?; + } + } + } return Err(x11rb::errors::ReplyError::from(e.clone()).into()); } _ => {} diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 0399c88c78..ae6be1094c 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -863,8 +863,6 @@ impl Window { } /// Frees all resources used for the Present extension, and falls back to just using EXPOSE. - // FIXME: figure out if we need this - #[allow(dead_code)] pub fn disable_present(&self) -> Result<(), Error> { if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { present.destroy_x_resources(self.app.connection()); From 8033eba4c6a0512594daf571f06dea0555a86bcc Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Sat, 20 Jun 2020 13:44:37 -0500 Subject: [PATCH 03/13] Some work on pixmap buffer allocation, but there's glitching on resize. --- druid-shell/src/platform/x11/window.rs | 252 ++++++++++++++++--------- 1 file changed, 159 insertions(+), 93 deletions(-) diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index ae6be1094c..76dc6b85cb 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -26,8 +26,8 @@ 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, ConnectionExt, EventMask, ExposeEvent, Pixmap, PropMode, Rectangle, Visualtype, - WindowClass, + self, AtomEnum, ConnectionExt, CreateGCAux, EventMask, ExposeEvent, Gcontext, Pixmap, PropMode, + Rectangle, Visualtype, WindowClass, }; use x11rb::wrapper::ConnectionExt as _; use x11rb::xcb_ffi::XCBConnection; @@ -151,12 +151,11 @@ impl WindowBuilder { Ok(atoms) } - /// Create a new cairo `Context`. - fn create_cairo_context( + fn create_cairo_surface( &self, window_id: u32, visual_type: &Visualtype, - ) -> Result, Error> { + ) -> Result { let conn = self.app.connection(); let cairo_xcb_connection = unsafe { CairoXCBConnection::from_raw_none( @@ -178,8 +177,7 @@ impl WindowBuilder { self.size.height as i32, ) .map_err(|status| anyhow!("Failed to create cairo surface: {}", status))?; - - Ok(RefCell::new(cairo::Context::new(&cairo_surface))) + Ok(cairo_surface) } // TODO(x11/menus): make menus if requested @@ -209,6 +207,7 @@ impl WindowBuilder { ); // Create the actual window + let (width, height) = (self.size.width as u16, self.size.height as u16); conn.create_window( // Window depth x11rb::COPY_FROM_PARENT.try_into().unwrap(), @@ -223,10 +222,10 @@ impl WindowBuilder { 0, // Width of the new window // TODO(x11/dpi_scaling): figure out DPI scaling - self.size.width as u16, + width, // Height of the new window // TODO(x11/dpi_scaling): figure out DPI scaling - self.size.height as u16, + height, // Border width 0, // Window class type @@ -238,16 +237,21 @@ impl WindowBuilder { )? .check()?; + // Allocate a graphics context (currently used only for copying pixels when present is + // unavailable). + let gc = conn.generate_id()?; + conn.create_gc(gc, id, &CreateGCAux::new())?.check()?; + // TODO(x11/errors): Should do proper cleanup (window destruction etc) in case of error let atoms = self.atoms(id)?; - let cairo_context = self.create_cairo_context(id, &visual_type)?; + 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, }); - let present_data = match self.initialize_present_data(id, screen.root_depth) { + let present_data = match self.initialize_present_data(id) { Ok(p) => Some(p), Err(e) => { log::info!("Failed to initialize present extension: {}", e); @@ -255,16 +259,30 @@ impl WindowBuilder { } }; let handler = RefCell::new(self.handler.unwrap()); + // When using present, we generally need two buffers (because after we present, we aren't + // allowed to use that buffer for a little while, and so we might want to render to the + // other one). Otherwise, we only need one. + let buf_count = if present_data.is_some() { 2 } else { 1 }; + let buffers = RefCell::new(Buffers::new( + conn, + id, + buf_count, + width, + height, + screen.root_depth, + )?); let window = Rc::new(Window { id, + gc, app: self.app.clone(), handler, - cairo_context, + cairo_surface, refresh_rate, atoms, state, present_data: RefCell::new(present_data), + buffers, }); window.set_title(&self.title); @@ -276,7 +294,7 @@ impl WindowBuilder { Ok(handle) } - fn initialize_present_data(&self, window_id: u32, depth: u8) -> Result { + fn initialize_present_data(&self, window_id: u32) -> Result { if self.app.present_opcode().is_some() { let conn = self.app.connection(); @@ -292,19 +310,12 @@ impl WindowBuilder { conn.xfixes_create_region(region_id, &[]) .context("create region")?; - let mut ret = PresentData { + Ok(PresentData { serial: 0, - idle_pixmaps: Vec::new(), - all_pixmaps: Vec::new(), region: region_id, waiting_on: None, needs_present: false, - width: self.size.width as u16, - height: self.size.height as u16, - depth, - }; - ret.create_pixmaps(&conn, window_id)?; - Ok(ret) + }) } else { Err(anyhow!("no present opcode")) } @@ -314,9 +325,10 @@ impl WindowBuilder { /// An X11 window. pub(crate) struct Window { id: u32, + gc: Gcontext, app: Application, handler: RefCell>, - cairo_context: RefCell, + cairo_surface: RefCell, refresh_rate: Option, atoms: WindowAtoms, state: RefCell, @@ -350,6 +362,7 @@ pub(crate) struct Window { /// notification might be useful here, because it allows us to check how many frames have /// actually been presented. present_data: RefCell>, + buffers: RefCell, } // This creates a `struct WindowAtoms` containing the specified atoms as members (along with some @@ -385,12 +398,15 @@ struct WindowState { invalid: Rect, } -/// The state involved in using X's Present extension. -#[derive(Debug)] -struct PresentData { - /// A monotonically increasing request counter. - serial: u32, - /// A list of idle pixmaps. We take a pixmap from here for rendering and presenting. +/// A collection of pixmaps for rendering to. This gets used in two different ways: if the present +/// extension is enabled, we render to a pixmap and then present it. If the present extension is +/// disabled, we render to a pixmap and then call `copy_area` on it (this probably isn't the best +/// way to imitate double buffering, but it's the fallback anyway). +struct Buffers { + /// A list of idle pixmaps. We take a pixmap from here for rendering to. + /// + /// When we're not using the present extension, all pixmaps belong in here; as soon as we copy + /// from one, we can use it again. /// /// When we submit a pixmap to present, we're not allowed to touch it again until we get a /// corresponding IDLE_NOTIFY event. In my limited experiments this happens shortly after @@ -405,18 +421,25 @@ struct PresentData { idle_pixmaps: Vec, /// A list of all the allocated pixmaps (including the idle ones). all_pixmaps: Vec, + /// The sizes of the pixmaps (they all have the same size). In order to avoid repeatedly + /// reallocating as the window size changes, we allow these to be bigger than the window. + width: u16, + height: u16, + /// The depth of the currently allocated pixmaps. + depth: u8, +} + +/// The state involved in using X's Present extension. +#[derive(Debug)] +struct PresentData { + /// A monotonically increasing request counter. + serial: u32, /// The region that we use for telling X what to present. region: Region, /// Did we submit a present that hasn't completed yet? If so, this is its serial number. waiting_on: Option, /// We need to render another frame as soon as the current one is done presenting. needs_present: bool, - /// The width of the currently allocated pixmaps. - width: u16, - /// The height of the currently allocated pixmaps. - height: u16, - /// The depth of the currently allocated pixmaps. - depth: u8, } impl Window { @@ -438,13 +461,6 @@ impl Window { log_x11!(self.app.connection().destroy_window(self.id)); } - fn cairo_surface(&self) -> Result { - borrow!(self.cairo_context)? - .get_target() - .try_into() - .map_err(|_| anyhow!("Window::cairo_surface try_into")) - } - fn size(&self) -> Result { Ok(borrow!(self.state)?.size) } @@ -461,7 +477,14 @@ impl Window { } }; if new_size { - self.cairo_surface()? + borrow_mut!(self.buffers)?.set_size_and_depth( + self.app.connection(), + self.id, + size.width as u16, + size.height as u16, + depth, + ); + borrow_mut!(self.cairo_surface)? .set_size(size.width as i32, size.height as i32) .map_err(|status| { anyhow!( @@ -472,35 +495,22 @@ impl Window { })?; borrow_mut!(self.handler)?.size(size); } - if let Some(present_data) = borrow_mut!(self.present_data)?.as_mut() { - present_data.resize( - self.app.connection(), - self.id, - size.width as u16, - size.height as u16, - depth, - ); - } Ok(()) } - // Ensure that our cairo context is targeting the right drawable, allocating one - // if necessary. - fn update_cairo_context(&self) -> Result<(), Error> { - if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { - let pixmap = if let Some(p) = present.idle_pixmaps.last() { - *p - } else { - present.create_pixmap(self.app.connection(), self.id)? - }; + // Ensure that our cairo context is targeting the right drawable, allocating one if necessary. + fn update_cairo_surface(&self) -> Result<(), Error> { + let mut buffers = borrow_mut!(self.buffers)?; + let pixmap = if let Some(p) = buffers.idle_pixmaps.last() { + *p + } else { + buffers.create_pixmap(self.app.connection(), self.id)? + }; - let cairo = self.cairo_surface()?; - let drawable = XCBDrawable(pixmap); - let state = borrow!(self.state)?; - cairo - .set_drawable(&drawable, state.size.width as i32, state.size.height as i32) - .map_err(|e| anyhow!("Failed to update cairo drawable: {}", e))?; - } + let drawable = XCBDrawable(pixmap); + borrow_mut!(self.cairo_surface)? + .set_drawable(&drawable, buffers.width as i32, buffers.height as i32) + .map_err(|e| anyhow!("Failed to update cairo drawable: {}", e))?; Ok(()) } @@ -512,9 +522,10 @@ impl Window { self.set_size_and_depth(size, reply.depth)?; let mut anim = false; - self.update_cairo_context()?; + self.update_cairo_surface()?; { - let mut cairo_ctx = borrow_mut!(self.cairo_context)?; + let surface = borrow!(self.cairo_surface)?; + let mut cairo_ctx = cairo::Context::new(&surface); let mut piet_ctx = Piet::new(&mut cairo_ctx); piet_ctx.clip(invalid_rect); @@ -545,13 +556,24 @@ impl Window { borrow_mut!(self.state)?.invalid = Rect::ZERO; self.set_needs_present(false)?; + let mut buffers = borrow_mut!(self.buffers)?; + let pixmap = *buffers + .idle_pixmaps + .last() + .ok_or_else(|| anyhow!("after rendering, no pixmap to present"))?; if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { - present.present(self.app.connection(), self.id, invalid_rect)?; + present.present(self.app.connection(), pixmap, self.id, invalid_rect)?; + buffers.idle_pixmaps.pop(); if anim { self.enlarge_invalid_rect(size.to_rect())?; present.needs_present = true; } } else { + let (x, y) = (invalid_rect.x0 as i16, invalid_rect.y0 as i16); + let (w, h) = (invalid_rect.width() as u16, invalid_rect.height() as u16); + 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. @@ -835,9 +857,7 @@ impl Window { } pub fn handle_idle_notify(&self, event: &IdleNotifyEvent) -> Result<(), Error> { - if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { - present.idle_pixmaps.push(event.pixmap); - } + borrow_mut!(self.buffers)?.idle_pixmaps.push(event.pixmap); Ok(()) } @@ -872,13 +892,24 @@ impl Window { } } -impl PresentData { - /// Frees all the X resources that we hold. Note the potential for memory leaks, because this - /// is not called automatically on drop. (Maybe we should add an `Rc` to - /// `PresentData` so that we can do it on drop?) - fn destroy_x_resources(&mut self, conn: &Rc) { - self.free_pixmaps(conn); - log_x11!(conn.xfixes_destroy_region(self.region)); +impl Buffers { + fn new( + conn: &Rc, + window_id: u32, + buf_count: usize, + width: u16, + height: u16, + depth: u8, + ) -> Result { + let mut ret = Buffers { + width, + height, + depth, + idle_pixmaps: Vec::new(), + all_pixmaps: Vec::new(), + }; + ret.create_pixmaps(conn, window_id, buf_count)?; + Ok(ret) } /// Frees all the X pixmaps that we hold. @@ -890,7 +921,7 @@ impl PresentData { self.idle_pixmaps.clear(); } - fn resize( + fn set_size_and_depth( &mut self, conn: &Rc, window_id: u32, @@ -898,13 +929,26 @@ impl PresentData { height: u16, depth: u8, ) { - // TODO: we can avoid reallocating so frequently by allowing "oversize" pixmaps. + // How big should the buffer be if we want at least x pixels? Rounding up to the next power + // of 2 has the potential to waste 75% of our memory (factor 2 in both directions), so + // instead we round up to the nearest number of the form 2^k or 3 * 2^k. + fn next_size(x: u16) -> u16 { + // We round up to the nearest multiple of `accuracy`, which is between x/2 and x/4. + // Don't bother rounding to anything smaller than 32 = 2^(7-1). + let accuracy = 1 << ((16 - x.leading_zeros()).max(7) - 2); + let mask = accuracy - 1; + (x + mask) & !mask + } + + let width = next_size(width); + let height = next_size(height); if (width, height, depth) != (self.width, self.height, self.depth) { + let count = self.all_pixmaps.len(); self.free_pixmaps(conn); self.width = width; self.height = height; self.depth = depth; - log_x11!(self.create_pixmaps(conn, window_id)); + log_x11!(self.create_pixmaps(conn, window_id, count)); } } @@ -917,23 +961,38 @@ impl PresentData { Ok(pixmap_id) } - fn create_pixmaps(&mut self, conn: &Rc, window_id: u32) -> Result<(), Error> { + fn create_pixmaps( + &mut self, + conn: &Rc, + window_id: u32, + count: usize, + ) -> Result<(), Error> { if !self.all_pixmaps.is_empty() { log::error!("BUG: need to free the pixmaps before creating new ones"); self.free_pixmaps(conn); } - // Two buffers is probably enough. We'll create more on demand. - self.create_pixmap(conn, window_id)?; - self.create_pixmap(conn, window_id)?; + for _ in 0..count { + self.create_pixmap(conn, window_id)?; + } Ok(()) } +} + +impl PresentData { + /// Frees all the X resources that we hold. Note the potential for memory leaks, because this + /// is not called automatically on drop. (Maybe we should add an `Rc` to + /// `PresentData` so that we can do it on drop?) + fn destroy_x_resources(&mut self, conn: &Rc) { + log_x11!(conn.xfixes_destroy_region(self.region)); + } // We have already rendered into the active pixmap buffer. Present it to the // X server, and then rotate the buffers. fn present( &mut self, conn: &Rc, + pixmap: Pixmap, window_id: u32, rect: Rect, ) -> Result<(), Error> { @@ -944,27 +1003,34 @@ impl PresentData { height: rect.height() as u16, }; - let pixmap = self - .idle_pixmaps - .pop() - .ok_or_else(|| anyhow!("PresentData::present didn't have a pixmap available"))?; - log_x11!(conn.xfixes_set_region(self.region, &[x_rect])); log_x11!(conn.present_pixmap( window_id, pixmap, self.serial, + // valid region of the pixmap self.region, + // region of the window that must get updated self.region, + // window-relative x-offset of the pixmap 0, + // window-relative y-offset of the pixmap 0, + // target CRTC x11rb::NONE, + // wait fence x11rb::NONE, + // idle fence x11rb::NONE, + // present options x11rb::protocol::present::Option::None.into(), + // target msc (0 means present at the next time that msc % divisor == remainder) 0, + // divisor 1, + // remainder 0, + // notifies &[], )); self.waiting_on = Some(self.serial); From 84941f24ddb0c4df642286b3b3db1c08488f8eb6 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Sat, 20 Jun 2020 16:22:06 -0500 Subject: [PATCH 04/13] Get size changes from ConfigureNotify. --- druid-shell/src/platform/x11/application.rs | 7 ++ druid-shell/src/platform/x11/window.rs | 78 ++++++++++----------- 2 files changed, 45 insertions(+), 40 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index bc9066f308..27bc01f545 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -308,6 +308,13 @@ impl Application { self.finalize_quit(); } } + Event::ConfigureNotify(ev) => { + let w = self + .window(ev.window) + .context("CONFIGURE_NOTIFY - failed to get window")?; + w.handle_configure_notify(ev) + .context("CONFIGURE_NOTIFY - failed to handle")?; + } Event::PresentCompleteNotify(ev) => { let w = self .window(ev.window) diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 76dc6b85cb..4eced17f40 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -26,8 +26,8 @@ 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, ConnectionExt, CreateGCAux, EventMask, ExposeEvent, Gcontext, Pixmap, PropMode, - Rectangle, Visualtype, WindowClass, + self, AtomEnum, ConfigureNotifyEvent, ConnectionExt, CreateGCAux, EventMask, ExposeEvent, + Gcontext, Pixmap, PropMode, Rectangle, Visualtype, WindowClass, }; use x11rb::wrapper::ConnectionExt as _; use x11rb::xcb_ffi::XCBConnection; @@ -194,17 +194,16 @@ impl WindowBuilder { .ok_or_else(|| anyhow!("Couldn't get visual from screen"))?; let visual_id = visual_type.visual_id; - let cw_values = xproto::CreateWindowAux::new() - .background_pixel(screen.white_pixel) - .event_mask( - EventMask::Exposure - | EventMask::StructureNotify - | EventMask::KeyPress - | EventMask::KeyRelease - | EventMask::ButtonPress - | EventMask::ButtonRelease - | EventMask::PointerMotion, - ); + let cw_values = xproto::CreateWindowAux::new().event_mask( + EventMask::Exposure + | EventMask::StructureNotify + | EventMask::KeyPress + | EventMask::KeyRelease + | EventMask::ButtonPress + | EventMask::ButtonRelease + | EventMask::PointerMotion + | EventMask::StructureNotify, + ); // Create the actual window let (width, height) = (self.size.width as u16, self.size.height as u16); @@ -465,7 +464,7 @@ impl Window { Ok(borrow!(self.state)?.size) } - fn set_size_and_depth(&self, size: Size, depth: u8) -> Result<(), Error> { + fn set_size(&self, size: Size) -> Result<(), Error> { // TODO(x11/dpi_scaling): detect DPI and scale size let new_size = { let mut state = borrow_mut!(self.state)?; @@ -477,12 +476,11 @@ impl Window { } }; if new_size { - borrow_mut!(self.buffers)?.set_size_and_depth( + borrow_mut!(self.buffers)?.set_size( self.app.connection(), self.id, size.width as u16, size.height as u16, - depth, ); borrow_mut!(self.cairo_surface)? .set_size(size.width as i32, size.height as i32) @@ -493,6 +491,7 @@ impl Window { status ) })?; + self.enlarge_invalid_rect(size.to_rect())?; borrow_mut!(self.handler)?.size(size); } Ok(()) @@ -514,13 +513,9 @@ impl Window { Ok(()) } - fn render(&self, invalid_rect: Rect) -> Result<(), Error> { - // Figure out the window's current size - // TODO: Store the size and depth instead of requiring a round-trip every frame. - let reply = self.app.connection().get_geometry(self.id)?.reply()?; - let size = Size::new(reply.width as f64, reply.height as f64); - self.set_size_and_depth(size, reply.depth)?; - + fn render(&self) -> Result<(), Error> { + let size = borrow!(self.state)?.size; + let invalid_rect = borrow!(self.state)?.invalid; let mut anim = false; self.update_cairo_surface()?; { @@ -699,11 +694,11 @@ impl Window { (expose.width as f64, expose.height as f64), ); + self.enlarge_invalid_rect(rect)?; if self.waiting_on_present()? { - self.enlarge_invalid_rect(rect)?; self.set_needs_present(true)?; - } else { - self.render(rect)?; + } else if expose.count == 0 { + self.render()?; } Ok(()) } @@ -833,6 +828,10 @@ impl Window { Ok(()) } + pub fn handle_configure_notify(&self, event: &ConfigureNotifyEvent) -> Result<(), Error> { + self.set_size(Size::new(event.width as f64, event.height as f64)) + } + pub fn handle_complete_notify(&self, event: &CompleteNotifyEvent) -> Result<(), Error> { if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { // A little sanity check (which isn't worth an early return): we should only have @@ -850,14 +849,19 @@ impl Window { } if self.needs_present()? { - let invalid = borrow!(self.state)?.invalid; - self.render(invalid)?; + self.render()?; } Ok(()) } pub fn handle_idle_notify(&self, event: &IdleNotifyEvent) -> Result<(), Error> { - borrow_mut!(self.buffers)?.idle_pixmaps.push(event.pixmap); + let mut buffers = borrow_mut!(self.buffers)?; + if buffers.all_pixmaps.contains(&event.pixmap) { + buffers.idle_pixmaps.push(event.pixmap); + } else { + // We must have reallocated the buffers while this pixmap was busy, so free it now. + self.app.connection().free_pixmap(event.pixmap)?; + } Ok(()) } @@ -914,21 +918,16 @@ impl Buffers { /// Frees all the X pixmaps that we hold. fn free_pixmaps(&mut self, conn: &Rc) { - for &p in &self.all_pixmaps { + // We can't touch pixmaps if the present extension is waiting on them, so only free the + // idle ones. We'll free the busy ones when we get notified that they're idle. + for &p in &self.idle_pixmaps { log_x11!(conn.free_pixmap(p)); } self.all_pixmaps.clear(); self.idle_pixmaps.clear(); } - fn set_size_and_depth( - &mut self, - conn: &Rc, - window_id: u32, - width: u16, - height: u16, - depth: u8, - ) { + fn set_size(&mut self, conn: &Rc, window_id: u32, width: u16, height: u16) { // How big should the buffer be if we want at least x pixels? Rounding up to the next power // of 2 has the potential to waste 75% of our memory (factor 2 in both directions), so // instead we round up to the nearest number of the form 2^k or 3 * 2^k. @@ -942,12 +941,11 @@ impl Buffers { let width = next_size(width); let height = next_size(height); - if (width, height, depth) != (self.width, self.height, self.depth) { + if (width, height) != (self.width, self.height) { let count = self.all_pixmaps.len(); self.free_pixmaps(conn); self.width = width; self.height = height; - self.depth = depth; log_x11!(self.create_pixmaps(conn, window_id, count)); } } From 09957ab4f06896d8c1c0e962b48e4f4ad4082d9f Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Sat, 20 Jun 2020 16:57:50 -0500 Subject: [PATCH 05/13] Fix issues with ConfigureNotify --- druid-shell/src/platform/x11/application.rs | 12 +++++++----- druid-shell/src/platform/x11/window.rs | 3 +-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 27bc01f545..b3abaacec9 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -309,11 +309,13 @@ impl Application { } } Event::ConfigureNotify(ev) => { - let w = self - .window(ev.window) - .context("CONFIGURE_NOTIFY - failed to get window")?; - w.handle_configure_notify(ev) - .context("CONFIGURE_NOTIFY - failed to handle")?; + if ev.window != self.window_id { + let w = self + .window(ev.window) + .context("CONFIGURE_NOTIFY - failed to get window")?; + w.handle_configure_notify(ev) + .context("CONFIGURE_NOTIFY - failed to handle")?; + } } Event::PresentCompleteNotify(ev) => { let w = self diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 4eced17f40..376096d4fa 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -201,8 +201,7 @@ impl WindowBuilder { | EventMask::KeyRelease | EventMask::ButtonPress | EventMask::ButtonRelease - | EventMask::PointerMotion - | EventMask::StructureNotify, + | EventMask::PointerMotion, ); // Create the actual window From 582a7e462e4c40df14e110bcd601af658a3df495 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Sun, 21 Jun 2020 09:21:46 -0500 Subject: [PATCH 06/13] Track missed presents. --- druid-shell/src/platform/x11/window.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 376096d4fa..ff93b1fec7 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -313,6 +313,8 @@ impl WindowBuilder { region: region_id, waiting_on: None, needs_present: false, + last_msc: None, + last_ust: None, }) } else { Err(anyhow!("no present opcode")) @@ -438,6 +440,12 @@ struct PresentData { waiting_on: Option, /// We need to render another frame as soon as the current one is done presenting. needs_present: bool, + /// The last MSC that was completed. This can be used to diagnose latency problems, because MSC + /// is a frame counter: we should be presenting on every frame, and storing the last completed + /// MSC lets us know if we missed one. + last_msc: Option, + /// The time at which the last frame was completed. + last_ust: Option, } impl Window { @@ -844,6 +852,22 @@ 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::warn!( + "missed a present: msc went from {} to {}", + last_msc, + event.msc + ); + if let Some(last_ust) = present.last_ust { + log::warn!("ust went from {} to {}", last_ust, event.ust); + } + } + } + + present.last_msc = Some(event.msc); + present.last_ust = Some(event.ust); present.waiting_on = None; } From 4175a2dba953e8d479d245da7096cb071d364366 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Sun, 21 Jun 2020 09:29:01 -0500 Subject: [PATCH 07/13] Add an environment variable for testing. --- druid-shell/src/platform/x11/application.rs | 17 ++++++++++++----- druid-shell/src/platform/x11/window.rs | 2 ++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index b3abaacec9..510abf8b97 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -91,13 +91,20 @@ impl Application { quitting: false, windows: HashMap::new(), })); - let present_opcode = match Application::query_present_opcode(&connection) { - Ok(p) => p, - Err(e) => { - log::info!("failed to find Present extension: {}", e); - None + + let present_opcode = if std::env::var_os("DRUID_SHELL_DISABLE_X11_PRESENT").is_some() { + // Allow disabling Present with an environment variable. + None + } else { + match Application::query_present_opcode(&connection) { + Ok(p) => p, + Err(e) => { + log::info!("failed to find Present extension: {}", e); + None + } } }; + Ok(Application { connection, screen_num: screen_num as i32, diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index ff93b1fec7..d912886e15 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -579,6 +579,8 @@ impl Window { // 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)); From 1d4f17d63b1d906959dc5b45c359173567194f0f Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Sun, 21 Jun 2020 18:33:57 -0500 Subject: [PATCH 08/13] Cleanups, comments, error handling. --- druid-shell/src/platform/x11/application.rs | 25 +++++------ druid-shell/src/platform/x11/window.rs | 46 ++++++++------------- 2 files changed, 30 insertions(+), 41 deletions(-) diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 510abf8b97..2664989d7d 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -116,6 +116,11 @@ impl Application { // Check if the Present extension is supported, returning its opcode if it is. fn query_present_opcode(conn: &Rc) -> Result, Error> { + // When checking for X11 errors synchronously, there are two places where the error could + // happen. An error on the request means the connection is broken. There's no need for + // extra error context here, because the fact that the connection broke has nothing to do + // with what we're trying to do. An error on the reply means there was something wrong with + // the request, and so we add context. This convention is used throughout the x11 backend. let query = conn .query_extension(b"Present")? .reply() @@ -194,7 +199,8 @@ impl Application { // Window properties mask &CreateWindowAux::new().event_mask(EventMask::StructureNotify), )? - .check()?; + .check() + .context("create input-only window")?; Ok(id) } @@ -237,7 +243,7 @@ impl Application { // to know if the event must be ignored. // Otherwise there will be a "failed to get window" error. // - // CIRCULATE_NOTIFY, CONFIGURE_NOTIFY, GRAVITY_NOTIFY + // CIRCULATE_NOTIFY, GRAVITY_NOTIFY // MAP_NOTIFY, REPARENT_NOTIFY, UNMAP_NOTIFY Event::Expose(ev) => { let w = self @@ -339,17 +345,12 @@ impl Application { .context("IDLE_NOTIFY - failed to handle")?; } // In XCB, errors are reported asynchronously by default, by sending them to the event - // loop. You can opt-out of this by using ..._checked variants of a function, and then - // getting the error synchronously. We use this in window initialization, but otherwise - // we take the async route. + // loop. You can also request a synchronous error for a given call; we use this in + // window initialization, but otherwise we take the async route. Event::Error(e) => { - if let x11rb::protocol::Error::Request(req) = e { - if self.present_opcode == Some(req.major_opcode) { - for window in borrow!(self.state)?.windows.values() { - window.disable_present()?; - } - } - } + // TODO: if an error is caused by the present extension, disable it and fall back + // to copying pixels. This is blocked on + // https://github.com/psychon/x11rb/issues/503 return Err(x11rb::errors::ReplyError::from(e.clone()).into()); } _ => {} diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index d912886e15..17d8ea2d2e 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -132,10 +132,9 @@ impl WindowBuilder { /// Registers and returns all the atoms that the window will need. fn atoms(&self, window_id: u32) -> Result { let conn = self.app.connection(); - let atoms = WindowAtoms::new(conn.as_ref()) - .context("failed to intern X11 atoms")? + let atoms = WindowAtoms::new(conn.as_ref())? .reply() - .context("failed to get back X11 atoms")?; + .context("get X11 atoms")?; // Replace the window's WM_PROTOCOLS with the following. let protocols = [atoms.WM_DELETE_WINDOW]; @@ -146,7 +145,8 @@ impl WindowBuilder { AtomEnum::ATOM, &protocols, )? - .check()?; + .check() + .context("set WM_PROTOCOLS")?; Ok(atoms) } @@ -233,12 +233,15 @@ impl WindowBuilder { // Window properties mask &cw_values, )? - .check()?; + .check() + .context("create window")?; // Allocate a graphics context (currently used only for copying pixels when present is // unavailable). let gc = conn.generate_id()?; - conn.create_gc(gc, id, &CreateGCAux::new())?.check()?; + conn.create_gc(gc, id, &CreateGCAux::new())? + .check() + .context("create graphics context")?; // TODO(x11/errors): Should do proper cleanup (window destruction etc) in case of error let atoms = self.atoms(id)?; @@ -296,7 +299,7 @@ impl WindowBuilder { if self.app.present_opcode().is_some() { let conn = self.app.connection(); - // We use the COMPLETE_NOTIFY events to schedule the next frame, and the IDLE_NOTIFY + // We use the CompleteNotify events to schedule the next frame, and the IdleNotify // events to manage our buffers. let id = conn.generate_id()?; use x11rb::protocol::present::EventMask::*; @@ -334,7 +337,8 @@ pub(crate) struct Window { state: RefCell, /// 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 as long as compositing is enabled. + /// presentation to vblank and it appears to prevent tearing (subject to various caveats + /// regarding broken video drivers). /// /// The Present extension works roughly like this: we submit a pixmap for presentation. It will /// get drawn at the next vblank, and some time shortly after that we'll get a notification @@ -857,13 +861,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::warn!( + log::info!( "missed a present: msc went from {} to {}", last_msc, event.msc ); if let Some(last_ust) = present.last_ust { - log::warn!("ust went from {} to {}", last_ust, event.ust); + log::info!("ust went from {} to {}", last_ust, event.ust); } } } @@ -910,15 +914,6 @@ impl Window { .map(|p| p.needs_present) .unwrap_or(false)) } - - /// Frees all resources used for the Present extension, and falls back to just using EXPOSE. - pub fn disable_present(&self) -> Result<(), Error> { - if let Some(present) = borrow_mut!(self.present_data)?.as_mut() { - present.destroy_x_resources(self.app.connection()); - } - *borrow_mut!(self.present_data)? = None; - Ok(()) - } } impl Buffers { @@ -1003,13 +998,6 @@ impl Buffers { } impl PresentData { - /// Frees all the X resources that we hold. Note the potential for memory leaks, because this - /// is not called automatically on drop. (Maybe we should add an `Rc` to - /// `PresentData` so that we can do it on drop?) - fn destroy_x_resources(&mut self, conn: &Rc) { - log_x11!(conn.xfixes_destroy_region(self.region)); - } - // We have already rendered into the active pixmap buffer. Present it to the // X server, and then rotate the buffers. fn present( @@ -1026,8 +1014,8 @@ impl PresentData { height: rect.height() as u16, }; - log_x11!(conn.xfixes_set_region(self.region, &[x_rect])); - log_x11!(conn.present_pixmap( + conn.xfixes_set_region(self.region, &[x_rect])?; + conn.present_pixmap( window_id, pixmap, self.serial, @@ -1055,7 +1043,7 @@ impl PresentData { 0, // notifies &[], - )); + )?; self.waiting_on = Some(self.serial); self.serial += 1; Ok(()) From f6120057754f0779a3e9051ebb940085bbe9e0fe Mon Sep 17 00:00:00 2001 From: jneem Date: Sun, 28 Jun 2020 10:25:58 -0500 Subject: [PATCH 09/13] Update druid-shell/src/platform/x11/window.rs Co-authored-by: Leopold Luley --- druid-shell/src/platform/x11/window.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index 17d8ea2d2e..effff87e65 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -652,11 +652,8 @@ impl Window { } fn invalidate_rect(&self, rect: Rect) { - match self.enlarge_invalid_rect(rect) { - Ok(rect) => rect, - Err(err) => { - log::error!("Window::invalidate_rect - failed to enlarge rect: {}", err); - } + 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 From aa2fb4448be7d6e7139ce795eff4dbc66c786a0c Mon Sep 17 00:00:00 2001 From: jneem Date: Sun, 28 Jun 2020 10:28:00 -0500 Subject: [PATCH 10/13] Update druid-shell/src/platform/x11/window.rs Co-authored-by: Leopold Luley --- druid-shell/src/platform/x11/window.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index effff87e65..b9a7ad8897 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -886,6 +886,7 @@ impl Window { buffers.idle_pixmaps.push(event.pixmap); } else { // We must have reallocated the buffers while this pixmap was busy, so free it now. + // Regular freeing happens in `Buffers::free_pixmaps`. self.app.connection().free_pixmap(event.pixmap)?; } Ok(()) From 0763d56ce3540d650a7f80aea10b5ff9f5be74de Mon Sep 17 00:00:00 2001 From: jneem Date: Sun, 28 Jun 2020 10:30:13 -0500 Subject: [PATCH 11/13] 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 b9a7ad8897..cc17ed3c18 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -436,7 +436,7 @@ struct Buffers { /// The state involved in using X's Present extension. #[derive(Debug)] struct PresentData { - /// A monotonically increasing request counter. + /// A monotonically increasing present request counter. serial: u32, /// The region that we use for telling X what to present. region: Region, From dcfe166d36b4c2b7d2177acad1df5f4528b03915 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Sun, 28 Jun 2020 10:36:43 -0500 Subject: [PATCH 12/13] Review comments. --- druid-shell/examples/invalidate.rs | 1 + druid-shell/examples/shello.rs | 1 + druid-shell/src/lib.rs | 8 ++++++++ druid-shell/src/platform/x11/application.rs | 8 -------- druid-shell/src/platform/x11/mod.rs | 12 ++++++++++++ druid-shell/src/platform/x11/window.rs | 16 ++++++++++------ 6 files changed, 32 insertions(+), 14 deletions(-) diff --git a/druid-shell/examples/invalidate.rs b/druid-shell/examples/invalidate.rs index 7c6b319b14..c774b2d51a 100644 --- a/druid-shell/examples/invalidate.rs +++ b/druid-shell/examples/invalidate.rs @@ -84,6 +84,7 @@ impl WinHandler for InvalidateTest { } fn main() { + simple_logger::init().expect("Failed to init simple logger"); let app = Application::new().unwrap(); let mut builder = WindowBuilder::new(app.clone()); let inv_test = InvalidateTest { diff --git a/druid-shell/examples/shello.rs b/druid-shell/examples/shello.rs index 36b9b1be73..ad48ecb523 100644 --- a/druid-shell/examples/shello.rs +++ b/druid-shell/examples/shello.rs @@ -105,6 +105,7 @@ impl WinHandler for HelloState { } fn main() { + simple_logger::init().expect("Failed to init simple logger"); let mut file_menu = Menu::new(); file_menu.add_item( 0x100, diff --git a/druid-shell/src/lib.rs b/druid-shell/src/lib.rs index 3619d4cca4..dd146bed0c 100644 --- a/druid-shell/src/lib.rs +++ b/druid-shell/src/lib.rs @@ -17,6 +17,14 @@ //! `druid-shell` is an abstraction around a given platform UI & application //! framework. It provides common types, which then defer to a platform-defined //! implementation. +//! +//! # Env +//! +//! For testing and debugging, `druid-shell` can change its behavior based on environment +//! variables. Here is a list of environment variables that `druid-shell` supports: +//! +//! - `DRUID_SHELL_DISABLE_X11_PRESENT`: if this is set and `druid-shell` is using the `x11` +//! backend, it will avoid using the Present extension. #![deny(intra_doc_link_resolution_failure)] #![allow(clippy::new_without_default)] diff --git a/druid-shell/src/platform/x11/application.rs b/druid-shell/src/platform/x11/application.rs index 2664989d7d..a5de3593cd 100644 --- a/druid-shell/src/platform/x11/application.rs +++ b/druid-shell/src/platform/x11/application.rs @@ -116,11 +116,6 @@ impl Application { // Check if the Present extension is supported, returning its opcode if it is. fn query_present_opcode(conn: &Rc) -> Result, Error> { - // When checking for X11 errors synchronously, there are two places where the error could - // happen. An error on the request means the connection is broken. There's no need for - // extra error context here, because the fact that the connection broke has nothing to do - // with what we're trying to do. An error on the reply means there was something wrong with - // the request, and so we add context. This convention is used throughout the x11 backend. let query = conn .query_extension(b"Present")? .reply() @@ -344,9 +339,6 @@ impl Application { w.handle_idle_notify(ev) .context("IDLE_NOTIFY - failed to handle")?; } - // In XCB, errors are reported asynchronously by default, by sending them to the event - // loop. You can also request a synchronous error for a given call; we use this in - // window initialization, but otherwise we take the async route. Event::Error(e) => { // TODO: if an error is caused by the present extension, disable it and fall back // to copying pixels. This is blocked on diff --git a/druid-shell/src/platform/x11/mod.rs b/druid-shell/src/platform/x11/mod.rs index 47fa934fea..86cf8411c1 100644 --- a/druid-shell/src/platform/x11/mod.rs +++ b/druid-shell/src/platform/x11/mod.rs @@ -18,6 +18,18 @@ // Might be related to the "sleep scheduler" in XWindow::render()? // TODO(x11/render_improvements): double-buffering / present strategies / etc? +// # Notes on error handling in X11 +// +// In XCB, errors are reported asynchronously by default, by sending them to the event +// loop. You can also request a synchronous error for a given call; we use this in +// window initialization, but otherwise we take the async route. +// +// When checking for X11 errors synchronously, there are two places where the error could +// happen. An error on the request means the connection is broken. There's no need for +// extra error context here, because the fact that the connection broke has nothing to do +// with what we're trying to do. An error on the reply means there was something wrong with +// the request, and so we add context. This convention is used throughout the x11 backend. + #[macro_use] mod util; diff --git a/druid-shell/src/platform/x11/window.rs b/druid-shell/src/platform/x11/window.rs index cc17ed3c18..06c45458b2 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -433,7 +433,9 @@ struct Buffers { depth: u8, } -/// The state involved in using X's Present extension. +/// The state involved in using X's [Present] extension. +/// +/// [Present]: https://cgit.freedesktop.org/xorg/proto/presentproto/tree/presentproto.txt #[derive(Debug)] struct PresentData { /// A monotonically increasing present request counter. @@ -444,11 +446,13 @@ struct PresentData { waiting_on: Option, /// We need to render another frame as soon as the current one is done presenting. needs_present: bool, - /// The last MSC that was completed. This can be used to diagnose latency problems, because MSC - /// is a frame counter: we should be presenting on every frame, and storing the last completed - /// MSC lets us know if we missed one. + /// The last MSC (media stream counter) that was completed. This can be used to diagnose + /// latency problems, because MSC is a frame counter: it increments once per frame. We should + /// be presenting on every frame, and storing the last completed MSC lets us know if we missed + /// one. last_msc: Option, - /// The time at which the last frame was completed. + /// The time at which the last frame was completed. The present protocol documentation doesn't + /// define the units, but it appears to be in microseconds. last_ust: Option, } @@ -514,6 +518,7 @@ impl Window { let pixmap = if let Some(p) = buffers.idle_pixmaps.last() { *p } else { + log::info!("ran out of idle pixmaps, creating a new one"); buffers.create_pixmap(self.app.connection(), self.id)? }; @@ -984,7 +989,6 @@ impl Buffers { count: usize, ) -> Result<(), Error> { if !self.all_pixmaps.is_empty() { - log::error!("BUG: need to free the pixmaps before creating new ones"); self.free_pixmaps(conn); } From 6023f6bf5f4466030b9d078dd19ffc6d6d4392ee Mon Sep 17 00:00:00 2001 From: jneem Date: Sun, 28 Jun 2020 12:40:07 -0500 Subject: [PATCH 13/13] 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 06c45458b2..b459455687 100644 --- a/druid-shell/src/platform/x11/window.rs +++ b/druid-shell/src/platform/x11/window.rs @@ -942,7 +942,7 @@ impl Buffers { /// Frees all the X pixmaps that we hold. fn free_pixmaps(&mut self, conn: &Rc) { // We can't touch pixmaps if the present extension is waiting on them, so only free the - // idle ones. We'll free the busy ones when we get notified that they're idle. + // idle ones. We'll free the busy ones when we get notified that they're idle in `Window::handle_idle_notify`. for &p in &self.idle_pixmaps { log_x11!(conn.free_pixmap(p)); }