Merge lp:~mir-team/mir/fix-1315302 into lp:mir

[Alberto Aguirre]

Ensure that if you have multiple client_acquire's outstanding, they each
get called back, eventually (LP: #1315302)

Rebase of http://code.launchpad.net/~vanvugt/mir/fix-1315302/+merge/218029

[Daniel van Vugt (vanvugt)]

Verified the regression test fails without the fix. Works with it. Looks reasonable.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1600
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1526/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/60
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/61
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/61
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/45
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/45/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/43
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/43/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/76
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/76/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1330
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6801

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1526/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

Looks good.

review: Approve

=== modified file 'src/server/compositor/buffer_queue.cpp'
--- src/server/compositor/buffer_queue.cpp	2014-04-29 19:29:53 +0000
+++ src/server/compositor/buffer_queue.cpp	2014-05-06 01:54:21 +0000
@@ -129,17 +129,11 @@
         free_buffers.push_back(current_compositor_buffer);
 }
 
-void mc::BufferQueue::client_acquire(std::function<void(graphics::Buffer* buffer)> complete)
+void mc::BufferQueue::client_acquire(mc::BufferQueue::Callback complete)
 {
     std::unique_lock<decltype(guard)> lock(guard);
 
-    if (give_to_client)
-    {
-        BOOST_THROW_EXCEPTION(
-            std::logic_error("unexpected acquire: there is a pending completion"));
-    }
-
-    give_to_client = std::move(complete);
+    pending_client_notifications.push_back(std::move(complete));
 
     if (!free_buffers.empty())
     {
@@ -284,7 +278,8 @@
 void mc::BufferQueue::force_requests_to_complete()
 {
     std::unique_lock<std::mutex> lock(guard);
-    if (give_to_client && !ready_to_composite_queue.empty())
+    if (!pending_client_notifications.empty() &&
+        !ready_to_composite_queue.empty())
     {
         auto const buffer = pop(ready_to_composite_queue);
         while (!ready_to_composite_queue.empty())
@@ -317,7 +312,8 @@
     std::unique_lock<std::mutex> lock)
 {
     /* Clears callback */
-    auto give_to_client_cb = std::move(give_to_client);
+    auto give_to_client_cb = std::move(pending_client_notifications.front());
+    pending_client_notifications.pop_front();
 
     bool const resize_buffer = buffer->size() != the_properties.size;
     if (resize_buffer)
@@ -381,7 +377,7 @@
     mg::Buffer* buffer,
     std::unique_lock<std::mutex> lock)
 {
-    if (give_to_client)
+    if (!pending_client_notifications.empty())
         give_buffer_to_client(buffer, std::move(lock));
     else
         free_buffers.push_back(buffer);

=== modified file 'src/server/compositor/buffer_queue.h'
--- src/server/compositor/buffer_queue.h	2014-04-29 19:29:53 +0000
+++ src/server/compositor/buffer_queue.h	2014-05-06 01:54:21 +0000
@@ -38,11 +38,13 @@
 class BufferQueue : public BufferBundle
 {
 public:
+    typedef std::function<void(graphics::Buffer* buffer)> Callback;
+
     BufferQueue(int nbuffers,
                 std::shared_ptr<graphics::GraphicBufferAllocator> const& alloc,
                 graphics::BufferProperties const& props);
 
-    void client_acquire(std::function<void(graphics::Buffer* buffer)> complete) override;
+    void client_acquire(Callback complete) override;
     void client_release(graphics::Buffer* buffer) override;
     std::shared_ptr<graphics::Buffer> compositor_acquire(void const* user_id) override;
     void compositor_release(std::shared_ptr<graphics::Buffer> const& buffer) override;
@@ -75,7 +77,7 @@
     std::vector<void const*> current_buffer_users;
     graphics::Buffer* current_compositor_buffer;
 
-    std::function<void(graphics::Buffer* buffer)> give_to_client;
+    std::deque<Callback> pending_client_notifications;
 
     int nbuffers;
     bool frame_dropping_enabled;

=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp	2014-05-05 21:50:59 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp	2014-05-06 01:54:21 +0000
@@ -289,29 +289,34 @@
     }
 }
 
-TEST_F(BufferQueueTest, throws_if_client_acquire_has_pending_completion)
+/* Regression test for LP: #1315302 */
+TEST_F(BufferQueueTest, clients_can_have_multiple_pending_completions)
 {
-    mc::BufferQueue q(2, allocator, basic_properties);
-    ASSERT_THAT(q.framedropping_allowed(), Eq(false));
-
-    auto handle = client_acquire_async(q);
-    ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
-
-    handle->release_buffer();
-
-    auto fail_if_called = [&](mg::Buffer* client_buffer)
+    int const nbuffers = 3;
+    mc::BufferQueue q(nbuffers, allocator, basic_properties);
+
+    for (int i = 0; i < nbuffers - 1; ++i)
     {
-        q.client_release(client_buffer);
-        FAIL();
-    };
-
-    /* All buffers have been exhausted,
-     * hence the callback should not be invoked
-     */
-    q.client_acquire(fail_if_called);
-
-    /* Now there is a pending completion */
-    EXPECT_THROW(q.client_acquire(fail_if_called), std::logic_error);
+        auto handle = client_acquire_async(q);
+        ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
+        handle->release_buffer();
+    }
+
+    auto handle1 = client_acquire_async(q);
+    ASSERT_THAT(handle1->has_acquired_buffer(), Eq(false));
+
+    auto handle2 = client_acquire_async(q);
+    ASSERT_THAT(handle1->has_acquired_buffer(), Eq(false));
+
+    for (int i = 0; i < nbuffers + 1; ++i)
+        q.compositor_release(q.compositor_acquire(this));
+
+    EXPECT_THAT(handle1->has_acquired_buffer(), Eq(true));
+    EXPECT_THAT(handle2->has_acquired_buffer(), Eq(true));
+    EXPECT_THAT(handle1->buffer(), Ne(handle2->buffer()));
+
+    handle1->release_buffer();
+    handle2->release_buffer();
 }
 
 TEST_F(BufferQueueTest, compositor_acquires_frames_in_order_for_synchronous_client)