set_inner_size called from inside the event loop causes issues on wayland

[m4b]

Trying to repro some wgpu-rs traces using wgpu player: https://github.com/gfx-rs/wgpu/blob/000bf85a20c00e88b847acf74701a325386edac0/player/src/main.rs#L540

Only on archlinux wayland, do I get what appears to be a deadlock of some kind? Specifically, an application icon becomes present, but no window is displayed; stopping in debugger reveals:

(gdb) bt
#0  0x00007ffff7f63620 in __lll_lock_wait () from /usr/lib/libpthread.so.0
#1  0x00007ffff7f5bdf3 in pthread_mutex_lock () from /usr/lib/libpthread.so.0
#2  0x0000555555cea94d in std::sys::unix::mutex::Mutex::lock (self=0x5555565dca10) at /rustc/8d69840ab92ea7f4d323420088dd8c9775f180cd/src/libstd/sys/unix/mutex.rs:57
#3  0x0000555555d1261a in std::sys_common::mutex::Mutex::raw_lock (self=0x5555565dca10)
    at /rustc/8d69840ab92ea7f4d323420088dd8c9775f180cd/src/libstd/sys_common/mutex.rs:41
#4  0x0000555555cc3856 in std::sync::mutex::Mutex<T>::lock (self=0x55555653b9f0) at /rustc/8d69840ab92ea7f4d323420088dd8c9775f180cd/src/libstd/sync/mutex.rs:218
#5  0x000055555593621b in winit::platform_impl::platform::wayland::window::Window::set_inner_size (self=0x7fffffffaaa8, size=...)
    at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/platform_impl/linux/wayland/window.rs:276
#6  0x000055555577eca9 in winit::platform_impl::platform::Window::set_inner_size (self=0x7fffffffaaa0, size=...)
    at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/platform_impl/linux/mod.rs:294
#7  0x0000555555929722 in winit::window::Window::set_inner_size (self=0x7fffffffaaa0, size=...)
    at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/window.rs:493
#8  0x00005555558edaee in player::main::{{closure}} (event=..., control_flow=0x7fffffff9d20) at player/src/main.rs:540
#9  0x000055555586e4e3 in winit::platform_impl::platform::sticky_exit_callback (evt=..., target=0x7fffffffb1d0, control_flow=0x7fffffff9d20, callback=0x7fffffffaaa0)
    at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/platform_impl/linux/mod.rs:698
#10 0x00005555556f06fd in winit::platform_impl::platform::wayland::event_loop::EventLoop<T>::run_return::{{closure}} (wid=..., window_target=0x7fffffffb1d0)
    at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/platform_impl/linux/wayland/event_loop.rs:524
#11 0x00005555556f097a in winit::platform_impl::platform::wayland::event_loop::EventLoop<T>::redraw_triggers::{{closure}} (refresh=true, frame_refresh=true, wid=..., 
    frame=...) at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/platform_impl/linux/wayland/event_loop.rs:678
#12 0x000055555577f131 in winit::platform_impl::platform::wayland::window::WindowStore::for_each_redraw_trigger (self=0x5555565285c0, f=...)
    at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/platform_impl/linux/wayland/window.rs:547
#13 0x00005555556f083e in winit::platform_impl::platform::wayland::event_loop::EventLoop<T>::redraw_triggers (self=0x7fffffffb110, callback=...)
    at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/platform_impl/linux/wayland/event_loop.rs:667
#14 0x00005555556ef9a2 in winit::platform_impl::platform::wayland::event_loop::EventLoop<T>::run_return (self=0x7fffffffb110, callback=...)
    at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/platform_impl/linux/wayland/event_loop.rs:523
#15 0x00005555556f185b in winit::platform_impl::platform::wayland::event_loop::EventLoop<T>::run (self=..., callback=...)
    at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/platform_impl/linux/wayland/event_loop.rs:455
#16 0x000055555586e320 in winit::platform_impl::platform::EventLoop<T>::run (self=..., callback=...)
    at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/platform_impl/linux/mod.rs:644
#17 0x00005555558ee9a4 in winit::event_loop::EventLoop<T>::run (self=..., event_handler=...)
    at /home/m4b/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.22.1/src/event_loop.rs:149
#18 0x000055555582b0fe in player::main () at player/src/main.rs:529

@kvark was wondering if the set inner size is allowed in the inner loop there, or what could be going wrong here on wayland?

Any suggestions would be great, thanks!

[kchibisov]

Could you retest with latest winit?

[m4b]

This appears to still be present on latest winit 49bcec1

[kchibisov]

Ok, it shouldn't block and seems like a bug in winit + Wayland backend for now. Using set_inner_size isn't something reliable I'd say(system can ignore your request, like tiling compositor or something like that), so basically you should just set some size on startup and then resize on Resize events.

However it doesn't justify winit bug.

[kchibisov]

Could you provide an example for me to repro it?

[kchibisov]

Ok, I've see the issue, so maybe you don't need to provide an example for me. It seems like calling anything window frame related from the event_loop on Wayland could block, since run_return also holding a lock on a window frame.

[m4b]

Unzip this:
shadow.trace.zip

Place it somewhere, e.g. in /tmp/; then build player in https://github.com/gfx-rs/wgpu:

cd player
cargo run --features winit -- /tmp/shadow.trace

[m4b]

The issue is indeed here: https://github.com/gfx-rs/wgpu/blob/000bf85a20c00e88b847acf74701a325386edac0/player/src/main.rs#L540

I've tried mitigating this by scanning the action list, and popping the swapchain creation call, but there can technically be more than 1, and they're a stream, where the previous gpu state, iiuc, requires the previous dimensions, and points after require the newer dimensions, etc. So wasn't as straightforward as workaround as I thought

[kchibisov]

I'll try to create some patch to fix it now. I'm not sure if it'll be the way to go, but I guess winit can just release the lock on a frame.

[m4b]

e.g., here is what i added before the winit event loop:

        // because: https://github.com/rust-windowing/winit/issues/1571
        while let Some(idx) = actions
            .iter()
            .position(|action| matches!(action, trace::Action::CreateSwapChain { .. }))
        {
            if let trace::Action::CreateSwapChain { id, desc } = actions.remove(idx) {
                log::info!("Initializing the swapchain");
                assert_eq!(id.to_surface_id(), surface);
                window.set_inner_size(winit::dpi::PhysicalSize::new(desc.width, desc.height));
                gfx_select!(device => global.device_create_swap_chain(device, surface, &desc));
            }
        }

this will panic though, because there are two swapchain creations, one with 570, another with 600:

    
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `Extent { width: 800, height: 570, depth: 1 }`,
 right: `Extent { width: 800, height: 600, depth: 1 }`: Extent state must match extent from view', <::std::macros::panic macros>:5:6
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[kchibisov]

It seems like it's somehow getting the size with decorations, which it shouldn't get in any way. Just ensuring, are you testing against winit 0.22.2 here?

[m4b]

I'm on master afaik:

diff --git a/player/Cargo.toml b/player/Cargo.toml
index cb16543..3cf5829 100644
--- a/player/Cargo.toml
+++ b/player/Cargo.toml
@@ -20,7 +20,7 @@ log = "0.4"
 raw-window-handle = "0.3"
 renderdoc = { version = "0.8", optional = true, default_features = false }
 ron = "0.5"
-winit = { version = "0.22", optional = true }
+winit = { git = "https://github.com/rust-windowing/winit", version = "0.22", optional = true }
 
 [dependencies.wgt]
 path = "../wgpu-types"

[kchibisov]

Can you try #1572 with set_inner_size?

[m4b]

yup confirmed, this fixes it!