Implement DPI Usability Upgrades for X11 and Wayland

[murarth]

• Tested on all platforms changed
• Compilation warnings were addressed
• cargo fmt has been run on this branch
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality
• Updated feature matrix, if new features were added or implemented

[murarth]

I don't have any high DPI monitors with which to test these changes, so both the X11 and Wayland implementations will need to be thoroughly tested by others.

[goddessfreya]

@murarth Afaik, if you do some fiddling with xrandr, you could probably pretend to have a larger DPI then normal, altho I can't recall the exact command.

[goddessfreya]

Okay, so first I tried using --dpi and --fbmm, but obviously if things worked the first time it wouldn't be X11. Anyways, I spent an hour and I found an article mentioning using --scale-from, and bam, it works:

xrandr --output VGA-0 --scale-from 3840x2160 --right-of HDMI-A-0 --mode 1920x1080

And yes, it broke my monitor if I only did xrandr --output VGA-0 --scale-from 3840x2160. Imagine the joy.

And yes, of course it broke some windows on the non-scaled (HDMI-A-0) display, just take a look at keepassxc: https://imgur.com/UVt36Md.png

I also learned firefox doesn't support proper DPI support, as I can barely read this text as I type it :/.

Testing the application, the window appears to more-or-less double it's size when traveling between the two windows, as expected.

One thing of interest is that during the resizeable example, winit will still change the size when dragging the window between monitors. What is the intended behavior here, @Osspial ?

Due to shear lazyness, I have not tested if the window locks up during resizing, and I'm hoping one of our testers can do this. @FliegendeWurst are you up for this task? You'ld need to modify the winit examples to: 1- Use 'Poll` 2- Measure the elapsed time between events, and make sure resizing the thing vigourously doesn't stop the events from printing or cause large delays. Thanks :p

I couldn't test the wayland backend, cause I don't know how to make weston work multiscreen. I think I'd have to set up a new dm like sway and boot into that, which sounds hard. I'm not really sure how this stuff works, so maybe you can look into this, @0xpr03?

[0xpr03]

I sadly can't seem to get the tests running on weston. No window show up in weston.
Edit: It's also broken for me on current master & 0.19.1. I don't get any windows under weston or kwin (wayland).

[goddessfreya]

@0xpr03 Are you starting the examples from a terminal inside weston?

[0xpr03]

Yes I am. It seems like mouse positions are there but no window.

[goddessfreya]

Maybe git bisect it and find where it first breaks?

[0xpr03]

Ok so window display broke somewhere between 0.13 and 0.14 and seems to be broken since.

[0xpr03]

Note that the example doesn't crash. It says to have acquired a window ID, it'll also display mouse movements, just no window.

[goddessfreya]

Note that the example doesn't crash. It says to have acquired a window ID, it'll also display mouse movements, just no window.

That's because nothing is drawing to the window. Wayland works slightly differently. You got to test it using glutin, with a [replace] placed in glutin's top level Cargo.toml

[0xpr03]

But why does 0.13.0 work then, with a blank window ? At least we should get the same behavior back.

[0xpr03]

You got to test it using glutin, with a [replace] placed in glutin's top level Cargo.toml

Using

[replace]
"winit:0.20.0-alpha1" = { path = '../winit' }

Won't work that well, at least with this PR, as examples uses alpha-1, glutin alpha-2. This branch targets alpha-1. It might be that I'm doing something wrong but that's the state.
Also running a clean gluting in weston will panic with eglCreateWindowSurface failed. It works on x11 on the host. It could be a nvidia thing, might retry with nvidia-passthrough tomorrow.

[murarth]

I've noticed an issue on Wayland: Even when using ControlFlow::Wait, the event loop will constantly wake up with no events (other than NewEvents and EventsCleared). I found the cause quite easily, but I'm uncertain how to fix it.

The cause is that I've switched from using mio instead of calloop to drive the event loop, as more flexibility was required to handle the new HiDpiFactorChanged event. Previously, the EventQueue was being registered with calloop, but with a no-op closure to consume the data. Now, mio is always reporting that readable data exists on the Wayland client connection, but it's not being consumed by the main event loop.

[0xpr03]

I'm not sure how this affects it but currently winit seems to be broken since 0.14 in wayland, showing no window. I'll try to bisect a little further tomorrow.
Sidenode: Found a eglCreateWIndowSurface failure panic on nvidia I can't circumvent when using glutin.

[murarth]

I've pushed a commit that I believe solves the primary Wayland issue. EventQueue has an implementation for the calloop trait EventSource, which reads data from the connection. Now that mio is used instead, winit must explicitly read data from the EventQueue.

[0xpr03]

That fixes it.

[0xpr03]

The window is now so small that it's just a titlebar, also on x11 but I think that's unrelated, or did we change something here ?

[elinorbgr]

