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

Wed Aug 2 09:36:43 UTC 2017

On 02/08/17 03:53 AM, Roman Gilg wrote:
> According to the feedback I received on my first mail (see:
> https://lists.x.org/archives/xorg-devel/2017-July/054136.html) I
> reworked huge parts of my code for enabling Present pixmap flips in
> Xwayland. The current code now provides the following functionality:
> 
> 
> --------
> Summary:
> --------
> 
> (1) Deactivate queued presentation support for now (until we know which
> kind of Wayland protocol to use for).
> (2) Per window flipping when the pixmap flipping window region equals
> the toplevel window.
> (3) Listen on buffer release by Wayland compositor and only then allow
> pixmap reuse. This is important but kind of controversial (see below).
> (4) Support window pixmap flipping via Wayland sub-surfaces for windows,
> that do _not_ region equal their toplevel window, i.e. windows which are
> composited into the toplevel window with the Composite extension.

Sounds like nice improvements in general.

> ad (2):
> 
> This was not simple at all. The DIX Present code had been written from
> the ground up to only support flips for one full screen windows, whereas
> full screen means spanning a whole _Screen. So I wrote a secondary code
> path "Rootless mode", selected by a DDX by setting a Boolean in the
> screen_info struct.
> 
> 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.

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

> * Present signals PresentCompleteNotify to the app and the app then
> directly starts drawing the next pixmap B, which is then again sent to
> Xwayland.
> * B gets immediately committed again and the flip is signaled es being
> complete
> * With B being flipped Present assumes, that A is directly reusable and
> signals PresentCompleteNotify for B _and_ PresentIdleNotify for A.
> * The app repaints into A or destroys it while still being read out by
> Weston in some way.
> 
> The solution is to listen for the buffer release signal by the Wayland
> compositor and only on this signal send PresentIdleNotify to the app
> instead of directly sending it on the next PresentCompleteNotify. To
> enable that in the Rootless code path I did the following:
> 
> * In Present don't directly signal PresentIdleNotify on
> PresentCompleteNotify of the next pixmap.
> * Don't destroy the vblank struct after the flip. Rather put it in a
> field present_window_priv_rec.flip_active and after the next flip has
> been processed put it in the added static xorg_list present_idle_queue.
> * To get further processed with PresentIdleNotify and deleted from this
> list again the DDX just needs to send an additional third
> present_event_notify with the event_id of the flip.
> 
> Reminder: For potentially other DDX, that want to use the Rootless code
> path in Present, comment the code, that they must not forget to send
> this third present_event_notify. Otherwise the old pixmaps get never freed.
> 
> The controversial part: Unfortunately the Present protocol specification
> mentions in one sentence in between, that PresentIdleNotify is directly
> send on the PresentCompleteNotify of the next pixmap:
> https://cgit.freedesktop.org/xorg/proto/presentproto/tree/presentproto.txt#n412

It doesn't make sense to me to have two different events, but always
send them out at the same time, so they both convey essentially the same
information. Maybe we just need to clarify the specification text.

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

> On the other side I can't think of another way of removing the visual
> artifacts than to hold out on PresentIdleNotify until the Wayland
> compositor has signaled the buffer release. It's also the most natural
> thing to do in the Wayland environment.

I think it would also be desirable with our Xorg drivers. It should be
easy to extend the DDX driver interface for this in a backwards
compatible way via a PresentCapability* flag.

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

Since this same issue affects clients that don't use the Present
extension anyway, maybe there's a different solution which helps for all
clients. E.g. Xwayland could use multiple buffers for each window pixmap
and flip between them, tracking damage and copying the minimum damaged
area necessary when moving to the next "back" buffer for rendering.
Anyway, that's probably not in the scope of your GSoC project anymore,
rather food for thought longer term.

> ---------------
> Open Questions:
> ---------------
> 
> * For (4) is the Composite extension the only explanation for the region
> of the xwl_window to differ from the presenting child window? Are there
> other cases I don't yet know about that I have to look out for?

I'd say the general case for this is that the presenting child window's
geometry doesn't completely cover the top-level window. (The Composite
extension would basically be the only way for the child window to
completely cover its pixmap anyway, though I'm not sure this can be the
case with Xwayland as well)

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