[PATCH xserver v2 5/5] xwayland: use _XWAYLAND_ALLOW_COMMITS property

Thu Jan 12 14:27:10 UTC 2017

On Tue, 3 Jan 2017 04:31:39 -0500 (EST)
Olivier Fourdan <ofourdan at redhat.com> wrote:

> Hi,
> 
> ----- Original Message -----
> > On Fri, 2016-12-09 at 14:24 +0200, Pekka Paalanen wrote:  
> > > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > > 
> > > The X11 window manager (XWM) of a Wayland compositor can use the
> > > _XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
> > > wl_surface.commit requests. If the property is not set, the behaviour
> > > remains what it was.  
> > 
> > I'm still thinking over whether I like this or whether I'd rather have
> > this keyed off the netwm sync props. (Did we think that was a thing
> > that could work? Forgive me, I'm freshly back from holidays.) [...]  
> 
> Maybe it's two different goals. If I understand correctly, this patch
> was initially intended for the initial map of the window.

Hi,

yes, that's correct.

Btw. I would love to write a documentation patch explaining what
_XWAYLAND_ALLOW_COMMITS is meant for, how it works, and how it should
be used. Where would such documentation live? I might suggest putting
it in the Wayland docbook, but that would be a bit odd, since it is
documenting X.org's Xwayland implementation. But I bet it would make
reviewing this patch easier.

Originally I was working on letting -geometry option of X11 programs to
allow setting the initial position of the window. I got it implemented
in Weston, but with the regression that all decorated windows would
appear to jump once when mapped (without -geometry, even). The root
cause for that was the racing between Xwayland and the XWM/compositor.

Xwayland did the first commit before the decorations were drawn, that
caused the compositor to position and show the window, then the
decorations appeared and the window needed to move to account for the
added decorations. Except that is not the whole truth, because if it
happened like that, I would see the decorations appear around the
window, but instead I see the decorated window jump to another
position, so there is probably also a drawing/compositing race going
on. Plus I am not quite sure if Xwayland is still single-buffered to
towards the compositor which would be incorrect behaviour of a Wayland
client (but was originally written so, because maintaining multiple
buffers would have cost significantly more and X11 drawing does not
have a concept of "completed frames" anyway...).

Anyway, the initial mapping is a mess if Xwayland commits before XWM is
ready, and it would be quite difficult for XWM to internally postpone
the mapping in the compositor. I hope we agreed at least that much
already.

This patch is particularly for the initial mapping, especially for apps
and compositors that do not use NET_WM_SYNC (yet).

Adding support for NET_WM_SYNC protocol would be the next step (in
Weston), but I am not sure when I would get there. I suppose the XWM
could drive it via _XWAYLAND_ALLOW_COMMITS or Xwayland could hook up to
the NET_WM_SYNC protocol messages itself like Adam and Daniel discussed.

However, would NET_WM_SYNC apply to initial mapping of windows for
clients/XWM that do support it?

I read the _NET_WM_SYNC_REQUEST spec [1] and it does not say, it only
talks about resizing and ConfigureNotify. Do apps generally set up
NET_WM_SYNC before they map the window?

[1] https://specifications.freedesktop.org/wm-spec/wm-spec-latest.html#idm140200472538288

> FWIW I spent quite a few days before the holidays experimenting with
> this, trying to see if that could be used to help with the black
> border issue reported in mutter with server-side decorations [1] on
> applications implementing the NET_WM_SYNC extended protocol [2].
> 
> Unfortunately, I didn't have much success in doing so.
> 
> Freezing the commit in either meta_window_actor_freeze() or
> meta_window_actor_pre_paint() and re-allowing the commit in
> meta_window_actor_thaw() or meta_window_actor_post_paint() didn't
> help at all with the visible black border during resize, and made the
> resize more sluggish because while the commit is frozen, the surface
> is not resized but can still be moved so resizing from the left or
> top looks awful, from a user stand point.
> 
> I think I could trace the black border issue in mutter back to
> meta_window_actor_update_opaque_region(), but I was not able to use
> the _XWAYLAND_ALLOW_COMMITS property to fix or even improve the
> problem, so I am not even sure _XWAYLAND_ALLOW_COMMITS  is the
> solution for the problem I am trying to fix - Granted, I am not an
> expert in this area of mutter, but I start to wonder if
> _XWAYLAND_ALLOW_COMMITS is the solution to the specific issue I am
> trying to solve.

Olivier, when we talked in IRC, you said you weren't sure at which
point mutter is actually emitting the X11 commands to draw the
decorations. Did you find out? I believe it would be very important to
draw decorations while commits are disabled.

Also mind that drawing decorations goes to X11, while compositing the
decorations happens much later when the content comes back from
Xwayland with a commit. So if you draw decors and immediately
composite, you likely will be compositing outdated content. If you do
that and use the new state instead of the old, I think you'd be pretty
guaranteed to see bad rendering. Can you check if that is happening?

Weston has some machinery to set up internal pending state, instead of
current state, when XWM updates decorations etc. via X11. That pending
state gets latched in on the next commit from Xwayland. But even that
has a false assumption that the next commit is right one - Xwayland
could be committing old stuff around the same time and be mistaken as
the commit XWM expected.

The latter problem is particularly with input regions in Weston
(theoretical - I'm not sure there are actually noticeable effects).
I've been thinking if we could give the Wayland state update to
Xwayland so that Xwayland could push it out with the correct commit.
Right now, Xwayland never uses wl_surface.set_input_region (or
set_opaque_region) which is part of the Wayland core protocol. That is
a whole another story though.

> So, we might eventually need both, something like
> _XWAYLAND_ALLOW_COMMITS for the initial map and also have Xwayland
> monitoring NET_WM_SYNC properties.

Yeah. I think it should be pretty easy and not hugely too bad to have
XWM use _XWAYLAND_ALLOW_COMMITS also for NET_WM_SYNC so we didn't need
the NET_WM_SYNC support in Xwayland. Of course, it would be more
efficient in Xwayland proper.

> Cheers,
> Olivier
> 
> [1] https://bugzilla.gnome.org/show_bug.cgi?id=767212
> [2] http://fishsoup.net/misc/wm-spec-synchronization.html
> 

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170112/d7cbc171/attachment.sig>