Merge lp:~albaguirre/mir/fix-1321861 into lp:mir

[Alberto Aguirre]

BufferQueue: return correct number in buffers_ready_for_compositor (LP:#1321861)

BufferQueue: fix buffers_ready_for_compositor (LP:#1321861)

Since once buffer is always ready, buffers_ready_for_compositor should return ready buffers + 1.
This fixes regression that was masked before by the buffer consuming compositor (now gone) since it held buffers long enough which delayed the consumption of the ready buffer queue.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1650
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1698/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/287
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/288
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/287
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/217
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/217/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/216
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/216/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/766
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/766/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1533
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/7478

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/1698/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Raising the minimum to one buffer always-ready in theory should make the compositor spin and continuously re-composite even when nothing is changing. That would be a regression of bug 1108725, although that's not happening due to this:

    for(auto const& renderable : renderable_list)
        uncomposited_buffers |= (renderable->buffers_ready_for_compositor() > 1);

which I think should be "> 0" anyway, and moved to the bottom of DefaultDisplayBufferCompositor::composite().

So, to make the logic easier to follow and ensure DefaultDisplayBufferCompositor doesn't need to know that BufferQueue is always lying, I think we should keep BufferQueue unmodified and fix DefaultDisplayBufferCompositor instead.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Actually I ran into another related issue recently where BufferQueue's restriction of always allocating one buffer minimum to compositing was reducing pipelining and triggering failures of a performance test. I wonder if that's something we need to revisit and perhaps allow zero compositors again, like SwitchingBundle used to.

[Alberto Aguirre (albaguirre)]

I'm fine with either one. So team let me know what you'd prefer.

It looks like buffers_ready_for_compositor will soon be removed in any case.

[Kevin DuBois (kdub)]

> Raising the minimum to one buffer always-ready in theory should make the
> compositor spin and continuously re-composite even when nothing is changing.
> That would be a regression of bug 1108725, although that's not happening due
> to this:
>
> for(auto const& renderable : renderable_list)
> uncomposited_buffers |= (renderable->buffers_ready_for_compositor() >
> 1);
>
> which I think should be "> 0" anyway, and moved to the bottom of
> DefaultDisplayBufferCompositor::composite().
>
> So, to make the logic easier to follow and ensure
> DefaultDisplayBufferCompositor doesn't need to know that BufferQueue is always
> lying, I think we should keep BufferQueue unmodified and fix
> DefaultDisplayBufferCompositor instead.

I was actually looking into this for the purposes of making DefaultDisplayBufferCompositor simpler, and I agree the logic on the compositor-system is misplaced. I think that we should shift it to MultiThreadedCompositor, which is the class that best can arrange the compositor's scheduling.

[Daniel van Vugt (vanvugt)]

I think returning the wrong answer from buffers_ready_for_compositor() just because the caller only works with a *wrong* answer is the wrong answer :)

Fix the caller instead, so we don't risk regressing on bug 1108725.

Unmerged revisions

1650. By Alberto Aguirre on 2014-05-21

check buffers_ready_for_compositor goes down after compositor_acquire

1649. By Alberto Aguirre on 2014-05-21

buffers_ready_for_compositor should returns at least 1.

A buffer is always available for compositing so return ready buffers + 1.

1648. By Alberto Aguirre on 2014-05-21

test: add check for buffers_ready_for_compositor

=== modified file 'src/server/compositor/buffer_queue.cpp'
--- src/server/compositor/buffer_queue.cpp	2014-05-20 23:21:10 +0000
+++ src/server/compositor/buffer_queue.cpp	2014-05-21 18:41:20 +0000
@@ -364,7 +364,7 @@
      * as the number of buffers that are truly ready
      * vary depending on concurrent compositors.
      */
-    return ready_to_composite_queue.size();
+    return ready_to_composite_queue.size() + 1;
 }
 
 void mc::BufferQueue::give_buffer_to_client(

=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp	2014-05-20 23:21:10 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp	2014-05-21 18:41:20 +0000
@@ -1404,6 +1404,23 @@
     }
 }
 
+TEST_F(BufferQueueTest, buffers_ready_for_compositor_returns_at_least_one)
+{
+    mc::BufferQueue q(3, allocator, basic_properties, policy_factory);
+
+    /* There's always one buffer ready for a compositor */
+    EXPECT_THAT(q.buffers_ready_for_compositor(), Eq(1));
+
+    auto handle = client_acquire_async(q);
+    ASSERT_THAT(handle->has_acquired_buffer(), Eq(true));
+    handle->release_buffer();
+
+    EXPECT_THAT(q.buffers_ready_for_compositor(), Eq(2));
+
+    auto buffer = q.compositor_acquire(this);
+    EXPECT_THAT(q.buffers_ready_for_compositor(), Eq(1));
+}
+
 /* FIXME (enabling this optimization breaks timing tests) */
 TEST_F(BufferQueueTest, DISABLED_synchronous_clients_only_get_two_real_buffers)
 {