[scene] Generate window quads for sub-surfaces
ClosedPublic

Authored by zzag on Apr 23 2020, 1:35 PM.

Details

Summary

No window quads are generated for sub-surfaces right now. This means that
effects that operate on window quads are not able to transform
sub-surfaces. Furthermore, the OpenGL scene needs window quads to clip
windows without using the scissor test.

The best (imho) way to render sub-surfaces would be with some help from a
scene graph. Unfortunately, KDE hasn't developed any scene graph
implementation that we could plug into kwin and let the scene graph do
its magic. As a short term solution, this patch adjusts relevant parts of
the scene to make it generate window quads for sub-surfaces.

In order to build window quads, we traverse the current window pixmap
tree in DFS manner. When we enter a pixmap, we generate window quads. An
id is assigned to each quad that can be later used to match a list of
window quads against some particular window pixmap.

With the proposed change, no changes to effects are required. Effects see
window quads for sub-surfaces as regular WindowQuadContents quads.

Cross-fading is still kind of broken. In order to cross-fade between two
window pixmap trees, we need to render each tree in a texture, then
cross-fade between those two textures in a shader. Unfortunately, this is
not easy to do with the current design, so I only left a FIXME comment.
Although it should be pretty straightforward with a scene graph.

BUG: 387313

Test Plan

Enabled the wobbly windows effect, moved weston-subsurfaces
around on the screen.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zzag created this revision.Apr 23 2020, 1:35 PM
Restricted Application added a project: KWin. · View Herald TranscriptApr 23 2020, 1:35 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Apr 23 2020, 1:35 PM
zzag added a comment.Apr 23 2020, 1:41 PM

For reviewers: note that this change is split into 6 commits.

zzag updated this revision to Diff 81003.Apr 23 2020, 2:26 PM

Fix a couple of mistakes.

zzag edited the summary of this revision. (Show Details)Apr 23 2020, 2:34 PM
apol added a subscriber: apol.Apr 23 2020, 3:06 PM

