Merge lp:~vanvugt/mir/always-triple-buffers into lp:mir

[Daniel van Vugt]

Turn off (but don't remove) dynamic double buffering -- back to always
triple buffering.

So close and yet so far... This is hopefully a temporary measure to
work around LP: #1476201 by un-fixing LP: #1240909, in order to
avoid blocking the imminent release of Mir 0.15.

There are obvious performance issues in QtMir that still need fixing, probably related to this. Less obvious may be additional bugs on the Mir side that we haven't identified yet. Either way, let's not block 0.15 waiting on a solution.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2782
http://jenkins.qa.ubuntu.com/job/mir-ci/4418/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3314
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/167
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/839
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3262
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/567
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/567/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3262
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3262/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6058
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22139

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

*Needs Discussion*

Using frame_deadlines_threshold/set_scaling_delay() as a proxy for disabling dynamic double buffering isn't the clearest code. (Although for a temporary workaround I won't block.)

More importantly, if we're waiting on qtmir changes before enabling it would be good if this were selectable at runtime to allow downstream testing & evaluation.

review: Needs Information

[Daniel van Vugt (vanvugt)]

It is at least documented already:

    /**
     * Set the minimum number of smooth frames the client must keep up with
     * the compositor for in order to qualify for queue scaling (dynamic
     * double buffering for reduced latency). A negative value means never
     * but it's recommended that you never change this.
     */
    void set_scaling_delay(int nframes);

[Alberto Aguirre (albaguirre)]

OK as a workaround, but would like to see it selectable at runtime when enabled again.

review: Approve

=== modified file 'src/server/compositor/buffer_queue.cpp'
--- src/server/compositor/buffer_queue.cpp	2015-07-21 03:49:25 +0000
+++ src/server/compositor/buffer_queue.cpp	2015-07-27 07:47:45 +0000
@@ -137,7 +137,7 @@
     graphics::BufferProperties const& props,
     mc::FrameDroppingPolicyFactory const& policy_provider)
     : nbuffers{nbuffers},
-      frame_deadlines_threshold{3},
+      frame_deadlines_threshold{-1},  // Disable scaling by default
       frame_deadlines_met{0},
       frame_dropping_enabled{false},
       current_compositor_buffer_valid{false},

=== modified file 'tests/integration-tests/test_buffer_scheduling.cpp'
--- tests/integration-tests/test_buffer_scheduling.cpp	2015-07-20 03:16:27 +0000
+++ tests/integration-tests/test_buffer_scheduling.cpp	2015-07-27 07:47:45 +0000
@@ -716,6 +716,8 @@
 
 TEST_P(WithAnyNumberOfBuffers, compositor_inflates_ready_count_for_slow_clients)
 {
+    queue.set_scaling_delay(3);
+
     for (int frame = 0; frame < 10; frame++)
     {
         ASSERT_EQ(0, queue.buffers_ready_for_compositor(&consumer));
@@ -724,7 +726,7 @@
         // Detecting a slow client requires scheduling at least one extra
         // frame...
         int nready = queue.buffers_ready_for_compositor(&consumer);
-        ASSERT_EQ(2, nready);
+        ASSERT_THAT(nready, Ge(2));
         for (int i = 0; i < nready; ++i)
             consumer.consume();
     }
@@ -933,7 +935,8 @@
 TEST_P(WithThreeOrMoreBuffers, queue_size_scales_with_client_performance)
 {
     //BufferQueue specific for now
-    auto discard = queue.scaling_delay();
+    int const discard = 3;
+    queue.set_scaling_delay(discard);
 
     for (int frame = 0; frame < 20; frame++)
     {
@@ -942,6 +945,7 @@
     }
     // Expect double-buffers as the steady state for fast clients
     auto log = producer.production_log();
+    ASSERT_THAT(log.size(), Gt(discard));  // avoid the below erase crashing
     log.erase(log.begin(), log.begin() + discard);
     EXPECT_THAT(unique_ids_in(log), Eq(2));
     producer.reset_log();

=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp	2015-07-22 02:54:31 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp	2015-07-27 07:47:45 +0000
@@ -1540,8 +1540,8 @@
 
     std::unordered_set<mg::Buffer *> buffers_acquired;
 
-    int const delay = q.scaling_delay();
-    EXPECT_EQ(3, delay);  // expect a sane default
+    int const delay = 3;
+    q.set_scaling_delay(delay);
 
     for (int frame = 0; frame < 10; frame++)
     {
@@ -1628,6 +1628,8 @@
     mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
     q.allow_framedropping(false);
 
+    q.set_scaling_delay(3);
+
     for (int frame = 0; frame < 10; frame++)
     {
         ASSERT_EQ(0, q.buffers_ready_for_compositor(this));
@@ -1636,7 +1638,7 @@
         // Detecting a slow client requires scheduling at least one extra
         // frame...
         int nready = q.buffers_ready_for_compositor(this);
-        ASSERT_EQ(2, nready);
+        ASSERT_THAT(nready, Ge(2));
         for (int i = 0; i < nready; ++i)
             q.compositor_release(q.compositor_acquire(this));
         ASSERT_EQ(0, q.buffers_ready_for_compositor(this));