Merge lp:~nick-dedekind/qtmir/fix-dropFrame-before-draw into lp:qtmir

[Nick Dedekind]

Fix a crash when dropping a surface frame before Qt draws a surface item.

 * 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

[Nick Dedekind (nick-dedekind)]

To reproduce this you need to make sure the items don't get drawn before a frame is dropped.
This can easily be done by ensuring it is never drawn.
=== modified file 'qml/Stages/PhoneStage.qml'
--- qml/Stages/PhoneStage.qml 2015-11-04 14:58:05 +0000
+++ qml/Stages/PhoneStage.qml 2015-11-17 15:31:55 +0000
@@ -403,7 +403,6 @@
             Repeater {
                 id: spreadRepeater
                 objectName: "spreadRepeater"
- model: root.applicationManager
                 delegate: TransformedSpreadDelegate {
                     id: appDelegate
                     objectName: "appDelegate" + index

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:409
http://jenkins.qa.ubuntu.com/job/qtmir-ci/566/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/262
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/262
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/262/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/144
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/299/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/299/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/144/console

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

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

Good catch!

review: Approve

[lp:~nick-dedekind/qtmir/fix-dropFrame-before-draw]

410. By Nick Dedekind on 2015-11-18

stop frame dropper if can't update the texture.

=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
--- src/modules/Unity/Application/mirsurface.cpp	2015-11-13 14:13:07 +0000
+++ src/modules/Unity/Application/mirsurface.cpp	2015-11-18 10:46:48 +0000
@@ -292,11 +292,18 @@
 
     int framesPending = m_surface->buffers_ready_for_compositor(userId);
     if (framesPending > 0) {
-        qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::dropPendingBuffer() left=" << framesPending-1;
-
         m_textureUpdated = false;
+        
         locker.unlock();
-        updateTexture();
+        if (updateTexture()) {
+            qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::dropPendingBuffer() dropped=1 left=" << framesPending-1;
+        } else {
+            // If we haven't managed to update the texture, don't keep banging away.
+            m_frameDropperTimer.stop();
+            qCDebug(QTMIR_SURFACES).nospace() << "MirSurface[" << appId() << "]::dropPendingBuffer() dropped=0"
+                                              << " left=" << framesPending << " - failed to upate texture";
+        }
+        Q_EMIT frameDropped();
     } 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
@@ -322,6 +329,8 @@
 
 QSharedPointer<QSGTexture> MirSurface::texture()
 {
+    QMutexLocker locker(&m_mutex);
+
     if (!m_texture) {
         QSharedPointer<QSGTexture> texture(new MirBufferSGTexture);
         m_texture = texture.toWeakRef();
@@ -334,9 +343,9 @@
 bool MirSurface::updateTexture()
 {
     QMutexLocker locker(&m_mutex);
-    Q_ASSERT(!m_texture.isNull());
 
     MirBufferSGTexture *texture = static_cast<MirBufferSGTexture*>(m_texture.data());
+    if (!texture) return false;
 
     if (m_textureUpdated) {
         return texture->hasBuffer();
@@ -697,6 +706,7 @@
 
 unsigned int MirSurface::currentFrameNumber() const
 {
+    QMutexLocker locker(&m_mutex);
     return m_currentFrameNumber;
 }
 

=== modified file 'src/modules/Unity/Application/mirsurface.h'
--- src/modules/Unity/Application/mirsurface.h	2015-11-13 14:13:07 +0000
+++ src/modules/Unity/Application/mirsurface.h	2015-11-18 10:46:48 +0000
@@ -141,7 +141,7 @@
 
     QTimer m_frameDropperTimer;
 
-    QMutex m_mutex;
+    mutable QMutex m_mutex;
 
     // Lives in the rendering (scene graph) thread
     QWeakPointer<QSGTexture> m_texture;

=== modified file 'src/modules/Unity/Application/mirsurfaceinterface.h'
--- src/modules/Unity/Application/mirsurfaceinterface.h	2015-10-26 18:29:24 +0000
+++ src/modules/Unity/Application/mirsurfaceinterface.h	2015-11-18 10:46:48 +0000
@@ -88,6 +88,7 @@
     void firstFrameDrawn();
     void framesPosted();
     void isBeingDisplayedChanged();
+    void frameDropped();
 };
 
 } // namespace qtmir

=== modified file 'tests/modules/SurfaceManager/CMakeLists.txt'
--- tests/modules/SurfaceManager/CMakeLists.txt	2015-10-20 09:57:17 +0000
+++ tests/modules/SurfaceManager/CMakeLists.txt	2015-11-18 10:46:48 +0000
@@ -9,6 +9,7 @@
 
 include_directories(
   ${CMAKE_SOURCE_DIR}/src/modules
+  ${CMAKE_SOURCE_DIR}/src/platforms/mirserver
   ${CMAKE_SOURCE_DIR}/tests/modules/common
   ${MIRSERVER_INCLUDE_DIRS}
 )

=== modified file 'tests/modules/SurfaceManager/mirsurface_test.cpp'
--- tests/modules/SurfaceManager/mirsurface_test.cpp	2015-10-20 10:03:37 +0000
+++ tests/modules/SurfaceManager/mirsurface_test.cpp	2015-11-18 10:46:48 +0000
@@ -20,6 +20,7 @@
 
 #include <QLoggingCategory>
 #include <QTest>
+#include <QSignalSpy>
 
 // the test subject
 #include <Unity/Application/mirsurface.h>
@@ -27,6 +28,7 @@
 // tests/modules/common
 #include <fake_session.h>
 #include <mock_surface.h>
+#include <surfaceobserver.h>
 
 using namespace qtmir;
 
@@ -42,6 +44,33 @@
     }
 };
 
+TEST_F(MirSurfaceTest, UpdateTextureBeforeDraw)
+{
+    using namespace testing;
+
+    int argc = 0;
+    char* argv[0];
+    QCoreApplication qtApp(argc, argv); // app for deleteLater event
+
+    auto fakeSession = new FakeSession();
+    auto mockSurface = std::make_shared<NiceMock<ms::MockSurface>>();
+    auto surfaceObserver = std::make_shared<SurfaceObserver>();
+
+    EXPECT_CALL(*mockSurface.get(),buffers_ready_for_compositor(_))
+        .WillRepeatedly(Return(1));
+
+    MirSurface *surface = new MirSurface(mockSurface, fakeSession, nullptr, surfaceObserver);
+    surfaceObserver->frame_posted(1);
+
+    QSignalSpy spyFrameDropped(surface, SIGNAL(frameDropped()));
+    QTest::qWait(300);
+    ASSERT_TRUE(spyFrameDropped.count() > 0);
+
+    delete fakeSession;
+    delete surface;
+}
+
+
 TEST_F(MirSurfaceTest, DeleteMirSurfaceOnLastNonLiveUnregisterView)
 {
     using namespace testing;