Merge lp:~kdub/mir/queueing-schedule into lp:mir

[Kevin DuBois]

compositor: add a yet-unused schedule for queueing (swapinterval 1) clients (ie, those that want all their submitted buffers to appear onscreen)

compositor: add a yet-unused schedule for queueing (swapinterval 1) clients (ie, those that want all their submitted buffers to appear onscreen)

note:
1) In the current algorithm, a swapinterval1 client will drop frames periodically according to its FrameDropPolicy to help out qt5. This feature isn't implemented yet... Either the serverside policy or the client (via a cancel rpc call) could provide this feature via the cancel() call.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2751
http://jenkins.qa.ubuntu.com/job/mir-ci/4366/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3226
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/79
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/751
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3174/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/515
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/515/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3174
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3174/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5976/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21989

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

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

Nits:

44 + std::unique_lock<decltype(mutex)> lk(mutex);

We could use lock_guard<> instead of the more unique_lock<>.

202 + for(auto i = 0u; i < num_buffers; i++)
203 + if (i & 1) schedule.schedule(buffers[i]);
204 + for(auto i = 0u; i < num_buffers; i++)
205 + if (!(i & 1)) schedule.schedule(buffers[i]);

209 + EXPECT_THAT(drain_queue(),
210 + ElementsAre(buffers[1], buffers[3], buffers[0], buffers[2], buffers[4]));

Since our expectation depends on num_buffers = 5, we don't gain anything for this test by introducing more general logic in our scheduling. We could have just have, e.g.:

std::vector<> buffer_schedule{buffers[1], buffers[3], buffers[0], buffers[2], buffers[4]};
for (auto b : buffer_schedule) schedule.schedule(b);
EXPECT_THAT(drain_queue(), ContainerEq(buffer_schedule));

Alternatively, we could keep the scheduling logic but make our expectation independent of the value of num_buffers.

review: Approve

[Alexandros Frantzis (afrantzis)]

"We could use lock_guard<> instead of the more unique_lock<>."

... of the more expensive unique_lock<>

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2754
http://jenkins.qa.ubuntu.com/job/mir-ci/4380/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3243
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/96
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/768
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3191
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/529
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/529/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3191
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3191/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5994
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22021

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

As with https://code.launchpad.net/~kdub/mir/dropping-schedule/+merge/265303/comments/666132, cancel() races with anything_scheduled().

Perhaps the Schedule shoud instead be called the BufferQueue, and should have the current_buffer()/advance_buffer() semantics I suggested in https://code.launchpad.net/~kdub/mir/multimonitor-arbiter/+merge/265301/comments/666115 ?

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2755
http://jenkins.qa.ubuntu.com/job/mir-ci/4394/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3268
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/121
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/793
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3216/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/543/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3216
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3216/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6015/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22057

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

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

I have the thought that these might be the kind of unit tests that should be implemented as guides and then thrown away...it's strange that they are larger than the implementation.

Seems ok to me though.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2756
http://jenkins.qa.ubuntu.com/job/mir-ci/4407/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3287
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/140
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/812
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3235/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/556
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/556/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3235
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3235/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6034/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22097

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

review: Needs Fixing (continuous-integration)

[Chris Halse Rogers (raof)]

Seems a reasonable transition step.

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

Ok

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/server/compositor/CMakeLists.txt'
--- src/server/compositor/CMakeLists.txt	2015-07-23 14:07:02 +0000
+++ src/server/compositor/CMakeLists.txt	2015-07-23 19:21:25 +0000
@@ -19,6 +19,7 @@
   recently_used_cache.cpp
   multi_monitor_arbiter.cpp
   buffer_map.cpp
+  queueing_schedule.cpp
 )
 
 ADD_LIBRARY(

=== added file 'src/server/compositor/queueing_schedule.cpp'
--- src/server/compositor/queueing_schedule.cpp	1970-01-01 00:00:00 +0000
+++ src/server/compositor/queueing_schedule.cpp	2015-07-23 19:21:25 +0000
@@ -0,0 +1,49 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Kevin DuBois <[e-mail]>
+ */
+
+#include "queueing_schedule.h"
+#include <boost/throw_exception.hpp>
+#include <algorithm>
+
+namespace mc = mir::compositor;
+namespace mg = mir::graphics;
+
+void mc::QueueingSchedule::schedule(std::shared_ptr<graphics::Buffer> const& buffer)
+{
+    std::lock_guard<decltype(mutex)> lk(mutex);
+    auto it = std::find(queue.begin(), queue.end(), buffer);
+    if (it != queue.end())
+        queue.erase(it);
+    queue.emplace_back(buffer);
+}
+
+bool mc::QueueingSchedule::anything_scheduled()
+{
+    std::lock_guard<decltype(mutex)> lk(mutex);
+    return !queue.empty();
+}
+
+std::shared_ptr<mg::Buffer> mc::QueueingSchedule::next_buffer()
+{
+    std::lock_guard<decltype(mutex)> lk(mutex);
+    if (queue.empty())
+        BOOST_THROW_EXCEPTION(std::logic_error("no buffer scheduled"));
+    auto buffer = queue.front();
+    queue.pop_front();
+    return buffer;
+}

=== added file 'src/server/compositor/queueing_schedule.h'
--- src/server/compositor/queueing_schedule.h	1970-01-01 00:00:00 +0000
+++ src/server/compositor/queueing_schedule.h	2015-07-23 19:21:25 +0000
@@ -0,0 +1,43 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Kevin DuBois <[e-mail]>
+ */
+
+#ifndef MIR_COMPOSITOR_QUEUEING_SCHEDULE_H_
+#define MIR_COMPOSITOR_QUEUEING_SCHEDULE_H_
+#include <memory>
+#include <deque>
+#include <mutex>
+
+namespace mir
+{
+namespace graphics { class Buffer; }
+namespace compositor
+{
+class QueueingSchedule
+{
+public:
+    void schedule(std::shared_ptr<graphics::Buffer> const& buffer);
+    bool anything_scheduled();
+    std::shared_ptr<graphics::Buffer> next_buffer();
+
+private:
+    std::mutex mutable mutex;
+    std::deque<std::shared_ptr<graphics::Buffer>> queue;
+};
+}
+}
+#endif /* MIR_COMPOSITOR_QUEUEING_SCHEDULE_H_ */

=== modified file 'tests/unit-tests/compositor/CMakeLists.txt'
--- tests/unit-tests/compositor/CMakeLists.txt	2015-07-23 14:07:02 +0000
+++ tests/unit-tests/compositor/CMakeLists.txt	2015-07-23 19:21:25 +0000
@@ -13,6 +13,7 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_timeout_frame_dropping_policy.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_multi_monitor_arbiter.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_client_buffers.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_queueing_schedule.cpp
 )
 
 set(UNIT_TEST_SOURCES ${UNIT_TEST_SOURCES} PARENT_SCOPE)

=== added file 'tests/unit-tests/compositor/test_queueing_schedule.cpp'
--- tests/unit-tests/compositor/test_queueing_schedule.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/compositor/test_queueing_schedule.cpp	2015-07-23 19:21:25 +0000
@@ -0,0 +1,85 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Kevin DuBois <[e-mail]>
+ */
+
+#include "src/server/compositor/queueing_schedule.h"
+#include "mir/test/doubles/stub_buffer.h"
+#include <gtest/gtest.h>
+
+using namespace testing;
+namespace mtd = mir::test::doubles;
+namespace mg = mir::graphics;
+namespace mc = mir::compositor;
+
+namespace
+{
+struct QueueingSchedule : Test
+{
+    QueueingSchedule()
+    {
+        for(auto i = 0u; i < num_buffers; i++)
+            buffers.emplace_back(std::make_shared<mtd::StubBuffer>());
+    }
+    unsigned int const num_buffers{5};
+    std::vector<std::shared_ptr<mg::Buffer>> buffers;
+
+    mc::QueueingSchedule schedule;
+    std::vector<std::shared_ptr<mg::Buffer>> drain_queue()
+    {
+        std::vector<std::shared_ptr<mg::Buffer>> scheduled_buffers;
+        while(schedule.anything_scheduled())
+            scheduled_buffers.emplace_back(schedule.next_buffer());
+        return scheduled_buffers;
+    }
+};
+}
+
+TEST_F(QueueingSchedule, throws_if_no_buffers)
+{
+    EXPECT_FALSE(schedule.anything_scheduled());
+    EXPECT_THROW({
+        schedule.next_buffer();
+    }, std::logic_error);
+}
+
+TEST_F(QueueingSchedule, queues_buffers_up)
+{
+    EXPECT_FALSE(schedule.anything_scheduled());
+
+    std::vector<std::shared_ptr<mg::Buffer>> scheduled_buffers {
+        buffers[1], buffers[3], buffers[0], buffers[2], buffers[4]
+    };
+
+    for (auto& buffer : scheduled_buffers)
+        schedule.schedule(buffer);
+
+    EXPECT_TRUE(schedule.anything_scheduled());
+
+    EXPECT_THAT(drain_queue(), ContainerEq(scheduled_buffers));
+
+    EXPECT_FALSE(schedule.anything_scheduled());
+}
+
+TEST_F(QueueingSchedule, queuing_the_same_buffer_moves_it_to_front_of_queue)
+{
+    for(auto i = 0u; i < num_buffers; i++)
+        schedule.schedule(buffers[i]);
+    schedule.schedule(buffers[0]);
+
+    EXPECT_THAT(drain_queue(),
+        ElementsAre(buffers[1], buffers[2], buffers[3], buffers[4], buffers[0]));
+}