Merge lp:~vanvugt/mir/earlier-release-2 into lp:mir

[Daniel van Vugt]

Reintroduce very short buffer holds so that clients can get a new
buffer to render to much sooner, and potentially appear much
smoother (LP: #1480164)

"very short" means the compositor only holds the buffer for its
render time (often 1ms or less) rather than the full frame time
(16ms).

This also helps in future work to increase concurrency as the
compositor can now better control where in its render loop the
buffer reply will occur (LP: #1395421).

Compositor report render time results from a relatively fast desktop running 50 instances of egltriangle:

Trunk branch: 4.0ms per frame
This branch : 1.7ms per frame

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2890
http://jenkins.qa.ubuntu.com/job/mir-ci/4693/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3698
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/119
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2609
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3648
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/842
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/842/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3648
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3648/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6413
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22862

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

LGTM. I think that the tricky part (single vs multimonitor detection) comes from the fact that the MM code doesn't know how many user id's can exist, so we have to infer after-the-call (of compositor acquire). We could resolve if we handed out the ID's, and then we'd know for sure.

I think the term 'overclocking' is introducing another loaded word into the system... Its really designating a different consumption pattern with different tradeoffs. At any rate, I think I have to update the logic in the nbs multimonitor....

review: Approve

[Kevin DuBois (kdub)]

+1 to the logic, could the integration tests make more use of the scheduling system? There's some rigging already for blocking detection, and the tests will be slightly tricky as-is to refactor to get the new buffer semantics under this test.

[Daniel van Vugt (vanvugt)]

All good points and they were on my mind already.

It really is a prediction algorithm, whether there's a single or multiple compositors. If we get the prediction wrong for a frame or so (second monitor hotplugged?) the client will get a buffer back a little too quickly, but that's really just one frame so not important.

Yes "overclocking" is an overloaded word here. I was looking for a single term to describe the problem and that's the best short description I've found. But it's definitely confusing and not related to physical monitor signal overclocking, which is a thing.

The new integration tests - I've used your new interfaces previously where it was clear how to do so. Sorry in some of this work it's not clear to me how to do so. I imagine some translation will happen later...

[Alan Griffiths (alan-griffiths)]

71 + mtd::MockFrameDroppingPolicyFactory policy_factory;
72 + mc::BufferQueue queue{nbuffers, mt::fake_shared(server_buffer_factory),
73 + properties, policy_factory};
74 + queue.allow_framedropping(false);
75 +
76 + mg::Buffer* client_buffer = nullptr;
77 + auto callback = [&](mg::Buffer* buffer)
78 + {
79 + client_buffer = buffer;
80 + };
81 +
82 + auto client_try_acquire = [&]() -> bool
83 + {
84 + queue.client_acquire(callback);
85 + return client_buffer != nullptr;
86 + };

C++11 is shiny bit this is being too clever.

Why aren't queue, client_try_acquire etc. just members of a fixture?

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

To keep the diff small. Also it's temporary and will be replaced when BufferQueue goes away, so better to not bake it into a fixture.

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1825/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3779/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-vivid-amd64-build/200
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2690
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3729
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/378
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/378/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3729
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3729/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6464
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22942

review: Needs Fixing (continuous-integration)

=== modified file 'src/server/compositor/buffer_queue.cpp'
--- src/server/compositor/buffer_queue.cpp	2015-08-24 10:23:21 +0000
+++ src/server/compositor/buffer_queue.cpp	2015-08-27 10:34:15 +0000
@@ -144,6 +144,7 @@
       the_properties{props},
       force_new_compositor_buffer{false},
       callbacks_allowed{true},
+      single_compositor{false},  // When true we can optimize performance
       gralloc{gralloc}
 {
     if (nbuffers < 1)
@@ -286,6 +287,9 @@
         current_buffer_users.push_back(user_id);
     }
 
+    single_compositor = current_compositor_buffer_valid &&
+                        current_buffer_users.size() <= 1;  // might be zero
+
     if (ready_to_composite_queue.empty())
     {
         use_current_buffer = true;
@@ -357,6 +361,26 @@
 
     if (current_compositor_buffer != buffer.get())
         release(buffer.get(), std::move(lock));
+    else if (!ready_to_composite_queue.empty() && single_compositor)
+    {
+        /*
+         * The "early release" optimisation: Note "single_compositor" above
+         * is because this path will break (actually just overclock) the
+         * multi-monitor frame sync algorithm. For the moment we prefer
+         * /perfect/ multi-monitor frame sync all the time. But if you so
+         * choose that overclocking with multi-monitors is acceptable then
+         * you could remove the above "single_compositor" check and get this
+         * optimised code path with multi-monitors too...
+         */
+        current_compositor_buffer = pop(ready_to_composite_queue);
+        current_buffer_users.clear();
+        /*
+         * As we have now caused the next compositor_acquire() to skip the
+         * frame_deadlines_met update, we need to do it here:
+         */
+        ++frame_deadlines_met;
+        release(buffer.get(), std::move(lock));
+    }
 }
 
 std::shared_ptr<mg::Buffer> mc::BufferQueue::snapshot_acquire()

=== modified file 'src/server/compositor/buffer_queue.h'
--- src/server/compositor/buffer_queue.h	2015-08-21 02:02:07 +0000
+++ src/server/compositor/buffer_queue.h	2015-08-27 10:34:15 +0000
@@ -114,6 +114,7 @@
     graphics::BufferProperties the_properties;
     bool force_new_compositor_buffer;
     bool callbacks_allowed;
+    bool single_compositor;
 
     std::condition_variable snapshot_released;
     std::shared_ptr<graphics::GraphicBufferAllocator> gralloc;

=== modified file 'tests/integration-tests/test_buffer_scheduling.cpp'
--- tests/integration-tests/test_buffer_scheduling.cpp	2015-08-26 09:20:44 +0000
+++ tests/integration-tests/test_buffer_scheduling.cpp	2015-08-27 10:34:15 +0000
@@ -714,6 +714,121 @@
     }
 }
 
