Reworking subsurface support
ClosedPublic

Authored by graesslin on Mar 21 2016, 3:17 PM.

Details

Summary

This is a commit series consisting of the following individual commits. If needed I can create reviews for the individual commits.

commit 710f9eaf042f65799518e3987de82018cedcfbde
Author: Martin Gräßlin <mgraesslin@kde.org>
Date: Fri Mar 18 09:11:28 2016 +0100

[tests] Add a sub-surface test application

The test application creates a sub-surface tree consisting of overall
three surfaces:
* blue main surface
* red sub surface
* green sub surface to the red sub surface

All surfaces are in synchronized mode. There is a timer to turn the
green surface into yellow after five seconds.

commit c1db900ad8621b6364b22a7889370423a72154cc
Author: Martin Gräßlin <mgraesslin@kde.org>
Date: Fri Mar 18 09:13:27 2016 +0100

[server] Send frameRendered to all sub-surfaces

If a surface got rendered it implies that all sub-surfaces also got
rendered. So pass the frameRendered to the complete sub-surface tree.

commit 43b7e10e1a31f1e09b0a86d8f9c32a8f155bfc53
Author: Martin Gräßlin <mgraesslin@kde.org>
Date: Fri Mar 18 10:41:56 2016 +0100

[server] Add a bool SubSurfaceInterface::isSynchronized() const

The mode is not sufficient to determine whether a SubSurface is in
synchronized mode.

Quoting spec:
"Even if a sub-surface is in desynchronized mode, it will behave as in
synchronized mode, if its parent surface behaves as in synchronized mode.
This rule is applied recursively throughout the tree of surfaces.
This means, that one can set a sub-surface into synchronized mode, and
then assume that all its child and grand-child sub-surfaces are
synchronized, too, without explicitly setting them."

commit 43b7e10e1a31f1e09b0a86d8f9c32a8f155bfc53
Author: Martin Gräßlin <mgraesslin@kde.org>
Date: Fri Mar 18 10:41:56 2016 +0100

[server] Add a bool SubSurfaceInterface::isSynchronized() const

The mode is not sufficient to determine whether a SubSurface is in
synchronized mode.

Quoting spec:
"Even if a sub-surface is in desynchronized mode, it will behave as in
synchronized mode, if its parent surface behaves as in synchronized mode.
This rule is applied recursively throughout the tree of surfaces.

When committing the state of a sub-surface, the state should not
be immediately applied if the sub-surface is in synchronized mode.
Instead it should be cached and only applied after the parent surface's
state is applied.

To implement this the Surface::Private has now a third cached state
buffer. When committing the state is either swapped between pending and
current or pending and subSurfacePending. Once the parent state is
applied the state is swapped between subSurfacePending and current.

The logic for applying state changes is changed. Instead of copying the
complete state object, the individual state changes are now copied and the
source gets completely reset to default values. Only the children tree is
copied back, as that list needs to be modified.

commit c61ad393e0b3a7ec44ac5f301a77137233c9feba
Author: Martin Gräßlin <mgraesslin@kde.org>
Date: Fri Mar 18 09:57:46 2016 +0100

[server] Cache the state of synchronized sub surfaces

When committing the state of a sub-surface, the state should not
be immediately applied if the sub-surface is in synchronized mode.
Instead it should be cached and only applied after the parent surface's
state is applied.

To implement this the Surface::Private has now a third cached state
buffer. When committing the state is either swapped between pending and
current or pending and subSurfacePending. Once the parent state is
applied the state is swapped between subSurfacePending and current.

The logic for applying state changes is changed. Instead of copying the
complete state object, the individual state changes are now copied and the
source gets completely reset to default values. Only the children tree is
copied back, as that list needs to be modified.

commit f616dc48b548580a1fe831b5d1e47e50e09d96b6
Author: Martin Gräßlin <mgraesslin@kde.org>
Date: Mon Mar 21 14:32:31 2016 +0100

[server] Add a subSurfaceTreeChanged signal to SurfaceInterface

The idea behind this signal is to notify whenever the tree of sub
surfaces changes in a way that a repaint of the Surface is required.
Possible situations are:
* surface damaged
* surface unmapped
* subsurface added/removed
* subsurface moved (position changed)
* subsurface stacking changed

Ideally it would be possible to provide the actual area which needs
repainting, but due to the possible complexity of the tree, synced
and desynced changes this doesn't look worth the effort. A user of
the signal might trigger too many repaints with it, but if it really
wants to be only notified about the actual changes, it can just track
the individual sub-surfaces.

commit 045f2b6de1462961004919d6ab0935c7b435571a
Author: Martin Gräßlin <mgraesslin@kde.org>
Date: Mon Mar 21 15:53:13 2016 +0100

[server] Don't double buffer adding/removing of sub-surfaces

QtWayland doesn't commit the parent surface when creating a sub-surface.
This results in a QtWayland application to freeze as it renders to the
surface and waits for the frame rendered, which it will never get as the
Compositor waits for the commit on the parent prior to mapping the
sub-surface.

To work around this behavior, we apply the adding/removing directly.
The behavior around this is actually not fully documented, so QtWayland
is not wrong per se. See:

https://lists.freedesktop.org/archives/wayland-devel/2016-March/027540.html

Once this is properly clarified and implemented in the Client, we should
revert this change.
Test Plan

Most of the testing is done with the added test case in f616dc48b548580a1fe831b5d1e47e50e09d96b6

Diff Detail

Repository
R127 KWayland
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
graesslin updated this revision to Diff 2881.Mar 21 2016, 3:17 PM
graesslin retitled this revision from to Reworking subsurface support.
graesslin updated this object.
graesslin edited the test plan for this revision. (Show Details)
graesslin set the repository for this revision to R127 KWayland.
graesslin added a project: Plasma.
graesslin added a subscriber: Plasma.
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptMar 21 2016, 3:17 PM
sebas accepted this revision.Mar 21 2016, 10:55 PM
sebas added a reviewer: sebas.
sebas added a subscriber: sebas.

I read over it, and as far as I understand what's the problem, your patches look like they make sense.

autotests/client/test_wayland_subsurface.cpp
220 ↗(On Diff #2881)

use the function pointer version of QSignalSpy?

589 ↗(On Diff #2881)

its pending state

src/server/surface_interface.cpp
243 ↗(On Diff #2881)

that's not a "factor", perhaps just "transformChanged"?

243 ↗(On Diff #2881)

whitespace after &&

I generally prefer to put parentheses around these right hand sides of the bool assignment, btw. More like a personal preference, which makes it a bit easier to read.

This revision is now accepted and ready to land.Mar 21 2016, 10:55 PM
graesslin marked 2 inline comments as done.Mar 23 2016, 1:16 PM
graesslin added inline comments.
autotests/client/test_wayland_subsurface.cpp
220 ↗(On Diff #2881)

yeah, but that's old code.

src/server/surface_interface.cpp
243 ↗(On Diff #2881)

yep I also noticed the incorrect wording while adjusting. Decided to leave it to not have too many unrelated changes in it. Will do a follow-up change to adjust it.

This revision was automatically updated to reflect the committed changes.