[wayland-protocols] xdg-output: deprecate the xdg_output.done event

Submitted by Simon Ser on April 27, 2019, 7:44 a.m.

Details

Message ID zUJ7IP0IkAb0skxnZRxiV7rYqczAMMAVNfKxl5kXgFCKZl0vyobW-Yfr7yKXMGzpi3dpfOmfBPqTNIVFq9SdBz3mReMt1Ao107lnjfbfHWA=@emersion.fr
State Superseded
Headers show
Series "xdg-output: deprecate the xdg_output.done event" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Simon Ser April 27, 2019, 7:44 a.m.
This commit makes it so a wl_output.done event is guaranteed to be sent with a
xdg_output.done event.

This protocol change has been discussed in a recent xorg-devel discussions [1].

First let's recap why a change is needed: Xwayland listens to both wl_output and
xdg_output changes. When an output's properties change, Xwayland expects to
receive both a wl_output.done event and an xdg_output.done event. If that's not
the case, Xwayland doesn't update its state (so old state is still exposed to
X11 clients).

Most of the time, both objects will be updated at the same time (e.g. the
current mode is changed, so both wl_output.mode and xdg_output.logical_size are
sent) so this won't be an issue. However in some situations only one of
wl_output or xdg_output changes. For instance:

- The mode is changed at the same time as the scale, resulting in the same
  logical_size.
- The compositor doesn't expose the outputs' position via wl_output, so whenever
  the position changes only xdg_output is updated.

Both KDE [2] and wlroots [3] have experienced this issue.

For this reason, I'd like to update the xdg-output protocol to make it mandatory
to always send a wl_output.done event after xdg_output changes. This effectively
makes wl_output.done atomically apply all output state (including the state of
add-on objects like xdg_output). This approach is pretty similar to
wl_surface.commit: this request will atomically apply surface state including
the state of e.g. the xdg_surface object tied to the wl_surface.

To update the protocol to reflect this new requirement we can either:

- **Bump xdg_output version**. The current protocol doesn't specify that
  wl_output.done must be sent, adding this new requirement would be a breaking
  change. We need to fix Xwayland for the current xdg_output version (maybe make
  it non-atomic for the current version, atomic for the new one?). Should we
  deprecate xdg_output.done in the new version?
- **Don't bump xdg_output version**. This clarifies what is expected in practice
  by Xwayland, a major xdg_output consumer, and what is currently implemented by
  all compositors.

There's one issue with the "don't bump" approach: indeed in practice compositors
always send wl_output.done and xdg_output.done in pairs, however the ordering
between those two events is not guaranteed. This means some compositors might
send this sequence:

    wl_output.geometry(…)
    wl_output.done()
    xdg_output.logical_position(…)
    xdg_output.done()

In this case the wl_output.done event fails to atomically apply the xdg_output
state.

For this reason, I think bumping the version is a better approach.

This commit also deprecates xdg_output.done, which doesn't have any purpose
anymore.

[1]: https://lists.x.org/archives/xorg-devel/2019-April/058148.html
[2]: https://phabricator.kde.org/D19253
[3]: https://github.com/swaywm/sway/issues/4064

Signed-off-by: Simon Ser <contact@emersion.fr>
---
 unstable/xdg-output/xdg-output-unstable-v1.xml | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

--
2.21.0

Patch hide | download patch | download mbox

diff --git a/unstable/xdg-output/xdg-output-unstable-v1.xml b/unstable/xdg-output/xdg-output-unstable-v1.xml
index ccbfe1c..01ac7a7 100644
--- a/unstable/xdg-output/xdg-output-unstable-v1.xml
+++ b/unstable/xdg-output/xdg-output-unstable-v1.xml
@@ -54,7 +54,7 @@ 
     reset.
   </description>

-  <interface name="zxdg_output_manager_v1" version="2">
+  <interface name="zxdg_output_manager_v1" version="3">
     <description summary="manage xdg_output objects">
       A global factory interface for xdg_output objects.
     </description>
@@ -77,7 +77,7 @@ 
     </request>
   </interface>

-  <interface name="zxdg_output_v1" version="2">
+  <interface name="zxdg_output_v1" version="3">
     <description summary="compositor logical output region">
       An xdg_output describes part of the compositor geometry.

