Merge into trunk : multiwindow.textures : Code : QtMir

Status:	Needs review
Proposed branch:	lp://staging/~nick-dedekind/qtmir/multiwindow.textures
Merge into:	lp://staging/qtmir
Prerequisite:	lp://staging/~unity-team/qtmir/qtmir.api
Diff against target:	834 lines (+353/-131) 12 files modified debian/changelog (+2/-1) src/modules/Unity/Application/CMakeLists.txt (+1/-0) src/modules/Unity/Application/compositortextureprovider.cpp (+87/-0) src/modules/Unity/Application/compositortextureprovider.h (+77/-0) src/modules/Unity/Application/mirsurface.cpp (+99/-78) src/modules/Unity/Application/mirsurface.h (+11/-9) src/modules/Unity/Application/mirsurfaceinterface.h (+5/-5) src/modules/Unity/Application/mirsurfaceitem.cpp (+39/-22) src/modules/Unity/Application/mirsurfaceitem.h (+2/-2) tests/framework/fake_mirsurface.cpp (+9/-9) tests/framework/fake_mirsurface.h (+5/-5) tests/modules/WindowManager/mirsurface_test.cpp (+16/-0)
To merge this branch:	bzr merge lp://staging/~nick-dedekind/qtmir/multiwindow.textures
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Unity8 CI Bot (community)	continuous-integration		Approve on 2017-04-06
Daniel d'Andrada (community)		2017-02-09	Needs Fixing on 2017-03-10
Review via email: mp+316864@code.staging.launchpad.net

Revision history for this message

Gerry Boland (gerboland) wrote on 2017-02-08: Posted in a previous version of this proposal

#

+ int curentFrame()
MirSurface::setTexturePorvider
typos

+ int m_currentFrameNumber;
unsigned would help, would give 136 years at 60fps instead of 68 :)

Overall, this looks ok. I'm curious about one thing. We're using Mir's user_id thing, passing it to generate_renderables for each display. I am curious if that returns a single texture to be shared between each display - or a distinct texture per display. If the later, I would wonder why, and we (later) might try eradicating the copy and just share one, to save GPU memory.

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-08: Posted in a previous version of this proposal

#

Worth checking how this affects the new frame dropping implementation in MirSurface

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-09:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/487/rebuild

review: Needs Fixing (continuous-integration)

Reply

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-09:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/488/rebuild

review: Needs Fixing (continuous-integration)

Reply

lp://staging/~nick-dedekind/qtmir/multiwindow.textures updated on 2017-02-10

596. By Nick Dedekind on 2017-02-10: merged parent
597. By Nick Dedekind on 2017-02-10: merged parent

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-10:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/491/rebuild

review: Approve (continuous-integration)

Reply

lp://staging/~nick-dedekind/qtmir/multiwindow.textures updated on 2017-02-13

598. By Nick Dedekind on 2017-02-13: merged parent

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-13:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/494/rebuild

review: Approve (continuous-integration)

Reply

lp://staging/~nick-dedekind/qtmir/multiwindow.textures updated on 2017-02-14

599. By Nick Dedekind on 2017-02-14: merged parent

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-14:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/502/rebuild

review: Needs Fixing (continuous-integration)

Reply

lp://staging/~nick-dedekind/qtmir/multiwindow.textures updated on 2017-02-22

600. By Nick Dedekind on 2017-02-22: merged with parent

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-22:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/520/rebuild

review: Approve (continuous-integration)

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-22:

#

Missing https://wiki.ubuntu.com/Process/Merges/Checklists/QtMir

Missing copyright headers in src/modules/Unity/Application/compositortextureprovider.*

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-22:

#

In src/modules/Unity/Application/CMakeLists.txt:

"""
windowmodel.cpp
+ compositortextureprovider.cpp
# We need to run moc on these headers
"""

Please try to put it as close as you can to an alphabetical order (thus near the top of that list).

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-22:

#

The cyclic dependency between MirSurfaceItem and its SurfaceItemTextureProvider looks unnecessary. Can't the former can update the latter upon QQuickItem::smoothChanged()?

That might even lead to more correct code as it will update SurfaceItemTextureProvider's filtering in more situations than the current implementation.

review: Needs Information

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-22:

#

"""
+++ src/modules/Unity/Application/mirsurface.cpp 2017-02-22 11:48:55 +0000
@@ -20,6 +20,7 @@
#include "session_interface.h"
#include "timer.h"
#include "timestamp.h"
+#include "compositortextureprovider.h"

"""

Again, striving for an alphabetical order would be nice.

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-22:

#

In MirSurface::dropPendingBuffer() looks like you're replacing it with its old implementation. Is that intentional? If not, please update the current implementation to work with that new multi-texture scenario.

review: Needs Information

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-22:

#

If you could keep MirSurface::updateTextureLocked in the same location as MirSurface::updateTexture we would get a better diff, since the former is essentially a slightly modified version of the latter, right?

Reply

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2017-02-22:

#

> The cyclic dependency between MirSurfaceItem and its
> SurfaceItemTextureProvider looks unnecessary. Can't the former can update the
> latter upon QQuickItem::smoothChanged()?
>
> That might even lead to more correct code as it will update
> SurfaceItemTextureProvider's filtering in more situations than the current
> implementation.

Where is there a cyclic dependency? The items texture is dependant on the items smooth property.

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-22:

#

