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

[Bug][Commit bisected] Commit breaks fullscreen SDL2 programs behaviour #1812

Open
vanfanel opened this issue May 13, 2024 · 12 comments
Open
Milestone

Comments

@vanfanel
Copy link

vanfanel commented May 13, 2024

Hi there,

This commit:
bd5dcb3
breaks certain SDL2 games fullscreen startup: when they are started in fullscreen mode, they fail to run in fullscreen mode, being displayed on a window instead.
Before this commit, they started nicely in fullscreen mode when told to do so.

I have found these games are affected by this commit (probably most of the programs and games are affected):
-OpenTyrian (https://github.com/opentyrian/opentyrian), configure it to start in fullscreen mode inside the game menu, then re-run the game.
-Schismtracker (https://github.com/schismtracker/schismtracker), start with schismtracker -f
-sm64ex-alo (https://github.com/AloUltraExt/sm64ex-alo), configure it to start in fullscreen mode inside the game menu, then re-run the game.

Before this commit, the games started nicely in fullscreen mode when configured or told to do so.

NOTES:
-Other WLRoots-based compositors are not affected. Current Sway for example runs the fullscreen SDL2 programs nicely.
-Weston compositor is not affected.

@tokyo4j
Copy link
Contributor

tokyo4j commented May 17, 2024

I did some debugging with OpenTyrian and found that the geometry of wl_surface committed by the client is windowed, even though the geometry of xdg_surface is fullscreen. I guess we are sending back the geometry of wl_surface to the client, so the client keeps drawing in the windowed geometry. I don't know if this is our bug or SDL2's bug as I'm missing the context of bd5dcb3.

Here's the debug log with some wlr_log()s:

diff --git a/src/xdg.c b/src/xdg.c
index 488f67fd..35dcff47 100644
--- a/src/xdg.c
+++ b/src/xdg.c
@@ -152,6 +152,14 @@ handle_commit(struct wl_listener *listener, void *data)
 
 	if (update_required) {
 		view_impl_apply_geometry(view, size.width, size.height);
+		wlr_log(WLR_ERROR, "applied geometry: %dx%d", size.width, size.height);
+		wlr_log(WLR_ERROR, "wl_surface geometry: %dx%d",
+			xdg_surface->surface->current.width, xdg_surface->surface->current.width);
+		wlr_log(WLR_ERROR, "xdg_surface geometry: %dx%d",
+			xdg_surface->current.geometry.width, xdg_surface->current.geometry.height);
+		struct wlr_box box;
+		wlr_xdg_surface_get_geometry(xdg_surface, &box);
+		wlr_log(WLR_ERROR, "computed xdg_surface geometry: %dx%d", box.width, box.height);
 
 		/*
 		 * Some views (e.g., terminals that scale as multiples of rows
[ 754287.173] [email protected]_fullscreen(wl_output@22)
[ 754287.253] [email protected](new id wl_callback@37)
[ 754287.256]  -> [email protected](34)
[ 754287.258]  -> [email protected]_id(37)
[ 754287.261]  -> [email protected](1280, 720, array[4])
[ 754287.263]  -> [email protected](34)
[ 754288.121] [email protected]_buffer_scale(1)
[ 754288.130] [email protected]_window_geometry(0, 0, 1280, 720)
[ 754288.133] [email protected]_region(new id wl_region@37)
[ 754288.136] [email protected](0, 0, 1280, 720)
[ 754288.139] [email protected]_opaque_region(wl_region@37)
[ 754288.141] [email protected]()
[ 754288.142]  -> [email protected]_id(37)
[ 754288.144] [email protected]_configure(34)
[ 754288.147] [email protected]_params(new id zwp_linux_buffer_params_v1@38)
[ 754288.149] [email protected](fd 56, 0, 0, 2560, 16777216, 8)
[ 754288.152] [email protected](fd 63, 1, 1064960, 320, 16777216, 8)
[ 754288.154] [email protected](fd 64, 2, 1073152, 64, 16777216, 8)
[ 754288.156] [email protected]_immed(new id wl_buffer@39, 640, 400, 875713112, 0)
[ 754288.166] [email protected]()
[ 754288.167]  -> [email protected]_id(38)
[ 754288.169] [email protected](wl_buffer@39, 0, 0)
[ 754288.172] [email protected](0, 0, 2147483647, 2147483647)
[ 754288.174] [email protected]()
[ 754288.218]  -> [email protected](35, wl_surface@32)
[ 754288.221]  -> [email protected](35, wl_surface@32)
[ 754288.223]  -> [email protected](36, wl_surface@29, array[4])
[ 754288.226]  -> [email protected](37, 0, 0, 16, 0)
[ 754288.228]  -> [email protected](nil)
[ 754288.270]  -> [email protected](nil)
[ 754288.282]  -> [email protected](wl_output@22)
00:00:03.885 [ERROR] [../src/xdg.c:155] applied geometry: 640x400
00:00:03.885 [ERROR] [../src/xdg.c:156] wl_surface geometry: 640x400
00:00:03.885 [ERROR] [../src/xdg.c:158] xdg_surface geometry: 1280x720
00:00:03.885 [ERROR] [../src/xdg.c:162] computed xdg_surface geometry: 640x400
[ 754288.313] [email protected](new id wl_callback@40)
[ 754288.315]  -> [email protected](39)
[ 754288.316]  -> [email protected]_id(40)
[ 754288.319]  -> [email protected](800, 600, array[0])
[ 754288.321]  -> [email protected](38)
[ 754288.323]  -> [email protected](640, 400, array[8])
[ 754288.325]  -> [email protected](39)
[ 754288.478] [email protected]_configure(38)

As a side note, the xdg-shell protocol says:

xdg_surface::set_window_geometry
...
When applied, the effective window geometry will be the set window geometry clamped to the bounding rectangle of the combined geometry of the surface of the xdg_surface and the associated subsurfaces.

So using the intersection of geometries of wl_surface and xdg_surface (which equals to the geometry of wl_surface in this case) is legitimate and that's what wlr_xdg_surface_get_geometry() is doing.

@droc12345
Copy link
Contributor

After the call to wlr_xdg_surface_get_geometry, sway adds a couple more checks than what labwc does
bool new_size = new_geo.width != view->geometry.width ||
new_geo.height != view->geometry.height ||
new_geo.x != view->geometry.x ||
new_geo.y != view->geometry.y;

where labwc only checks for width/height not sure if that makes a difference in this case.

Sway - sway/desktop/xdg_shell.c ~ line 300
labwc - src/xdg.c ~ line 116

@Consolatis
Copy link
Member

@ahesford, thoughts?

@ahesford
Copy link
Member

ahesford commented May 19, 2024

I don't fully understand the problem, but it seems like the client lies about its size in the commit. If the value of toplevel->scheduled.{width,height} are zero when we force them to match view->current in the commit handler, a simple fix might be to avoid forcing toplevel->scheduled unless at least one of the values is nonzero.

This would override what wlroots thinks of the window dimensions only if it thinks something about the dimensions, in which case it would be forcing the geometry in the next configure request anyway.

@tokyo4j
Copy link
Contributor

tokyo4j commented May 21, 2024

If the value of toplevel->scheduled.{width,height} are zero when we force them to match view->current in the commit handler, a simple fix might be to avoid forcing toplevel->scheduled unless at least one of the values is nonzero.

The values of toplevel->scheduled.{width,height} are not zero when we send back the geometry that the client doesn't expect, because we call wlr_xdg_toplevel_set_size() with the fullscreen geometry when the client sends xdg_toplevel.set_fullscreen request.

The problem happens like this:

  • Client sends xdg_toplevel.set_fullscreen request
  • Labwc updates toplevel->scheduled with the fullscreen geometry, then sends xdg_toplevel.configure event with the fullscreen geometry.
  • Client sends wl_surface.commit request with xdg_surface in the fullscreen geometry, but also with wl_buffer in the windowed geometry for some reason.
  • Labwc updates toplevel->scheduled with the windowed geometry returned by wlr_xdg_surface_get_geometry(), then sends configure event with the windowed geometry.

How about syncing toplevel->scheduled and toplevel->current like below?
I confirmed this fixes the problem.

diff --git a/src/xdg.c b/src/xdg.c
index 2b8bf96e..46bb8fa4 100644
--- a/src/xdg.c
+++ b/src/xdg.c
@@ -156,8 +156,8 @@ handle_commit(struct wl_listener *listener, void *data)
 			 */
 			struct wlr_xdg_toplevel *toplevel =
 				xdg_toplevel_from_view(view);
-			toplevel->scheduled.width = view->current.width;
-			toplevel->scheduled.height = view->current.height;
+			toplevel->scheduled.width = toplevel->current.width;
+			toplevel->scheduled.height = toplevel->current.height;
 		}
 	}
 }

@ahesford
Copy link
Member

How about syncing toplevel->scheduled and toplevel->current like below? I confirmed this fixes the problem. [...]

That change breaks the behavior that bd5dcb3 was intended to fix:

  1. Open a foot terminal (preferrably the newest version, 0.17.2)
  2. Manually resize the foot window with a mouse, to some arbitrary size
  3. Change the font size from the keyboard (by default, Ctrl-+ or Ctrl-<Hyphen>); this will cause the foot window to resize itself to keep the same character grid with the larger or smaller font
  4. Activate another window

With bd5dcb3 in place, nothing unexpected will happen. Reverting bd5dcb3 or replacing view->current with toplevel->current will cause foot to jump back to the window size that was set by step 2 as soon as the window is deactivated. Looking further, it seems that your proposal is a no-op anyway, because wlroots should have already copied toplevel->pending into toplevel->current when handling the client commit.

I see no reason why the client should be committing a buffer with the windowed geometry, but if we want to work around seemingly broken clients, we'll have to get clever.

@Consolatis Consolatis added this to the 0.7.3 milestone May 21, 2024
@Consolatis
Copy link
Member

Consolatis commented May 21, 2024

[ 754288.319] -> [email protected](800, 600, array[0])

I don't understand the log above, where does that size come from?
The debug log seems to miss view->current.width/height (although it should be equal to size after view_impl_apply_geometry()).

Maybe the issue here is actually the Qt-workaround above and view->pending is 0 and thus the extents match the new surface? However, there should be a debug message and I am not sure why modifying toplevel->scheduled.height/width would then fix that.

@ahesford
Copy link
Member

If these clients behave as expected in Sway, I wonder if Sway handle the foot resize issue properly if foot is floating rather than tiled (is this even possible?).

If not, it may be the case that we are correct, the client and Sway are both wrong, and two wrongs make a right here.

@nekopsykose
Copy link

If these clients behave as expected in Sway, I wonder if Sway handle the #1812 (comment) properly if foot is floating rather than tiled (is this even possible?).

changing font size in latest foot on latest sway has foot resize correctly, then picking another window doesn't make it change size (stays the same). seems to behave correctly

@ahesford
Copy link
Member

changing font size in latest foot on latest sway has foot resize correctly, then picking another window doesn't make it change size (stays the same). seems to behave correctly

Thanks for testing. Was the font change done after a manual resize of the window, or was foot still in its initially configured size? In labwc without the fix, foot will behave even after font resizing until a manual resize triggers wlr_xdg_toplevel_set_size and, through that, caching of the toplevel geometry.

@nekopsykose
Copy link

Thanks for testing. Was the font change done after a manual resize of the window, or was foot still in its initially configured size? In labwc without the fix, foot will behave even after font resizing until a manual resize triggers wlr_xdg_toplevel_set_size and, through that, caching of the toplevel geometry.

any order of operations i test seems to work ok and not have it resize itself to some older size inadvertantly

@ahesford
Copy link
Member

For foot, sway works properly because it sends a second configure in the commit handler when it detects a size change. We explicitly avoid doing so because I was worried about infinite loops. Maybe this concern is unwarranted if we always send back the exact same size that the client just committed. In any case, I don't think that difference explains the SDL anomaly here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants