Merge lp://staging/~raof/qtmir/multi-bufferstream-support into lp://staging/qtmir

PASSED: Continuous integration, rev:387
http://jenkins.qa.ubuntu.com/job/qtmir-ci/520/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/216
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/216
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/216/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/98
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/253
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/253
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/253/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/98

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/520/rebuild

Test with mir_demo_client_multistream

demo client is here: https://code.launchpad.net/~raof/mir/bufferstream-example/+merge/274354, and to actually get the client API supported right you need https://code.launchpad.net/~raof/mir/handle-set-streams-in-mir/+merge/274506 (or a trivial extra patch on qtmir I have locally)

Those not versed in Mir internal jargon will probably find substituting “Subsurface” for “BufferStream” helpful in understanding this.

- One difference is that MirSurfaceItems sharing the same MirSurface won't share the same texture anymore which in terms of rendering calls will result in surface buffers bound to different textures (instead of a single one). The implied resource management simplification is nice but can you confirm the cost of it (the binding) is not too high?

- The removal of the texture provider will prevent the item to be used in a ShaderEffect (or an UbuntuShape) for post-processing. So this might need to be validated with Unity guys. IMO it's not an issue since we should actually use a custom rendering node (with dedicated shaders) to do that kind of things (the custom node is actually WIP [1]).

- That call in updateSubgraph() is useless since setMatrix() on the transform node does that for you: "transformNode->markDirty(QSGNode::DirtyMatrix);".

- For performance reasons, I think clipping of subsurfaces should be done by setting the QSGMirRenderableNode size and texture coordinates appropriately instead of relying on the QtQuick renderer. The renderer efficiently uses glScissors() when the combined matrix of a node has no rotation, but it uses a slow stencil buffer based solution otherwise (which is the case for all the nodes in spread mode). That can be implemented later though.

[1] https://code.launchpad.net/~loic.molinari/qtmir/qtmir-custom-mirsurfacenode/+merge/274869

I'm using this branch as a prerequisite for another one since we're changing the same files. I pushed a few fixes [1] that you might be interested in:

1) Merged trunk and fixed a few conflicts
2) Removed the useless markDirty call as mentioned before
3) Fixed a rendering issue (black surfaces) in spread mode (update() must be called after setAntialiasing() otherwise the node states are not sync'd)
4) Avoid iterating the nodes twice when there are new buffers available (not mandatory but a bit faster)

[1] https://code.launchpad.net/~loic.molinari/qtmir/multi-bufferstream-support-fixes

Ta.

I'll get you the performance (texture bind/unbind numbers etc) figures
you were after soon. Just as soon as this damn laptop updates properly,
as I want to do a proper test with a full unity8 session.

OK, good to get some numbers.

I've just tried the stream.cpp example but I'm just getting a red translucent surface and one renderable in generate_renderables(). Is there anything to enable?

multi_stream.cpp, I mean.

You'll need to rebuild qtmir against Mir trunk; the configure_streams call doesn't do anything unless you've got recent Mir (or make the same change in mirwindowmanager.cpp modify_surface() that https://code.launchpad.net/~raof/mir/handle-set-streams-in-mir/+merge/274506 makes.

+ root->removeChildNode(root->firstChild());
I suspect this is leaking nodes

> + root->removeChildNode(root->firstChild());
> I suspect this is leaking nodes

Hey Gerry. I don't think so because QSGNodes have the OwnedByParent flag set at creation which makes the destruction recursive.

There's a change in that MR that will make the rendering different when a QML scene decides to render the MirSurfaceItem at a different size than the MirSurface size (I guess this the case in the spread view on desktop). The reason is the size is not taken from the item anymore but from the renderable. So the current behaviour is a scale and the new behaviour is a an item not respecting its given size anymore (clipped by the renderer when exceeding if the clip property is enabled).

So I guess we should fix that too.

Chris: Is Mir clipping the streams, or should that be done in by the compositor?

Mir is not clipping the streams. I don't think the streams *should* be clipped to the main surface - there are sensible use-cases for allowing it. For example, GTK uses subsurfaces on Wayland to draw its decorations, and they're outside the main surface pretty much by definition.

I'd probably be able to be convinced that clipping the streams to the main surface would be a good idea, though.

Bah! http://doc.qt.io/qtcreator/creator-qml-performance-monitor.html suggests that setting QSG_RENDER_TIMING=1 we should get detailed timing out of the scenegraph - and, indeed, I do get _some_ - but clearly not one-per-frame, and nowhere do I see MirSurface textures being bound.

GAH.

> > + root->removeChildNode(root->firstChild());
> > I suspect this is leaking nodes
>
> Hey Gerry. I don't think so because QSGNodes have the OwnedByParent flag set
> at creation which makes the destruction recursive.

Looking at QSGNode::removeChildNode(QSGNode *node) source, I don't see any delete call, and it unparents the node. http://pastebin.ubuntu.com/13008194/

