Mir

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

Proposed by Daniel van Vugt on 2015-07-27
Status: Merged
Approved by: Daniel van Vugt on 2015-07-31
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 on 2015-07-31
Alan Griffiths 2015-07-27 Needs Information on 2015-07-27
PS Jenkins bot (community) continuous-integration Approve on 2015-07-27
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.
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
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);

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
1=== modified file 'src/server/compositor/buffer_queue.cpp'
2--- src/server/compositor/buffer_queue.cpp 2015-07-21 03:49:25 +0000
3+++ src/server/compositor/buffer_queue.cpp 2015-07-27 07:47:45 +0000
4@@ -137,7 +137,7 @@
5 graphics::BufferProperties const& props,
6 mc::FrameDroppingPolicyFactory const& policy_provider)
7 : nbuffers{nbuffers},
8- frame_deadlines_threshold{3},
9+ frame_deadlines_threshold{-1}, // Disable scaling by default
10 frame_deadlines_met{0},
11 frame_dropping_enabled{false},
12 current_compositor_buffer_valid{false},
13
14=== modified file 'tests/integration-tests/test_buffer_scheduling.cpp'
15--- tests/integration-tests/test_buffer_scheduling.cpp 2015-07-20 03:16:27 +0000
16+++ tests/integration-tests/test_buffer_scheduling.cpp 2015-07-27 07:47:45 +0000
17@@ -716,6 +716,8 @@
18
19 TEST_P(WithAnyNumberOfBuffers, compositor_inflates_ready_count_for_slow_clients)
20 {
21+ queue.set_scaling_delay(3);
22+
23 for (int frame = 0; frame < 10; frame++)
24 {
25 ASSERT_EQ(0, queue.buffers_ready_for_compositor(&consumer));
26@@ -724,7 +726,7 @@
27 // Detecting a slow client requires scheduling at least one extra
28 // frame...
29 int nready = queue.buffers_ready_for_compositor(&consumer);
30- ASSERT_EQ(2, nready);
31+ ASSERT_THAT(nready, Ge(2));
32 for (int i = 0; i < nready; ++i)
33 consumer.consume();
34 }
35@@ -933,7 +935,8 @@
36 TEST_P(WithThreeOrMoreBuffers, queue_size_scales_with_client_performance)
37 {
38 //BufferQueue specific for now
39- auto discard = queue.scaling_delay();
40+ int const discard = 3;
41+ queue.set_scaling_delay(discard);
42
43 for (int frame = 0; frame < 20; frame++)
44 {
45@@ -942,6 +945,7 @@
46 }
47 // Expect double-buffers as the steady state for fast clients
48 auto log = producer.production_log();
49+ ASSERT_THAT(log.size(), Gt(discard)); // avoid the below erase crashing
50 log.erase(log.begin(), log.begin() + discard);
51 EXPECT_THAT(unique_ids_in(log), Eq(2));
52 producer.reset_log();
53
54=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
55--- tests/unit-tests/compositor/test_buffer_queue.cpp 2015-07-22 02:54:31 +0000
56+++ tests/unit-tests/compositor/test_buffer_queue.cpp 2015-07-27 07:47:45 +0000
57@@ -1540,8 +1540,8 @@
58
59 std::unordered_set<mg::Buffer *> buffers_acquired;
60
61- int const delay = q.scaling_delay();
62- EXPECT_EQ(3, delay); // expect a sane default
63+ int const delay = 3;
64+ q.set_scaling_delay(delay);
65
66 for (int frame = 0; frame < 10; frame++)
67 {
68@@ -1628,6 +1628,8 @@
69 mc::BufferQueue q(nbuffers, allocator, basic_properties, policy_factory);
70 q.allow_framedropping(false);
71
72+ q.set_scaling_delay(3);
73+
74 for (int frame = 0; frame < 10; frame++)
75 {
76 ASSERT_EQ(0, q.buffers_ready_for_compositor(this));
77@@ -1636,7 +1638,7 @@
78 // Detecting a slow client requires scheduling at least one extra
79 // frame...
80 int nready = q.buffers_ready_for_compositor(this);
81- ASSERT_EQ(2, nready);
82+ ASSERT_THAT(nready, Ge(2));
83 for (int i = 0; i < nready; ++i)
84 q.compositor_release(q.compositor_acquire(this));
85 ASSERT_EQ(0, q.buffers_ready_for_compositor(this));

Subscribers

People subscribed via source and target branches