@@ -157,6 +157,10 @@ 

 	This allows changes to the xdg_output properties to be seen as
 	atomic, even if they happen via multiple events.
+
+	For objects version 3 onwards, this event is deprecated. Compositors
+	are not required to send it anymore and must send wl_output.done
+	instead.
       </description>
     </event>


Comments


On Sunday, April 28, 2019 12:04 PM, Olivier Fourdan <ofourdan@redhat.com> wrote:
> Hi,
>
> On Sat, Apr 27, 2019 at 9:44 AM Simon Ser <contact@emersion.fr> wrote:
>
> > This commit makes it so a wl_output.done event is guaranteed to be sent with a
> > xdg_output.done event.
>
> Humm, I am not sure I like changing xdg-output to rely on another protocol
> event, it's looks like a weird mixup to me...

As noted below, this is idiomatic in the Wayland protocol. The
wl_surface.commit request applies xdg_surface's state.

> > This protocol change has been discussed in a recent xorg-devel discussions [1].
> >
> > First let's recap why a change is needed: Xwayland listens to both wl_output and
> > xdg_output changes. When an output's properties change, Xwayland expects to
> > receive both a wl_output.done event and an xdg_output.done event. If that's not
> > the case, Xwayland doesn't update its state (so old state is still exposed to
> > X11 clients).
> >
> > Most of the time, both objects will be updated at the same time (e.g. the
> > current mode is changed, so both wl_output.mode and xdg_output.logical_size are
> > sent) so this won't be an issue. However in some situations only one of
> > wl_output or xdg_output changes. For instance:
> >
> > - The mode is changed at the same time as the scale, resulting in the same
> >   logical_size.
>
> If the mode or scale changes, wl_output events should be sent. If the
> resulting xdg-output remains the same, that's fine, no xdg-output
> event would be sent and Xwayland should be fixed for that case.
>
> Same for rotation actually, I remember seing displays (Barco, iirc)
> used by ATM that were square (both physically and in resolution),
> applying a rotation to such an output device would not change the
> resulting xdg-output definition.
>
> > - The compositor doesn't expose the outputs' position via wl_output, so whenever
> >   the position changes only xdg_output is updated.
>
> That sounds like a bug in the compositor, wl_output is core protocol
> and the geometry event should be sent “[...] whenever any of the
> properties change.”

No, this is not a bug, this is deliberate. wlroots doesn't expose the
output positions to regular clients, because regular clients shouldn't
care about it. If they do, either it's a client bug, or either they
should use xdg-output.

GNOME also doesn't expose all output properties IIRC: the whole list of
output modes is not sent.

> > Both KDE [2] and wlroots [3] have experienced this issue.
>
> To me, that feels like changing the protocol definitions to please
> specific implementations  or work around bugs.