+TEST_P(WithTwoOrMoreBuffers, clients_get_new_buffers_on_compositor_release)
+{   // Regression test for LP: #1480164
+    mtd::MockFrameDroppingPolicyFactory policy_factory;
+    mc::BufferQueue queue{nbuffers, mt::fake_shared(server_buffer_factory),
+                          properties, policy_factory};
+    queue.allow_framedropping(false);
+
+    mg::Buffer* client_buffer = nullptr;
+    auto callback = [&](mg::Buffer* buffer)
+    {
+        client_buffer = buffer;
+    };
+
+    auto client_try_acquire = [&]() -> bool
+    {
+        queue.client_acquire(callback);
+        return client_buffer != nullptr;
+    };
+
+    auto client_release = [&]()
+    {
+        EXPECT_TRUE(client_buffer);
+        queue.client_release(client_buffer);
+        client_buffer = nullptr;
+    };
+
+    // Skip over the first frame. The early release optimization is too
+    // conservative to allow it to happen right at the start (so as to
+    // maintain correct multimonitor frame rates if required).
+    ASSERT_TRUE(client_try_acquire());
+    client_release();
+    queue.compositor_release(queue.compositor_acquire(this));
+
+    auto onscreen = queue.compositor_acquire(this);
+
+    bool blocking;
+    do
+    {
+        blocking = !client_try_acquire();
+        if (!blocking)
+            client_release();
+    } while (!blocking);
+
+    for (int f = 0; f < 100; ++f)
+    {
+        ASSERT_FALSE(client_buffer);
+        queue.compositor_release(onscreen);
+        ASSERT_TRUE(client_buffer) << "frame# " << f;
+        client_release();
+        onscreen = queue.compositor_acquire(this);
+        client_try_acquire();
+    }
+}
+
+TEST_P(WithTwoOrMoreBuffers, short_buffer_holds_dont_overclock_multimonitor)
+{   // Regression test related to LP: #1480164
+    mtd::MockFrameDroppingPolicyFactory policy_factory;
+    mc::BufferQueue queue{nbuffers, mt::fake_shared(server_buffer_factory),
+                          properties, policy_factory};
+    queue.allow_framedropping(false);
+
+    const void* const leftid = "left";
+    const void* const rightid = "right";
+
+    mg::Buffer* client_buffer = nullptr;
+    auto callback = [&](mg::Buffer* buffer)
+    {
+        client_buffer = buffer;
+    };
+
+    auto client_try_acquire = [&]() -> bool
+    {
+        queue.client_acquire(callback);
+        return client_buffer != nullptr;
+    };
+
+    auto client_release = [&]()
+    {
+        EXPECT_TRUE(client_buffer);
+        queue.client_release(client_buffer);
+        client_buffer = nullptr;
+    };
+
+    // Skip over the first frame. The early release optimization is too
+    // conservative to allow it to happen right at the start (so as to
+    // maintain correct multimonitor frame rates if required).
+    ASSERT_TRUE(client_try_acquire());
+    client_release();
+    queue.compositor_release(queue.compositor_acquire(leftid));
+
+    auto left = queue.compositor_acquire(leftid);
+    auto right = queue.compositor_acquire(rightid);
+
+    bool blocking;
+    do
+    {
+        blocking = !client_try_acquire();
+        if (!blocking)
+            client_release();
+    } while (!blocking);
+
+    for (int f = 0; f < 100; ++f)
+    {
+        ASSERT_FALSE(client_buffer);
+        queue.compositor_release(left);
+        queue.compositor_release(right);
+        ASSERT_FALSE(client_buffer);
+        left = queue.compositor_acquire(leftid);
+        right = queue.compositor_acquire(rightid);
+        ASSERT_TRUE(client_buffer);
+        client_release();
+        client_try_acquire();
+    }
+}
+
 TEST_P(WithAnyNumberOfBuffers, compositor_inflates_ready_count_for_slow_clients)
 {
     queue.set_scaling_delay(3);

=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp	2015-08-24 10:22:40 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp	2015-08-27 10:34:15 +0000
@@ -477,6 +477,118 @@
     EXPECT_NO_THROW(q.compositor_release(comp_buffer));
 }
 
