Second Feedback request for my GSoC project to improve Present support in Xwayland

Mon Aug 7 08:36:27 UTC 2017

On 04/08/17 06:57 PM, Roman Gilg wrote:
> On Fri, Aug 4, 2017 at 8:44 AM, Michel Dänzer <michel at daenzer.net
> <mailto:michel at daenzer.net>> wrote:
> 
>     On 03/08/17 09:11 PM, Roman Gilg wrote:
>     > On Wed, Aug 2, 2017 at 11:36 AM, Michel Dänzer <michel at daenzer.net <mailto:michel at daenzer.net>
>     > <mailto:michel at daenzer.net <mailto:michel at daenzer.net>>> wrote:
>     >
>     >     On 02/08/17 03:53 AM, Roman Gilg wrote:
>     >     >
>     >     > Overview of differences in Rootless mode:
>     >     >
>     >     > * Maintain a list of all windows, that are currently flipping pixmaps.
>     >     > * Instead of saving the current flip_pixmap and so on in
>     >     > present_screen_priv save it in present_window_priv.
>     >     > * Don't restore the screen pixmap, instead save and refcount the pixmap
>     >     > used by the window before Present took over and restore the window with
>     >     > this pixmap.
>     >     > * Introduce an additional DDX function hook flip_executed, that is
>     >     > called directly before the flip is finished being executed in Present to
>     >     > then fully commit the flipped window pixmap with the associated damage
>     >     > and directly signal the flip as being complete.
>     >
>     >     I don't understand what the flip_executed hook is good for.
>     >
>     > On flip the Present extension has not yet calculated the damage for the
>     > pixmap. This is not important on full screen flips in hardware, where we
>     > used Present before only. But when we talk to the Wayland compositor, we
>     > can tell it what parts of the surface are damaged.
> 
>     A flip always damages the full window pixmap?
> 
> 
> If the client specifies an update area, Present will only mark this area
> as damaged:
> https://cgit.freedesktop.org/xorg/xserver/tree/present/present.c#n703
> It's also important to note that it does this after the tree pixmap has
> been set to the flip pixmap. So it's really applied to the flip pixmap.

Hah, never noticed this before, thanks.

>     > Also there is the following issue, which I encountered: The Present
>     > extension assumes, that on flip the DDX is not able to flip immediately,
>     > but signals present_even_notify for the flip a little bit later (this
>     > doesn't mean that's not async, just that the flip is pending for a
>     > little while). But in the Xwayland  case (or maybe other DDX), we can
>     > and want flip immediately.
> 
>     Then you can call present_event_notify immediately. :)
> 
> The problem I faced when doing that, was that present_event_notify
> directly does some changes to the current and pending flips, while the
> present_execute routine assumes that these changes are not yet done.
> 
> For example vblank->pixmap gets nulled on present_event_notify here:
> https://cgit.freedesktop.org/xorg/xserver/tree/present/present.c#n501
> But it's still needed after the flip here:
> https://cgit.freedesktop.org/xorg/xserver/tree/present/present.c#n698
> 
> While I tried in the beginning to simply guard against these changes
> individually, for example by setting a local variable vblank_pixmap =
> vblank->pixmap in present_execute and then working with the local
> variable, I felt that the more clean and secure version is to introduce
> the additional hook for DDX' with immediate flips.

Is there such a thing as an "immediate flip" though? All you can do in
the flip hook is send the "flip" request to the Wayland compositor,
right? The intent of the current interface is that the driver only calls
present_event_notify once it receives confirmation that the flip has
completed. Are there no appropriate Wayland events that can be used for
this?

