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

[Daniel van Vugt]

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

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1592
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1501/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/22
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/22
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/18/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/18/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/18
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/18/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/28
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/28/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1294
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6700

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

review: Needs Fixing (continuous-integration)

[lp:~vanvugt/mir/fix-1315302]

1593. By Daniel van Vugt on 2014-05-05

Merge latest development-branch

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1593
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1516/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/45
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/46
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/46
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/35
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/35/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/33
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/33/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/52
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/52/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1316
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/6769

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Robert Carr (robertcarr)]

LGTM.

Nit:
+ std::deque<Callback> client_acquires_todo;

client_acquire_completions reads better to me.

review: Approve

[Alberto Aguirre (albaguirre)]

Since the alternate implementation has been merged I've rebased this fix here:

https://code.launchpad.net/~mir-team/mir/fix-1315302/+merge/218348

[Daniel van Vugt (vanvugt)]

Yeah this needs rewriting now BufferQueue has landed.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Rejected. The alternate fix has landed.

Unmerged revisions

1593. By Daniel van Vugt on 2014-05-05

Merge latest development-branch

1592. By Daniel van Vugt on 2014-05-02

Correct comment

1591. By Daniel van Vugt on 2014-05-02

Enable the failing test and fix it, using a queue of callbacks.

1590. By Daniel van Vugt on 2014-05-02

Add a regression test for LP: #1315302, currently failing.

=== modified file 'src/server/compositor/switching_bundle.cpp'
--- src/server/compositor/switching_bundle.cpp	2014-04-15 05:31:19 +0000
+++ src/server/compositor/switching_bundle.cpp	2014-05-05 04:09:18 +0000
@@ -205,10 +205,10 @@
     return nfree() >= min_free;
 }
 
-void mc::SwitchingBundle::client_acquire(std::function<void(graphics::Buffer* buffer)> complete)
+void mc::SwitchingBundle::client_acquire(Callback complete)
 {
     std::unique_lock<std::mutex> lock(guard);
-    client_acquire_todo = std::move(complete);
+    client_acquires_todo.push_back(std::move(complete));
 
     if ((framedropping || force_drop) && nbuffers > 1)
     {
@@ -230,7 +230,8 @@
 
 void mc::SwitchingBundle::complete_client_acquire(std::unique_lock<std::mutex> lock)
 {
-    auto complete = std::move(client_acquire_todo);
+    auto complete = std::move(client_acquires_todo.front());
+    client_acquires_todo.pop_front();
 
     if ((framedropping || force_drop) && nbuffers > 1)
     {
@@ -381,8 +382,10 @@
             ncompositors--;
         }
 
-        if (client_buffers_available(lock) && client_acquire_todo)
+        if (client_buffers_available(lock) && !client_acquires_todo.empty())
+        {
             complete_client_acquire(std::move(lock));
+        }
     }
 }
 
@@ -435,7 +438,7 @@
 void mc::SwitchingBundle::force_requests_to_complete()
 {
     std::unique_lock<std::mutex> lock(guard);
-    if (client_acquire_todo)
+    if (!client_acquires_todo.empty())
     {
         drop_frames(nready);
         force_drop = nbuffers + 1;

=== modified file 'src/server/compositor/switching_bundle.h'
--- src/server/compositor/switching_bundle.h	2014-03-26 05:48:59 +0000
+++ src/server/compositor/switching_bundle.h	2014-05-05 04:09:18 +0000
@@ -26,6 +26,7 @@
 #include <memory>
 #include <iosfwd>
 #include <unordered_set>
+#include <deque>
 
 namespace mir
 {
@@ -41,6 +42,7 @@
 {
 public:
     enum {min_buffers = 1, max_buffers = 5};
+    typedef std::function<void(graphics::Buffer* buffer)> Callback;
 
     SwitchingBundle(int nbuffers,
                     const std::shared_ptr<graphics::GraphicBufferAllocator> &,
@@ -50,7 +52,7 @@
 
     graphics::BufferProperties properties() const;
 
-    void client_acquire(std::function<void(graphics::Buffer* buffer)> complete) override;
+    void client_acquire(Callback complete) override;
     void client_release(graphics::Buffer* buffer);
     std::shared_ptr<graphics::Buffer>
         compositor_acquire(void const* user_id) override;
@@ -111,7 +113,7 @@
     bool framedropping;
     int force_drop;
 
-    std::function<void(graphics::Buffer* buffer)> client_acquire_todo;
+    std::deque<Callback> client_acquires_todo;
 
     friend std::ostream& operator<<(std::ostream& os, const SwitchingBundle& bundle);
 };

=== modified file 'tests/unit-tests/compositor/test_switching_bundle.cpp'
--- tests/unit-tests/compositor/test_switching_bundle.cpp	2014-04-15 05:31:19 +0000
+++ tests/unit-tests/compositor/test_switching_bundle.cpp	2014-05-05 04:09:18 +0000
@@ -254,6 +254,37 @@
     }
 }
 
+TEST_F(SwitchingBundleTest, clients_can_acquire_multiple_buffers)
+{   // Regression test for LP: #1315302
+    int const nbuffers = 3;
+    mc::SwitchingBundle bundle(nbuffers, allocator, basic_properties);
+
+    for (int i = 0; i < nbuffers; ++i)
+        bundle.client_release(client_acquire_blocking(bundle));
+
+    mg::Buffer* acquired1 = nullptr;
+    bundle.client_acquire([&](mg::Buffer* buffer)
+        {
+            acquired1 = buffer;
+        });
+
+    mg::Buffer* acquired2 = nullptr;
+    bundle.client_acquire([&](mg::Buffer* buffer)
+        {
+            acquired2 = buffer;
+        });
+
+    for (int i = 0; i < nbuffers+2; ++i)
+        bundle.compositor_release(bundle.compositor_acquire(this));
+
+    EXPECT_NE(acquired1, acquired2);
+    ASSERT_NE(nullptr, acquired2);
+    ASSERT_NE(nullptr, acquired1);
+
+    bundle.client_release(acquired1);
+    bundle.client_release(acquired2);
+}
+
 TEST_F(SwitchingBundleTest, compositor_acquire_basic)
 {
     for (int nbuffers = mc::SwitchingBundle::min_buffers;