Avoid sending frame callbacks to invisible surfaces

4 files changed, 135 insertions, 132 deletions

diff --git a/src/backend/tty.rs b/src/backend/tty.rs
index 6f665432..763a5bc1 100644
--- a/src/backend/tty.rs
+++ b/src/backend/tty.rs
@@ -807,6 +807,8 @@ impl Tty {
                     }
                 }
 
+                niri.update_primary_scanout_output(output, &res.states);
+
                 if res.damage.is_some() {
                     let presentation_feedbacks =
                         niri.take_presentation_feedbacks(output, &res.states);
diff --git a/src/backend/winit.rs b/src/backend/winit.rs
index 7639b568..d1abfac2 100644
--- a/src/backend/winit.rs
+++ b/src/backend/winit.rs
@@ -161,6 +161,9 @@ impl Winit {
             .damage_tracker
             .render_output(self.backend.renderer(), age, elements, [0.1, 0.1, 0.1, 1.0])
             .unwrap();
+
+        niri.update_primary_scanout_output(output, &res.states);
+
         if let Some(damage) = res.damage {
             if self
                 .config
diff --git a/src/layout.rs b/src/layout.rs
index 6476598e..89fc0a29 100644
--- a/src/layout.rs
+++ b/src/layout.rs
@@ -48,8 +48,7 @@ use smithay::reexports::wayland_protocols::xdg::shell::server::xdg_toplevel;
 use smithay::reexports::wayland_server::protocol::wl_surface::WlSurface;
 use smithay::render_elements;
 use smithay::utils::{Logical, Point, Rectangle, Scale, Size};
-use smithay::wayland::compositor::{with_states, SurfaceData};
-use smithay::wayland::dmabuf::DmabufFeedback;
+use smithay::wayland::compositor::with_states;
 use smithay::wayland::shell::xdg::SurfaceCachedState;
 
 use crate::animation::Animation;
@@ -80,23 +79,6 @@ pub trait LayoutElement: SpaceElement + PartialEq + Clone {
     fn min_size(&self) -> Size<i32, Logical>;
     fn max_size(&self) -> Size<i32, Logical>;
     fn is_wl_surface(&self, wl_surface: &WlSurface) -> bool;
-    fn send_frame<T, F>(
-        &self,
-        output: &Output,
-        time: T,
-        throttle: Option<Duration>,
-        primary_scan_out_output: F,
-    ) where
-        T: Into<Duration>,
-        F: FnMut(&WlSurface, &SurfaceData) -> Option<Output> + Copy;
-    fn send_dmabuf_feedback<'a, P, F>(
-        &self,
-        output: &Output,
-        primary_scan_out_output: P,
-        select_dmabuf_feedback: F,
-    ) where
-        P: FnMut(&WlSurface, &SurfaceData) -> Option<Output> + Copy,
-        F: Fn(&WlSurface, &SurfaceData) -> &'a DmabufFeedback + Copy;
 }
 
 #[derive(Debug)]
@@ -253,31 +235,6 @@ impl LayoutElement for Window {
     fn is_wl_surface(&self, wl_surface: &WlSurface) -> bool {
         self.toplevel().wl_surface() == wl_surface
     }
-
-    fn send_frame<T, F>(
-        &self,
-        output: &Output,
-        time: T,
-        throttle: Option<Duration>,
-        primary_scan_out_output: F,
-    ) where
-        T: Into<Duration>,
-        F: FnMut(&WlSurface, &SurfaceData) -> Option<Output> + Copy,
-    {
-        self.send_frame(output, time, throttle, primary_scan_out_output);
-    }
-
-    fn send_dmabuf_feedback<'a, P, F>(
-        &self,
-        output: &Output,
-        primary_scan_out_output: P,
-        select_dmabuf_feedback: F,
-    ) where
-        P: FnMut(&WlSurface, &SurfaceData) -> Option<Output> + Copy,
-        F: Fn(&WlSurface, &SurfaceData) -> &'a DmabufFeedback + Copy,
-    {
-        self.send_dmabuf_feedback(output, primary_scan_out_output, select_dmabuf_feedback);
-    }
 }
 
 impl FocusRing {
@@ -579,31 +536,6 @@ impl<W: LayoutElement> MonitorSet<W> {
         }
     }
 
-    pub fn send_frame(
-        &self,
-        output: &Output,
-        time: Duration,
-        should_send: &impl Fn(&SurfaceData) -> bool,
-    ) {
-        if let MonitorSet::Normal { monitors, .. } = self {
-            for mon in monitors {
-                if &mon.output == output {
-                    mon.workspaces[mon.active_workspace_idx].send_frame(time, should_send);
-                }
-            }
-        }
-    }
-
-    pub fn send_dmabuf_feedback(&self, output: &Output, feedback: &DmabufFeedback) {
-        if let MonitorSet::Normal { monitors, .. } = self {
-            for mon in monitors {
-                if &mon.output == output {
-                    mon.workspaces[mon.active_workspace_idx].send_dmabuf_feedback(feedback);
-                }
-            }
-        }
-    }
-
     pub fn find_window_and_output(&self, wl_surface: &WlSurface) -> Option<(W, Output)> {
         if let MonitorSet::Normal { monitors, .. } = self {
             for mon in monitors {
@@ -1853,22 +1785,6 @@ impl<W: LayoutElement> Workspace<W> {
         self.add_window(window, true);
     }
 
-    fn send_frame(&self, time: Duration, should_send: &impl Fn(&SurfaceData) -> bool) {
-        let output = self.output.as_ref().unwrap();
-        for win in self.windows() {
-            win.send_frame(output, time, None, |_, states| {
-                should_send(states).then(|| output.clone())
-            });
-        }
-    }
-
-    fn send_dmabuf_feedback(&self, feedback: &DmabufFeedback) {
-        let output = self.output.as_ref().unwrap();
-        for win in self.windows() {
-            win.send_dmabuf_feedback(output, |_, _| Some(output.clone()), |_, _| feedback);
-        }
-    }
-
     fn view_pos(&self) -> i32 {
         self.column_x(self.active_column_idx) + self.view_offset - PADDING
     }
@@ -2458,29 +2374,6 @@ mod tests {
         fn is_wl_surface(&self, _wl_surface: &WlSurface) -> bool {
             false
         }
-
-        fn send_frame<T, F>(
-            &self,
-            _output: &Output,
-            _time: T,
-            _throttle: Option<Duration>,
-            _primary_scan_out_output: F,
-        ) where
-            T: Into<Duration>,
-            F: FnMut(&WlSurface, &SurfaceData) -> Option<Output> + Copy,
-        {
-        }
-
-        fn send_dmabuf_feedback<'a, P, F>(
-            &self,
-            _output: &Output,
-            _primary_scan_out_output: P,
-            _select_dmabuf_feedback: F,
-        ) where
-            P: FnMut(&WlSurface, &SurfaceData) -> Option<Output> + Copy,
-            F: Fn(&WlSurface, &SurfaceData) -> &'a DmabufFeedback + Copy,
-        {
-        }
     }
 
     fn arbitrary_bbox() -> impl Strategy<Value = Rectangle<i32, Logical>> {
diff --git a/src/niri.rs b/src/niri.rs
index 8c4579fc..527282a7 100644
--- a/src/niri.rs
+++ b/src/niri.rs
@@ -18,13 +18,15 @@ use smithay::backend::renderer::element::surface::{
 use smithay::backend::renderer::element::texture::TextureRenderElement;
 use smithay::backend::renderer::element::utils::{Relocate, RelocateRenderElement};
 use smithay::backend::renderer::element::{
-    render_elements, AsRenderElements, Kind, RenderElement, RenderElementStates,
+    default_primary_scanout_output_compare, render_elements, AsRenderElements, Kind, RenderElement,
+    RenderElementStates,
 };
 use smithay::backend::renderer::gles::{GlesMapping, GlesRenderer, GlesTexture};
 use smithay::backend::renderer::{Bind, ExportMem, Frame, ImportAll, Offscreen, Renderer};
 use smithay::desktop::utils::{
     send_dmabuf_feedback_surface_tree, send_frames_surface_tree,
-    surface_presentation_feedback_flags_from_states, take_presentation_feedback_surface_tree,
+    surface_presentation_feedback_flags_from_states, surface_primary_scanout_output,
+    take_presentation_feedback_surface_tree, update_surface_primary_scanout_output,
     OutputPresentationFeedback,
 };
 use smithay::desktop::{layer_map_for_output, PopupManager, Space, Window, WindowSurfaceType};
@@ -49,7 +51,8 @@ use smithay::utils::{
     IsAlive, Logical, Physical, Point, Rectangle, Scale, Size, Transform, SERIAL_COUNTER,
 };
 use smithay::wayland::compositor::{
-    with_states, CompositorClientState, CompositorState, SurfaceData,
+    with_states, with_surface_tree_downward, CompositorClientState, CompositorState, SurfaceData,
+    TraversalAction,
 };
 use smithay::wayland::data_device::DataDeviceState;
 use smithay::wayland::dmabuf::DmabufFeedback;
@@ -1160,6 +1163,16 @@ impl Niri {
         }
 
         // Send the frame callbacks.
+        //
+        // FIXME: The logic here could be a bit smarter. Currently, during an animation, the
+        // surfaces that are visible for the very last frame (e.g. because the camera is moving
+        // away) will receive frame callbacks, and the surfaces that are invisible but will become
+        // visible next frame will not receive frame callbacks (so they will show stale contents for
+        // one frame). We could advance the animations for the next frame and send frame callbacks
+        // according to the expected new positions.
+        //
+        // However, this should probably be restricted to sending frame callbacks to more surfaces,
+        // to err on the safe side.
         self.send_frame_callbacks(output);
 
         // Render and send to PipeWire screencast streams.
@@ -1172,10 +1185,95 @@ impl Niri {
         }
     }
 
+    pub fn update_primary_scanout_output(
+        &self,
+        output: &Output,
+        render_element_states: &RenderElementStates,
+    ) {
+        // FIXME: potentially tweak the compare function. The default one currently always prefers a
+        // higher refresh-rate output, which is not always desirable (i.e. with a very small
+        // overlap).
+        //
+        // While we only have cursors and DnD icons crossing output boundaries though, it doesn't
+        // matter all that much.
+        if let CursorImageStatus::Surface(surface) = &self.cursor_image {
+            with_surface_tree_downward(
+                surface,
+                (),
+                |_, _, _| TraversalAction::DoChildren(()),
+                |surface, states, _| {
+                    update_surface_primary_scanout_output(
+                        surface,
+                        output,
+                        states,
+                        render_element_states,
+                        default_primary_scanout_output_compare,
+                    );
+                },
+                |_, _, _| true,
+            );
+        }
+
+        if let Some(surface) = &self.dnd_icon {
+            with_surface_tree_downward(
+                surface,
+                (),
+                |_, _, _| TraversalAction::DoChildren(()),
+                |surface, states, _| {
+                    update_surface_primary_scanout_output(
+                        surface,
+                        output,
+                        states,
+                        render_element_states,
+                        default_primary_scanout_output_compare,
+                    );
+                },
+                |_, _, _| true,
+            );
+        }
+
+        // We're only updating the current output's windows and layer surfaces. This should be fine
+        // as in niri they can only be rendered on a single output at a time.
+        //
+        // The reason to do this at all is that it keeps track of whether the surface is visible or
+        // not in a unified way with the pointer surfaces, which makes the logic elsewhere simpler.
+
+        for win in self.monitor_set.windows_for_output(output) {
+            win.with_surfaces(|surface, states| {
+                update_surface_primary_scanout_output(
+                    surface,
+                    output,
+                    states,
+                    render_element_states,
+                    // Windows are shown only on one output at a time.
+                    |_, _, output, _| output,
+                );
+            });
+        }
+
+        for surface in layer_map_for_output(output).layers() {
+            surface.with_surfaces(|surface, states| {
+                update_surface_primary_scanout_output(
+                    surface,
+                    output,
+                    states,
+                    render_element_states,
+                    // Layer surfaces are shown only on one output at a time.
+                    |_, _, output, _| output,
+                );
+            });
+        }
+    }
+
     fn send_dmabuf_feedbacks(&self, output: &Output, feedback: &DmabufFeedback) {
         let _span = tracy_client::span!("Niri::send_dmabuf_feedbacks");
 
-        self.monitor_set.send_dmabuf_feedback(output, feedback);
+        // We can unconditionally send the current output's feedback to regular and layer-shell
+        // surfaces, as they can only be displayed on a single output at a time. Even if a surface
+        // is currently invisible, this is the DMABUF feedback that it should know about.
+        for win in self.monitor_set.windows_for_output(output) {
+            win.send_dmabuf_feedback(output, |_, _| Some(output.clone()), |_, _| feedback);
+        }
 
         for surface in layer_map_for_output(output).layers() {
             surface.send_dmabuf_feedback(output, |_, _| Some(output.clone()), |_, _| feedback);
@@ -1185,7 +1283,7 @@ impl Niri {
             send_dmabuf_feedback_surface_tree(
                 surface,
                 output,
-                |_, _| Some(output.clone()),
+                surface_primary_scanout_output,
                 |_, _| feedback,
             );
         }
@@ -1194,7 +1292,7 @@ impl Niri {
             send_dmabuf_feedback_surface_tree(
                 surface,
                 output,
-                |_, _| Some(output.clone()),
+                surface_primary_scanout_output,
                 |_, _| feedback,
             );
         }
@@ -1206,7 +1304,16 @@ impl Niri {
         let state = self.output_state.get(output).unwrap();
         let sequence = state.current_estimated_sequence;
 
-        let should_send = |states: &SurfaceData| {
+        let should_send = |surface: &WlSurface, states: &SurfaceData| {
+            // Do the standard primary scanout output check. For pointer surfaces it deduplicates
+            // the frame callbacks across potentially multiple outputs, and for regular windows and
+            // layer-shell surfaces it avoids sending frame callbacks to invisible surfaces.
+            let current_primary_output = surface_primary_scanout_output(surface, states);
+            if current_primary_output.as_ref() != Some(output) {
+                return None;
+            }
+
+            // Next, check the throttling status.
             let frame_throttling_state = states
                 .data_map
                 .get_or_insert(SurfaceFrameThrottlingState::default);
@@ -1226,31 +1333,29 @@ impl Niri {
                 if let Some(sequence) = sequence {
                     *last_sent_at = Some((output.clone(), sequence));
                 }
-            }
 
-            send
+                Some(output.clone())
+            } else {
+                None
+            }
         };
 
         let frame_callback_time = get_monotonic_time();
-        self.monitor_set
-            .send_frame(output, frame_callback_time, &should_send);
+
+        for win in self.monitor_set.windows_for_output(output) {
+            win.send_frame(output, frame_callback_time, None, should_send);
+        }
 
         for surface in layer_map_for_output(output).layers() {
-            surface.send_frame(output, frame_callback_time, None, |_, states| {
-                should_send(states).then(|| output.clone())
-            });
+            surface.send_frame(output, frame_callback_time, None, should_send);
         }
 
         if let Some(surface) = &self.dnd_icon {
-            send_frames_surface_tree(surface, output, frame_callback_time, None, |_, states| {
-                should_send(states).then(|| output.clone())
-            });
+            send_frames_surface_tree(surface, output, frame_callback_time, None, should_send);
         }
 
         if let CursorImageStatus::Surface(surface) = &self.cursor_image {
-            send_frames_surface_tree(surface, output, frame_callback_time, None, |_, states| {
-                should_send(states).then(|| output.clone())
-            });
+            send_frames_surface_tree(surface, output, frame_callback_time, None, should_send);
         }
     }
 
@@ -1265,7 +1370,7 @@ impl Niri {
             take_presentation_feedback_surface_tree(
                 surface,
                 &mut feedback,
-                |_, _| Some(output.clone()),
+                surface_primary_scanout_output,
                 |surface, _| {
                     surface_presentation_feedback_flags_from_states(surface, render_element_states)
                 },
@@ -1276,7 +1381,7 @@ impl Niri {
             take_presentation_feedback_surface_tree(
                 surface,
                 &mut feedback,
-                |_, _| Some(output.clone()),
+                surface_primary_scanout_output,
                 |surface, _| {
                     surface_presentation_feedback_flags_from_states(surface, render_element_states)
                 },
@@ -1286,7 +1391,7 @@ impl Niri {
         for win in self.monitor_set.windows_for_output(output) {
             win.take_presentation_feedback(
                 &mut feedback,
-                |_, _| Some(output.clone()),
+                surface_primary_scanout_output,
                 |surface, _| {
                     surface_presentation_feedback_flags_from_states(surface, render_element_states)
                 },
@@ -1296,7 +1401,7 @@ impl Niri {
         for surface in layer_map_for_output(output).layers() {
             surface.take_presentation_feedback(
                 &mut feedback,
-                |_, _| Some(output.clone()),
+                surface_primary_scanout_output,
                 |surface, _| {
                     surface_presentation_feedback_flags_from_states(surface, render_element_states)
                 },