Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leak and stopped animation when connecting/disconnecting monitor #375

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kerty0
Copy link
Contributor

@kerty0 kerty0 commented Oct 31, 2024

  1. Memory leak when connecting/disconnecting monitor (happens on KDE Plasma and Hyprland): if I understand it correctly, Compositor sends LayerSurface closed event which removes Wallpaper belonging to closed surface from Daemon.wallpapers, later Compositor sends WL_REGISTRY global_remove which tries to delete Wallpaper belonging to closed output and stop it animation by searching Daemon.wallpapers by output_name, but fails because Wallpaper has already been removed. This leads to not deleted TransitionAnimator and ImageAnimator, which stops Wallpaper from being dropped and starts memory leak in buffer leading to swww-daemon termination.
    Because I don't know right way to handle this I just duplicated code from global_remove event to closed event so both now stop animations.
  2. Stop of animation on primary monitor when connecting/disconnecting second one (only on Hyprland): when disconnecting second, monitor Hyprland sends wl_output done event for primary monitor, and when connecting second monitor Hyprland sends sequence of events with done event in the end for both monitors. wl_output done event stops animations so connecting/disconnecting second monitor stops wallpaper animation on first one.
    I don't know right way to handle this either so I just removed stop_animations from wl_output done event.

@LGFae
Copy link
Owner

LGFae commented Oct 31, 2024

I will have to test this manually (might take a little bit), but I am pretty sure solution 2 is wrong because animations will continue to play if you change the output's configuration. Looking at Wallpaper::commit_surface_changes, it appears to return a boolean indicating whether or not we have to stop the animation. I think you can just use it instead. This is what we do in the preferred_scale function:

if wallpaper
       .borrow_mut()
       .commit_surface_changes(self.use_cache)
{
        self.stop_animations(&[wallpaper.clone()]);
}

Solution 1, on the other hand, appears fine.

@kerty0
Copy link
Contributor Author

kerty0 commented Nov 1, 2024

I thinking about adding to Daemon something like this:

fn reconfigure_wallpaper(&self, output_name: String) {
    std::thread::Builder::new()
        .name("wallpaper reconfigurator".to_string())
        .stack_size(1 << 14)
        .spawn(move || {
            if let Err(e) = || -> io::Result<()> {
                std::process::Command::new("swaww")
                    .arg("restore")
                    .arg(format!("--outputs={}", output_name))
                    .spawn()?
                    .wait()?;
                Ok(())
            }() {
                warn!("failed to reconfigure wallpaper: {e}");
            }
        })
        .unwrap();
}

And calling it when scaling or resolution of monitor changes so wallpaper always displays correctly. What do you think?

@LGFae
Copy link
Owner

LGFae commented Nov 1, 2024

We are already doing this though. When we receive an output done event we call wallpaper::commit_surface_changes, which calls common::cache::load in a different thread.

There is a problem in that if the daemon is called with the --no-cache flag, then it will never reload the image, even when the output is reconfigured. Maybe that's the problem you would like to solve?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants