Merge lp:~afrantzis/mir/swapper-multiple-compositor-acquire into lp:~mir-team/mir/trunk

[Alexandros Frantzis]

compositor: Support multiple compositor_acquire() calls for BufferSwapperMulti

compositor: Support multiple compositor_acquire() calls for BufferSwapperMulti

This is the next step in the session snapshots saga: being able to hand out compositor buffers to multiple consumers. Note that this deals with normal acquires, not buffer peeking. Buffer peeking complicates the swapper even more, and after a lot of experimentation I decided not to pursue it further until there is demonstrated need for it (the current code can be modified to support peeking).

Acquiring (instead of peeking) a buffer when taking a snapshot may lead to the compositor losing a buffer (= glitch). However, this may not actually be a problem in practice since in the current use cases snapshotting happens when a surface is about to be hidden. In any case, consider this to be first step which we can refine in the future if there is a need.

Also, note that this work will benefit multiple monitor support, for which the compositor may need to acquire the buffer multiple times (one for each screen), for example, if a surface is visible on more than one screens (and we are not in clone mode).

The semantics of multiple compositor acquires for BufferSwapperMulti are:

1. All calls in a series of acquires without any intervening releases will return the same buffer, even if newer buffers have been posted.
2. The acquired buffer will return to the client only after all acquires have been released.
3. The next acquire after any release, even if it is not a final release, will get the last posted buffer (which may be the already acquired buffer if there is no newer buffer).

(1) Reduces the missing of buffers:

Although this scenario may still cause the compositors to miss buffers:

Comp1: AR__AR
Comp2: __AR__AR

...in the following scenarios buffers are not missed:

Comp1: A__R
Comp2: _AR_

Comp1: A_R_
Comp2: _A_R_

(3) This ensures we don't get stuck with the same buffer forever, e.g., in the following scenario:

Comp1: A_RA__RA__RA ...
Comp2: _A__RA__RA__ ...

In the scenario above the buffer stays acquired forever. If we didn't have (3) or a similar escape route, the two compositors would get the same buffer forever (because of (1))!

These rules are not set in stone, they were chosen as a decent starting poing that seems to work well, but we are free to change them as needed. For example we can get rid of (1)+(3), and decide that acquire() will always return the last posted buffer.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:743
http://jenkins.qa.ubuntu.com/job/mir-ci/734/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/913
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/795
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/219/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/421

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

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Spurious jenkins failure... resubmitted.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:743
http://jenkins.qa.ubuntu.com/job/mir-ci/738/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/917
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/799
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/223
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/223/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/425

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

the new tests look like what we want to do. I'm not sure if the approach with AcquiredBuffers is the simplest though. We could do something like have SwitchingSwapper take care of 'cloning' and refcounting the outgoing buffers on the compositor side (and keeping the mc::BufferSwapper requirements the same).

I also don't mind having a strong reference to the Buffer in both the TemporaryBuffer the compositor owns and the BufferBundle, if that makes it simpler.

[Kevin DuBois (kdub)]

I guess with all the new swapper functionality we're finding that we need (like, resizing a bundle, switching swapping algorithms mid-flight, multimonitor, and peeking for individual snapshots (maybe even screencasting?)) I'd rather solve the problems in the layer between the BufferStream and the BufferSwappers than by making BufferSwapper have more responsibilities. Hopefully this will keep our BufferSwappers steady state algorithms for swapping buffers, and the 'corner cases' (mentioned above) are taken care of mostly by diverting the buffers once we've ensured sync in the BufferSwappers.

[Alexandros Frantzis (afrantzis)]

> I'd rather solve the problems in the layer between the
> BufferStream and the BufferSwappers than by making BufferSwapper have more
> responsibilities. Hopefully this will keep our BufferSwappers steady state
> algorithms for swapping buffers, and the 'corner cases' (mentioned above)
> are taken care of mostly by diverting the buffers once we've ensured sync
> in the BufferSwappers.

Good idea. I am unhappy with adding complexity to BufferSwapper myself.

I will move the multiple acquire support to SwitchingBundle. Note that we still need to use the AcquiredBuffers class (or something similar) in SwitchingBundle, to enforce the behaviour mentioned in the description.

Unmerged revisions

743. By Alexandros Frantzis on 2013-06-14

compositor: Support multiple compositor_acquire() calls for BufferSwapperMulti

742. By Daniel van Vugt on 2013-06-14

setup-android-dependencies.sh: Fix typo causing libxkbcommon0 to not be
installed and the script never quite finish. (LP: #1190884). Fixes: https://bugs.launchpad.net/bugs/1190884.

Approved by Alexandros Frantzis, PS Jenkins bot.

741. By Daniel van Vugt on 2013-06-14

Introducing fingerpaint. It's not just a multi-touch demo that responds to
pressure and size, but more importantly it's a future testbed for localized
input coordinates.

Pretty boring on desktop. Less so on Nexus 4.
.

Approved by Kevin DuBois, Alan Griffiths, PS Jenkins bot.

740. By Alan Griffiths on 2013-06-13

compositor: Restore force_requests_to_complete() logic to what we had before -r 728. Fixes: https://bugs.launchpad.net/bugs/1189443.

Approved by PS Jenkins bot, Kevin DuBois, Alexandros Frantzis.

739. By Daniel van Vugt on 2013-06-13

Fix debuild failures on saucy / gcc-4.8.

Approved by Alexandros Frantzis, Alan Griffiths, PS Jenkins bot, Chris Halse Rogers.

738. By Daniel van Vugt on 2013-06-13

Minor correction to documentation building_source_for_pc so that the
instructions will actually work :)

Remember sudo.

Approved by Alexandros Frantzis, PS Jenkins bot.

737. By Daniel van Vugt on 2013-06-13

Resolve the two differing interpretations of "stride" which were causing
software surfaces to be distorted on Android. (LP: #1116452)

Just make sure that "stride" in any Android structure is a number of pixels
and in Mir types it is always a number of bytes. Fixes: https://bugs.launchpad.net/bugs/1116452.

Approved by Kevin DuBois, Alexandros Frantzis, Alan Griffiths, PS Jenkins bot.

736. By Kevin DuBois on 2013-06-12

Iterate on the SwapperFactory so it can meet the more complex demands of different allocations. This is the cleanup of the the rough edges of the SwapperSwitcher algorithm branch.

Approved by PS Jenkins bot, Alexandros Frantzis, Alan Griffiths.

735. By Robert Carr on 2013-06-12

Depthify the surface stack to support the launcher use case.

Approved by Alan Griffiths, PS Jenkins bot, Alexandros Frantzis.

734. By Daniel van Vugt on 2013-06-12

Fix backward error message and improve error details (mention the errno).
(LP: #1189726). Fixes: https://bugs.launchpad.net/bugs/1189726.

Approved by Alexandros Frantzis, Alan Griffiths, PS Jenkins bot.

=== modified file 'include/server/mir/compositor/buffer_swapper.h'
--- include/server/mir/compositor/buffer_swapper.h	2013-06-13 10:40:42 +0000
+++ include/server/mir/compositor/buffer_swapper.h	2013-06-14 15:58:28 +0000
@@ -41,11 +41,26 @@
        it. This modifies the buffer the compositor posts to the screen */
     virtual void client_release(std::shared_ptr<Buffer> const& queued_buffer) = 0;
 
-    /* caller of compositor_acquire buffer should get no-wait access to the
-        last posted buffer. However, the client will potentially stall
-        until control of the buffer is returned via compositor_release() */
+    /**
+     * Acquires the last posted buffer.
+     *
+     * Callers of compositor_acquire() should get no-wait access to the
+     * last posted buffer. However, the client will potentially stall
+     * until control of the buffer is returned via matching calls to
+     * compositor_release().
+     *
+     * Each call in a series of compositor_acquire() calls is guaranteed to
+     * return a buffer no older than the ones returned in the previous calls
+     * (may return the same buffer).
+     */
     virtual std::shared_ptr<Buffer> compositor_acquire() = 0;
 
+    /**
+     * Releases a buffer from the compositor to the client.
+     *
+     * Note that the buffer is made available to the client only when all users
+     * who have called compositor_acquire() have released the buffer.
+     */
     virtual void compositor_release(std::shared_ptr<Buffer> const& released_buffer) = 0;
 
     /**

=== modified file 'include/server/mir/compositor/buffer_swapper_multi.h'
--- include/server/mir/compositor/buffer_swapper_multi.h	2013-06-12 16:56:32 +0000
+++ include/server/mir/compositor/buffer_swapper_multi.h	2013-06-14 15:58:28 +0000
@@ -27,12 +27,36 @@
 #include <vector>
 #include <memory>
 #include <deque>
-#include <map>
 
 namespace mir
 {
 namespace compositor
 {
+namespace detail
+{
+
+struct AcquiredBufferInfo
+{
+    std::weak_ptr<Buffer> buffer;
+    bool can_be_reacquired;
+    int use_count;
+};
+
+class AcquiredBuffers
+{
+public:
+    void acquire(std::shared_ptr<Buffer> const& buffer);
+    std::pair<bool,bool> release(std::shared_ptr<Buffer> const& buffer);
+    std::shared_ptr<Buffer> find_reacquirable_buffer();
+    std::shared_ptr<Buffer> last_buffer();
+
+private:
+    std::vector<AcquiredBufferInfo>::iterator find_info(std::shared_ptr<Buffer> const& buffer);
+
+    std::vector<AcquiredBufferInfo> acquired_buffers;
+};
+
+}
 
 class Buffer;
 
@@ -56,6 +80,7 @@
 
     std::deque<std::shared_ptr<Buffer>> client_queue;
     std::deque<std::shared_ptr<Buffer>> compositor_queue;
+    detail::AcquiredBuffers acquired_buffers;
     unsigned int in_use_by_client;
     unsigned int const swapper_size;
     int clients_trying_to_acquire;

=== modified file 'src/server/compositor/buffer_swapper_multi.cpp'
--- src/server/compositor/buffer_swapper_multi.cpp	2013-06-12 16:56:32 +0000
+++ src/server/compositor/buffer_swapper_multi.cpp	2013-06-14 15:58:28 +0000
@@ -18,9 +18,90 @@
 
 #include "mir/compositor/buffer_swapper_multi.h"
 #include <boost/throw_exception.hpp>
+#include <algorithm>
 
 namespace mc = mir::compositor;
 
+std::vector<mc::detail::AcquiredBufferInfo>::iterator
+mc::detail::AcquiredBuffers::find_info(std::shared_ptr<Buffer> const& buffer)
+{
+    return std::find_if(acquired_buffers.begin(), acquired_buffers.end(),
+                        [&buffer] (AcquiredBufferInfo const& info)
+                        {
+                            return info.buffer.lock() == buffer;
+                        });
+}
+
+void mc::detail::AcquiredBuffers::acquire(std::shared_ptr<Buffer> const& buffer)
+{
+    auto iter = find_info(buffer);
+
+    if (iter != acquired_buffers.end())
+        ++iter->use_count;
+    else
+        acquired_buffers.push_back(AcquiredBufferInfo{buffer, true, 1});
+
+}
+
+std::pair<bool,bool>
+mc::detail::AcquiredBuffers::release(std::shared_ptr<Buffer> const& buffer)
+{
+    auto iter = find_info(buffer);
+
+    bool released{false};
+    bool found{false};
+
+    if (iter != acquired_buffers.end())
+    {
+        AcquiredBufferInfo& info = *iter;
+        found = true;
+
+        --info.use_count;
+
+        if (info.use_count == 0)
+        {
+            acquired_buffers.erase(iter);
+            released = true;
+        }
+        else
+        {
+            info.can_be_reacquired = false;
+        }
+    }
+
+    return std::make_pair(released, found);
+}
+
+std::shared_ptr<mc::Buffer> mc::detail::AcquiredBuffers::find_reacquirable_buffer()
+{
+    auto iter = std::find_if(acquired_buffers.begin(), acquired_buffers.end(),
+                    [] (AcquiredBufferInfo const& info)
+                    {
+                        return info.can_be_reacquired == true;
+                    });
+
+    if (iter != acquired_buffers.end())
+    {
+        return iter->buffer.lock();
+    }
+    else
+    {
+        return {};
+    }
+}
+
+std::shared_ptr<mc::Buffer> mc::detail::AcquiredBuffers::last_buffer()
+{
+    if (!acquired_buffers.empty())
+    {
+        return acquired_buffers.back().buffer.lock();
+    }
+    else
+    {
+        return {};
+    }
+}
+
 mc::BufferSwapperMulti::BufferSwapperMulti(std::vector<std::shared_ptr<compositor::Buffer>>& buffer_list, size_t swapper_size)
  : in_use_by_client(0),
    swapper_size(swapper_size),
@@ -57,6 +138,7 @@
     //we have been forced to shutdown
     if (force_clients_to_complete)
     {
+        clients_trying_to_acquire--;
         BOOST_THROW_EXCEPTION(std::logic_error("forced_completion"));
     }
 
@@ -89,31 +171,48 @@
 {
     std::unique_lock<std::mutex> lk(swapper_mutex);
 
-    if (compositor_queue.empty() && client_queue.empty())
-    {
-        BOOST_THROW_EXCEPTION(std::logic_error("forced_completion"));
-    }
-
     std::shared_ptr<mc::Buffer> dequeued_buffer;
-    if (compositor_queue.empty())
+
+    if ((dequeued_buffer = acquired_buffers.find_reacquirable_buffer()) != nullptr)
+    {
+    }
+    else if (!compositor_queue.empty())
+    {
+        dequeued_buffer = compositor_queue.front();
+        compositor_queue.pop_front();
+    }
+    else if (!client_queue.empty())
     {
         dequeued_buffer = client_queue.back();
         client_queue.pop_back();
     }
+    else if ((dequeued_buffer = acquired_buffers.last_buffer()) != nullptr)
+    {
+    }
     else
     {
-        dequeued_buffer = compositor_queue.front();
-        compositor_queue.pop_front();
+        BOOST_THROW_EXCEPTION(std::logic_error("forced_completion"));
     }
 
+    acquired_buffers.acquire(dequeued_buffer);
+
     return dequeued_buffer;
 }
 
 void mc::BufferSwapperMulti::compositor_release(std::shared_ptr<Buffer> const& released_buffer)
 {
     std::unique_lock<std::mutex> lk(swapper_mutex);
-    client_queue.push_back(released_buffer);
-    client_available_cv.notify_one();
+
+    auto const rel_pair = acquired_buffers.release(released_buffer);
+
+    bool const buffer_released{rel_pair.first};
+    bool const buffer_found{rel_pair.second};
+
+    if (buffer_released || !buffer_found)
+    {
+        client_queue.push_back(released_buffer);
+        client_available_cv.notify_one();
+    }
 }
 
 void mc::BufferSwapperMulti::force_client_abort()

=== modified file 'src/server/compositor/buffer_swapper_spin.cpp'
--- src/server/compositor/buffer_swapper_spin.cpp	2013-06-12 16:56:32 +0000
+++ src/server/compositor/buffer_swapper_spin.cpp	2013-06-14 15:58:28 +0000
@@ -17,6 +17,9 @@
  */
 
 #include "mir/compositor/buffer_swapper_spin.h"
+#include <boost/throw_exception.hpp>
+
+#include <stdexcept>
 
 namespace mc = mir::compositor;
 

=== modified file 'tests/integration-tests/compositor/test_stress_buffer_swapper.cpp'
--- tests/integration-tests/compositor/test_stress_buffer_swapper.cpp	2013-05-29 23:49:32 +0000
+++ tests/integration-tests/compositor/test_stress_buffer_swapper.cpp	2013-06-14 15:58:28 +0000
@@ -51,12 +51,14 @@
 
     mt::Synchronizer compositor_controller;
     mt::Synchronizer client_controller;
+    mt::Synchronizer compositor_controller1;
 
     std::chrono::microseconds const sleep_duration;
     int const num_iterations;
 
     std::thread thread1;
     std::thread thread2;
+    std::thread thread3;
 
     void test_distinct_buffers(mc::BufferSwapper& swapper);
     void test_valid_buffers(mc::BufferSwapper& swapper);
@@ -107,33 +109,43 @@
     }
 }
 