+TEST_P(WithTwoOrMoreBuffers, clients_get_new_buffers_on_compositor_release)
+{   // Regression test for LP: #1480164
+    q.allow_framedropping(false);
+
+    // Skip over the first frame. The early release optimization is too
+    // conservative to allow it to happen right at the start (so as to
+    // maintain correct multimonitor frame rates if required).
+    auto handle = client_acquire_async(q);
+    ASSERT_TRUE(handle->has_acquired_buffer());
+    handle->release_buffer();
+    q.compositor_release(q.compositor_acquire(this));
+
+    auto onscreen = q.compositor_acquire(this);
+
+    // This is what tests should do instead of using buffers_free_for_client()
+    bool blocking;
+    do
+    {
+        handle = client_acquire_async(q);
+        blocking = !handle->has_acquired_buffer();
+        if (!blocking)
+            handle->release_buffer();
+    } while (!blocking);
+
+    for (int f = 0; f < 100; ++f)
+    {
+        ASSERT_FALSE(handle->has_acquired_buffer());
+        q.compositor_release(onscreen);
+        ASSERT_TRUE(handle->has_acquired_buffer()) << "frame# " << f;
+        handle->release_buffer();
+        onscreen = q.compositor_acquire(this);
+        handle = client_acquire_async(q);
+    }
+}
+
+TEST_P(WithTwoOrMoreBuffers, short_buffer_holds_dont_overclock_multimonitor)
+{   // Regression test related to LP: #1480164
+    q.allow_framedropping(false);
+
+    // Skip over the first frame. The early release optimization is too
+    // conservative to allow it to happen right at the start (so as to
+    // maintain correct multimonitor frame rates if required).
+    auto handle = client_acquire_async(q);
+    ASSERT_TRUE(handle->has_acquired_buffer());
+    handle->release_buffer();
+
+    const void* const leftid = "left";
+    const void* const rightid = "right";
+    auto left = q.compositor_acquire(leftid);
+    q.compositor_release(left);
+    left = q.compositor_acquire(leftid);
+    auto right = q.compositor_acquire(rightid);
+
+    // This is what tests should do instead of using buffers_free_for_client()
+    bool blocking;
+    do
+    {
+        handle = client_acquire_async(q);
+        blocking = !handle->has_acquired_buffer();
+        if (!blocking)
+            handle->release_buffer();
+    } while (!blocking);
+
+    for (int f = 0; f < 100; ++f)
+    {
+        ASSERT_FALSE(handle->has_acquired_buffer());
+        q.compositor_release(left);
+        q.compositor_release(right);
+        ASSERT_FALSE(handle->has_acquired_buffer());
+        left = q.compositor_acquire(leftid);
+        right = q.compositor_acquire(rightid);
+        ASSERT_TRUE(handle->has_acquired_buffer());
+        handle->release_buffer();
+        handle = client_acquire_async(q);
+    }
+}
+
+TEST_P(WithThreeOrMoreBuffers, greedy_clients_get_new_buffers_on_compositor_release)
+{   // Regression test for LP: #1480164
+    q.allow_framedropping(false);
+
+    // Skip over the first frame. The early release optimization is too
+    // conservative to allow it to happen right at the start (so as to
+    // maintain correct multimonitor frame rates if required).
+    auto handle = client_acquire_async(q);
+    ASSERT_TRUE(handle->has_acquired_buffer());
+    handle->release_buffer();
+    q.compositor_release(q.compositor_acquire(this));
+
+    auto onscreen = q.compositor_acquire(this);
+    auto old_handle = handle;
+    old_handle.reset();
+    bool blocking;
+    do
+    {
+        handle = client_acquire_async(q);
+        blocking = !handle->has_acquired_buffer();
+        if (!blocking)
+        {
+            if (old_handle)
+                old_handle->release_buffer();
+            old_handle = handle;
+            handle.reset();
+        }
+    } while (!blocking);
+
+    ASSERT_TRUE(old_handle->has_acquired_buffer());
+    ASSERT_FALSE(handle->has_acquired_buffer());
+    q.compositor_release(onscreen);
+    ASSERT_TRUE(handle->has_acquired_buffer());
+}
+
 TEST_P(WithAnyNumberOfBuffers, multiple_compositors_are_in_sync)
 {
     auto handle = client_acquire_async(q);