Deal with scale factor changes/configure races on Wayland

[kchibisov]

When doing rendering on a different thread than the event loop is located and not doing any sort of synchronization with the way rendering is being done winit could change the scale factor of the buffer the surface will submit, leading to the protocol errors due to a fact that the buffer is not
dividable by the scale factor of the buffer.

The same situation happens with the configure events which we should generally manually ack, but we just ack unconditionally, so the client could theoretically render with the wrong configure information.

To solve this one solution would be to let the users confirm the scaling/sizes they are trying to use before they do the actual rendering with the serial of the configure (could be encapsulated inside the Resized event), etc. The same should apply to scale they are using.

We've developed a concept of Window::pre_present_notify in #2896 , but it doesn't really help here, because it happens right before the presentation.

So what we'll need is Window::pre_rendering_notify(Resized, Scale) (maybe something else?), so the clients could manually provide the sizes they actually end up using and once they do so, winit will update its inner logic and set the scale factors clients have picked.

• I have no clue how it'll all work with the other backends, they likely don't need such a messy handling.
• I don't think all of that could be done automatically.
• Even if we have a special event users must draw on it's all weird with the Window being on the different thread, because realistically you don't have to wait for the event.
• Having to wait is a real issue if you have like 20 windows each using its own thread, because you don't want to wait 20 threads to finish their rendering, it's just a nonsense thing.

Maybe we should have an opt-in behavior for all of that, like when building window, because realistically only multithreaded clients do want this sort of sync, but it'll be a blood mess to maintain internally, since sometimes you maintain configure queue, sometimes don't, sometimes you apply the scaling, sometimes don't, it'll be just all over the place?

--

Alternative name to all of that could be Window::ack_size_and_scale(Resized, scale). Also, it's not clear in which form we should accept size and scale in such function, I'd assume that we'd need serials, and not real sizes.

[kchibisov]

cc @notgull @rib @i509VCB on some thoughts how such a thing could be approached.

cc @fredizzimo as one of the potential consumers of said API.

The issue was raised here neovide/neovide#1870 (comment) . And it's a real world issue we should help solving.

[i509VCB]

A solution to avoid the race is to make the thread that is rendering set the scale vs having it be some part of the main winit event loop. The main event loop will just tell you preferences for scale.

[kchibisov]

@i509VCB that's what I've said in the proposal if you've read it, the thing is that do we really want to force the burden on the users to do that? And how the API should look for it? What other states we might want to commit from the rendering thread? Do you happen to know if other platforms have similar things (at least X11, but I don't think it has)?

Like we can solve this, it's a matter of convenience for the end users.

[fredizzimo]

@kchibisov, your proposal sounds good. But I'm not sure how critical it is really.

For Neovide, the rendering thread is mostly optional, except on Windows, where we use it to work around some of the slow event processing: #2782 (I'm sorry, I still haven't had time to look at @rib's work). The rendering on the other platforms are also done in separate threads for consistency between them, but I don't think it would be a big deal to refactor it to be Windows only.

[kchibisov]

@fredizzimo I mean there's a clear issue, working around the real issue is just not how software should be written in the first place. My only concern is how to make it all convenient for the end users to use. And what should we do on platforms outside of the wayland.

[kchibisov]

The other issue is that some platforms really want you to draw synchronously, like we have macOS. And I'm not sure what it supposed to do with e.g. But I'd assume they want so only when the window is being resized, winit for example could pass a fence to the user to signal along the resize/scale factor changes? So winit for example could stay in the drawRect for longer than it should waiting for such fence to be signaled.

[notgull]

On the X11 end, the RandR extension can change the DPI of the window. Windows also has the WM_DPICHANGED method. That being said, I'm not sure how this affects rendering in the worst case scenario.

My knee-jerk reaction would be to introduce an RAII API on Window, like so:

impl Window {
    pub fn begin_render(&mut self) -> WindowRender<'_>;
}

pub struct WindowRender<'a> { /* … */ }

impl<'a> WindowRender<'a> {
    pub fn begin_present(&mut self);
    pub fn ack_configure(self) { drop(self); }
}

