D29131: [scene] Generate window quads for sub-surfaces

[David Edmundson]

davidedmundson added a comment.


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

INLINE COMMENTS

> scene_opengl.cpp:1343
>          OpenGLWindowPixmap *previous = previousWindowPixmap<OpenGLWindowPixmap>();
> -        nodes[PreviousContentLeaf].texture = previous ? previous->texture() : nullptr;
> -        nodes[PreviousContentLeaf].hasAlpha = !isOpaque();
> -        nodes[PreviousContentLeaf].opacity = data.opacity() * (1.0 - data.crossFadeProgress());
> -        nodes[PreviousContentLeaf].coordinateType = NormalizedCoordinates;
> +        if (previous) { // TODO(vlad): Should cross-fading be disabled on Wayland?
> +            const QRect &oldGeometry = previous->contentsRect();

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:1180
>                  continue;
>              }
>              auto it = std::find_if(oldTree.begin(), oldTree.end(), [subSurface] (WindowPixmap *p) { return p->m_subSurface == subSurface; });

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

> scene.cpp:411
> +        // TODO(vlad): Is there a more efficient way to manage window pixmap trees?
> +        connect(monitor, &SubSurfaceMonitor::subSurfaceAdded, this, discardPixmap);
> +        connect(monitor, &SubSurfaceMonitor::subSurfaceRemoved, this, discardPixmap);

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.

> subsurfacemonitor.cpp:65
> +               this, &SubSurfaceMonitor::subSurfaceResized);
> +#if 0 // TODO(vlad): Add SurfaceInterface::mapped()
> +    disconnect(surface, &SurfaceInterface::mapped,

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?

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D29131

To: zzag, #kwin
Cc: davidedmundson, meven, apol, ngraham, kwin, Orage, cacarry, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, mkulinski, ragreen, jackyalcine, iodelay, crozbo, bwowk, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, romangg, jensreuterberg, abetts, sebas, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwin/attachments/20200428/2e34b567/attachment-0001.html>