Merge lp:~afrantzis/mir/fix-1306464 into lp:mir

[Alexandros Frantzis]

compositor: Don't block in SwitchingBundle::client_acquire() when a framedropping bundle has no free or ready buffers

compositor: Don't block in SwitchingBundle::client_acquire() when a framedropping bundle has no free or ready buffers

Fixes https://bugs.launchpad.net/mir/+bug/1306464 which cause intermittent CI failures.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1547
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1312/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1576
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1574
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/1149
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1044
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/1044/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1049
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/1049/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1150
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/1150/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1065
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5959

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

LGTM

review: Approve

[Alan Griffiths (alan-griffiths)]

Seems OK

review: Approve

=== modified file 'include/test/mir_test/wait_condition.h'
--- include/test/mir_test/wait_condition.h	2013-04-24 05:22:20 +0000
+++ include/test/mir_test/wait_condition.h	2014-04-11 10:56:45 +0000
@@ -31,24 +31,31 @@
 {
 struct WaitCondition
 {
-    WaitCondition() : woken(false) {}
+    WaitCondition() : woken_(false) {}
 
     void wait_for_at_most_seconds(int seconds)
     {
         std::unique_lock<std::mutex> ul(guard);
-        if (!woken) condition.wait_for(ul, std::chrono::seconds(seconds));
+        condition.wait_for(ul, std::chrono::seconds(seconds),
+                           [this] { return woken_; });
     }
 
     void wake_up_everyone()
     {
         std::unique_lock<std::mutex> ul(guard);
-        woken = true;
+        woken_ = true;
         condition.notify_all();
     }
 
+    bool woken()
+    {
+        std::unique_lock<std::mutex> ul(guard);
+        return woken_;
+    }
+
     std::mutex guard;
     std::condition_variable condition;
-    bool woken;
+    bool woken_;
 };
 
 ACTION_P(ReturnFalseAndWakeUp, wait_condition)

=== modified file 'src/server/compositor/switching_bundle.cpp'
--- src/server/compositor/switching_bundle.cpp	2014-03-26 05:48:59 +0000
+++ src/server/compositor/switching_bundle.cpp	2014-04-11 10:56:45 +0000
@@ -212,13 +212,12 @@
 
     if ((framedropping || force_drop) && nbuffers > 1)
     {
-        if (nfree() <= 0)
-        {
-            while (nready == 0)
-                cond.wait(lock);
-
-            drop_frames(1);
-        }
+        /*
+         * If we don't have a free buffer to return or we cannot free a ready buffer
+         * by dropping it, we would block in complete_client_acquire, so just return.
+         */
+        if (nfree() <= 0 && nready == 0)
+            return;
     }
     else
     {
@@ -233,6 +232,17 @@
 {
     auto complete = std::move(client_acquire_todo);
 
+    if ((framedropping || force_drop) && nbuffers > 1)
+    {
+        if (nfree() <= 0)
+        {
+            while (nready == 0)
+                cond.wait(lock);
+
+            drop_frames(1);
+        }
+    }
+
     if (force_drop > 0)
         force_drop--;
 

=== modified file 'tests/unit-tests/compositor/test_switching_bundle.cpp'
--- tests/unit-tests/compositor/test_switching_bundle.cpp	2014-03-26 05:48:59 +0000
+++ tests/unit-tests/compositor/test_switching_bundle.cpp	2014-04-11 10:56:45 +0000
@@ -19,6 +19,7 @@
 #include "src/server/compositor/switching_bundle.h"
 #include "mir_test_doubles/stub_buffer_allocator.h"
 #include "mir_test_doubles/stub_buffer.h"
+#include "mir_test/wait_condition.h"
 
 #include <gtest/gtest.h>
 
@@ -944,3 +945,59 @@
         }
     }
 }
+
+TEST_F(SwitchingBundleTest, framedropping_client_acquire_does_not_block_when_no_available_buffers)
+{
+    /* Regression test for LP: #1306464 */
+    using namespace testing;
+
+    int const nbuffers{3};
+    mc::SwitchingBundle bundle{nbuffers, allocator, basic_properties};
+
+    bundle.allow_framedropping(true);
+
+    mg::Buffer* buffer{nullptr};
+
+    for (int i = 0; i < nbuffers; ++i)
+    {
+        bundle.client_acquire([&] (mg::Buffer* b) { buffer = b; });
+        bundle.client_release(buffer);
+    }
+
+    /*
+     * We need nbuffers - 1 compositors to acquire all the buffers,
+     * each compositor acquiring buffers twice (see below)
+     */
+    std::vector<int> compositors(nbuffers - 1);
+    std::vector<std::shared_ptr<mg::Buffer>> buffers;
+
+    for (auto const& id : compositors)
+    {
+        buffers.push_back(bundle.compositor_acquire(&id));
+        buffers.push_back(bundle.compositor_acquire(&id));
+    }
+
+    /* At this point the bundle has 0 free buffers and 0 ready buffers */
+
+    mir::test::WaitCondition client_acquire_complete;
+
+    /*
+     * Although we neither have a free buffer, nor can we free a buffer by dropping it,
+     * this shouldn't block. BufferBundle::client_acquire() should *never* block.
+     */
+    bundle.client_acquire(
+        [&] (mg::Buffer*)
+        {
+            client_acquire_complete.wake_up_everyone();
+        });
+
+    /* Release compositor buffers so that the client can get one */
+    for (auto const& buffer : buffers)
+    {
+        bundle.compositor_release(buffer);
+    }
+
+    client_acquire_complete.wait_for_at_most_seconds(5);
+
+    EXPECT_TRUE(client_acquire_complete.woken());
+}