Merge lp:~nick-dedekind/qtmir/drop-frame-texture-update into lp:qtmir

[Nick Dedekind]

Update surface textures when dropping frames.

Update surface textures when dropping frames.

 * Are there any related MPs required for this MP to build/function as expected? Please list.
No

 * Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

[Gerry Boland (gerboland)]

 qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::dropPendingBuffer()"
would you please fix this pet-peeve of mine while you're here? MirSurface::...

I got a crash with phone idle:

Program received signal SIGSEGV, Segmentation fault.
0xa9f6c69a in qtmir::MirSurface::dropPendingBuffer (this=0x28afc78)
    at /home/gerry/dev/projects/qtmir/qtmir-ubuntu-xenial-landing-044/src/modules/Unity/Application/mirsurface.cpp:309
309 /home/gerry/dev/projects/qtmir/qtmir-ubuntu-xenial-landing-044/src/modules/Unity/Application/mirsurface.cpp: No such file or directory.
(gdb) bt
#0 0xa9f6c69a in qtmir::MirSurface::dropPendingBuffer (this=0x28afc78)
    at /home/gerry/dev/projects/qtmir/qtmir-ubuntu-xenial-landing-044/src/modules/Unity/Application/mirsurface.cpp:309
#1 0xb6535ad2 in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/arm-linux-gnueabihf/libQt5Core.so.5
#2 0xb653ed06 in QTimer::timerEvent(QTimerEvent*) () from /usr/lib/arm-linux-gnueabihf/libQt5Core.so.5
#3 0xb6536a2a in QObject::event(QEvent*) () from /usr/lib/arm-linux-gnueabihf/libQt5Core.so.5
#4 0xb6515cb2 in QCoreApplication::notify(QObject*, QEvent*) () from /usr/lib/arm-linux-gnueabihf/libQt5Core.so.5
#5 0xb6515d78 in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /usr/lib/arm-linux-gnueabihf/libQt5Core.so.5
#6 0xb655009c in QTimerInfoList::activateTimers() () from /usr/lib/arm-linux-gnueabihf/libQt5Core.so.5
#7 0xb65503b2 in ?? () from /usr/lib/arm-linux-gnueabihf/libQt5Core.so.5
#8 0xb5ca7f68 in g_main_context_dispatch () from /lib/arm-linux-gnueabihf/libglib-2.0.so.0
#9 0xb5ca8114 in ?? () from /lib/arm-linux-gnueabihf/libglib-2.0.so.0
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

[Gerry Boland (gerboland)]

(gdb) info locals
i = 1
userId = 0x7b
renderables = std::vector of length 1, capacity 1 = {std::shared_ptr (count 1, weak 0) 0x2355ce4}
locker = {val = 42663093}
framesPending = 2

[Gerry Boland (gerboland)]

