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