+1
Looks good to me overall (not that I'm overly familiar with subsurfaces), commented some nitpicks.

plugins/scenes/opengl/scene_opengl.cpp
1267

Doesn't look like a fixme, more of a phabricator task to tackle in the future?

Reading this makes others feel sad but doesn't make one implement anything.

1292

For readability it would make sense to keep a reference of the objects we're interacting with.

auto &shadowRenderNode = renderNodes[context.shadowOffset];
if (!shadowRenderNode.quads.isEmpty())
...
scene.cpp
1014

How about

for (const QRectF &rect : region) {
  const QRect positioned = rect.translated(position);
  const QRect scaled = rect * scale;

  quad[0] = WindowVertex(positioned.topLeft(), scaled.topLeft());
  quad[1] = WindowVertex(positioned.topRight(), scaled.topRight());
  quad[2] = WindowVertex(positioned.bottomRight(), scaled.bottomRight());
  quad[3] = WindowVertex(positioned.bottomLeft(), scaled.bottomLeft());

  quads << quad;
}
subsurfacemonitor.cpp
45 ↗(On Diff #81003)

Why's that commented?

zzag added inline comments.Apr 23 2020, 5:11 PM
plugins/scenes/opengl/scene_opengl.cpp
1267

Ok, I'll create a task.

subsurfacemonitor.cpp
45 ↗(On Diff #81003)

Urgh, I forgot to add "TODO". SurfaceInterface currently doesn't have mapped() signal.

zzag added inline comments.Apr 23 2020, 5:17 PM
scene.cpp
1014

Can't do rect * scale because it rounds numbers.

zzag updated this revision to Diff 81029.Apr 23 2020, 5:45 PM

Implement Aleix's suggestions.

zzag marked 7 inline comments as done.Apr 23 2020, 5:46 PM
meven added a subscriber: meven.Apr 24 2020, 8:20 AM
meven added inline comments.
scene.cpp
1014

Even using QRectF instead of QRect ?

zzag added inline comments.Apr 24 2020, 8:25 AM
scene.cpp
1014

Oh, yeah. I use QRectF because right() and bottom() return expected values, but now I recall that I should avoid using right() and bottom(). I'll change QRectF to QRect. This will also make Qt Creator a bit happier.

zzag updated this revision to Diff 81072.Apr 24 2020, 8:29 AM
zzag marked 2 inline comments as done.

Use QRect instead of QRectF

apol added a comment.EditedApr 24 2020, 5:48 PM

This patch makes my kwin crash when running firefox on it.

#0  0x00007f8db878cce5 in raise () at /usr/lib/libc.so.6
#1  0x00007f8db877692c in abort () at /usr/lib/libc.so.6
#2  0x00007f8db8e4bbd1 in qt_message_fatal (context=..., message=<synthetic pointer>...) at /home/apol/devel/frameworks/qt5/qtbase/src/corelib/global/qlogging.cpp:1914
#3  QMessageLogger::fatal(char const*, ...) const (this=this@entry=0x7fff438d4ae0, msg=msg@entry=0x7f8db9143ec8 "ASSERT failure in %s: \"%s\", file %s, line %d") at /home/apol/devel/frameworks/qt5/qtbase/src/corelib/global/qlogging.cpp:893
#4  0x00007f8db8e4b01e in qt_assert_x(char const*, char const*, char const*, int) (where=<optimized out>, what=<optimized out>, file=<optimized out>, line=<optimized out>)
    at ../../include/QtCore/../../../../../devel/frameworks/qt5/qtbase/src/corelib/global/qlogging.h:90
#5  0x00007f8db10d5ca5 in QVector<KWin::OpenGLWindow::RenderNode>::operator[](int) (this=0x7fff438d4e30, i=3) at /home/apol/devel/kde5/include/QtCore/qvector.h:463
#6  0x00007f8db10c5fa3 in KWin::OpenGLWindow::initializeRenderContext(KWin::OpenGLWindow::RenderContext&, KWin::WindowPaintData const&) (this=0x5596973cbbc0, context=..., data=...) at /home/apol/devel/frameworks/kwin/plugins/scenes/opengl/scene_opengl.cpp:1315
#7  0x00007f8db10c6b51 in KWin::OpenGLWindow::performPaint(int, QRegion const&, KWin::WindowPaintData const&) (this=0x5596973cbbc0, mask=10, region=..., _data=...) at /home/apol/devel/frameworks/kwin/plugins/scenes/opengl/scene_opengl.cpp:1439
#8  0x00007f8db10c4c33 in KWin::SceneOpenGL2::performPaintWindow(KWin::EffectWindowImpl*, int, QRegion const&, KWin::WindowPaintData&) (this=0x5596962b18a0, w=0x5596973805c0, mask=10, region=..., data=...)
    at /home/apol/devel/frameworks/kwin/plugins/scenes/opengl/scene_opengl.cpp:1063
#9  0x00007f8db10c4aa0 in KWin::SceneOpenGL2::finalDrawWindow(KWin::EffectWindowImpl*, int, QRegion const&, KWin::WindowPaintData&) (this=0x5596962b18a0, w=0x5596973805c0, mask=10, region=..., data=...)
    at /home/apol/devel/frameworks/kwin/plugins/scenes/opengl/scene_opengl.cpp:1045
#10 0x00007f8dbc9fbf21 in KWin::EffectsHandlerImpl::drawWindow(KWin::EffectWindow*, int, QRegion const&, KWin::WindowPaintData&) (this=0x559696d53a50, w=0x5596973805c0, mask=10, region=..., data=...) at /home/apol/devel/frameworks/kwin/effects.cpp:488
#11 0x00007f8dbc6311d5 in KWin::BlurEffect::drawWindow(KWin::EffectWindow*, int, QRegion const&, KWin::WindowPaintData&) (this=0x559696bc1db0, w=0x5596973805c0, mask=10, region=..., data=...) at /home/apol/devel/frameworks/kwin/effects/blur/blur.cpp:615
#12 0x00007f8dbc9fbed0 in KWin::EffectsHandlerImpl::drawWindow(KWin::EffectWindow*, int, QRegion const&, KWin::WindowPaintData&) (this=0x559696d53a50, w=0x5596973805c0, mask=10, region=..., data=...) at /home/apol/devel/frameworks/kwin/effects.cpp:485
#13 0x00007f8dbcacaf7e in KWin::Scene::finalPaintWindow(KWin::EffectWindowImpl*, int, QRegion const&, KWin::WindowPaintData&) (this=0x5596962b18a0, w=0x5596973805c0, mask=10, region=..., data=...) at /home/apol/devel/frameworks/kwin/scene.cpp:627
#14 0x00007f8dbc9fbc03 in KWin::EffectsHandlerImpl::paintWindow(KWin::EffectWindow*, int, QRegion const&, KWin::WindowPaintData&) (this=0x559696d53a50, w=0x5596973805c0, mask=10, region=..., data=...) at /home/apol/devel/frameworks/kwin/effects.cpp:451
#15 0x00007f8dbc5cd81f in KWin::Effect::paintWindow(KWin::EffectWindow*, int, QRegion, KWin::WindowPaintData&) (this=0x559696bc1db0, w=0x5596973805c0, mask=10, region=..., data=...) at /home/apol/devel/frameworks/kwin/libkwineffects/kwineffects.cpp:592
#16 0x00007f8dbc9fbbbc in KWin::EffectsHandlerImpl::paintWindow(KWin::EffectWindow*, int, QRegion const&, KWin::WindowPaintData&) (this=0x559696d53a50, w=0x5596973805c0, mask=10, region=..., data=...) at /home/apol/devel/frameworks/kwin/effects.cpp:448
#17 0x00007f8dbcac9bcc in KWin::Scene::paintWindow(KWin::Scene::Window*, int, QRegion const&, KWin::WindowQuadList const&) (this=0x5596962b18a0, w=0x5596973cbbc0, mask=10, _region=..., quads=...) at /home/apol/devel/frameworks/kwin/scene.cpp:499
#18 0x00007f8dbcac8e1d in KWin::Scene::paintSimpleScreen(int, QRegion const&) (this=0x5596962b18a0, orig_mask=8, region=...) at /home/apol/devel/frameworks/kwin/scene.cpp:375
#19 0x00007f8db10c4793 in KWin::SceneOpenGL2::paintSimpleScreen(int, QRegion const&) (this=0x5596962b18a0, mask=8, region=...) at /home/apol/devel/frameworks/kwin/plugins/scenes/opengl/scene_opengl.cpp:1010
#20 0x00007f8dbcac7d3b in KWin::Scene::finalPaintScreen(int, QRegion const&, KWin::ScreenPaintData&) (this=0x5596962b18a0, mask=8, region=..., data=...) at /home/apol/devel/frameworks/kwin/scene.cpp:200
#21 0x00007f8dbc9fb93c in KWin::EffectsHandlerImpl::paintScreen(int, QRegion const&, KWin::ScreenPaintData&) (this=0x559696d53a50, mask=8, region=..., data=...) at /home/apol/devel/frameworks/kwin/effects.cpp:408
#22 0x00007f8dbc5cd74e in KWin::Effect::paintScreen(int, QRegion const&, KWin::ScreenPaintData&) (this=0x559696bc1db0, mask=8, region=..., data=...) at /home/apol/devel/frameworks/kwin/libkwineffects/kwineffects.cpp:577
#23 0x00007f8dbc9fb908 in KWin::EffectsHandlerImpl::paintScreen(int, QRegion const&, KWin::ScreenPaintData&) (this=0x559696d53a50, mask=8, region=..., data=...) at /home/apol/devel/frameworks/kwin/effects.cpp:405
#24 0x00007f8dbcac7a0e in KWin::Scene::paintScreen(int*, QRegion const&, QRegion const&, QRegion*, QRegion*, QMatrix4x4 const&, QRect const&, double)
    (this=0x5596962b18a0, mask=0x7fff438d5830, damage=..., repaint=..., updateRegion=0x7fff438d5850, validRegion=0x7fff438d5858, projection=..., outputGeometry=..., screenScale=2) at /home/apol/devel/frameworks/kwin/scene.cpp:150
#25 0x00007f8db10c2738 in KWin::SceneOpenGL::paint(QRegion const&, QList<KWin::Toplevel*> const&) (this=0x5596962b18a0, damage=..., toplevels=...) at /home/apol/devel/frameworks/kwin/plugins/scenes/opengl/scene_opengl.cpp:652
#26 0x00007f8dbc9a1439 in KWin::Compositor::performCompositing() (this=0x5596962a2b10) at /home/apol/devel/frameworks/kwin/composite.cpp:692
#27 0x00007f8dbc9a0d31 in KWin::Compositor::bufferSwapComplete() (this=0x5596962a2b10) at /home/apol/devel/frameworks/kwin/composite.cpp:596
#28 0x00007f8db2bf8b2b in KWin::DrmBackend::pageFlipHandler(int, unsigned int, unsigned int, unsigned int, void*) (fd=48, frame=1576831773, sec=3137, usec=384026, data=0x5596962b02d0) at /home/apol/devel/frameworks/kwin/plugins/platforms/drm/drm_backend.cpp:248
#29 0x00007f8db3938875 in drmHandleEvent () at /usr/lib/libdrm.so.2
#30 0x00007f8db2bf8ba0 in KWin::DrmBackend::<lambda()>::operator()(void) const (__closure=0x559696331710) at /home/apol/devel/frameworks/kwin/plugins/platforms/drm/drm_backend.cpp:279
#31 0x00007f8db2bfd62b in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, KWin::DrmBackend::openDrm()::<lambda()> >::call(KWin::DrmBackend::<lambda()> &, void **) (f=..., arg=0x7fff438d6090)

I'd suggest stress testing firefox and nested weston.

zzag added a comment.Apr 24 2020, 8:13 PM

Oh, I know what goes wrong. I'll come with a more robust algorithm for initialization of contents render nodes.

zzag planned changes to this revision.Apr 24 2020, 8:13 PM
zzag updated this revision to Diff 81298.Apr 27 2020, 6:58 AM

More robust window pixmap counting.

apol added a comment.Apr 27 2020, 10:40 AM

Would it be possible to get some testing for it?

zzag added a comment.Apr 27 2020, 11:17 AM
In D29131#658164, @apol wrote:

Would it be possible to get some testing for it?

What do you mean?

apol added a comment.Apr 27 2020, 3:17 PM
In D29131#658192, @zzag wrote:
In D29131#658164, @apol wrote:

Would it be possible to get some testing for it?

What do you mean?

Would it be possible to set up a unit test for subsurfaces?

zzag added a comment.Apr 28 2020, 6:02 AM
In D29131#658374, @apol wrote:

Would it be possible to set up a unit test for subsurfaces?

After implementing a similar autotest for shadow quads, I would rather prefer not do so because the integration test will become extremely difficult to work with and ugly. I think it's better for now to use test clients to verify that generated window quads are okay. However, it would be nice to have a test or two that verify rendered frames. Are there any projects or compositors that check pixel data in their test suites? I would really love to see how those projects organize their tests and how overall this approach looks in code.

Would it be possible to set up a unit test for subsurfaces?

After implementing a similar autotest for shadow quads, I would rather prefer not do so because the integration test will become extremely difficult to work with and ugly. I think it's better for now to use test clients to verify that generated window quads are okay. However, it would be nice to have a test or two that verify rendered frames. Are there any projects or compositors that check pixel data in their test suites? I would really love to see how those projects organize their tests and how overall this approach looks in code.

The simple approach is we have a test and simply confirm we don't crash without actually verifying anything.
Or we can read the pixmap tree and check that has the right layout of nodes with the right sizes.

We don't necessarily need to check the final visuals in order to do something useful.

For high level tests openQA is pretty amazing and an area that can be explored, even if it means things are done at a distro level.

plugins/scenes/opengl/scene_opengl.cpp
1281

IMHO no, i'tll be fine for the majority cases.

Whilst we're doing the opacity blend we do have another option here.

Instead of one tree with an old and new pixmap in each node we can reference an old and new tree where each node has one pixmap.
Means some slight changes, but it seems relatively doable.

In any case, that's a task for another day.

scene.cpp
407

I feel like the monitor should be responsible for abstracting added/removed/mapped/unmapped into just 2 signals when we gain a valid subsurface and when a subsurface loses validity (either from being removed or unmapped)

If the subsurface changes and it has no buffer then we don't need to do anything at a tree level.

1126

I know it's existing code, but we should be checking for a buffer here

the docs say "A sub-surface is hidden if the parent becomes hidden, "

subsurfacemonitor.cpp
65 ↗(On Diff #81298)

I don't understand the problem here.

We added the other signals in kwayland for this patch, we're clearly doing some buffer logic as we have an unmapped signal.

Why can't we add this mapped signal in kwayland?

zzag updated this revision to Diff 81433.Apr 28 2020, 1:24 PM

Uncomment subSurfaceMapped stuff in the sub-surface monitor.

zzag marked an inline comment as done.Apr 28 2020, 1:32 PM
zzag added inline comments.
scene.cpp
407

Hmm, I don't particularly see the need for a new sub-surface state given the job can be done with the existing signals and states.

1126

I didn't do it because I didn't want to change how kwin references buffers. Should I change it?

zzag added a comment.Apr 28 2020, 1:34 PM

Note that in order to test this patch you need to apply https://phabricator.kde.org/D29256 first.

zzag updated this revision to Diff 81876.May 4 2020, 12:09 PM

Rebase on the recent KWaylandServer changes.

davidedmundson accepted this revision.May 4 2020, 12:11 PM
This revision is now accepted and ready to land.May 4 2020, 12:11 PM
This revision was automatically updated to reflect the committed changes.