Process pending resize events before sizing viewport on startup

[Xyene]

Addresses #2447.

This commit processes pending resize events after glfwCreateWindow, so that glfwGetFramebufferSize returns the correct size for viewport sizing.

This fixes flickering visible when starting kitty in a tiling window manager like sway.

I've imported glfwPollEvents and supporting functions from glfw master.

[kovidgoyal]

This is not safe to do. Calling pollevents inside the function that creates a window means that some random callback that depend on a fully initialized window could be called, leading to bad things.

[kovidgoyal]

Do you notice the flicker when creating secondary OS windows in the same kitty instance, with ctrl+shift+n?

[Xyene]

Agreed on safety, though I figured it would've been fine here since it's being called prior to installing callbacks on the window, and the GIL is still held on the Python side so there's nothing that could enumerate the window before we return. Is this understanding wrong?

The issue is present in single-instance mode as well. The issue's cause is that glfwCreateWindow is called with 640x480, the window is actually created with size 640x480 and then immediately resized to the correct size of the tile. This hops through the Python side Window.set_geometry twice, once with a wrong geometry.

[kovidgoyal]

On Thu, Mar 19, 2020 at 07:24:10AM -0700, Tudor Brindus wrote: Agreed on safety, though I figured it would've been fine here since it's being called prior to installing callbacks on the window, and the GIL is still held on the Python side so there's nothing that could enumerate the window before we return. Is this understanding wrong?

It's not a threading issue, given the design of threading in kitty, and its possible that doing this is safe, given the lack of callbacks, but I'd really rather not go with "it's possible". And this is rather fragile as well, since it could change in the future, and whoever changes it would need to remember to ensure that this invariant does not change.

The issue is present in single-instance mode as well. The issue's cause is that `glfwCreateWindow` is called with 640x480, the window is actually created with size 640x480 and then immediately resized to the correct size of the tile. This hops through the Python side `Window.set_geometry` twice, once with a wrong geometry.

Isn't this fundamentally race-y though? You basically have to hope that the window manager resizes the window before the first window is created in the OS Window. There is no guarantee that will happen, and I suspect if your system is under load it will definitely not happen. The proper fix for this is to have a way for the window manager to signal an initial size when creating the window surface. I dont know if wayland has such a feature. But if it does, one could have the glfwCreateWindow implementation in wl_window.c use it to change the window size before returning. X11 for instance allows you to fetch just specific types of events from the queue, dont know wayland well enough to say if that is possible. If it is possible, then one could simply get the resize events of the queus inside glfwCreateWindow.

[Xyene]

And this is rather fragile as well, since it could change in the future, and whoever changes it would need to remember to ensure that this invariant does not change.

Agreed.

Isn't this fundamentally race-y though?

Yes :)

There is no guarantee that will happen, and I suspect if your system is under load it will definitely not happen.

Anecdotally, it happens reliably on my machine even with all cores pegged to 100% via stress. I imagine the WM sends the resize event immediately. The failure condition when the resize event isn't received in time isn't catastrophic, it just degrades the visual output to what is currently the default behaviour. I suppose it comes down to if you want to have a hack supporting a quality-of-life improvement or not.

Concurrently with opening this PR, I asked around in #sway-devel whether the resize event was in any way sent "atomically" at window creation time. The answer I got was no, and that the underlying issue is swaywm/sway#2176 — unfortunately, that issue hasn't seen activity since 2018.

That said, kitty is one of very few Wayland-native applications that aren't slow, so I imagine quality-of-life improvements might go a long way. I can understand if you don't wish to have a hack like this in place (using a tiling WM on Wayland seems like a niche within a niche), and am satisfied with simply applying it locally whenever I update kitty, but it does seem generally useful for those in that niche.

I was curious to see what alacritty does, since it doesn't have visual glitches on startup. It seems they just handle the resize event normally, and somehow there are no glitches. I don't imagine the Python bit of kitty would be slow enough to add up to ~100ms per resize, but since alacritty doesn't use GLFW I don't think there's any immediately useful information to gain from there.

[kovidgoyal]

On Thu, Mar 19, 2020 at 11:17:42AM -0700, Tudor Brindus wrote: > And this is rather fragile as well, since it could change in the future, and whoever changes it would need to remember to ensure that this invariant does not change. Agreed. > Isn't this fundamentally race-y though? Yes :) > There is no guarantee that will happen, and I suspect if your system is under load it will definitely not happen. Anecdotally, it happens reliably on my machine even with all cores pegged to 100% via `stress`. I imagine the WM sends the resize event immediately. The failure condition when the resize event isn't received in time isn't catastrophic, it just degrades the visual output to what is currently the default behaviour. I suppose it comes down to if you want to have a hack supporting a quality-of-life improvement or not.

I mean its up to the CPU scheduler no? The scheduler could simply refuse to schedule the compositor for a second after kitty recceives the initial surface.

Concurrently with opening this PR, I asked around in #sway-devel whether the resize event was in any way sent "atomically" at window creation time. The answer I got was _no_, and that the underlying issue is swaywm/sway#2176 — unfortunately, that issue hasn't seen activity since 2018.

That's too bad.

That said, kitty is one of _very_ few Wayland-native applications that aren't slow, so I imagine quality-of-life improvements might go a long way. I can understand if you don't wish to have a hack like this in place (using a tiling WM on Wayland seems like a niche within a niche), and am satisfied with simply applying it locally whenever I update kitty, but it does seem generally useful for those in that niche.

I am fine with a hack as long as the hack is not fragile/could lead to crashes. For instance it might be better to expose pollevents to pthon and call it when adding the first window to an OS Window.

[Xyene]

Alright, here's another take on it based on the discussion: on Wayland, wait 5ms for any resize events to appear before returning from glfwCreateWindow. At this point we will have no callbacks installed, and it seems harder to accidentally mess things up in the future.

This seems to actually work better than the original PR, since it also eliminates the first "stage" from the linked issue, whereas before only the second, more visible, glitch would be hidden. (I think because it gets in before glfwSwapBuffers?) 5ms seems like a good trade-off, in the frequent case where it avoids a ~100ms resize.

[emersion]

This is caused by a Sway bug I believe: swaywm/sway#2176

Please don't merge any Sway workarounds in Kitty. Sway bugs ought to be fixed in Sway, not in each and every client.

[Xyene]

Since @emersion is a major contributor to Sway and objects to this patch, I'll honor their wishes and close this PR. Until such a time when the Sway bug is fixed, I suppose anyone else encountering this issue who searches the issue tracker will find this patch easy enough to apply.

[kovidgoyal]

I agree that we should avoid compositor specific workarounds as far as possible. That said, I was thinking about this (in the shower) and it occurs to me that kitty already uses the mechanism for requesting render frames from the compositor and only renders after receiving one (grep for USE_RENDER_FRAMES). Assuming the compositor sends the initial resize event before the first render frame, it may be possible to refactor the main loop a bit to ensure the resize event is fully processed before rendering.

[kovidgoyal]

Although I guess reading the sway bug, it wont work, since sway only sends the correct size after the first render.

[Xyene]

I guess it's alright, at the end of the day it's just another patch to maintain locally, but I already have so many for fixing random problems in programs I use that one more isn't a big deal. Thanks for your time in reviewing this PR, though :)