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

Fri Aug 4 09:57:22 UTC 2017

On Fri, Aug 4, 2017 at 8:44 AM, Michel Dänzer <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>> 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.

> > 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. This way such a DDX can be
sure that present_event_notify doesn't anymore screw with the ongoing
present_execute. Also this way we also get the current damage region easily
(which is computed after the flip).

>
> >     > 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. 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

>
> >     > 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

It just doesn't set it as root pixmap. But since the rootless Xwayland mode
is built with the implicit assumption that the root pixmap is not needed
for its rendering I assume setting the root pixmap is neither necessary nor
desirable.

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, 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'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.

Cheers
Roman
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170804/82a0c1e4/attachment-0001.html>