> > The cyclic dependency between MirSurfaceItem and its
> > SurfaceItemTextureProvider looks unnecessary. Can't the former can update
> the
> > latter upon QQuickItem::smoothChanged()?
> >
> > That might even lead to more correct code as it will update
> > SurfaceItemTextureProvider's filtering in more situations than the current
> > implementation.
>
> Where is there a cyclic dependency?

Item has TextureProvider and TextureProvider has Item.

Reply

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2017-02-22:

#

> In MirSurface::dropPendingBuffer() looks like you're replacing it with its old
> implementation. Is that intentional? If not, please update the current
> implementation to work with that new multi-texture scenario.

It was intentional.
Your reason for changing the code was "Make frame dropper work even when there's no texture around".
In this impl for the frame dropper we iterate over the textures which we have already generated and update the texture. We wouldn't have any renderables for something we hadn't requested (and therefore have a texture).

Reply

lp://staging/~nick-dedekind/qtmir/multiwindow.textures updated on 2017-02-22

601. By Nick Dedekind on 2017-02-22: review comments

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2017-02-22:

#

> > > The cyclic dependency between MirSurfaceItem and its
> > > SurfaceItemTextureProvider looks unnecessary. Can't the former can update
> > the
> > > latter upon QQuickItem::smoothChanged()?
> > >
> > > That might even lead to more correct code as it will update
> > > SurfaceItemTextureProvider's filtering in more situations than the current
> > > implementation.
> >
> > Where is there a cyclic dependency?
>
> Item has TextureProvider and TextureProvider has Item.

Removed. Added TextureProvider::setSmooth

Reply

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-02-22:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/523/rebuild

review: Approve (continuous-integration)

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-02-22:

#

> > In MirSurface::dropPendingBuffer() looks like you're replacing it with its
> old
> > implementation. Is that intentional? If not, please update the current
> > implementation to work with that new multi-texture scenario.
>
> It was intentional.
> Your reason for changing the code was "Make frame dropper work even when
> there's no texture around".
> In this impl for the frame dropper we iterate over the textures which we have
> already generated and update the texture. We wouldn't have any renderables for
> something we hadn't requested (and therefore have a texture).

What you're explaining is the wrong assumption that the previous implementation had.

The problematic case is this: When closing and application, QSG might have already freed the texture by the time you try to drop the pending surface frame. Since there's no texture anymore, updateTexture() fails to consume the frame.

The client application will then get stuck on swap_buffers(), unable to consume the pending close_surface event coming from mir.

See https://code.launchpad.net/~dandrader/qtmir/frameDropperNoTexture/+merge/315911 for details

Reply

lp://staging/~nick-dedekind/qtmir/multiwindow.textures updated on 2017-03-01

602. By Nick Dedekind on 2017-03-01: merged with parent

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-01:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/539/rebuild

review: Approve (continuous-integration)

Reply

lp://staging/~nick-dedekind/qtmir/multiwindow.textures updated on 2017-03-08

603. By Nick Dedekind on 2017-03-08: use newer drop frame algorithm

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2017-03-08:

#

> > > In MirSurface::dropPendingBuffer() looks like you're replacing it with its
> > old
> > > implementation. Is that intentional? If not, please update the current
> > > implementation to work with that new multi-texture scenario.
> >
> > It was intentional.
> > Your reason for changing the code was "Make frame dropper work even when
> > there's no texture around".
> > In this impl for the frame dropper we iterate over the textures which we
> have
> > already generated and update the texture. We wouldn't have any renderables
> for
> > something we hadn't requested (and therefore have a texture).
>
> What you're explaining is the wrong assumption that the previous
> implementation had.
>
> The problematic case is this: When closing and application, QSG might have
> already freed the texture by the time you try to drop the pending surface
> frame. Since there's no texture anymore, updateTexture() fails to consume the
> frame.
>
> The client application will then get stuck on swap_buffers(), unable to
> consume the pending close_surface event coming from mir.
>
> See https://code.launchpad.net/~dandrader/qtmir/frameDropperNoTexture/+merge/3
> 15911 for details

I see! I've changed it to match the current impl + multiple textures.

Reply

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-08:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/562/rebuild

review: Approve (continuous-integration)

Reply

lp://staging/~nick-dedekind/qtmir/multiwindow.textures updated on 2017-03-10

604. By Nick Dedekind on 2017-03-10: merged with parent

Revision history for this message

Unity8 CI Bot (unity8-ci-bot) wrote on 2017-03-10:

#

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-qtmir-ci/564/rebuild

review: Approve (continuous-integration)

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-10:

#

Since CompositorTexture is only meant to be created and filled in by CompositorTextureProvider, it makes sense to make its constructor and setTexture methods private and declaring CompositorTextureProvider its friend. That would make roles more clear when you read the header

review: Needs Fixing

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-10:

#

That would simplify your code:
http://pastebin.ubuntu.com/24151803/

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-10:

#

MirSurface::dropPendingBuffer() is looking good now. Thanks!

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-10:

#

> That would simplify your code:
> http://pastebin.ubuntu.com/24151803/

Actually not, as the new texture would lose its only strong ref when leaving the if(){}
Ignore me. :)

Reply

Revision history for this message

Daniel d'Andrada (dandrader) wrote on 2017-03-10:

#

> Since CompositorTexture is only meant to be created and filled in by
> CompositorTextureProvider, it makes sense to make its constructor and
> setTexture methods private and declaring CompositorTextureProvider its friend.
> That would make roles more clear when you read the header

Aside from this nit, code looks good and unity8 still works.

Reply

lp://staging/~nick-dedekind/qtmir/multiwindow.textures updated on 2017-03-16