Xwayland: fix output changes not getting applied in some, cases

Submitted by Ferdi265 on April 22, 2019, 11:30 a.m.

Details

Message ID 1922beb4-36d2-e914-eb6b-3b5acc2f8b1d@gmail.com
State New
Headers show
Series "Xwayland: fix output changes not getting applied in some, cases" ( rev: 1 ) in X.org

Not browsing as part of any series.

Commit Message

Ferdi265 April 22, 2019, 11:30 a.m.
Hello,

I investigated a bug I encountered while using the Sway Compositor and
traced it back to some behaviour in Xwayland:

The xdg_output and wl_output events are handled in such a way that
output changes (e.g. position) sometimes don't get applied if only
xdg_output is sent instead of both xdg_output and wl_output. (Sway does
this)

Below I attached a patch that fixes this. I am not very familiar with
the Xorg codebase, so I hope this patch is fine.

Here (https://github.com/swaywm/sway/issues/4064) is the Sway bug report
in question.

Kind Regards,
Ferdinand "Ferdi265" Bachmann

--- PATCH BELOW ---

From 431a176c112d373a07b861fdf95766b3741ba95c Mon Sep 17 00:00:00 2001
From: Ferdinand Bachmann <theferdi265@gmail.com>
Date: Mon, 22 Apr 2019 00:35:41 +0200
Subject: [PATCH] Xwayland: fix output changes not getting applied in
some cases

On some wayland compositors wl_output and xdg_output events don't
neccessarily happen together, so waiting until the wl_output event is
done might wait indefinitely.

This fixes output changes not getting applied on newer wlroots-based
compositors, such as sway.

Since these event's don't always happen together, the changes need to
be applied regardless of whether the other event is finished or not,
making the wl_output_done and xdg_output_done flags redundant.
---
 hw/xwayland/xwayland-output.c | 17 ++---------------
 hw/xwayland/xwayland.h        |  2 --
 2 files changed, 2 insertions(+), 17 deletions(-)

Patch hide | download patch | download mbox

diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index cc68f0340..497e56db4 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -218,10 +218,6 @@  apply_output_change(struct xwl_output *xwl_output)
     RRModePtr randr_mode;
     Bool need_rotate;

-    /* Clear out the "done" received flags */
-    xwl_output->wl_output_done = FALSE;
-    xwl_output->xdg_output_done = FALSE;
-
     /* xdg-output sends output size in compositor space. so already
rotated */
     need_rotate = (xwl_output->xdg_output == NULL);

@@ -268,13 +264,7 @@  static void
 output_handle_done(void *data, struct wl_output *wl_output)
 {
     struct xwl_output *xwl_output = data;
-
-    xwl_output->wl_output_done = TRUE;
-    /* Apply the changes from wl_output only if both "done" events are
received,
-     * or if xdg-output is not supported.
-     */
-    if (xwl_output->xdg_output_done || !xwl_output->xdg_output)
-        apply_output_change(xwl_output);
+    apply_output_change(xwl_output);
 }

 static void
@@ -313,10 +303,7 @@  static void
 xdg_output_handle_done(void *data, struct zxdg_output_v1 *xdg_output)
 {
     struct xwl_output *xwl_output = data;
-
-    xwl_output->xdg_output_done = TRUE;
-    if (xwl_output->wl_output_done)
-        apply_output_change(xwl_output);
+    apply_output_change(xwl_output);
 }

 static const struct zxdg_output_v1_listener xdg_output_listener = {
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 92664e812..1ab2f04b5 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -371,8 +371,6 @@  struct xwl_output {
     RRCrtcPtr randr_crtc;
     int32_t x, y, width, height, refresh;
     Rotation rotation;
-    Bool wl_output_done;
-    Bool xdg_output_done;
 };

 void xwl_sync_events (struct xwl_screen *xwl_screen);

Comments

On Mon, Apr 22, 2019 at 1:30 PM Ferdi265 <theferdi265@gmail.com> wrote:
>
> Hello,
>
> I investigated a bug I encountered while using the Sway Compositor and
> traced it back to some behaviour in Xwayland:
>
> The xdg_output and wl_output events are handled in such a way that
> output changes (e.g. position) sometimes don't get applied if only
> xdg_output is sent instead of both xdg_output and wl_output. (Sway does
> this)

This issue was something we noted in KWin as well and fixed it
downstream by making sure the events get always sent together.
More info here: https://phabricator.kde.org/D19253

Would a downstream fix work for you as well? Not saying it can't be
fixed upstream in XWayland, but I haven't yet looked at your patch in
detail and I just remember the output code in Xwayland being somewhat
difficult, so maybe a downstream fix would be easier.

> Below I attached a patch that fixes this. I am not very familiar with
> the Xorg codebase, so I hope this patch is fine.
>
> Here (https://github.com/swaywm/sway/issues/4064) is the Sway bug report
> in question.
>
> Kind Regards,
> Ferdinand "Ferdi265" Bachmann
>
> --- PATCH BELOW ---
>
> From 431a176c112d373a07b861fdf95766b3741ba95c Mon Sep 17 00:00:00 2001
> From: Ferdinand Bachmann <theferdi265@gmail.com>
> Date: Mon, 22 Apr 2019 00:35:41 +0200
> Subject: [PATCH] Xwayland: fix output changes not getting applied in
> some cases
>
> On some wayland compositors wl_output and xdg_output events don't
> neccessarily happen together, so waiting until the wl_output event is
> done might wait indefinitely.
>
> This fixes output changes not getting applied on newer wlroots-based
> compositors, such as sway.
>
> Since these event's don't always happen together, the changes need to
> be applied regardless of whether the other event is finished or not,
> making the wl_output_done and xdg_output_done flags redundant.
> ---
>  hw/xwayland/xwayland-output.c | 17 ++---------------
>  hw/xwayland/xwayland.h        |  2 --
>  2 files changed, 2 insertions(+), 17 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
> index cc68f0340..497e56db4 100644
> --- a/hw/xwayland/xwayland-output.c
> +++ b/hw/xwayland/xwayland-output.c
> @@ -218,10 +218,6 @@ apply_output_change(struct xwl_output *xwl_output)
>      RRModePtr randr_mode;
>      Bool need_rotate;
>
> -    /* Clear out the "done" received flags */
> -    xwl_output->wl_output_done = FALSE;
> -    xwl_output->xdg_output_done = FALSE;
> -
>      /* xdg-output sends output size in compositor space. so already
> rotated */
>      need_rotate = (xwl_output->xdg_output == NULL);
>
> @@ -268,13 +264,7 @@ static void
>  output_handle_done(void *data, struct wl_output *wl_output)
>  {
>      struct xwl_output *xwl_output = data;
> -
> -    xwl_output->wl_output_done = TRUE;
> -    /* Apply the changes from wl_output only if both "done" events are
> received,
> -     * or if xdg-output is not supported.
> -     */
> -    if (xwl_output->xdg_output_done || !xwl_output->xdg_output)
> -        apply_output_change(xwl_output);
> +    apply_output_change(xwl_output);
>  }
>
>  static void
> @@ -313,10 +303,7 @@ static void
>  xdg_output_handle_done(void *data, struct zxdg_output_v1 *xdg_output)
>  {
>      struct xwl_output *xwl_output = data;
> -
> -    xwl_output->xdg_output_done = TRUE;
> -    if (xwl_output->wl_output_done)
> -        apply_output_change(xwl_output);
> +    apply_output_change(xwl_output);
>  }
>
>  static const struct zxdg_output_v1_listener xdg_output_listener = {
> diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
> index 92664e812..1ab2f04b5 100644
> --- a/hw/xwayland/xwayland.h
> +++ b/hw/xwayland/xwayland.h
> @@ -371,8 +371,6 @@ struct xwl_output {
>      RRCrtcPtr randr_crtc;
>      int32_t x, y, width, height, refresh;
>      Rotation rotation;
> -    Bool wl_output_done;
> -    Bool xdg_output_done;
>  };
>
>  void xwl_sync_events (struct xwl_screen *xwl_screen);
> --
> 2.21.0
>
> _______________________________________________
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel

[Paging wayland-devel@ for the protocol discussion at the end of this
e-mail]

On Monday, April 22, 2019 3:21 PM, Ferdi265 <theferdi265@gmail.com> wrote:
> On 22/04/2019 14:01, Roman Gilg wrote:
> > On Mon, Apr 22, 2019 at 1:30 PM Ferdi265 theferdi265@gmail.com wrote:
> > > Hello,
> > > I investigated a bug I encountered while using the Sway Compositor and
> > > traced it back to some behaviour in Xwayland:
> > > The xdg_output and wl_output events are handled in such a way that
> > > output changes (e.g. position) sometimes don't get applied if only
> > > xdg_output is sent instead of both xdg_output and wl_output. (Sway does
> > > this)
> >
> > This issue was something we noted in KWin as well and fixed it
> > downstream by making sure the events get always sent together.
> > More info here: https://phabricator.kde.org/D19253
> > Would a downstream fix work for you as well? Not saying it can't be
> > fixed upstream in XWayland, but I haven't yet looked at your patch in
> > detail and I just remember the output code in Xwayland being somewhat
> > difficult, so maybe a downstream fix would be easier.
>
> The change to remove some information from the wl_output event and
> instead rely only on xdg_output in wlroots was deliberate as it
> simplifies the compositor code and might not even make much sense for
> some types of compositors (e.g. 3D desktop systems).
>
> The reason the events now don't fire together is that no information in
> wl_output actually changed, while the information in xdg_output did.
>
> the downstream discussion for this is here
> https://github.com/swaywm/wlroots/issues/1610
>
> I can't really speak for the whole wlroots / sway project, so I added
> Simon Ser aka "emersion" as CC, who is one of the main developers of
> sway and wlroots.
>
> Personally, I think Xwayland shouldn't attach too much meaning to when
> these events happen, and if they happen together. They are separate
> events after all, and should be treated this way.
>
> Assuming that this information is always updated at the same time or
> even re-sending it without a change (e.g. changing the output position
> in sway doesn't change any information in the wl_output event) just so
> the events happen together just sounds backwards to me.

I agree that Xwayland should be a good client and should handle
xdg_output.done sent even when wl_output.done isn't sent.

However I'm not sure this is the right fix: this patch breaks
atomicity. For instance the first time the output is advertised to
Xwayland, it will first update its internal state with the metadata
from wl_output and then take into account xdg-output and apply a second
update. This results in invalid intermediate state. I'm not sure how to
fix it, the only idea I can come up with is to have a timer to debounce
updates (yeah, it's not a good idea).

[Below is the part for wayland-devel@]

Wild idea: maybe xdg_output.done should never have existed, and
xdg_output should have used the wl_output.done event to atomically
submit state -- just like wl_surface.commit is used by add-on
interfaces like xdg_surface. This would more-or-less result in the
workaround Roman suggested on the server side, but it would be nice to
formalize it if we want to go down this road. Thoughts?