Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

video: wayland: Display non-native fullscreen aspect ratios centered with a black border #5534

Closed
wants to merge 1 commit into from

Conversation

Kontrabant
Copy link
Contributor

@Kontrabant Kontrabant commented Apr 13, 2022

Employ subsurfaces and viewports to maintain a proper aspect ratio for fullscreen modes that don't match the display's native aspect ratio, and display the content centered, with a black border. The mask surface uses a 1x1 SHM buffer, which is scaled to the display output dimensions using a viewport. The draw surface becomes a subsurface of the parent mask surface and runs in desynced mode.

This will need to be rebased and updated when #5510 is merged.

Notes

  • Pointer confinement in Gnome and KDE doesn't seem to work with subsurfaces, so to work around this, the input region on the drawable surface is set to empty and the size of the drawable surface is set as the input region on the parent surface, so all input is handled through the parent surface.

@flibitijibibo
@sulix

@flibitijibibo
Copy link
Collaborator

This might go a bit too far in my book... this isn't something we do for any other video implementation, the fullscreen mode emulation is probably as far as we should go. For fancier display work something like gamescope seems more appropriate, but I would check the behavior on other video backends in case I'm wrong here.

@Kontrabant
Copy link
Contributor Author

This behavior is similar to what sdl12-compat does for emulated video modes, it just does it with Wayland surfaces instead of OpenGL. Granted other SDL2 backends don't do this, but they also operate under the assumption that they can actually change the video mode and let something further down the chain sort it out.

The current behavior, while it works, is not ideal overall. To leave scaling up to compositors or tools like Gamescope, a completely unscaled image should be output, which wouldn't provide a pleasant default user experience in most cases, while making everything stretched fullscreen with no aspect ratio correction at all isn't desirable either, especially on ultrawide monitors.

On the topic of things like Gamescope, it probably would be a good idea to have an envvar switch such as SDL_WAYLAND_SCALING=0 to disable scaling so something else can handle it.

@sulix
Copy link
Contributor

sulix commented Apr 14, 2022

Personally, I'm all for this, though it'd be great if we could share a bit more code with the SDL_RenderSetLogicalSize() implementation. (I don't think we'd want two implementations of letterboxing, for example.)

IIRC, there is precedent for implementing something like this in a video backend, too. X11 (at least in SDL 1.2) would center the user-visible display within a black fullscreen window if the desired mode couldn't be set. It still appears to exist in the legacy fullscreen code, though that's broken enough that no-one's probably seen it in ages.

And I think XWayland is implementing something like this, and we wouldn't want to have a regression switching from XWayland→Wayland if we can help it, IMHO.

At a quick glance, this seems to work well on my machine, but I'll need to check it out in more detail (alas, probably after the weekend), particularly on my weird build system which, IIRC, doesn't like the new memfd_create fuctions.

@Kontrabant
Copy link
Contributor Author

Autotools build is updated.

@flibitijibibo flibitijibibo added the early in milestone This change should be made early in the milestone for additional testing label Apr 14, 2022
include/SDL_config.h.in Outdated Show resolved Hide resolved
@Kontrabant
Copy link
Contributor Author

Fixed SDL_config.h.in

@Kontrabant Kontrabant force-pushed the wayland_aspect_mask branch 2 times, most recently from 1d967d4 to 75030d3 Compare April 17, 2022 03:03
@flibitijibibo flibitijibibo self-assigned this Apr 18, 2022
@Kontrabant Kontrabant force-pushed the wayland_aspect_mask branch 2 times, most recently from 916ff00 to a4db899 Compare April 18, 2022 20:04
@sulix
Copy link
Contributor

sulix commented Apr 24, 2022

Tried this again, rebased on top of the git HEAD, and it's working pretty well, even when I force all of the fallback paths, and build with autotools:

/* #undef HAVE_MEMFD_CREATE */
/* #undef HAVE_MKOSTEMP */
/* #undef HAVE_POSIX_FALLOCATE */

The only issue I've noticed thus far is that mouse clicks seem to "pass through" the letterboxes/pillarboxes. This usually isn't a problem, as the mouse cursor is confined to the actual game content, but it's possible to trigger it (at least under KDE), by:

  • Alt+Tabbing away from the game
  • Moving the cursor to an area which is outside the game's content area (i.e., in the letterbox/pillarbox)
  • Alt_Tabbing back.
  • Moving the mouse within the letterbox/pillarbox area and clicking.
  • The click is passed through to the window underneath (which will likely bring it to the foreground).

Otherwise, it's working perfectly for me!

@Kontrabant
Copy link
Contributor Author

