Wlroots: xwayland clients apply scaling twice when switching between TTYs

[Timidger]

When switching between TTYs in rootston, xwayland apps double in scaling size. This is a continuation of this sway bug report.

The commit that introduced this regression is bf1f461eba320cdeded1d5d8ae80caaa9d4cef98.

The problem line is this one in that diff. We send the "done" event twice and xwayland handles that poorly (e.g it applies scaling twice). This path is only executed when it's re-sent when you switch between TTYs.

This bug is also present in wlc and was probably copied over from there.

Commenting out done signal send works, but I haven't gone through the logic on master to see if it's actually necessary here.

[5nefarious]

I am still seeing this bug in sway with the latest commit of wlroots on branch master.

[Timidger]

Is this reproducible on rootston?

[5nefarious]

Yes, I just tried it on rootston and it has the same issue.

[Timidger]

Damn probably a regression...I'll look into it, this was a bug that _really_ annoyed me with wlc and I will not have it reappear in wlroots.

[ddevault]

Can we fix the problem in Xwayland? It seems like their bug.

[emersion]

It seems like their bug.

Can you provide a Wayland debug log to check?

It'll probably be fixed anyway with xdg-output support in Xwayland.

[ddevault]

I dunno what a wayland debug log is going to tell us. It's not ideal to send done twice but we're allowed to per the spec.

[Timidger]

Can reproduce in rootston.

Can we fix the problem in Xwayland? It seems like their bug.

Does this bug happen in Kwin or Gnome though?

[emersion]

I dunno what a wayland debug log is going to tell us. It's not ideal to send done twice but we're allowed to per the spec.

I just want to make sure we're not sending scale twice and then done, that would be correct but dumb. Also I'm not sure why this happens on VT switching, since all connectors are destroyed and re-created.

[Timidger]

I did a git bisect and found the regression to be at 7adf13e2840bf57a2b96b73462fc7c6ca8e363da, it's "fixed" today with the following patch:

diff --git a/types/wlr_output.c b/types/wlr_output.c
index d8b026b7..0806c8b1 100644
--- a/types/wlr_output.c
+++ b/types/wlr_output.c
@@ -178,6 +178,9 @@ void wlr_output_update_mode(struct wlr_output *output,

 void wlr_output_update_custom_mode(struct wlr_output *output, int32_t width,
        int32_t height, int32_t refresh) {
+   if (output->width == width && output->height == height && output->refresh == refresh) {
+       return;
+   }
    output->width = width;
    output->height = height;
    wlr_output_update_matrix(output);

That causes obvious flickering though.

[ddevault]

I'm not saying we shouldn't fix our side, too. But Xwayland has a bug and we should let them know, if not send a patch.

[emersion]

I did a git bisect and found the regression to be at 7adf13e

Thanks for the bisect. In fact it doesn't look like the wlr_output is removed and re-added on VT switching. And we're just sending the current mode (and then the done event), there's no bug on our side.

The patch _looks_ fine but unfortunately it'll introduce a bug with damage tracking: outputs need swap when switching VTs. The Good Fix is probably to apply your patch and call wlr_output_update_needs_swap on all outputs when switching VTs. But since the session is separate from the backend, I don't know if it's easy to do.

[Timidger]

Issue is related to this line in xwayland. If this branch is removed then the bug goes away. Unsure of the consequences of this change.

[emersion]

Proposal: only do it if xwl_output->width != width || xwl_output->height != height?

Meh, not sure about this.

[Timidger]

That does seem to work and at least fixes the issue for switching TTYs.

This requires no changes in wlroots in order to work. Just need to open a PR to xwayland.

EDIT: the patch:

diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index 48faeb142..dc988ed2f 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -194,7 +194,7 @@ update_screen_size(struct xwl_output *xwl_output, int width, int height)

     SetRootClip(xwl_screen->screen, xwl_screen->root_clip_mode);

-    if (xwl_screen->screen->root) {
+    if (xwl_screen->screen->root && xwl_output->width != width && xwl_output->height != height) {
         BoxRec box = { 0, 0, width, height };

         xwl_screen->screen->root->drawable.width = width;