> - The removal of the texture provider will prevent the item to be used in a
> ShaderEffect (or an UbuntuShape) for post-processing. So this might need to be
> validated with Unity guys. IMO it's not an issue since we should actually use
> a custom rendering node (with dedicated shaders) to do that kind of things
> (the custom node is actually WIP [1]).

It is a bit of a pity, I had dreams of wobbly windows with a simple QML shader. But can always use an FBO, or else write custom rendering node.

> Mir is not clipping the streams. I don't think the streams *should* be clipped
> to the main surface - there are sensible use-cases for allowing it. For
> example, GTK uses subsurfaces on Wayland to draw its decorations, and they're
> outside the main surface pretty much by definition.

Sensible if you're a toolkit and want to deliver client-side decorations automatically to applications in a simple way, but making the window manager/compositors' job much harder.

> I'd probably be able to be convinced that clipping the streams to the main
> surface would be a good idea, though.

What is the geometry of the overall surface if it is made of disjoint subsurfaces? The bounding box geometry?

I can think of ways to abuse this to force my window to be positioned where I'd like.

Aside from window decoration case, are there other use-cases for subsurfaces being positioned outside the parent surface?

Conflicts with trunk.

> > > + root->removeChildNode(root->firstChild());
> > > I suspect this is leaking nodes
> >
> > Hey Gerry. I don't think so because QSGNodes have the OwnedByParent flag set
> > at creation which makes the destruction recursive.
>
> Looking at QSGNode::removeChildNode(QSGNode *node) source, I don't see any
> delete call, and it unparents the node. http://pastebin.ubuntu.com/13008194/

Right! The destruction is recursive, but the removal doesn't delete. So yes, the child must be delted after the removal.

> > > > + root->removeChildNode(root->firstChild());
> > > > I suspect this is leaking nodes
> > >
> > > Hey Gerry. I don't think so because QSGNodes have the OwnedByParent flag
> set
> > > at creation which makes the destruction recursive.
> >
> > Looking at QSGNode::removeChildNode(QSGNode *node) source, I don't see any
> > delete call, and it unparents the node. http://pastebin.ubuntu.com/13008194/
>
> Right! The destruction is recursive, but the removal doesn't delete. So yes,
> the child must be delted after the removal.

Well that's super-unintuitive behaviour!

I don't know Mir enough to say whether letting clients paint outside of their bounding rect is good or not, but if so, the compositor would sometimes have to switch on clipping based on UI constraints, like for instance in spread mode. (in this case, which seems likeley, I guess we'll have to expose a property like "clipRenderables" on MirSurfaceItem that makes clipping more efficient than the QtQuick default technique, by simply setting vertex positions and texcoords of the renderable textured quads accordingly.)

I updated the branch [1] with more fixes.

5) Fixed the node leak.

6) Switched back to stretched fill mode. I had to rework the loop a bit so that the geometry size is updated when the item size changes.

Feel free to merge that if you think it's fine.

[1] https://code.launchpad.net/~loic.molinari/qtmir/multi-bufferstream-support-fixes

On 02/11/2015 14:48, Loïc Molinari wrote:
> I don't know Mir enough to say whether letting clients paint outside of their bounding rect is good or not, but if so, the compositor would sometimes have to switch on clipping based on UI constraints, like for instance in spread mode. (in this case, which seems likeley, I guess we'll have to expose a property like "clipRenderables" on MirSurfaceItem that makes clipping more efficient than the QtQuick default technique, by simply setting vertex positions and texcoords of the renderable textured quads accordingly.)

https://code.launchpad.net/~dandrader/qtmir/surfaceItemFillMode/+merge/274750
does that.

FAILED: Continuous integration, rev:388
http://jenkins.qa.ubuntu.com/job/qtmir-ci/549/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/245
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/245
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/245/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/127
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/282/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/282/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/127/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/549/rebuild

FAILED: Continuous integration, rev:389
http://jenkins.qa.ubuntu.com/job/qtmir-ci/550/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/246
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/246
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/246/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/128
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/283/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/283/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/128/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/550/rebuild

Thanks.

One last thing that could be different is the check for the emitSizeChanged emission. It's currently using the size of the buffer from the first renderable and the new branch uses the size of the surface. Is that guaranteed to be the same?

No, it's not. The size of the first renderable may be arbitrarily different to the size of the surface; it's not guaranteed to be the “main” stream.

Having discussed it with Gerry, I think the behaviour we'll propose is:
1) All the streams of a surface will be clipped to the surface size, and
2) The surface size will be entirely independent of the size of any particular bufferstream.

Oh, grr. In updating this against latest trunk I discover that we need to make some architectural decisions before qtmir has the information required to do this correctly. See also: https://docs.google.com/document/d/1Se4kbUHZNbgB9e3lrr3EQnVnQgSgFThyiBJ0MDgkZ0c