-/* test that the compositor and the client are never in ownership of the same
-   buffer */
+/*
+ * Test that multiple compositor buffer consumers and the
+ * client are never in ownership of the same buffer.
+ */
 void BufferSwapperStress::test_distinct_buffers(mc::BufferSwapper& swapper)
 {
     std::shared_ptr<mc::Buffer> compositor_buffer;
     std::shared_ptr<mc::Buffer> client_buffer;
+    std::shared_ptr<mc::Buffer> compositor_buffer1;
 
     thread1 = std::thread(compositor_grab_loop, std::ref(compositor_buffer),
                           std::ref(compositor_controller), std::ref(swapper));
     thread2 = std::thread(client_request_loop,  std::ref(client_buffer),
                           std::ref(client_controller), std::ref(swapper));
+    thread3 = std::thread(compositor_grab_loop, std::ref(compositor_buffer1),
+                          std::ref(compositor_controller1), std::ref(swapper));
 
-    for(int i=0; i<  num_iterations; i++)
+    for(int i = 0; i < num_iterations; i++)
     {
         compositor_controller.ensure_child_is_waiting();
         client_controller.ensure_child_is_waiting();
+        compositor_controller1.ensure_child_is_waiting();
 
         EXPECT_NE(compositor_buffer, client_buffer);
+        EXPECT_NE(compositor_buffer1, client_buffer);
 
         compositor_controller.activate_waiting_child();
         client_controller.activate_waiting_child();
+        compositor_controller1.activate_waiting_child();
 
         main_test_loop_pause(sleep_duration);
     }
 
+    terminate_child_thread(compositor_controller1);
     terminate_child_thread(client_controller);
     terminate_child_thread(compositor_controller);
+    thread3.join();
     thread2.join();
     thread1.join();
 }