No, this proposal aims to add a way to atomically apply output
properties. Since output properties include both wl_output properties
and xdg_output properties, the proposal suggests to remove
xdg_output.done (since two atomic events can't be combined into one).

> > For this reason, I'd like to update the xdg-output protocol to make it mandatory
> > to always send a wl_output.done event after xdg_output changes. This effectively
> > makes wl_output.done atomically apply all output state (including the state of
> > add-on objects like xdg_output). This approach is pretty similar to
> > wl_surface.commit: this request will atomically apply surface state including
> > the state of e.g. the xdg_surface object tied to the wl_surface.
> >
> > To update the protocol to reflect this new requirement we can either:
> >
> > - **Bump xdg_output version**. The current protocol doesn't specify that
> >   wl_output.done must be sent, adding this new requirement would be a breaking
> >   change. We need to fix Xwayland for the current xdg_output version (maybe make
> >   it non-atomic for the current version, atomic for the new one?). Should we
> >   deprecate xdg_output.done in the new version?
> > - **Don't bump xdg_output version**. This clarifies what is expected in practice
> >   by Xwayland, a major xdg_output consumer, and what is currently implemented by
> >   all compositors.
> >
> > There's one issue with the "don't bump" approach: indeed in practice compositors
> > always send wl_output.done and xdg_output.done in pairs, however the ordering
> > between those two events is not guaranteed. This means some compositors might
> > send this sequence:
> >
> >     wl_output.geometry(…)
> >     wl_output.done()
> >     xdg_output.logical_position(…)
> >     xdg_output.done()
> >
> > In this case the wl_output.done event fails to atomically apply the xdg_output
> > state.
> >
> > For this reason, I think bumping the version is a better approach.
>
> I'd rather we try harder to fix the implementations. In the worse
> case, Xwayland would send two XRandr events instead of one, which
> IMHO is no big deal.

The Wayland protocol really tries hard to make properties atomic when
useful. IMHO it would be unfortunate to say "it's no big deal if it's
not atomic", especially when the intermediate state *is* invalid.

Additionally, these protocols are not only used by Xwayland -- other
clients (e.g. slurp, a desktop selection tool) will draw invalid frames
if output properties are not atomically applied.

> Cheers,
> Olivier
>
> > This commit also deprecates xdg_output.done, which doesn't have any purpose
> > anymore.
> >
> > [1]: https://lists.x.org/archives/xorg-devel/2019-April/058148.html
> > [2]: https://phabricator.kde.org/D19253
> > [3]: https://github.com/swaywm/sway/issues/4064
> >
> > Signed-off-by: Simon Ser <contact@emersion.fr>
> > ---
> >  unstable/xdg-output/xdg-output-unstable-v1.xml | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/unstable/xdg-output/xdg-output-unstable-v1.xml b/unstable/xdg-output/xdg-output-unstable-v1.xml
> > index ccbfe1c..01ac7a7 100644
> > --- a/unstable/xdg-output/xdg-output-unstable-v1.xml
> > +++ b/unstable/xdg-output/xdg-output-unstable-v1.xml
> > @@ -54,7 +54,7 @@
> >      reset.
> >    </description>
> >
> > -  <interface name="zxdg_output_manager_v1" version="2">
> > +  <interface name="zxdg_output_manager_v1" version="3">
> >      <description summary="manage xdg_output objects">
> >        A global factory interface for xdg_output objects.
> >      </description>
> > @@ -77,7 +77,7 @@
> >      </request>
> >    </interface>
> >
> > -  <interface name="zxdg_output_v1" version="2">
> > +  <interface name="zxdg_output_v1" version="3">
> >      <description summary="compositor logical output region">
> >        An xdg_output describes part of the compositor geometry.
> >
> > @@ -157,6 +157,10 @@
> >
> >         This allows changes to the xdg_output properties to be seen as
> >         atomic, even if they happen via multiple events.
> > +
> > +       For objects version 3 onwards, this event is deprecated. Compositors
> > +       are not required to send it anymore and must send wl_output.done
> > +       instead.
> >        </description>
> >      </event>
> >
> > --
> > 2.21.0
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>> This commit makes it so a wl_output.done event is guaranteed to be sent with a
>>> xdg_output.done event.
>>
>> Humm, I am not sure I like changing xdg-output to rely on another protocol
>> event, it's looks like a weird mixup to me...
> 
> As noted below, this is idiomatic in the Wayland protocol. The
> wl_surface.commit request applies xdg_surface's state.

Just to expand on this with my two-cents, many of the Wayland protocol 
objects are conceptually extensions of other protocol objects. 
xdg_surface extends wl_surface, xdg_toplevel extends xdg_surface, and if 
we want to go cross-extension-protocol, xdg_toplevel_decoration extends 
xdg_toplevel.

While this is 4 protocol objects, we're conceptually just dealing with a 
single surface's state. The protocol objects are simply adding new 
interfaces to that surface and are meaningless on their own. We want to 
keep that surface's state consistent, so they need to share the same 
synchronisation to ensure atomicity, which includes wl_output.commit and 
xdg_surface.configure.

So there is certainly precedence for relying on the events and requests 
of other protocols.

Now applying that to xdg_output, it's simply adding interfaces to an 
wl_output object, and isn't meaningful on its own. It only makes sense 
to synchronise it properly with wl_output.

Scott

On Monday, April 29, 2019 9:51 AM, Olivier Fourdan <ofourdan@redhat.com> wrote:
> Hi,
>
> On Sun, 28 Apr 2019 at 11:19, Simon Ser <contact@emersion.fr> wrote:
> > On Sunday, April 28, 2019 12:04 PM, Olivier Fourdan <ofourdan@redhat.com> wrote:
> > > Humm, I am not sure I like changing xdg-output to rely on another protocol
> > > event, it's looks like a weird mixup to me...
> >
> > As noted below, this is idiomatic in the Wayland protocol. The
> > wl_surface.commit request applies xdg_surface's state.
>
> Sure, yet I reckon it's a bit different, a `commit` is a request and
> not an event

I don't see how this is different. "Request" and "event" are just fancy
names for "protocol message". It's just that one is sent by the client
whereas the other is sent by the server.

> `wl_output.done` is supposed to be sent once all wl_output's
> properties are sent, not other objects' properties such as xdg-output
> own props.

Well the proposal's point is to change this.

> Well, maybe I'm too picky, but xdg-output is a superset of wl_output,
> so I think it's weird to rely on an event from the subset (wl_output)
> to tell when the properties on the superset (xdg-output) are all set.

xdg_surface is a superset of wl_surface too. However it's natural to
rely on wl_surface.commit. I don't understand why it wouldn't be for
xdg_output too.

> IMHO, it would better, semantically, to keep `xdg-output.done` and
> clarify (in xdg-output) that the `xdg-output.done` event is sent once
> all properties on wl_output _and_ xdg-output are set, xdg-output
> being a superset of wl_output. This way we would keep things apart
> but also still guarantee atomicity.

What if we have another protocol that extends wl_output? How can you
atomically send xdg_output state and the other protocol's state?

This solution doesn't scale.

> Besides, that would be easier on the existing clients that would
> (still) expect the `xdg-output.done` event and compositors that
> wouldn't need to emit the event based on the protocol version used by
> the client.

That's a good point. I still think it's worth it to properly fix this
issue.

> > [...]
> > GNOME also doesn't expose all output properties IIRC: the whole list of
> > output modes is not sent.
>
> It does, mutter sends the full list of outputs with their size, scale
> and position and gtk+ relies on it for its GdkMonitor, although there
> are plans to move gtk to use xdg-output as well for its GdkMonitor
> definitions.

The whole list of outputs is sent, however the whole list of output
*modes* isn't. Only the current mode is sent.

I don't think GTK+ should rely on output positions, modes or logical
geometry.

Hi Simon,

On Mon, Apr 29, 2019 at 9:22 AM Olivier Fourdan <ofourdan@redhat.com> wrote:
> On Mon, Apr 29, 2019 at 9:05 AM Simon Ser <contact@emersion.fr> wrote:
>>
>> On Monday, April 29, 2019 9:51 AM, Olivier Fourdan <ofourdan@redhat.com> wrote:
>> > > On Sunday, April 28, 2019 12:04 PM, Olivier Fourdan <ofourdan@redhat.com> wrote:
>> > IMHO, it would better, semantically, to keep `xdg-output.done` and
>> > clarify (in xdg-output) that the `xdg-output.done` event is sent once
>> > all properties on wl_output _and_ xdg-output are set, xdg-output
>> > being a superset of wl_output. This way we would keep things apart
>> > but also still guarantee atomicity.
>>
>> What if we have another protocol that extends wl_output? How can you
>> atomically send xdg_output state and the other protocol's state?
>>
>> This solution doesn't scale.
>
>
> Humm, that's a good point.
>
>> > Besides, that would be easier on the existing clients that would
>> > (still) expect the `xdg-output.done` event and compositors that
>> > wouldn't need to emit the event based on the protocol version used by
>> > the client.
>>
>> That's a good point. I still think it's worth it to properly fix this
>> issue.
>
>
> Then we have no choice but bumping the protocol version to differentiate those client which expect the xdg-output.done from those who won't.

I wonder if https://gitlab.freedesktop.org/xorg/xserver/issues/680 is
related (works with GNOME but not with KDE).

Anyway, you convinced me, I agree removing `xdg-output.done` would
help and make things simpler for clients.

Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>

Cheers,
Olivier

On Sat, Apr 27, 2019 at 07:44:39AM +0000, Simon Ser wrote:
> This commit makes it so a wl_output.done event is guaranteed to be sent with a
> xdg_output.done event.
> 
> This protocol change has been discussed in a recent xorg-devel discussions [1].
> 
> First let's recap why a change is needed: Xwayland listens to both wl_output and
> xdg_output changes. When an output's properties change, Xwayland expects to
> receive both a wl_output.done event and an xdg_output.done event. If that's not
> the case, Xwayland doesn't update its state (so old state is still exposed to
> X11 clients).
> 
> Most of the time, both objects will be updated at the same time (e.g. the
> current mode is changed, so both wl_output.mode and xdg_output.logical_size are
> sent) so this won't be an issue. However in some situations only one of
> wl_output or xdg_output changes. For instance:
> 
> - The mode is changed at the same time as the scale, resulting in the same
>   logical_size.
> - The compositor doesn't expose the outputs' position via wl_output, so whenever
>   the position changes only xdg_output is updated.
> 
> Both KDE [2] and wlroots [3] have experienced this issue.
> 
> For this reason, I'd like to update the xdg-output protocol to make it mandatory
> to always send a wl_output.done event after xdg_output changes. This effectively
> makes wl_output.done atomically apply all output state (including the state of
> add-on objects like xdg_output). This approach is pretty similar to
> wl_surface.commit: this request will atomically apply surface state including
> the state of e.g. the xdg_surface object tied to the wl_surface.
> 
> To update the protocol to reflect this new requirement we can either:
> 
> - **Bump xdg_output version**. The current protocol doesn't specify that
>   wl_output.done must be sent, adding this new requirement would be a breaking
>   change. We need to fix Xwayland for the current xdg_output version (maybe make
>   it non-atomic for the current version, atomic for the new one?). Should we
>   deprecate xdg_output.done in the new version?
> - **Don't bump xdg_output version**. This clarifies what is expected in practice
>   by Xwayland, a major xdg_output consumer, and what is currently implemented by
>   all compositors.
> 
> There's one issue with the "don't bump" approach: indeed in practice compositors
> always send wl_output.done and xdg_output.done in pairs, however the ordering
> between those two events is not guaranteed. This means some compositors might
> send this sequence:
> 
>     wl_output.geometry(…)
>     wl_output.done()
>     xdg_output.logical_position(…)
>     xdg_output.done()
> 
> In this case the wl_output.done event fails to atomically apply the xdg_output
> state.
> 
> For this reason, I think bumping the version is a better approach.
> 
> This commit also deprecates xdg_output.done, which doesn't have any purpose
> anymore.