Kontrabant commented Apr 24, 2022

The Wayland spec states that any input events outside of a surface input region are passed through to the next surface, and I guess that applies even if the next surface isn't in the window currently in focus. I think the cleanest solution is to have 3 surfaces: the input surface as the parent, the desynced draw subsurface placed above it and the mask subsurface placed below it. The input region of the draw surface is set to empty so that all input is handled through the parent input surface, as compositors seem to have some issues with subsurfaces and certain input features the SDL backend needs, and events outside of the parent input region are caught by the mask surface and discarded. I have it fixed in my working branch along with a few other changes, which I'll push after I've had a chance to review everything and clean things up a bit.

This would be a lot easier if pointer confinement regions worked on subsurfaces, but it seems that neither GNOME's Mutter nor KDE's KWin support this...

@sulix
Copy link
Contributor

sulix commented Apr 25, 2022

This would be a lot easier if pointer confinement regions worked on subsurfaces, but it seems that neither GNOME's Mutter nor KDE's KWin support this...

Ah, that might explain this bug.

@Kontrabant
Copy link
Contributor Author

Kontrabant commented Apr 27, 2022

New update that should fix the input passthrough in the mask areas. It also adds 2 new hints: SDL_WAYLAND_MODE_EMULATION and SDL_WAYLAND_SCALING, used to disable mode emulation and output scaling respectively. The first could be useful when dealing with a microcompositor and wanting to force one specific desktop resolution. The second is useful when dealing with capture software such as OBS, which I've found doesn't work properly with scaled Wayland surfaces when using window capture, regardless of whether the scaling is done via viewports or a surface scale factor (desktop capture is fine though).

This also streamlines the mode emulation and viewport logic as it was getting a bit convoluted.

edit: Updated the configure script, so no need to run autoconf first anymore.

@Kontrabant Kontrabant force-pushed the wayland_aspect_mask branch 6 times, most recently from b432f19 to b0e0dc0 Compare April 28, 2022 14:05
configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
configure Outdated Show resolved Hide resolved
@Kontrabant
Copy link
Contributor Author

Cleaning up the surface commit chain seems to have fixed the libdecor issue where the content area was initially reported as slightly undersized. With the latest fixes, everything seems to be working smoothly.

Copy link
Collaborator

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't be the best judge for the build system changes but the code in terms of style seems fine. But overall I wonder if we should start compartmentalizing all of this work in such a way that we have clear lines drawn between the levels of detail we're getting into. It'd be good to know, for example, what the bare minimum for a window would look like, then the desktop-friendly window (aka xdg-shell windows), then DPI-friendly, then finally fake-modeset-friendly, all written in such a way that it doesn't look like every window is going to be making an entire subcompositor.

That's about all I can review with any kind of authority - as far as functionality is concerned I haven't touched SDL_WINDOW_FULLSCREEN in almost a decade, so I'll actually defer to @sulix and @icculus for the final review since sdl12-compat will care about this more than anyone else in 2022 (even though they do have a scaling system already).

Comment on lines 66 to 65
struct wl_surface *mask_surface;
struct wl_surface *input_surface;
struct wl_buffer *mask_surface_buffer;
struct wl_buffer *input_surface_buffer;
struct wl_subsurface *mask_subsurface;
struct wl_subsurface *draw_subsurface;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documenting these variables as being part of mode emulation may help with readability - most of these variables will be used at all times but these might not.

Comment on lines 92 to 94
struct wp_viewport *draw_viewport;
struct wp_viewport *mask_viewport;
struct wp_viewport *input_viewport;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same idea as the new surface/buffer variables.

src/video/wayland/SDL_waylandwindow.h Show resolved Hide resolved
src/video/wayland/SDL_waylandwindow.c Outdated Show resolved Hide resolved
@Kontrabant
Copy link
Contributor Author

Kontrabant commented May 11, 2022

One of my goals when writing this patch was to detangle mode emulation vs native high-DPI/unscaled, and, at this point, things should be fairly streamlined and compartmentalized. Any bits related to mode emulation are in blocks surrounded by FullscreenModeEmulation(), and NeedViewport() covers non-mode emulation viewport/scaling cases. I tried ripping out the mode emulation code just to see how much work it would be, and it was actually quite fast and easy, on the order of a couple of minutes.

The handling of scale factors could be cleaned up a bit, however, that's something for near-future work as it looks like Wayland is finally getting a proper fractional scale protocol that reports the fractional scale factor per-surface and will require a bit of refactoring in that domain anyways.

