Merge lp:~albaguirre/mir/fix-not-compositing-last-client-posted-buffer into lp:mir

[Alberto Aguirre]

Fix not compositing the client's last posted buffer (LP: #1294048, LP: #1294051, LP: #1294053, LP: #1290306, LP: #1293896)

In single-display cases the number of ready buffers decreases after a buffer is acquired by the rendering operator.
Determine if there will be uncomposited buffers before acquiring a buffer so it works for single and multi display use cases.

Fix not compositing the client's last posted buffer (LP: #1294048, LP: #1294051, LP: #1294053, LP: #1290306, LP: #1293896)

In single-display cases the number of ready buffers decreases after a buffer is acquired by the rendering operator.
Determine if there will be uncomposited buffers before acquiring a buffer so it works for single and multi display use cases.

[Alan Griffiths (alan-griffiths)]

Great!

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1480
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1098/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1278
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1276
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/859
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/830/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/835
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/835/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/860
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/860/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/802
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4941

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

this might conflict with mir rev 1480 too

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1481
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1102/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1285
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1283
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/865
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/834
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/834/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/839
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/839/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/866
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/866/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/808
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4952

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Nice catch.

I think it would be clearer to keep the statement at the end of the function and just change it from:
   uncomposited_buffers_ |= renderable.buffers_ready_for_compositor() > 1;
to:
   uncomposited_buffers_ |= renderable.buffers_ready_for_compositor() > 0;

Because I supposedly know how this stuff works and even I was tripped up when I first saw "> 1".

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

I approve either solution because it's a critical bug. But I would prefer to see the "> 0" form.

review: Approve

[Alan Griffiths (alan-griffiths)]

> I approve either solution because it's a critical bug. But I would prefer to
> see the "> 0" form.

I arrived at the original form after checking the behavior on a multi-monitor setup. In that scenario buffers_ready_for_compositor() usually doesn't change between the start and end of this function. (When it does we see bug 290306.)

Hence I don't think that > 0 works in multi-montor setups.

I agree it is confusing that nready is updated differently in single and multi-montor configurations.

[Daniel van Vugt (vanvugt)]

Sounds like we might be racing compositor threads in using "buffers_ready_for_compositor". But I'm not yet sure how.

[Alan Griffiths (alan-griffiths)]

> Sounds like we might be racing compositor threads in using
> "buffers_ready_for_compositor". But I'm not yet sure how.

The race that leads to bug 290306 is that in multi-monitor the client thread can update nready after the composite and before the (original) call to buffers_ready_for_compositor(). Moving the call avoids this and AFAICS give correct results on both single and multi-montor setups.

[Daniel van Vugt (vanvugt)]

My concern is that the result of buffers_ready_for_compositor() changes depending on how many threads/monitors there are. This is dangerous unless what you're suggesting has somehow resolved that danger. I'm not sure.

[Daniel van Vugt (vanvugt)]

Incidentally, I'm working on a separate proposal that would resolve the raciness (if any). So we don't need to block on it. Just be concerned until proven otherwise.

[Alan Griffiths (alan-griffiths)]

The worst case with this proposal is that:

1. compositor A renders the first of two ready buffers
2. compositor B sees "2" and sets uncomposited_buffers_
3. the client takes the buffer and nready is decremented
4. compositor B composites the second buffer

For compositor B uncomposited_buffers_ is now set unnecessarily and an unnecessary composite pass happens. Not perfect, but not a serious problem.

[Alan Griffiths (alan-griffiths)]

Top approving as Daniel and I are probably the most familiar with this code and we've both decided its good enough. (And it is already -r 1181 on trunk)

=== modified file 'src/server/compositor/rendering_operator.cpp'
--- src/server/compositor/rendering_operator.cpp	2014-03-13 00:13:17 +0000
+++ src/server/compositor/rendering_operator.cpp	2014-03-19 00:23:05 +0000
@@ -33,11 +33,11 @@
 
 void mc::RenderingOperator::operator()(graphics::Renderable const& renderable)
 {
+    uncomposited_buffers_ |= renderable.buffers_ready_for_compositor() > 1;
     auto compositor_buffer = renderable.buffer(frameno);
     // preserves buffers used in rendering until after post_update()
     saved_resources.push_back(compositor_buffer);
     renderer.render(renderable, *compositor_buffer);
-    uncomposited_buffers_ |= renderable.buffers_ready_for_compositor() > 1;
 }
 
 bool mc::RenderingOperator::uncomposited_buffers() const