Merge pull request #66 from Ottatop/fix_flickering

Fix flickering

README.md

@@ -16,7 +16,6 @@ It sports high configurability through a (soon to be) extensive Lua API, with pl
 > <summary>Click me</summary>
 >
 > All videos were recorded using [Screenkey](https://gitlab.com/screenkey/screenkey) and the Winit backend.
-> Expect minor flickering here and there, but I hope to fix that in the future.
 > 
 > https://github.com/Ottatop/pinnacle/assets/120758733/5b6b224b-3031-4a1c-9375-1143f1bfc0e3
 >

src/backend/winit.rs

@@ -232,6 +232,16 @@ pub fn run_winit() -> anyhow::Result<()> {
 
                 pointer_element.set_status(state.cursor_status.clone());
 
+                if state.pause_rendering {
+                    state.space.refresh();
+                    state.popup_manager.cleanup();
+                    display
+                        .flush_clients()
+                        .expect("failed to flush client buffers");
+
+                    return TimeoutAction::ToDuration(Duration::from_millis(1));
+                }
+
                 let Backend::Winit(backend) = &mut state.backend else { unreachable!() };
                 let full_redraw = &mut backend.full_redraw;
                 *full_redraw = full_redraw.saturating_sub(1);

src/handlers.rs

@@ -95,7 +95,7 @@ impl CompositorHandler for State {
     }
 
     fn commit(&mut self, surface: &WlSurface) {
-        // tracing::debug!("commit");
+        // tracing::debug!("commit on surface {:?}", surface);
 
         X11Wm::commit_hook::<CalloopData>(surface);
 

src/handlers/xdg_shell.rs

@@ -15,7 +15,7 @@ use smithay::{
     },
     utils::{Serial, SERIAL_COUNTER},
     wayland::{
-        compositor::{self, CompositorHandler},
+        compositor::{self},
         shell::xdg::{
             Configure, PopupSurface, PositionerState, ToplevelSurface, XdgShellHandler,
             XdgShellState,
@@ -26,7 +26,7 @@ use smithay::{
 use crate::{
     focus::FocusTarget,
     state::{State, WithState},
-    window::{window_state::LocationRequestState, WindowElement, BLOCKER_COUNTER},
+    window::{window_state::LocationRequestState, WindowElement},
 };
 
 impl XdgShellHandler for State {
@@ -65,26 +65,6 @@ impl XdgShellHandler for State {
             tracing::debug!("new window, tags are {:?}", state.tags);
         });
 
-        let windows_on_output = self
-            .windows
-            .iter()
-            .filter(|win| {
-                win.with_state(|state| {
-                    self.focus_state
-                        .focused_output
-                        .as_ref()
-                        .expect("no focused output")
-                        .with_state(|op_state| {
-                            op_state
-                                .tags
-                                .iter()
-                                .any(|tag| state.tags.iter().any(|tg| tg == tag))
-                        })
-                })
-            })
-            .cloned()
-            .collect::<Vec<_>>();
-
         // note to self: don't reorder this
         // TODO: fix it so that reordering this doesn't break stuff
         self.windows.push(window.clone());
@@ -116,34 +96,6 @@ impl XdgShellHandler for State {
 
                 if let Some(focused_output) = data.state.focus_state.focused_output.clone() {
                     data.state.update_windows(&focused_output);
-                    BLOCKER_COUNTER.store(1, std::sync::atomic::Ordering::SeqCst);
-                    tracing::debug!(
-                        "blocker {}",
-                        BLOCKER_COUNTER.load(std::sync::atomic::Ordering::SeqCst)
-                    );
-                    for win in windows_on_output.iter() {
-                        if let Some(surf) = win.wl_surface() {
-                            compositor::add_blocker(&surf, crate::window::WindowBlocker);
-                        }
-                    }
-                    let clone = window.clone();
-                    data.state.loop_handle.insert_idle(|data| {
-                        crate::state::schedule_on_commit(data, vec![clone], move |data| {
-                            BLOCKER_COUNTER.store(0, std::sync::atomic::Ordering::SeqCst);
-                            tracing::debug!(
-                                "blocker {}",
-                                BLOCKER_COUNTER.load(std::sync::atomic::Ordering::SeqCst)
-                            );
-                            for client in windows_on_output
-                                .iter()
-                                .filter_map(|win| win.wl_surface()?.client())
-                            {
-                                data.state
-                                    .client_compositor_state(&client)
-                                    .blocker_cleared(&mut data.state, &data.display.handle())
-                            }
-                        })
-                    });
                 }
                 data.state.loop_handle.insert_idle(move |data| {
                     data.state
@@ -283,7 +235,7 @@ impl XdgShellHandler for State {
                     match &configure {
                         Configure::Toplevel(configure) => {
                             if configure.serial >= serial {
-                                // tracing::debug!("acked configure, new loc is {:?}", new_loc);
+                                tracing::debug!("acked configure, new loc is {:?}", new_loc);
                                 state.loc_request_state =
                                     LocationRequestState::Acknowledged(new_loc);
                             }

src/handlers/xwayland.rs

@@ -1,10 +1,8 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 
 use smithay::{
-    reexports::wayland_server::Resource,
     utils::{Logical, Point, Rectangle, SERIAL_COUNTER},
     wayland::{
-        compositor::{self, CompositorHandler},
         data_device::{
             clear_data_device_selection, current_data_device_selection_userdata,
             request_data_device_client_selection, set_data_device_selection,
@@ -23,7 +21,7 @@ use smithay::{
 use crate::{
     focus::FocusTarget,
     state::{CalloopData, WithState},
-    window::{window_state::FloatingOrTiled, WindowBlocker, WindowElement, BLOCKER_COUNTER},
+    window::{window_state::FloatingOrTiled, WindowElement},
 };
 
 impl XwmHandler for CalloopData {
@@ -127,28 +125,6 @@ impl XwmHandler for CalloopData {
                 });
             }
 
-            let windows_on_output = self
-                .state
-                .windows
-                .iter()
-                .filter(|win| {
-                    win.with_state(|state| {
-                        self.state
-                            .focus_state
-                            .focused_output
-                            .as_ref()
-                            .expect("no focused output")
-                            .with_state(|op_state| {
-                                op_state
-                                    .tags
-                                    .iter()
-                                    .any(|tag| state.tags.iter().any(|tg| tg == tag))
-                            })
-                    })
-                })
-                .cloned()
-                .collect::<Vec<_>>();
-
             self.state.windows.push(window.clone());
 
             self.state.focus_state.set_focus(window.clone());
@@ -157,34 +133,6 @@ impl XwmHandler for CalloopData {
 
             if let Some(focused_output) = self.state.focus_state.focused_output.clone() {
                 self.state.update_windows(&focused_output);
-                BLOCKER_COUNTER.store(1, std::sync::atomic::Ordering::SeqCst);
-                tracing::debug!(
-                    "blocker {}",
-                    BLOCKER_COUNTER.load(std::sync::atomic::Ordering::SeqCst)
-                );
-                for win in windows_on_output.iter() {
-                    if let Some(surf) = win.wl_surface() {
-                        compositor::add_blocker(&surf, WindowBlocker);
-                    }
-                }
-                let clone = window.clone();
-                self.state.loop_handle.insert_idle(move |data| {
-                    crate::state::schedule_on_commit(data, vec![clone.clone()], move |data| {
-                        BLOCKER_COUNTER.store(0, std::sync::atomic::Ordering::SeqCst);
-                        tracing::debug!(
-                            "blocker {}",
-                            BLOCKER_COUNTER.load(std::sync::atomic::Ordering::SeqCst)
-                        );
-                        for client in windows_on_output
-                            .iter()
-                            .filter_map(|win| win.wl_surface()?.client())
-                        {
-                            data.state
-                                .client_compositor_state(&client)
-                                .blocker_cleared(&mut data.state, &data.display.handle())
-                        }
-                    });
-                });
             }
             self.state.loop_handle.insert_idle(move |data| {
                 data.state

src/layout.rs

@@ -4,14 +4,16 @@ use itertools::{Either, Itertools};
 use smithay::{
     desktop::layer_map_for_output,
     output::Output,
+    reexports::wayland_server::Resource,
     utils::{Logical, Point, Rectangle, Size},
+    wayland::compositor::{self, CompositorHandler},
 };
 
 use crate::{
     state::{State, WithState},
     window::{
         window_state::{FloatingOrTiled, FullscreenOrMaximized, LocationRequestState},
-        WindowElement,
+        WindowElement, BLOCKER_COUNTER,
     },
 };
 
@@ -110,6 +112,9 @@ impl State {
             }
         }
 
+        let mut pending_wins = Vec::<(Point<_, _>, WindowElement)>::new();
+        let mut non_pending_wins = Vec::<(Point<_, _>, WindowElement)>::new();
+
         for window in windows_on_foc_tags.iter() {
             window.with_state(|state| {
                 if let LocationRequestState::Sent(loc) = state.loc_request_state {
@@ -119,34 +124,103 @@ impl State {
                             // map the window.
                             if !win.toplevel().has_pending_changes() {
                                 state.loc_request_state = LocationRequestState::Idle;
-                                self.space.map_element(window.clone(), loc, false);
+                                non_pending_wins.push((loc, window.clone()));
+                                // TODO: wait for windows with pending state to ack and commit
+                                // self.space.map_element(window.clone(), loc, false);
                             } else {
                                 let serial = win.toplevel().send_configure();
                                 state.loc_request_state =
                                     LocationRequestState::Requested(serial, loc);
+                                pending_wins.push((loc, window.clone()));
                             }
                         }
                         WindowElement::X11(surface) => {
                             // already configured, just need to map
                             // maybe wait for all wayland windows to commit before mapping
-                            self.space.map_element(window.clone(), loc, false);
+                            // self.space.map_element(window.clone(), loc, false);
                             surface
                                 .set_mapped(true)
                                 .expect("failed to set x11 win to mapped");
                             state.loc_request_state = LocationRequestState::Idle;
+                            non_pending_wins.push((loc, window.clone()));
                         }
                     }
                 }
             });
         }
 
-        self.loop_handle.insert_idle(|data| {
+        BLOCKER_COUNTER.store(1, std::sync::atomic::Ordering::SeqCst);
-            crate::state::schedule_on_commit(data, windows_on_foc_tags, |dt| {
+        tracing::debug!(
+            "blocker {}",
+            BLOCKER_COUNTER.load(std::sync::atomic::Ordering::SeqCst)
+        );
+
+        let start_time = self.clock.now();
+
+        // Pause rendering. Here we'll wait until all windows have ack'ed and committed,
+        // then resume rendering. This prevents flickering because some windows will commit before
+        // others.
+        //
+        // This *will* cause everything to freeze for a few frames, but it should'nt impact
+        // anything meaningfully.
+        self.pause_rendering = true;
+
+        for (_loc, win) in pending_wins.iter() {
+            if let Some(surf) = win.wl_surface() {
+                tracing::debug!("adding blocker");
+                compositor::add_blocker(&surf, crate::window::WindowBlocker);
+            }
+        }
+
+        let pending_wins_clone = pending_wins.clone();
+
+        self.schedule(
+            move |_data| {
+                pending_wins_clone.iter().all(|(_, win)| {
+                    win.with_state(|state| state.loc_request_state.is_acknowledged())
+                })
+            },
+            move |data| {
+                // remove and trigger blockers
+                BLOCKER_COUNTER.store(0, std::sync::atomic::Ordering::SeqCst);
+                tracing::debug!(
+                    "blocker {}",
+                    BLOCKER_COUNTER.load(std::sync::atomic::Ordering::SeqCst)
+                );
+                for client in pending_wins
+                    .iter()
+                    .filter_map(|(_, win)| win.wl_surface()?.client())
+                {
+                    data.state
+                        .client_compositor_state(&client)
+                        .blocker_cleared(&mut data.state, &data.display.handle())
+                }
+
+                // schedule on all idle
+                data.state.schedule(
+                    move |_dt| {
+                        pending_wins.iter().all(|(_, win)| {
+                            win.with_state(|state| state.loc_request_state.is_idle())
+                        })
+                    },
+                    move |dt| {
+                        for (loc, win) in non_pending_wins {
+                            dt.state.space.map_element(win, loc, false);
+                        }
                         for win in windows_not_on_foc_tags {
                             dt.state.space.unmap_elem(&win);
                         }
-            })
+                        dt.state.pause_rendering = false;
-        });
+                        let finish_time =
+                            smithay::utils::Time::elapsed(&start_time, dt.state.clock.now());
+                        tracing::debug!(
+                            "spent {} microseconds not rendering",
+                            finish_time.as_micros()
+                        );
+                    },
+                );
+            },
+        );
     }
 }
 

src/state.rs

@@ -24,7 +24,7 @@ use crate::{
     grab::resize_grab::ResizeSurfaceState,
     metaconfig::Metaconfig,
     tag::TagId,
-    window::{window_state::LocationRequestState, WindowElement},
+    window::WindowElement,
 };
 use anyhow::Context;
 use calloop::futures::Scheduler;
@@ -50,7 +50,7 @@ use smithay::{
             Display, DisplayHandle,
         },
     },
-    utils::{Clock, IsAlive, Logical, Monotonic, Point, Size},
+    utils::{Clock, Logical, Monotonic, Point, Size},
     wayland::{
         compositor::{self, CompositorClientState, CompositorState},
         data_device::DataDeviceState,
@@ -138,25 +138,8 @@ pub struct State {
     pub xwayland: XWayland,
     pub xwm: Option<X11Wm>,
     pub xdisplay: Option<u32>,
-}
 
-/// Schedule something to be done when windows have finished committing and have become
+    pub pause_rendering: bool,
-/// idle.
-pub fn schedule_on_commit<F>(data: &mut CalloopData, windows: Vec<WindowElement>, on_commit: F)
-where
-    F: FnOnce(&mut CalloopData) + 'static,
-{
-    for window in windows.iter().filter(|win| win.alive()) {
-        if window.with_state(|state| !matches!(state.loc_request_state, LocationRequestState::Idle))
-        {
-            data.state.loop_handle.insert_idle(|data| {
-                schedule_on_commit(data, windows, on_commit);
-            });
-            return;
-        }
-    }
-
-    on_commit(data);
 }
 
 impl State {
@@ -376,6 +359,8 @@ impl State {
             xwayland,
             xwm: None,
             xdisplay: None,
+
+            pause_rendering: false,
         })
     }
 

src/window/window_state.rs

@@ -114,6 +114,24 @@ pub enum LocationRequestState {
     Acknowledged(Point<i32, Logical>),
 }
 
+impl LocationRequestState {
+    /// Returns `true` if the location request state is [`Idle`].
+    ///
+    /// [`Idle`]: LocationRequestState::Idle
+    #[must_use]
+    pub fn is_idle(&self) -> bool {
+        matches!(self, Self::Idle)
+    }
+
+    /// Returns `true` if the location request state is [`Acknowledged`].
+    ///
+    /// [`Acknowledged`]: LocationRequestState::Acknowledged
+    #[must_use]
+    pub fn is_acknowledged(&self) -> bool {
+        matches!(self, Self::Acknowledged(..))
+    }
+}
+
 impl WindowElement {
     /// This method uses a [`RefCell`].
     pub fn toggle_floating(&self) {