Merge lp:~vanvugt/mir/fix-1420678 into lp:mir

[Daniel van Vugt]

Fix the multi-monitor frame sync logic to support high speed
compositors (tm) which only need to hold buffers very briefly
(introduced in r2183). (LP: #1420678)

Both the multi-monitor frame sync logic and the optimization of r2183
behave correctly independently. It's only when combined that they start
to interfere with each other. But we'd very much like to keep both...

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2317
http://jenkins.qa.ubuntu.com/job/mir-ci/2960/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1281
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1280
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1237
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/957/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1237
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1237/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4276
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18002

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Failure in ServerShutdown.normal_exit_removes_endpoint appears to be unrelated valgrind issue

4: [ RUN ] ServerShutdown.normal_exit_removes_endpoint
4: ==27126==
4: ==27126== HEAP SUMMARY:
4: ==27126== in use at exit: 13,363 bytes in 53 blocks
4: ==27126== total heap usage: 533,640 allocs, 533,587 frees, 34,288,827 bytes allocated
4: ==27126==
4: ==27126== LEAK SUMMARY:
4: ==27126== definitely lost: 0 bytes in 0 blocks
4: ==27126== indirectly lost: 0 bytes in 0 blocks
4: ==27126== possibly lost: 944 bytes in 9 blocks
4: ==27126== still reachable: 12,419 bytes in 44 blocks
4: ==27126== suppressed: 0 bytes in 0 blocks
4: ==27126== Reachable blocks (those to which a pointer was found) are not shown.
4: ==27126== To see them, rerun with: --leak-check=full --show-leak-kinds=all
4: ==27126==
4: ==27126== For counts of detected and suppressed errors, rerun with: -v
4: ==27126== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
4: /tmp/buildd/mir-0.11.0bzr2317pkg0vivid957/tests/mir_test_framework/interprocess_client_server_test.cpp:153: Failure
4: Value of: result.exit_code
4: Expected: is equal to 0
4: Actual: 1 (of type int)
4: [ FAILED ] ServerShutdown.normal_exit_removes_endpoint (532 ms)

[Alan Griffiths (alan-griffiths)]

I don't see any problems with this, but I don't feel confident that it covers all cases. Further opinions would be good.

review: Approve

[Daniel van Vugt (vanvugt)]

Arguably a more elegant solution was what I had up to r2316. That just removed the whole block of code (removed the double buffering fix for LP: #1319765).

But I too would like a third more elegant solution... Haven't found it yet. It's hard to make logic changes there and still pass all the existing tests (which is good at least).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2318
http://jenkins.qa.ubuntu.com/job/mir-ci/2971/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1296
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1295
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1252
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/968
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/968/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1252
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1252/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4289
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18042

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

Hm. This doesn't look incorrect, but it also looks like it's papering over a problem elsewhere.

I *think* this might be an artefact of an incorrect fix for bug #1319765? The problem identified there being that there's no additional composition scheduled when the client has submitted a frame and is blocking for its next buffer. This was solved by fake-consuming the client frame; it would seem to be more correct to ensure that the frame that the client has already submitted appropriately triggers composition?

review: Needs Information

[Daniel van Vugt (vanvugt)]

It would take me a while to get my head around the problem again. Sounds like we might have been papering over bug 1395581, which was only fixed very recently.

[Daniel van Vugt (vanvugt)]

raof: Fixed. Thanks!

It's complicated. Took two brains and the coordination of three different bug fixes to realise the interplay...

[Chris Halse Rogers (raof)]

Looks good now.

review: Approve

=== modified file 'src/server/compositor/buffer_queue.cpp'
--- src/server/compositor/buffer_queue.cpp	2015-01-22 09:12:04 +0000
+++ src/server/compositor/buffer_queue.cpp	2015-02-17 06:41:51 +0000
@@ -271,25 +271,6 @@
     if (nbuffers <= 1)
         return;
 
-    /*
-     * We can't release the current_compositor_buffer because we need to keep
-     * a compositor buffer always-available. But there might be a new
-     * compositor buffer available to take its place immediately. Moving to
-     * that one immediately will free up the old compositor buffer, allowing
-     * us to call back the client with a buffer where otherwise we couldn't.
-     */
-    if (current_compositor_buffer == buffer.get() &&
-        !ready_to_composite_queue.empty())
-    {
-        current_compositor_buffer = pop(ready_to_composite_queue);
-
-        // Ensure current_compositor_buffer gets reused by the next
-        // compositor_acquire:
-        current_buffer_users.clear();
-        void const* const impossible_user_id = this;
-        current_buffer_users.push_back(impossible_user_id);
-    }
-
     if (current_compositor_buffer != buffer.get())
         release(buffer.get(), std::move(lock));
 }

=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp	2015-01-22 09:12:04 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp	2015-02-17 06:41:51 +0000
@@ -489,6 +489,44 @@
     }
 }
 
+TEST_F(BufferQueueTest, multiple_fast_compositors_are_in_sync)
+{  // Regression test for LP: #1420678
+    for (int nbuffers = 3; nbuffers <= max_nbuffers_to_test; ++nbuffers)
+    {
+        mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
+
+        // Client generates first frame
+        auto handle1 = client_acquire_async(q);
+        ASSERT_TRUE(handle1->has_acquired_buffer());
+        auto client_id1 = handle1->id();
+        handle1->release_buffer();
+
+        // Client generates second frame
+        auto handle2 = client_acquire_async(q);
+        ASSERT_TRUE(handle2->has_acquired_buffer());
+        auto client_id2 = handle2->id();
+        handle2->release_buffer();
+
+        // Many monitors... verify they all get the first frame.
+        for (int monitor = 0; monitor < 10; monitor++)
+        {
+            void const* user_id = reinterpret_cast<void const*>(monitor);
+            auto comp_buffer = q.compositor_acquire(user_id);
+            ASSERT_EQ(client_id1, comp_buffer->id());
+            q.compositor_release(comp_buffer);
+        }
+
+        // Still many monitors... verify they all get the second frame.
+        for (int monitor = 0; monitor < 10; monitor++)
+        {
+            void const* user_id = reinterpret_cast<void const*>(monitor);
+            auto comp_buffer = q.compositor_acquire(user_id);
+            ASSERT_EQ(client_id2, comp_buffer->id());
+            q.compositor_release(comp_buffer);
+        }
+    }
+}
+
 TEST_F(BufferQueueTest, compositor_acquires_frames_in_order)
 {
     for (int nbuffers = 2; nbuffers <= max_nbuffers_to_test; ++nbuffers)
@@ -1236,6 +1274,14 @@
     q.client_release(client_acquire_sync(q));
 
     q.compositor_release(b);
+
+    /*
+     * Update to the original test case; This additional compositor acquire
+     * represents the fixing of LP: #1395581 in the compositor logic.
+     */
+    if (q.buffers_ready_for_compositor(this))
+        q.compositor_release(q.compositor_acquire(this));
+
     auto handle = client_acquire_async(q);
     // With the fix, a buffer will be available instantaneously:
     ASSERT_TRUE(handle->has_acquired_buffer());
@@ -1272,6 +1318,13 @@
     
         q.compositor_release(b);
 
+        /*
+         * Update to the original test case; This additional compositor acquire
+         * represents the fixing of LP: #1395581 in the compositor logic.
+         */
+        if (q.buffers_ready_for_compositor(this))
+            q.compositor_release(q.compositor_acquire(this));
+
         auto z = client_acquire_async(q);
         ASSERT_TRUE(z->has_acquired_buffer());
         z->release_buffer();