When you want to start rendering, you would call begin_render(). At this point, configure/DPI events would be delayed and not acked (to the greatest extent possible by the backend). Once you’re about to present, you would call begin_present for the Wayland frame callbacks. Finally once rendering is done and everything is presented, you would call ack_configure to drop the lock and then ack all of the events that were previously delayed.

Not sure if this would fix Wayland, but I think that all of this could probably be implemented as a no-op for most backends. There’s probably some optimizations for thread-safety versus thread-unsafety, as rendering on another thread is a weird use case.

[kchibisov]

The issue is acking the right sizes here, so the users won't draw with 101x50 buffer when the scaling is set to 2 for example. So maybe passing PhysicalSize and scale_factor in begin_render is what should we do? And we ack them once you call begin_render?

Like the whole issue here is not to ack in the right time but ensure that what client uses for sizes and scale makes sense. What you suggest is basically a fence approach, maybe we should pass the fence in RedrawRequested? So we could sync things on macOS? The issue with such a fence would be on how to move it across the threads in a sane way if we, let's say deliver it along the RedrawRequsted as well.

[notgull]

The issue is acking the right sizes here, so the users won't draw with 101x50 buffer when the scaling is set to 2 for example. So maybe passing PhysicalSize and scale_factor in begin_render is what should we do? And we ack them once you call begin_render?

This seems like it would be the best approach to me.

Like the whole issue here is not to ack in the right time but ensure that what client uses for sizes and scale makes sense. What you suggest is basically a fence approach, maybe we should pass the fence in RedrawRequested? So we could sync things on macOS? The issue with such a fence would be on how to move it across the threads in a sane way if we, let's say deliver it along the RedrawRequsted as well.

Passing it along with RedrawRequested is probably the best solution from a type system standpoint. Maybe it's !Send by default, but it has a method that acquires an atomic lock and then makes it Send?

[kchibisov]

The issue with the RedrawRequested though is that we need to ack/set_scale before we start rendering and we don't populate RedrawRequested due to resize events. We also need a manual way to ack all of that, because the user could be in the middle of the rendering.

Some users could also do contiguous drawing without using RedrawRequested and we should account for them as well, so it means a separate method will be required on the window.

Other issue is that drawRect on macOS and what we expect from it, I start to thing that holding drawRect from returning is a completely different issue to the original issue. So RedrawRequested at this point sounds like a completely different issue to what we have and will require different solution.

And as for the original issue should be solved with the Window::begin_render like function, and maybe the way winit decides whether it does implicit configure acking and setting scale based on some event loop creation flag? Because RAII can't really help here, unless we somehow pass the RAII object along the resizes/scale factor changes? Such object should be without a lifetime as well for example (like Arc?). I think that way it'll work automatically, since multithreaded examples could store this fence somewhere or send to another thread.

[kchibisov]

And as for the original issue should be solved with the Window::begin_render like function, and maybe the way winit decides whether it does implicit configure acking and setting scale based on some event loop creation flag? Because RAII can't really help here, unless we somehow pass the RAII object along the resizes/scale factor changes? Such object should be without a lifetime as well for example (like Arc?). I think that way it'll work automatically, since multithreaded examples could store this fence somewhere or send to another thread.

Hm, no, the main issue with such fence is that only on Wayland the user controls the scale factor of the window, so for something like macOS it won't make any sense and will be just confusing. I'd assume on X11/Windows it'll be pointless and confusing as well. So I guess a flag on the event loop and begin_rendering is what we need...

[kchibisov]

Maybe what we should do with scaling is to tell when it's a suggestion and when it's a mandatory change, so the clients can pick how they want to deal with them? For example, on Wayland the scale is a suggestion, and the client would need to tell winit what scale factor it should be using, but on other backends this is a bit different.

So for example a suggestion scale would imply that the client must communicate to winit (we can just deliver a fence and make the client auto agree when passing back to window), but for the rest of the backends we could just say that it was changed and now you should deal with it.

Other possible solution just for Wayland could be, to change the scale for events, but not touch the buffer scale and let the client do the scale change in such a case on the window. We'd simply have a call like Window::set_buffer_scale and make the clients require it when enabling something like with_multithredad_rendreing for event loop.

There're other alternatives, like fences, but they might not work reliably...