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

[Michel Dänzer]

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?

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


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


>     > Another more serious problem would be, if an application only uses a
>     > fixed number of pixmap and never uses one which hasn't yet received
>     > the PresentIdleNotify event. In this case the app execution could halt.
> 
>     Not sure that's an issue. Every PresentCompleteNotify event should be
>     followed by the corresponding PresentCompleteIdle event within
>     finite time.
> 
> Yes, you're right in probably all relevant cases. It's just a principal
> concern, because the Wayland specification mentions explicitly that a
> compositor is free to send the buffer release event at any time. If a
> compositor now would for whatever reason hoard these buffers and send
> the release event back in batches after few seconds the client could run
> out of pixmaps in these few seconds and stop refreshing its content. But
> yea, it's probably not a real world problem.

Yeah, that would presumably hurt native Wayland clients as well.


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


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