About no window showing on Wayland, this is actually expected behavior: the way wayland works is that a window does not appear to the screen until its contents have been drawn at least once. In earlier versions, winit used to setup the window differently, but it caused other, more serious issues. My most recent attempt to circumvent that led to #981 . Ideally we'd need a wayland-specific example that draws dummy content in the window, just for tests. And for mio, it is indeed the expected behavior, whenever it notifies that the socket is readable, winit should read & dispatch all available events. Le 13 août 2019 00:32:07 GMT+02:00, Aron Heinecke <notifications@github.com> a écrit :

…

I'm not sure how this affects it but currently winit seems to be broken since 0.14 in wayland, showing no window. I'll try to bisect a little further tomorrow. Sidenode: Found a eglCreateWIndowSurface crash on nvidia I can't circumvent when using glutin. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: #1098 (comment)

[aleksander]

Ideally we'd need a wayland-specific example that draws dummy content in the window, just for tests.

I'll try to reuse code from #780 to implement this.

[kchibisov]

@murarth I can help with testing on wayland with HiDPI (I have 4k and 2k monitors and can hopefully test this PR in alacritty).

Just a small note, it seems like #981 wasn't rebased to your PR. So testing things on wayland are a little bit problematic without it, since I don't know whether the issue is coming from your PR or #981. You can take a look on how alacritty renders without this fix applied here (screenshot in this comment).

[goddessfreya]

@murarth Have you backported #981?

[chrisduerr]

However the main concern I have is that run_return API was changed a bit, so our current run_return "callback" wasn't compiling due to lifetime problems, which I've fixed for now with std::mem::transmute.

This is likely unsafe. I'm not entirely certain about the changes, since it seems to infer lifetime, but if this has caused run_return to require a static lifetime, that would make this unusable for Alacritty.

[Osspial]

@kchibisov @chrisduerr I don't see the transmute in that link, but it shouldn't be necessary. My hunch is that it's happening because Event now has a lifetime for the HiDpiFactorChanged event, but event_queue needs a 'static lifetime because it lasts longer than a single invocation of the event handler function.

[kchibisov]

I don't see the transmute in that link

I've just linked our evlp2 branch and the code we're expecting to work without unsafe tricks in alacritty.

My hunch is that it's happening because Event now has a lifetime for the HiDpiFactorChanged event, but event_queue needs a 'static lifetime because it lasts longer than a single invocation of the event handler function.

You're absolutely correct here. In my local patched version I have to write something like this for now.

event_queue.push(unsafe { std::mem::transmute(event) });

I'm aware of to_static method, but that doesn't help us unfortunately.

[Osspial]

@kchibisov I've done some work on creating an async event loop wrapper for Winit, which lets you structure the event loop like this:

event_loop.run_async(async move |mut runner| {
    'main: loop {
        runner.wait().await;

        let mut recv_events = runner.recv_events().await;
        while let Some(event) = recv_events.next().await {
            match event {
                Event::WindowEvent {
                    event: WindowEvent::CloseRequested,
                    ..
                } => {
                    break 'main;
                },
                _ => println!("{:?}", event),
            }
        }

        window.request_redraw();

        let mut redraw_requests = recv_events.redraw_requests().await;
        while let Some(window_id) = redraw_requests.next().await {
            println!("redraw {:?}", window_id);
        }
        println!();
    }
})

That work isn't complete, and it hasn't been integrated with the DPI upgrades yet, but once that's integrated you may be able to use that to get rid of the event buffer entirely and avoid the safety issues you're running into.

[Osspial]

What's the status on this?

[goddessfreya]

I'm not quite sure If I implemented all new changes correctly in alacritty, but at least it was running and handling dpi changes correctly, there were some startup issues from time to time on Wayland, since I'm not very familiar with new structure in evlp2 and probably missed handling of something.

I suspect the startup issues are from the backporting or some other bug. I think this is good to merge, no?

[kchibisov]

FYI, startup issues are due to this #1119

[chrisduerr]

@murarth Just out of curiosity, is there a reason why the HiDPIFactorChanged event requires a borrow? It seems like it should be Copy, right?

[murarth]

@chrisduerr I'm just following the API as designed. I raised that same question in the tracking issue: #939 (comment)

[chrisduerr]

I guess I just don't understand why we would want to modify a mutable reference in the HiDPI event to change the window size. It seems like that could be extremely confusing an unexpected behavior.

It seems like it would be way easier to just change the window size explicitly on the window.

However I'm sure @zegentzy had a good reason for this, possibly requested by other dowstream users. So this PR is not really the best place to discuss this, sorry.

[Osspial]

@chrisduerr I was the one that made the decision to include a lifetime, and here's my main reasons for doing that:

1. This is by far the cleanest way for us to implement it internally and expose it as an API. The resizing logic can behave significantly differently when you're resizing in response to a DPI switch, and there's no way for us to reliably account for that once the event handler is over. Using a lifetime forces the user to set it at the right time so we can handle the resize properly.
2. We're likely going to need to introduce a lifetime to expose other events cleanly (Sizing comes to mind), so introducing it here is a good way to make the major breaking change and minimize disruption in the future. HiDpiFactorChanged benefits from the lifetime as illustrated above, so it's a natural place to do that.