>     >     > ad (3):
>     >     >
>     >     > In the Weston session I noticed extreme graphical artifacts in Neverball
>     >     > (for a description of this app see below in Testing section). For sure
>     >     > this was because of the reuse of pixmaps by the application after
>     >     > Xwayland signaled that the pixmap is free to use again, but before the
>     >     > compositor has released it. There were no artifacts in KWin, so I assume
>     >     > KWin just quickly enough copies the pixel content and doesn't touch it
>     >     > again afterwards. In the Weston case though the following happened:
>     >     >
>     >     > * Present gives pixmap A to Xwayland.
>     >     > * A gets immediately committed and the flip is signaled as being
>     >     > complete to Present (if we would wait for the frame callback, we would
>     >     > have an implicit Vblank frame rate limit).
>     >
>     >     Ideally, this should be triggered by and use the information from the
>     >     Wayland Presentation-time extension events?
>     >
>     > As far as I've understood the Presentation time extension, it informs
>     > the client about the time the front buffer has flipped and probably the
>     > next one will flip (which can happen for some compositors according to
>     > the refresh rate, which might be on a 60 Hz display 60 times a second).
>     > But we can't use this in our case, because it would limit the client's
>     > frame rate to the one of the compositor,
> 
>     I'd say that's what we want, at least without PresentOptionAsync.
> 
>     > but we want to allow the client to render at its own speed.
> 
>     That might not be much faster than the compositor's rendering cycle
>     anyway when flipping, because the compositor has to release presented
>     buffers before they can be reused.
> 
> It is though. I tested it with Neverball. It has a frame rate of over
> 1000 on my workstation while the compositor runs at 60.

That makes sense when PresentOptionAsync was passed in, but probably not
otherwise.

> A Wayland buffer is released immediately by any sane compositor if another
> one for the same surface is commited before the end of the current rendering
> cycle.
> The client is then free to reuse this
> buffer: https://wayland.freedesktop.org/docs/html/apa.html#protocol-spec-wl_buffer-event-release

Makes sense.

>     >     > ad (4):
>     >     >
>     >     > With the above I'm able to reliable flip pixmaps, when the flipping
>     >     > window region equals the region of the toplevel window. This is
>     >     > according to my experiments the case when not using any Composite
>     >     > clipping - see VLC in Testing below for a good example. But tearing also
>     >     > happens in composite windows, especially if a Wayland server wants to
>     >     > outscan a Xwayland buffer to an overlay plane.
>     >     >
>     >     > For that I use a sub-surface for the flipping window. The flipping
>     >     > window's pixmap buffer is flipped exclusively on the sub-surface, while
>     >     > the rest of the window is committed to the main surface.
>     >
>     >     This would require wrapping all Screen/GC rendering hooks to handle the
>     >     sub-surfaces correctly.
>     >
>     > Can you give an example why the wrapping would be necessary? I don't
>     > quite see it. With the current code only the client's pixmap pixel data
>     > is depicted on the Wayland sub-surface. Since the pixmap is rendered by
>     > the client directly and independently to the rendering on the rest of
>     > the Screen, everything else stays the same.
> 
>     The semantics of PresentPixmap are such that it must appear to clients
>     as though it copies the contents of the source pixmap to the destination
>     window. In particular, reading the window contents e.g. with GetImage
>     must return the presented contents from the source pixmap. (That's why
>     the existing flipping implementation does all the gymnastics changing
>     the window pixmap of the flip and root windows) It's also possible in
>     theory (though unlikely in practice) for a client to interleave
>     PresentPixmap and other drawing requests to the same window, in which
>     case the drawing from other requests must appear on top of the contents
>     from the last (completed) PresentPixmap request.
> 
> Well, after you explained the TraverseTree stuff in the first mail to me
> my code does now use it to set the flip pixmap as the window pixmap:
> https://github.com/subdiff/xserver/blob/presentInXwayland/present/present.c#L875

That only applies for "full" flips, whereas this sub-thread is about the
sub-surface case (4). In that case the window pixmap cannot be replaced
by the flip pixmap. So when a drawing operation writes to or reads from
the flip window, it has to write to or read from the sub-surface where
it is visible and the window pixmap otherwise.

E.g. try taking a screenshot using an X11 tool of a toplevel window of
which a part is flipping using a sub-surface.

> I'm not sure what you mean with "client to interleave". If you mean that
> for a particular window the client switches forth and back between
> flipping pixmap and painting to the normal window pixmap,

I mean the client switching back and forth between PresentPixmap and
other drawing requests (e.g. CopyArea) using the same window as the
destination.

> this should be possible with my code, since I remember the normal window pixmap and
> restore it on unflip:
> https://github.com/subdiff/xserver/blob/presentInXwayland/present/present.c#L512

I don't think the above would trigger an unflip for a sub-surface flip.

> I'm not sure how this as a whole is connected to the usage of
> sub-surfaces though. To the internals of the X server the sub-surface is
> practically invisible. It's basically just a buffer display at the right
> location on the Wayland side of things.

That is exactly the problem, X11 clients must see the sub-surface contents.

-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer