From b351f6ff220560d96a260d8dd3ad794000923481 Mon Sep 17 00:00:00 2001 From: Ivan Molodetskikh Date: Sat, 1 Mar 2025 09:45:57 +0300 Subject: Keep track of RenderElementStates in offscreens This both avoids sending frame callbacks to surfaces invisible on the offscreen (fixing Firefox with subsurface compositing in the process), and fixes searching for split popups during the resize animation. --- niri-visual-tests/src/test_window.rs | 3 ++- src/layout/mod.rs | 4 +-- src/layout/opening_window.rs | 20 ++++++++------ src/layout/tests.rs | 2 +- src/layout/tile.rs | 27 ++++++++++--------- src/niri.rs | 52 +++++++++++++++++++++++++++--------- src/render_helpers/offscreen.rs | 21 ++++++++++++--- src/window/mapped.rs | 33 +++++++++++++++++------ 8 files changed, 114 insertions(+), 48 deletions(-) diff --git a/niri-visual-tests/src/test_window.rs b/niri-visual-tests/src/test_window.rs index 158f8b4b..56d3c3fd 100644 --- a/niri-visual-tests/src/test_window.rs +++ b/niri-visual-tests/src/test_window.rs @@ -6,6 +6,7 @@ use niri::layout::{ ConfigureIntent, InteractiveResizeData, LayoutElement, LayoutElementRenderElement, LayoutElementRenderSnapshot, }; +use niri::render_helpers::offscreen::OffscreenData; use niri::render_helpers::renderer::NiriRenderer; use niri::render_helpers::solid_color::{SolidColorBuffer, SolidColorRenderElement}; use niri::render_helpers::{RenderTarget, SplitElements}; @@ -214,7 +215,7 @@ impl LayoutElement for TestWindow { fn output_leave(&self, _output: &Output) {} - fn set_offscreen_element_id(&self, _id: Option) {} + fn set_offscreen_data(&self, _data: Option) {} fn set_activated(&mut self, _active: bool) {} diff --git a/src/layout/mod.rs b/src/layout/mod.rs index ac83a0cf..dc7daa65 100644 --- a/src/layout/mod.rs +++ b/src/layout/mod.rs @@ -45,7 +45,6 @@ use niri_config::{ use niri_ipc::{ColumnDisplay, PositionChange, SizeChange}; use scrolling::{Column, ColumnWidth, InsertHint, InsertPosition}; use smithay::backend::renderer::element::surface::WaylandSurfaceRenderElement; -use smithay::backend::renderer::element::Id; use smithay::backend::renderer::gles::{GlesRenderer, GlesTexture}; use smithay::output::{self, Output}; use smithay::reexports::wayland_server::protocol::wl_surface::WlSurface; @@ -59,6 +58,7 @@ use self::workspace::{OutputId, Workspace}; use crate::animation::Clock; use crate::layout::scrolling::ScrollDirection; use crate::niri_render_elements; +use crate::render_helpers::offscreen::OffscreenData; use crate::render_helpers::renderer::NiriRenderer; use crate::render_helpers::snapshot::RenderSnapshot; use crate::render_helpers::solid_color::{SolidColorBuffer, SolidColorRenderElement}; @@ -194,7 +194,7 @@ pub trait LayoutElement { fn set_preferred_scale_transform(&self, scale: output::Scale, transform: Transform); fn output_enter(&self, output: &Output); fn output_leave(&self, output: &Output); - fn set_offscreen_element_id(&self, id: Option); + fn set_offscreen_data(&self, data: Option); fn set_activated(&mut self, active: bool); fn set_active_in_column(&mut self, active: bool); fn set_floating(&mut self, floating: bool); diff --git a/src/layout/opening_window.rs b/src/layout/opening_window.rs index 8a1db409..19727551 100644 --- a/src/layout/opening_window.rs +++ b/src/layout/opening_window.rs @@ -5,14 +5,14 @@ use glam::{Mat3, Vec2}; use smithay::backend::renderer::element::utils::{ Relocate, RelocateRenderElement, RescaleRenderElement, }; -use smithay::backend::renderer::element::{Kind, RenderElement}; +use smithay::backend::renderer::element::{Element as _, Kind, RenderElement}; use smithay::backend::renderer::gles::{GlesRenderer, Uniform}; use smithay::backend::renderer::Texture; use smithay::utils::{Logical, Point, Rectangle, Scale, Size}; use crate::animation::Animation; use crate::niri_render_elements; -use crate::render_helpers::offscreen::{OffscreenBuffer, OffscreenRenderElement}; +use crate::render_helpers::offscreen::{OffscreenBuffer, OffscreenData, OffscreenRenderElement}; use crate::render_helpers::shader_element::ShaderRenderElement; use crate::render_helpers::shaders::{mat3_uniform, ProgramType, Shaders}; @@ -55,11 +55,11 @@ impl OpenAnimation { location: Point, scale: Scale, alpha: f32, - ) -> anyhow::Result { + ) -> anyhow::Result<(OpeningWindowRenderElement, OffscreenData)> { let progress = self.anim.value(); let clamped_progress = self.anim.clamped_value().clamp(0., 1.); - let (elem, _sync_point) = self + let (elem, _sync_point, mut data) = self .buffer .render(renderer, scale, elements) .context("error rendering to offscreen buffer")?; @@ -98,7 +98,7 @@ impl OpenAnimation { let geo_to_tex = Mat3::from_translation(-tex_loc / tex_size) * Mat3::from_scale(geo_size / tex_size); - return Ok(ShaderRenderElement::new( + let elem = ShaderRenderElement::new( ProgramType::Open, area.size, None, @@ -115,8 +115,12 @@ impl OpenAnimation { HashMap::from([(String::from("niri_tex"), texture.clone())]), Kind::Unspecified, ) - .with_location(area.loc) - .into()); + .with_location(area.loc); + + // We're drawing the shader, not the offscreen itself. + data.id = elem.id().clone(); + + return Ok((elem.into(), data)); } let elem = elem.with_alpha(clamped_progress as f32 * alpha); @@ -134,6 +138,6 @@ impl OpenAnimation { Relocate::Relative, ); - Ok(elem.into()) + Ok((elem.into(), data)) } } diff --git a/src/layout/tests.rs b/src/layout/tests.rs index 9d3e8634..160e4e97 100644 --- a/src/layout/tests.rs +++ b/src/layout/tests.rs @@ -162,7 +162,7 @@ impl LayoutElement for TestWindow { fn output_leave(&self, _output: &Output) {} - fn set_offscreen_element_id(&self, _id: Option) {} + fn set_offscreen_data(&self, _data: Option) {} fn set_activated(&mut self, active: bool) { self.0.pending_activated.set(active); diff --git a/src/layout/tile.rs b/src/layout/tile.rs index a93e4a6f..89f0e6ab 100644 --- a/src/layout/tile.rs +++ b/src/layout/tile.rs @@ -888,7 +888,7 @@ impl Tile { clip_to_geometry }; - if let Some((elem_current, _sync_point)) = current { + if let Some((elem_current, _sync_point, mut data)) = current { let texture_current = elem_current.texture().clone(); // The offset and size are computed in physical pixels and converted to // logical with the same `scale`, so converting them back with rounding @@ -908,10 +908,13 @@ impl Tile { clip_to_geometry, win_alpha, ); - // FIXME: with split popups, this will use the resize element ID for - // popups, but we want the real IDs. - self.window - .set_offscreen_element_id(Some(elem.id().clone())); + + // We're drawing the resize shader, not the offscreen directly. + data.id = elem.id().clone(); + + // This is not a problem for split popups as the code will look for them by + // original id when it doesn't find them on the offscreen. + self.window.set_offscreen_data(Some(data)); resize_shader = Some(elem.into()); } } @@ -928,7 +931,6 @@ impl Tile { ) .into(), ); - self.window.set_offscreen_element_id(None); } } @@ -1058,6 +1060,8 @@ impl Tile { let mut alpha_anim_elem = None; let mut window_elems = None; + self.window().set_offscreen_data(None); + if let Some(open) = &self.open_animation { let renderer = renderer.as_gles_renderer(); let elements = @@ -1071,9 +1075,8 @@ impl Tile { scale, tile_alpha, ) { - Ok(elem) => { - self.window() - .set_offscreen_element_id(Some(elem.id().clone())); + Ok((elem, data)) => { + self.window().set_offscreen_data(Some(data)); open_anim_elem = Some(elem.into()); } Err(err) => { @@ -1086,12 +1089,11 @@ impl Tile { self.render_inner(renderer, Point::from((0., 0.)), scale, focus_ring, target); let elements = elements.collect::>>(); match alpha.offscreen.render(renderer, scale, &elements) { - Ok((elem, _sync)) => { + Ok((elem, _sync, data)) => { let offset = elem.offset(); let elem = elem.with_alpha(tile_alpha).with_offset(location + offset); - self.window() - .set_offscreen_element_id(Some(elem.id().clone())); + self.window().set_offscreen_data(Some(data)); alpha_anim_elem = Some(elem.into()); } Err(err) => { @@ -1101,7 +1103,6 @@ impl Tile { } if open_anim_elem.is_none() && alpha_anim_elem.is_none() { - self.window().set_offscreen_element_id(None); window_elems = Some(self.render_inner(renderer, location, scale, focus_ring, target)); } diff --git a/src/niri.rs b/src/niri.rs index 4575de21..08830090 100644 --- a/src/niri.rs +++ b/src/niri.rs @@ -29,7 +29,7 @@ use smithay::backend::renderer::element::utils::{ select_dmabuf_feedback, Relocate, RelocateRenderElement, }; use smithay::backend::renderer::element::{ - default_primary_scanout_output_compare, Kind, PrimaryScanoutOutput, RenderElementStates, + default_primary_scanout_output_compare, Id, Kind, PrimaryScanoutOutput, RenderElementStates, }; use smithay::backend::renderer::gles::GlesRenderer; use smithay::backend::renderer::sync::SyncPoint; @@ -3793,21 +3793,49 @@ impl Niri { for mapped in self.layout.windows_for_output(output) { let win = &mapped.window; - let offscreen_id = mapped.offscreen_element_id().clone(); + let offscreen_data = mapped.offscreen_data(); + let offscreen_data = offscreen_data.as_ref(); win.with_surfaces(|surface, states| { - let surface_primary_scanout_output = states + let primary_scanout_output = states .data_map .get_or_insert_threadsafe(Mutex::::default); - surface_primary_scanout_output - .lock() - .unwrap() - .update_from_render_element_states( - offscreen_id.clone().unwrap_or_else(|| surface.into()), - output, - render_element_states, - |_, _, output, _| output, - ); + let mut primary_scanout_output = primary_scanout_output.lock().unwrap(); + + let mut id = Id::from_wayland_resource(surface); + + if let Some(data) = offscreen_data { + // We have offscreen data; it's likely that all surfaces are on it. + if data.states.element_was_presented(id.clone()) { + // If the surface was presented to the offscreen, use the offscreen's id. + id = data.id.clone(); + } + + // If we the surface wasn't presented to the offscreen it can mean: + // + // - The surface was invisible. For example, it's obscured by another surface on + // the offscreen, or simply isn't mapped. + // - The surface is rendered separately from the offscreen, for example: popups + // during the window resize animation. + // + // In both of these cases, using the original surface element id and the + // original states is the correct thing to do. We may find the surface in the + // original states (in the second case). Either way, we definitely know it is + // *not* in the offscreen, and we won't miss it. + // + // There's one edge case: if the surface is both in the offscreen and separate, + // and the offscreen itself is invisible, while the separate surface is + // visible. In this case we'll currently mark the surface as invisible. We + // don't really use offscreens like that however, and if we start, it's easy + // enough to fix (need an extra check). + } + + primary_scanout_output.update_from_render_element_states( + id, + output, + render_element_states, + |_, _, output, _| output, + ); }); } diff --git a/src/render_helpers/offscreen.rs b/src/render_helpers/offscreen.rs index 62c123ea..71959404 100644 --- a/src/render_helpers/offscreen.rs +++ b/src/render_helpers/offscreen.rs @@ -4,7 +4,9 @@ use anyhow::Context as _; use smithay::backend::allocator::Fourcc; use smithay::backend::renderer::damage::OutputDamageTracker; use smithay::backend::renderer::element::utils::{Relocate, RelocateRenderElement}; -use smithay::backend::renderer::element::{Element, Id, Kind, RenderElement, UnderlyingStorage}; +use smithay::backend::renderer::element::{ + Element, Id, Kind, RenderElement, RenderElementStates, UnderlyingStorage, +}; use smithay::backend::renderer::gles::{GlesError, GlesFrame, GlesRenderer, GlesTexture}; use smithay::backend::renderer::sync::SyncPoint; use smithay::backend::renderer::utils::{ @@ -57,13 +59,21 @@ pub struct OffscreenRenderElement { kind: Kind, } +#[derive(Debug)] +pub struct OffscreenData { + /// Id of the offscreen element. + pub id: Id, + /// States for the render into the offscreen buffer. + pub states: RenderElementStates, +} + impl OffscreenBuffer { pub fn render( &self, renderer: &mut GlesRenderer, scale: Scale, elements: &[impl RenderElement], - ) -> anyhow::Result<(OffscreenRenderElement, SyncPoint)> { + ) -> anyhow::Result<(OffscreenRenderElement, SyncPoint, OffscreenData)> { let _span = tracy_client::span!("OffscreenBuffer::render"); let geo = encompassing_geo(scale, elements.iter()); @@ -179,7 +189,12 @@ impl OffscreenBuffer { kind: Kind::Unspecified, }; - Ok((elem, res.sync)) + let data = OffscreenData { + id: self.id.clone(), + states: res.states, + }; + + Ok((elem, res.sync, data)) } } diff --git a/src/window/mapped.rs b/src/window/mapped.rs index 8263c078..6b539e46 100644 --- a/src/window/mapped.rs +++ b/src/window/mapped.rs @@ -3,7 +3,7 @@ use std::time::Duration; use niri_config::{Color, CornerRadius, GradientInterpolation, WindowRule}; use smithay::backend::renderer::element::surface::render_elements_from_surface_tree; -use smithay::backend::renderer::element::{Id, Kind}; +use smithay::backend::renderer::element::Kind; use smithay::backend::renderer::gles::GlesRenderer; use smithay::desktop::space::SpaceElement as _; use smithay::desktop::{PopupManager, Window}; @@ -26,6 +26,7 @@ use crate::layout::{ }; use crate::niri_render_elements; use crate::render_helpers::border::BorderRenderElement; +use crate::render_helpers::offscreen::OffscreenData; use crate::render_helpers::renderer::NiriRenderer; use crate::render_helpers::snapshot::RenderSnapshot; use crate::render_helpers::solid_color::{SolidColorBuffer, SolidColorRenderElement}; @@ -70,10 +71,10 @@ pub struct Mapped { /// resizes immediately, without waiting for a 1 second throttled callback. needs_frame_callback: bool, - /// Id of the offscreen element rendered in place of this window. + /// Data of the offscreen element rendered in place of this window. /// /// If `None`, then the window is not offscreened. - offscreen_element_id: RefCell>, + offscreen_data: RefCell>, /// Whether this window has the keyboard focus. is_focused: bool, @@ -192,7 +193,7 @@ impl Mapped { need_to_recompute_rules: false, needs_configure: false, needs_frame_callback: false, - offscreen_element_id: RefCell::new(None), + offscreen_data: RefCell::new(None), is_focused: false, is_active_in_column: true, is_floating: false, @@ -257,8 +258,8 @@ impl Mapped { self.credentials.as_ref() } - pub fn offscreen_element_id(&self) -> Ref> { - self.offscreen_element_id.borrow() + pub fn offscreen_data(&self) -> Ref> { + self.offscreen_data.borrow() } pub fn is_focused(&self) -> bool { @@ -751,8 +752,24 @@ impl LayoutElement for Mapped { self.window.output_leave(output) } - fn set_offscreen_element_id(&self, id: Option) { - self.offscreen_element_id.replace(id); + fn set_offscreen_data(&self, data: Option) { + let Some(data) = data else { + self.offscreen_data.replace(None); + return; + }; + + let mut offscreen_data = self.offscreen_data.borrow_mut(); + match &mut *offscreen_data { + None => { + *offscreen_data = Some(data); + } + Some(existing) => { + // Replace the id, amend existing element states. This is necessary to handle + // multiple layers of offscreen (e.g. resize animation + alpha animation). + existing.id = data.id; + existing.states.states.extend(data.states.states); + } + } } fn set_activated(&mut self, active: bool) { -- cgit