+ auto renderables = m_surface->generate_renderables(userId);
+ for (uint i = 0; i < renderables.size()-2; i++) {
I don't think this is correct. A single Mir surface can have multiple "Renderables" - something that's not really happening much at the moment, but will more when we start using sub-surfaces/bufferstreams. Use-case is embedding a video into a webbrowser. Video is in a sub-surface.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:407
http://jenkins.qa.ubuntu.com/job/qtmir-ci/560/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/256
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/256
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/256/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/138
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/293/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/293/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/138/console

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

review: Needs Fixing (continuous-integration)

[lp:~nick-dedekind/qtmir/drop-frame-texture-update]

408. By Nick Dedekind on 2015-11-13

proper drop frame texture update

[Gerry Boland (gerboland)]

Ok, looks reasonable. I'm running every AP test under the sun on all my devices, just to be sure.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:408
http://jenkins.qa.ubuntu.com/job/qtmir-ci/561/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/257
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/257
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/257/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/139
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/294/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/294/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/139/console

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

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

Fixes bug, and no regressions that I can observe. +1

review: Approve

=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
--- src/modules/Unity/Application/mirsurface.cpp	2015-11-02 11:22:24 +0000
+++ src/modules/Unity/Application/mirsurface.cpp	2015-11-13 14:13:14 +0000
@@ -292,16 +292,11 @@
 
     int framesPending = m_surface->buffers_ready_for_compositor(userId);
     if (framesPending > 0) {
-        // The line below looks like an innocent, effect-less, getter. But as this
-        // method returns a unique_pointer, not holding its reference causes the
-        // buffer to be destroyed/released straight away.
-        for (auto const & item : m_surface->generate_renderables(userId))
-            item->buffer();
-        qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::dropPendingBuffer()"
-            << "surface =" << this
-            << "buffer dropped."
-            << framesPending-1
-            << "left.";
+        qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::dropPendingBuffer() left=" << framesPending-1;
+
+        m_textureUpdated = false;
+        locker.unlock();
+        updateTexture();
     } else {
         // The client can't possibly be blocked in swap buffers if the
         // queue is empty. So we can safely enter deep sleep now. If the
@@ -313,13 +308,13 @@
 
 void MirSurface::stopFrameDropper()
 {
-    qCDebug(QTMIR_SURFACES) << "MirSurface::stopFrameDropper surface = " << this;
+    qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::stopFrameDropper()";
     m_frameDropperTimer.stop();
 }
 
 void MirSurface::startFrameDropper()
 {
-    qCDebug(QTMIR_SURFACES) << "MirSurface::startFrameDropper surface = " << this;
+    qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::startFrameDropper()";
     if (!m_frameDropperTimer.isActive()) {
         m_frameDropperTimer.start();
     }
@@ -363,6 +358,8 @@
             m_size = texture->textureSize();
             QMetaObject::invokeMethod(this, "emitSizeChanged", Qt::QueuedConnection);
         }
+
+        m_textureUpdated = true;
     }
 
     if (m_surface->buffers_ready_for_compositor(userId) > 0) {
@@ -371,13 +368,12 @@
         QMetaObject::invokeMethod(&m_frameDropperTimer, "start", Qt::QueuedConnection);
     }
 
-    m_textureUpdated = true;
-
     return texture->hasBuffer();
 }
 
 void MirSurface::onCompositorSwappedBuffers()
 {
+    QMutexLocker locker(&m_mutex);
     m_textureUpdated = false;
 }
 
@@ -421,9 +417,8 @@
     if (clientIsRunning() && mirSizeIsDifferent) {
         mir::geometry::Size newMirSize(width, height);
         m_surface->resize(newMirSize);
-        qCDebug(QTMIR_SURFACES) << "MirSurface::resize"
-                << "surface =" << this
-                << ", old (" << mirWidth << "," << mirHeight << ")"
+        qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::resize"
+                << " old (" << mirWidth << "," << mirHeight << ")"
                 << ", new (" << width << "," << height << ")";
     }
 }
@@ -682,6 +677,9 @@
 
 void MirSurface::updateVisibility()
 {
+    // FIXME: https://bugs.launchpad.net/ubuntu/+source/unity8/+bug/1514556
+    return;
+
     bool newVisible = false;
     QHashIterator<qintptr, View> i(m_views);
     while (i.hasNext()) {

=== modified file 'src/modules/Unity/Application/mirsurfaceitem.cpp'
--- src/modules/Unity/Application/mirsurfaceitem.cpp	2015-11-09 19:25:26 +0000
+++ src/modules/Unity/Application/mirsurfaceitem.cpp	2015-11-13 14:13:14 +0000
@@ -531,8 +531,7 @@
         return;
     }
 
-    // FIXME: https://bugs.launchpad.net/ubuntu/+source/unity8/+bug/1514556
-    // m_surface->setViewVisibility((qintptr)this, isVisible());
+    m_surface->setViewVisibility((qintptr)this, isVisible());
 }
 
 void MirSurfaceItem::updateMirSurfaceFocus(bool focused)

=== modified file 'tests/modules/SurfaceManager/mirsurfaceitem_test.cpp'
--- tests/modules/SurfaceManager/mirsurfaceitem_test.cpp	2015-11-10 09:04:43 +0000
+++ tests/modules/SurfaceManager/mirsurfaceitem_test.cpp	2015-11-13 14:13:14 +0000
@@ -100,7 +100,7 @@
     delete fakeSurface;
 }
 
-TEST_F(MirSurfaceItemTest, DISABLED_SetSurfaceInitializesVisiblity)
+TEST_F(MirSurfaceItemTest, SetSurfaceInitializesVisiblity)
 {
     MirSurfaceItem *surfaceItem = new MirSurfaceItem;
     surfaceItem->setVisible(false);
@@ -114,7 +114,7 @@
     delete fakeSurface;
 }
 
-TEST_F(MirSurfaceItemTest, DISABLED_ggregateSurfaceVisibility)
+TEST_F(MirSurfaceItemTest, AggregateSurfaceVisibility)
 {
     MirSurfaceItem *surfaceItem1 = new MirSurfaceItem;
     surfaceItem1->setVisible(true);