[PATCH 0/1] MR5418: winewayland: Move wayland_surface_ensure_contents to wayland_vkCreateWin32SurfaceKHR.
I would like to get rid of the driver-specific swapchain code and this is getting in the way. Any reason not to do that? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5418
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winewayland.drv/vulkan.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/dlls/winewayland.drv/vulkan.c b/dlls/winewayland.drv/vulkan.c index ec5e9fe6ae8..231f82a06bd 100644 --- a/dlls/winewayland.drv/vulkan.c +++ b/dlls/winewayland.drv/vulkan.c @@ -218,17 +218,6 @@ static VkResult check_queue_present(const VkPresentInfoKHR *present_info, int client_height = wayland_surface->window.client_rect.bottom - wayland_surface->window.client_rect.top; - wayland_surface_ensure_contents(wayland_surface); - - /* Handle any processed configure request, to ensure the related - * surface state is applied by the compositor. */ - if (wayland_surface->processing.serial && - wayland_surface->processing.processed && - wayland_surface_reconfigure(wayland_surface)) - { - wl_surface_commit(wayland_surface->wl_surface); - } - pthread_mutex_unlock(&wayland_surface->mutex); if (client_width == wine_vk_swapchain->extent.width && @@ -359,6 +348,17 @@ static VkResult wayland_vkCreateWin32SurfaceKHR(VkInstance instance, goto err; } + wayland_surface_ensure_contents(wayland_surface); + + /* Handle any processed configure request, to ensure the related + * surface state is applied by the compositor. */ + if (wayland_surface->processing.serial && + wayland_surface->processing.processed && + wayland_surface_reconfigure(wayland_surface)) + { + wl_surface_commit(wayland_surface->wl_surface); + } + wine_vk_surface->client = wayland_surface_get_client(wayland_surface); pthread_mutex_unlock(&wayland_surface->mutex); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5418
Unfortunately the proposed move breaks a few important presentation scenarios for GL/Vulkan contents. Here are the relevant details: 1. The OpenGL/Vulkan rendering occurs in a subsurface placed over the client area of the main window surface. Wayland subsurfaces are only shown if the parent surface is mapped. 2. Surface reconfiguration (ack-ing new state etc) happens primarily during a (window_surface_)flush to ensure contents (e.g., size) and surface state are in sync and compatible in the same surface commit (lest we violate state constraints and the compositor disconnects us). Many games do not perform any non-Vulkan/GL draw to the window when fullscreen, thus no (window_surface_)flush for the window, and in such cases: For (1), committing contents to the subsurface will not show anything on screen unless we ensure the main window surface is mapped. Moving the `wayland_surface_ensure_contents` operation to VkSurface creation is not enough, since the associated window may not even be visible at that point (in which case we don't map), and then visibility can arbitrarily change during the lifetime of the VkSurface. For (2), the only other chance (outside window_surface_flush) we get to reconfigure is when Vulkan/GL presents the contents. Doing this during VkSurface creation is not enough, since reconfigures (e.g., changing to fullscreen) don't typically involve the application recreating the VkSurface. Sometimes they might not even involve recreating the VkSwapchain, e.g., if the size doesn't change. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5418#note_66458
Hmm okay, but does it need to be done before vkQueuePresent? Regarding `wayland_surface_ensure_contents`, could we just for instance ensure the surface has contents when it is resized / made visible in wayland_WindowPosChanged? With `wayland_surface_reconfigure`, it looks awkward that the client drawing is driving the window surface size, I think it should be done separately and client drawing should not involve any windowing logic but simply return errors if the size doesn't match. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5418#note_66463
Hmm okay, but does it need to be done before vkQueuePresent? Regarding wayland_surface_ensure_contents, could we just for instance ensure the surface has contents when it is resized / made visible in wayland_WindowPosChanged?
Here are some concerns: * If we unconditionally call `wayland_surface_ensure_contents` on resize/visibility, in most cases we will get another flush once the application actually draws, and end up with double commit, which is both inefficient and can cause flicker (since the first commit will be empty). * If we try to avoid the double commit, by detecting that the surface has old contents, but then the window surface is never flushed, we will get stuck with the old, incorrectly sized buffer (the committed buffer size matters for configuration reasons). In essence, in WindowPosChanged we don't have all the information to properly decide whether we want to "ensure contents", and doing it unconditionally has some cost. The current approach avoids these problems by only performing the "ensure contents" commit in the few cases it is absolutely needed (i.e., when we want to show the GL/Vulkan subsurface contents and won't be able to otherwise).
With wayland_surface_reconfigure, it looks awkward that the client drawing is driving the window surface size, I think it should be done separately and client drawing should not involve any windowing logic but simply return errors if the size doesn't match.
This seems awkward because the Wayland surface model is different: surfaces have no independent inherent size, it's the buffer size (+transformations) that determines the "surface size", which makes drawing and sizing interlinked. In Wayland's model ack-ing a new state and providing new contents (buffer + transformations) for that state should ideally happen in the same commit to get a glitch-free experience and also to prevent the compositor from disconnecting us. If we unconditionally ack a state (let's say maximized) in the window management code (i.e., WindowPosChanged), and we have current contents with incompatible size attached (i.e., > max size) the compositor will disconnects us. If we don't ack the state in order to avoid the disconnect, then we will have to ack it somewhere else, and the only option is when we get new contents. The wp_viewport protocol and xdg geometry do provide a sort of an escape hatch, by allowing us to fake the effective "surface size", but that means that contents and size will (at least transiently) disagree and lead to visual glitches/stretches etc. Note that all of the alternatives I described are options I have previously considered (and tried in various experiments) and in the end decided against, due to concerns about complexity/edge-cases, efficiency, glitchiness and generally going against the grain of the Wayland model/expectations. These options are certainly available to us to explore again if we are absolutely determined to move away from the current approach, but I am not convinced it's currently worth going down this path. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5418#note_66481
On Fri Mar 29 14:02:40 2024 +0000, Alexandros Frantzis wrote: > > Hmm okay, but does it need to be done before vkQueuePresent? Regarding > wayland_surface_ensure_contents, could we just for instance ensure the > surface has contents when it is resized / made visible in wayland_WindowPosChanged? > Here are some concerns: > * If we unconditionally call `wayland_surface_ensure_contents` on > resize/visibility, in most cases we will get another flush once the > application actually draws, and end up with double commit, which is both > inefficient and can cause flicker (since the first commit will be empty). > * If we try to avoid the double commit, by detecting that the surface > has old contents, but then the window surface is never flushed, we will > get stuck with the old, incorrectly sized buffer (the committed buffer > size matters for configuration reasons). > In essence, in WindowPosChanged we don't have all the information to > properly decide whether we want to "ensure contents", and doing it > unconditionally has some cost. The current approach avoids these > problems by only performing the "ensure contents" commit in the few > cases it is absolutely needed (i.e., when we want to show the GL/Vulkan > subsurface contents and won't be able to otherwise). > > With wayland_surface_reconfigure, it looks awkward that the client > drawing is driving the window surface size, I think it should be done > separately and client drawing should not involve any windowing logic but > simply return errors if the size doesn't match. > This seems awkward because the Wayland surface model is different: > surfaces have no independent inherent size, it's the buffer size > (+transformations) that determines the "surface size", which makes > drawing and sizing interlinked. > In Wayland's model ack-ing a new state and providing new contents > (buffer + transformations) for that state should ideally happen in the > same commit to get a glitch-free experience and also to prevent the > compositor from disconnecting us. > If we unconditionally ack a state (let's say maximized) in the window > management code (i.e., WindowPosChanged), and we have current contents > with incompatible size attached (i.e., > max size) the compositor will > disconnects us. If we don't ack the state in order to avoid the > disconnect, then we will have to ack it somewhere else, and the only > option is when we get new contents. > The wp_viewport protocol and xdg geometry do provide a sort of an escape > hatch, by allowing us to fake the effective "surface size", but that > means that contents and size will (at least transiently) disagree and > lead to visual glitches/stretches etc. > Note that all of the alternatives I described are options I have > previously considered (and tried in various experiments) and in the end > decided against, due to concerns about complexity/edge-cases, > efficiency, glitchiness and generally going against the grain of the > Wayland model/expectations. These options are certainly available to us > to explore again if we are absolutely determined to move away from the > current approach, but I am not convinced it's currently worth going down > this path. But the OpenGL/Vulkan surface is not the window surface, and I don't see why GL/VK drawing should have anything to do with their parent window surface, except passively reacting to its size changes. You can create a GL/VK surface on a message or an invisible window, with no window surface, or a child window which would ultimately use its ancestor toplevel surface as a support. Assuming we have some logic in win32u, do you think it could work if we ensure that the toplevel window surface is flushed before presenting (ie: calling `surface->funcs->flush` -> `wayland_window_surface_flush`)? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5418#note_66482
But the OpenGL/Vulkan surface is not the window surface, and I don't see why GL/VK drawing should have anything to do with their parent window surface, except passively reacting to its size changes.
(I am assuming by "window surface" you are referring to the `window_surface` abstraction) I think the difference in perspectives stems from me considering both GDI+flushing (window surface) and GL/VK drawing to be different but equal ways of presenting contents to a window/surface, and thus the current "ack-configure along with new contents" logic applies equally to both. The fact that in the Wayland driver we have to use a subsurface to allow us to mix GDI+GL/VK content is an extra implementation detail that additionally makes us introduce the "ensure contents" logic. My perspective was formed because these ways of providing content can and do work independently in some cases (e.g., presenting GL/VK without window surface contents, and hence this whole discussion here). This seems not to matter for other systems, but it does for Wayland as described in previous comments. If I am not misunderstanding, your perspective is that the window surface plays a special/authoritative role in terms of window content and other ways are secondary and are, at least conceptually, dependent on the window surface. I do think both viewpoints have merit, but perhaps that doesn't even matter in the end: if we are able to implement your proposal to ensure a flush before presentation it will effectively remove the main reason that formed my perspective.
You can create a GL/VK surface on a message or an invisible window, with no window surface, or a child window which would ultimately use its ancestor toplevel surface as a support.
Sure, but I don't see how this invalidates the necessity of "ensure contents" or "ack-configure" when rendering to a visible toplevel HWND. If these actions are not needed they just become noops. Note that for child windows GL/VK rendering we may actually need both of these recursively up the window chain (depending on what funky stuff apps want to do and how we end up implementing child GL rendering, e.g., fullscreen toplevel->fullscreen child->fullscreen GL, and toplevel/child are never drawn with GDI).
Assuming we have some logic in win32u, do you think it could work if we ensure that the toplevel window surface is flushed before presenting (ie: calling surface->funcs->flush -> wayland_window_surface_flush)?
Yes, I think doing a flush before every present would work, although I would like to experiment to verify. In the flush function the Wayland driver could check if there is a client subsurface and perform the necessary actions (i.e. ensure contents, ack configure) as needed, even if the flush bounds are empty. One potential downside is that without other information we would base our decision just on the existence of a client subsurface, regardless of whether that subsurface is going to be rendered to. The current approach only performs the extra actions when we know it's absolutely necessary. Perhaps a hint that this is a pre-presentation flush would help if this turns out to be a problem, or use a different function altogether, e.g., "prepare_for_external_present". I do wonder, though, if forcing a flush in this way could lead to glitches, e.g., with multiple threads, and there is complex GDI drawing going on and we flush in the middle of it (between operations that lock the window surface). Perhaps we should only perform this flush if the bounds are empty, or go with the "prepare_for_external_present" as mentioned above. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5418#note_66572
If I am not misunderstanding, your perspective is that the window surface plays a special/authoritative role in terms of window content and other ways are secondary and are, at least conceptually, dependent on the window surface.
Fwiw we've seen cases where, on Windows, a game (Bayonetta, Dragon's Dogma) uses fullscreen window with 1px borders, thus has its window surface fullscreen but client area not, but still manages to have fullscreen D3D swapchain. The presented image was cropped (or scaled, i don't remember exactly but more likely cropped) automatically, but still didn't require to recreate the swapchain, and swapchain images were fullscreen-sized. On Wine, and if we make the D3D/VK/GL surfaces closely follow the window surface dimensions, it requires of course to recreate the swapchain, and its images are then sized like the client area and smaller than the fullscreen dimensions. Not a big issue, things work, but as the images aren't stricly fullscreen sized it can cause some suboptimal present to occur (and in Proton we have a hack for this with gamescope to make sure the present is optimal). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5418#note_66616
I think something like https://gitlab.winehq.org/rbernon/wine/-/commit/1314374cf82a972e0bdf688e6b53... on top of https://gitlab.winehq.org/wine/wine/-/merge_requests/5465 should do the trick and will allow us to move vkQueuePresent WSI logic entirely out of the drivers. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5418#note_67013
On Wed Apr 10 10:52:59 2024 +0000, Rémi Bernon wrote:
I think something like https://gitlab.winehq.org/rbernon/wine/-/commit/1314374cf82a972e0bdf688e6b53... on top of https://gitlab.winehq.org/wine/wine/-/merge_requests/5465 should do the trick and will allow us to move vkQueuePresent WSI logic entirely out of the drivers. Thanks for the patch. Unfortunately this approach is not able to handle all required scenarios, since it forces a flush at most once per VkSurface. For example, a game starts windowed, it presents with Vulkan (so `flushed` is now true), and then becomes fullscreen. While becoming or being fullscreen the game never draws/flushes the window_surface itself, and the proposed vulkan logic also never forces a flush of the window_surface (since `flushed` is true). Another scenario involves hiding and re-showing a VK fullscreen window (i.e., anything that doesn't involve app GDI draws, so our logic needs to kick in).
For the above to work we need to be able to force flush the window_surface in all instances when it hasn't previously flushed contents, so we need to track flushing state per-window_surface. In reality we want to track this per-wayland_surface (as the current code does, because that's what actually determines subsurface visibility), but for now the window_surface is a good enough proxy for that. Even if we change the logic and achieve the above at the win32u level, this only handles the `wayland_surface_ensure_contents()` functionality, and we also need the additional reconfigure logic, to be able to reconfigure/ack even if no new contents are needed (e.g., changing states without a change in size). If we want to stick with the flush-after-present approach, perhaps we can achieve this by making `wayland_window_surface_flush` perform this check and operation when it would otherwise not flush anything (e.g.,not forced/bounds empty). Perhaps that's the right to do regardless, but I would need to experiment a bit, to ensure it doesn't have adverse affects under normal (non-VK) operation. Alternatively, if we are introducing the whole flush-before-present win32u logic only for the benefit of the Wayland driver, perhaps the simplest way forward would be to provide an (optional) `vulkan_funcs.p_queue_present_done(HWND)` callback and just let each driver do what it needs to do (so NULL for most drivers). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5418#note_67478
This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5418
Alternatively, if we are introducing the whole flush-before-present win32u logic only for the benefit of the Wayland driver, perhaps the simplest way forward would be to provide an (optional) `vulkan_funcs.p_queue_present_done(HWND)` callback and just let each driver do what it needs to do (so NULL for most drivers).
Right, I'll do something like that and leave the other considerations for later. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5418#note_67500
participants (2)
-
Alexandros Frantzis (＠afrantzis) -
Rémi Bernon