=== modified file 'tests/integration-tests/compositor/test_swapping_swappers.cpp'
--- tests/integration-tests/compositor/test_swapping_swappers.cpp	2013-06-11 19:55:10 +0000
+++ tests/integration-tests/compositor/test_swapping_swappers.cpp	2013-06-14 15:58:28 +0000
@@ -39,25 +39,24 @@
 namespace
 {
 
-struct MockBufferAllocator : public mc::GraphicBufferAllocator
+struct StubBufferAllocator : public mc::GraphicBufferAllocator
 {
-    MockBufferAllocator()
+    std::shared_ptr<mc::Buffer> alloc_buffer(mc::BufferProperties const&)
     {
-        using namespace testing;
-        ON_CALL(*this, alloc_buffer(_))
-            .WillByDefault(Return(std::make_shared<mtd::StubBuffer>()));
+        return std::make_shared<mtd::StubBuffer>();
     }
-    ~MockBufferAllocator() noexcept{}
 
-    MOCK_METHOD1(alloc_buffer, std::shared_ptr<mc::Buffer>(mc::BufferProperties const&));
-    MOCK_METHOD0(supported_pixel_formats, std::vector<geom::PixelFormat>());
+    std::vector<geom::PixelFormat> supported_pixel_formats()
+    {
+        return {};
+    }
 };
 
 struct SwapperSwappingStress : public ::testing::Test
 {
     void SetUp()
     {
-        auto allocator = std::make_shared<MockBufferAllocator>();
+        auto allocator = std::make_shared<StubBufferAllocator>();
         auto factory = std::make_shared<mc::SwapperFactory>(allocator, 3);
         auto properties = mc::BufferProperties{geom::Size{geom::Width{380}, geom::Height{210}},
                                           geom::PixelFormat::abgr_8888, mc::BufferUsage::hardware};

=== modified file 'tests/unit-tests/compositor/test_buffer_swapper_double.cpp'
--- tests/unit-tests/compositor/test_buffer_swapper_double.cpp	2013-06-13 10:40:42 +0000
+++ tests/unit-tests/compositor/test_buffer_swapper_double.cpp	2013-06-14 15:58:28 +0000
@@ -166,3 +166,62 @@
         swapper->client_acquire();
     }, std::logic_error);
 }
+
+TEST_F(BufferSwapperDouble, compositor_acquire_gives_same_buffer_until_released)
+{
+    auto a = swapper->client_acquire();
+    swapper->client_release(a);
+    auto b = swapper->client_acquire();
+    swapper->client_release(b);
+
+    auto comp1 = swapper->compositor_acquire();
+    auto comp2 = swapper->compositor_acquire();
+
+    EXPECT_EQ(comp1, comp2);
+
+    swapper->compositor_release(comp1);
+
+    auto comp3 = swapper->compositor_acquire();
+    swapper->compositor_release(comp3);
+
+    EXPECT_NE(comp1, comp3);
+}
+
+TEST_F(BufferSwapperDouble, multiple_compositor_releases_return_buffer_to_client)
+{
+    auto a = swapper->client_acquire();
+    swapper->client_release(a);
+    auto b = swapper->client_acquire();
+    swapper->client_release(b);
+
+    auto comp1 = swapper->compositor_acquire();
+    auto comp2 = swapper->compositor_acquire();
+
+    EXPECT_EQ(comp1, comp2);
+
+    swapper->compositor_release(comp1);
+    swapper->compositor_release(comp2);
+
+    auto client1 = swapper->client_acquire();
+    swapper->client_release(client1);
+
+    EXPECT_EQ(comp1, comp2);
+}
+
+TEST_F(BufferSwapperDouble, compositor_acquire_can_return_partly_released_buffer)
+{
+    auto a = swapper->client_acquire();
+    swapper->client_release(a);
+    auto b = swapper->client_acquire();
+
+    auto comp1 = swapper->compositor_acquire();
+    auto comp2 = swapper->compositor_acquire();
+
+    EXPECT_EQ(comp1, comp2);
+
+    swapper->compositor_release(comp1);
+
+    auto comp3 = swapper->compositor_acquire();
+
+    EXPECT_EQ(comp1, comp3);
+}