Relying only on wl_output.done sounds good to me. Since the 'done' event
here is eventally to go away, I think it'd be wise to state somewhere
other than the 'done' documentation that the wl_output.done event is
used to notify all changes have been communicated.


Jonas


(P.S. sorry for the ones in the To:/Cc: field for the multiple mails,
for some reason mailman thinks I'm sending from my @redhat.com address,
thus gets stuck on moderation. D.S.).

> 
> [1]: https://lists.x.org/archives/xorg-devel/2019-April/058148.html
> [2]: https://phabricator.kde.org/D19253
> [3]: https://github.com/swaywm/sway/issues/4064
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> ---
>  unstable/xdg-output/xdg-output-unstable-v1.xml | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/unstable/xdg-output/xdg-output-unstable-v1.xml b/unstable/xdg-output/xdg-output-unstable-v1.xml
> index ccbfe1c..01ac7a7 100644
> --- a/unstable/xdg-output/xdg-output-unstable-v1.xml
> +++ b/unstable/xdg-output/xdg-output-unstable-v1.xml
> @@ -54,7 +54,7 @@
>      reset.
>    </description>
> 
> -  <interface name="zxdg_output_manager_v1" version="2">
> +  <interface name="zxdg_output_manager_v1" version="3">
>      <description summary="manage xdg_output objects">
>        A global factory interface for xdg_output objects.
>      </description>
> @@ -77,7 +77,7 @@
>      </request>
>    </interface>
> 
> -  <interface name="zxdg_output_v1" version="2">
> +  <interface name="zxdg_output_v1" version="3">
>      <description summary="compositor logical output region">
>        An xdg_output describes part of the compositor geometry.
> 
> @@ -157,6 +157,10 @@
> 
>  	This allows changes to the xdg_output properties to be seen as
>  	atomic, even if they happen via multiple events.
> +
> +	For objects version 3 onwards, this event is deprecated. Compositors
> +	are not required to send it anymore and must send wl_output.done
> +	instead.
>        </description>
>      </event>
> 
> --
> 2.21.0
> 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel