Mir

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

Proposed by Daniel van Vugt
Status: Merged
Approved by: Daniel van Vugt
Approved revision: 2782
Merged at revision: 2799
Proposed branch: lp:~vanvugt/mir/always-triple-buffers
Merge into: lp:mir
Diff against target: 85 lines (+12/-6)
3 files modified
src/server/compositor/buffer_queue.cpp (+1/-1)
tests/integration-tests/test_buffer_scheduling.cpp (+6/-2)
tests/unit-tests/compositor/test_buffer_queue.cpp (+5/-3)
To merge this branch: bzr merge lp:~vanvugt/mir/always-triple-buffers
Reviewer Review Type Date Requested Status
Alberto Aguirre (community) Approve
Alan Griffiths Needs Information
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+265935@code.launchpad.net

Commit message

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.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

*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
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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);

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
137 graphics::BufferProperties const& props,137 graphics::BufferProperties const& props,
138 mc::FrameDroppingPolicyFactory const& policy_provider)138 mc::FrameDroppingPolicyFactory const& policy_provider)
139 : nbuffers{nbuffers},139 : nbuffers{nbuffers},
140 frame_deadlines_threshold{3},140 frame_deadlines_threshold{-1}, // Disable scaling by default
141 frame_deadlines_met{0},141 frame_deadlines_met{0},
142 frame_dropping_enabled{false},142 frame_dropping_enabled{false},
143 current_compositor_buffer_valid{false},143 current_compositor_buffer_valid{false},
144144
=== 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 @@
716716
717TEST_P(WithAnyNumberOfBuffers, compositor_inflates_ready_count_for_slow_clients)717TEST_P(WithAnyNumberOfBuffers, compositor_inflates_ready_count_for_slow_clients)
718{718{
719 queue.set_scaling_delay(3);
720
719 for (int frame = 0; frame < 10; frame++)721 for (int frame = 0; frame < 10; frame++)
720 {722 {
721 ASSERT_EQ(0, queue.buffers_ready_for_compositor(&consumer));723 ASSERT_EQ(0, queue.buffers_ready_for_compositor(&consumer));
@@ -724,7 +726,7 @@
724 // Detecting a slow client requires scheduling at least one extra726 // Detecting a slow client requires scheduling at least one extra
725 // frame...727 // frame...
726 int nready = queue.buffers_ready_for_compositor(&consumer);728 int nready = queue.buffers_ready_for_compositor(&consumer);
727 ASSERT_EQ(2, nready);729 ASSERT_THAT(nready, Ge(2));
728 for (int i = 0; i < nready; ++i)730 for (int i = 0; i < nready; ++i)
729 consumer.consume();731 consumer.consume();
730 }732 }
@@ -933,7 +935,8 @@
933TEST_P(WithThreeOrMoreBuffers, queue_size_scales_with_client_performance)935TEST_P(WithThreeOrMoreBuffers, queue_size_scales_with_client_performance)
934{936{
935 //BufferQueue specific for now937 //BufferQueue specific for now
936 auto discard = queue.scaling_delay();938 int const discard = 3;
939 queue.set_scaling_delay(discard);
937940
938 for (int frame = 0; frame < 20; frame++)941 for (int frame = 0; frame < 20; frame++)
939 {942 {
@@ -942,6 +945,7 @@
942 }945 }
943 // Expect double-buffers as the steady state for fast clients946 // Expect double-buffers as the steady state for fast clients
944 auto log = producer.production_log();947 auto log = producer.production_log();
948 ASSERT_THAT(log.size(), Gt(discard)); // avoid the below erase crashing
945 log.erase(log.begin(), log.begin() + discard);949 log.erase(log.begin(), log.begin() + discard);
946 EXPECT_THAT(unique_ids_in(log), Eq(2));950 EXPECT_THAT(unique_ids_in(log), Eq(2));
947 producer.reset_log();951 producer.reset_log();
948952
=== 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 @@
15401540
1541 std::unordered_set<mg::Buffer *> buffers_acquired;1541 std::unordered_set<mg::Buffer *> buffers_acquired;
15421542
1543 int const delay = q.scaling_delay();1543 int const delay = 3;
1544 EXPECT_EQ(3, delay); // expect a sane default1544 q.set_scaling_delay(delay);
15451545
1546 for (int frame = 0; frame < 10; frame++)1546 for (int frame = 0; frame < 10; frame++)
1547 {1547 {
@@ -1628,6 +1628,8 @@
1628 mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);1628 mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
1629 q.allow_framedropping(false);1629 q.allow_framedropping(false);
16301630
1631 q.set_scaling_delay(3);
1632
1631 for (int frame = 0; frame < 10; frame++)1633 for (int frame = 0; frame < 10; frame++)
1632 {1634 {
1633 ASSERT_EQ(0, q.buffers_ready_for_compositor(this));1635 ASSERT_EQ(0, q.buffers_ready_for_compositor(this));
@@ -1636,7 +1638,7 @@
1636 // Detecting a slow client requires scheduling at least one extra1638 // Detecting a slow client requires scheduling at least one extra
1637 // frame...1639 // frame...
1638 int nready = q.buffers_ready_for_compositor(this);1640 int nready = q.buffers_ready_for_compositor(this);
1639 ASSERT_EQ(2, nready);1641 ASSERT_THAT(nready, Ge(2));
1640 for (int i = 0; i < nready; ++i)1642 for (int i = 0; i < nready; ++i)
1641 q.compositor_release(q.compositor_acquire(this));1643 q.compositor_release(q.compositor_acquire(this));
1642 ASSERT_EQ(0, q.buffers_ready_for_compositor(this));1644 ASSERT_EQ(0, q.buffers_ready_for_compositor(this));

Subscribers

People subscribed via source and target branches