As for SDL_WINDOW_FULLSCREEN, even for native SDL2 apps, mode emulation allows applications not built specifically with Wayland in mind or not designed to handle scaling themselves to use the display's native resolution or render at a lower resolution for lower-end GPUs. Whether or not SDL_WINDOW_FULLSCREEN and mode switching in general is antiquated and not something applications should be using in this day and age (they shouldn't, imo) is another matter. If it's deemed obsolete and removed in version 2.1 or 3, the mode emulation code in the driver is fairly trivial to rip out, as previously stated.

I'll work on getting the surfaces/subsurface/viewport/pointer variables better commented.

@Kontrabant
Copy link
Contributor Author

Rebased against the latest changes and reworked and commented the SDL_WindowData struct. All of the mode emulation variables are now grouped into two blocks: one for the Wayland surface/buffer/viewport/subsurface structs and one for the pointer translation variables. Both blocks are noted as such. If mode emulation is ever removed, removing those two blocks is all that's required.

While working on it, I added some comments for the other variables as well and generally compartmentalized things a bit more, so related structure members are now more closely grouped.

Copy link
Collaborator

@flibitijibibo flibitijibibo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest revision is fine with me - this should get at least one more approval from someone using this path.

src/video/wayland/SDL_waylandwindow.c Outdated Show resolved Hide resolved
@Kontrabant Kontrabant force-pushed the wayland_aspect_mask branch 2 times, most recently from ab105f9 to a9e935e Compare May 13, 2022 12:58
@sulix
Copy link
Contributor

sulix commented May 13, 2022

This is currently breaking fullscreen games under KWin for me: the screen's only updating when some other event occurs (such as mouse input). Works fine in windowed mode: possibly the frame callback isn't triggering.

It happens pretty consistently across every fullscreen game here, though if windowed or running minimized, it seems to work okay.

@Kontrabant
Copy link
Contributor Author

I took a quick look and was able to replicate this, but it's very strange. Just moving the mouse is enough to get the screen to update, nothing that triggers any kind of window events that results in surface commits. I put a print statement in the frame callback, and the callback does indeed only fire in fullscreen mode when the mouse cursor is moving. I'm inclined to say that this is a compositor bug, as the subsurface tree is present and identical in both windowed and fullscreen mode, yet the bug happens only in fullscreen, and adding explicit surface commits with each frame doesn't help either. The subsurface tree isn't exactly elaborate either: parent input handler surface in the middle, draw surface above, mask below. Whether or not they are marked as opaque has no effect.

I'm guessing that what's happening is that the compositor, for some reason, thinks that the draw surface is obscured and only triggers repaints when something like the mouse cursor moving occurs. I'll experiment a bit more later, but I'm not sure if there's anything that can be done on the SDL side if the compositor just refuses to repaint the screen.

Is there a way to turn off fullscreen unredirection on KDE just to see if that has any effect?

…with a black border

Employ subsurfaces and viewports to maintain a proper aspect ratio for fullscreen modes that don't match the display's native aspect ratio, and display the content centered, with a black border.  The mask surface uses a 1x1 SHM buffer, which is scaled to the display output dimensions using a viewport.  The draw surface becomes a subsurface of the parent surface and runs in desynced mode.

The hints SDL_WAYLAND_MODE_EMULATION and SDL_WAYLAND_SCALING are provided to disable non-desktop fullscreen resolutions and disable any output scaling/masking respectively when set to 0.
@Kontrabant
Copy link
Contributor Author

Kontrabant commented May 14, 2022

I tracked down the KDE issue, and it was something in the compositor behaving strangely when the mask subsurface was set to opaque and the SHM buffers used for the input and mask surfaces used the XRGB8888 format instead of ARGB8888. With these changes it works, although I have no idea why.

I'm increasingly thinking that this is not the direction to go in, as it seems that there are compositor bugs at every turn and I really don't like adding ever increasing complexity or "string and gum" solutions to work around them. Needing an extra surface to accommodate pointer confinement not working on subsurfaces was already feeling a bit hacky. Needing to seemingly randomly change buffer formats and surface opaqueness settings to avoid the KDE repaint issue is worse, and who knows what bugs will manifest in less common compositors. Couple that with compositors starting to build in functionality to handle this themselves, probably more efficiently than we can, and it compounds my thoughts that a different direction is needed.

The recent push fixes the KDE issue, but I'm closing this for now. The work that went into this won't be wasted, but I want to step back and rework the overall approach.

@Kontrabant Kontrabant closed this May 14, 2022
@Kontrabant Kontrabant deleted the wayland_aspect_mask branch June 14, 2022 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
early in milestone This change should be made early in the milestone for additional testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants