Merge lp:~kdub/mir/rendering-operator-even-simpler into lp:mir

[Kevin DuBois]

compositor: now that we have a filtered list of renderables we want to draw, directly access the information needed to implement a frig instead of burdening the RenderingOperator to provide it

compositor: now that we have a filtered list of renderables we want to draw, directly access the information needed to implement a frig instead of burdening the RenderingOperator to provide it

I'm taking aim at eliminating RenderingOperator altogether in favor of just rendering the list from generate_renderables_list. The code eliminated from RenderingOperator in this MP moves frig information gathering to a more appropriate place.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1525
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1182/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1396/console
    None: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1394/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/976/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/914/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/919/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/977/console

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

review: Needs Fixing (continuous-integration)

[Kevin DuBois (kdub)]

^^cancelled in-flight test run because there was a merge conflict

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1526
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1183/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1397
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1395
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/977
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/915
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/915/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/920
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/920/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/978
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/978/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/911
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5277

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

(1) You could optimize and break this loop early when the answer becomes true:
9 + for(auto const& renderable : renderable_list)
10 + uncomposited_buffers |= (renderable->buffers_ready_for_compositor() > 1);

(2) In the coming step(s) please remember to pass the Renderer the whole list and let it do the iteration. This is necessary for non-linear and multi-pass effects like 3D shadow grouping (see how your Unity panel shadow merges with window shadows). Another example is multi-pass rendering like reflections and live previews in workspace switchers. Both involve painting the same surfaces multiple times (transformed differently) per frame. So the renderer needs the whole list to work with.

I don't understand the "frig" in this code but it seems enough of the team does :)

review: Approve

[Alexandros Frantzis (afrantzis)]

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

We are essentially moving the "uncomposited_buffers |= ..." operation after mc::RenderingOperator::operator(). This was the cause of various issues and was fixed by https://code.launchpad.net/~albaguirre/mir/fix-not-compositing-client-last-posted-buffer-trunk/+merge/211601 . Has this problem been dealt with in another way?

review: Needs Fixing

[Kevin DuBois (kdub)]

> 9 + for(auto const& renderable : renderable_list)
> 10 + uncomposited_buffers |= (renderable->buffers_ready_for_compositor()
> > 1);
>
> We are essentially moving the "uncomposited_buffers |= ..." operation after
> mc::RenderingOperator::operator(). This was the cause of various issues and
> was fixed by https://code.launchpad.net/~albaguirre/mir/fix-not-compositing-
> client-last-posted-buffer-trunk/+merge/211601 . Has this problem been dealt
> with in another way?

good catch, will correct.

Its concerning that we don't have a test for this (especially because its not obvious what this is doing)

[Alan Griffiths (alan-griffiths)]

> Its concerning that we don't have a test for this (especially because its not
> obvious what this is doing)

Agreed. This was panic fixed during last push to trunk and a test is missing.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1529
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1191/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1408
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1406
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/988
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/923
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/923/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/928
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/928/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/989
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/989/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/921
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5322

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

+ TEST_F(DefaultDisplayBufferCompositor, decides_whether_to_recomposite_before_rendering)

I'd rather go with pure state verification vs behavior verification for this. The reason is that the current ordering of operations is an implementation detail of the current solution, which is one of possible solutions. For example, if we change the condition to renderable->buffers_ready_for_compositor() >= 1 we will need to change the ordering.

I think a test like "informs_when_recomposition_is_needed", that only checks whether composite() returns correct values would be more appropriate. Granted that the current test does that, but its overall focus, as stated in the name too, is on checking the implementation rather than the functionality.

A weak "needs fixing".

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> + TEST_F(DefaultDisplayBufferCompositor,
> decides_whether_to_recomposite_before_rendering)
>
> I'd rather go with pure state verification vs behavior verification for this.
> The reason is that the current ordering of operations is an implementation
> detail of the current solution, which is one of possible solutions. For
> example, if we change the condition to
> renderable->buffers_ready_for_compositor() >= 1 we will need to change the
> ordering.
>
> I think a test like "informs_when_recomposition_is_needed", that only checks
> whether composite() returns correct values would be more appropriate. Granted
> that the current test does that, but its overall focus, as stated in the name
> too, is on checking the implementation rather than the functionality.
>
> A weak "needs fixing".

This test isn't entirely satisfactory and nor is the solution it validated. However, it is hard to design something better and I think this is a reasonable solution.

While you say the ordering is a detail, it is a detail required by the implicit guarantees offered by SwitchingBundle: it is indeterminate (for the compositor) whether a recently composited buffer is included in buffers_ready_for_compositor(). Asking before compositing the buffer might result in an spurious composite, but doesn't skip a required one.

review: Approve

[Alexandros Frantzis (afrantzis)]

> While you say the ordering is a detail, it is a detail required by the implicit guarantees offered by SwitchingBundle:

I am trying to wrap my head around this but I still haven't able to do so, lost in all the intricate interactions of SwitchingBundle, not convinced one way or the other.

If we indeed depend on such fragile interactions, it's all the more reason to have a test that provides a scenario that fails when internally we don't follow the strict ordering imposed on us, instead of a test that only validates that we do follow an ordering without "proving" that if we don't follow it there is going to be trouble. For example, changing the condition to >= 1 and moving the snippet after post_update() in composite() still works after amending the order of mock_renderable expectations.

Anyway, the test is just tangential to this MP, and is good to have one, so OK. However, I am puzzled by some of what I have seen and I want to look deeper.

review: Approve

[Kevin DuBois (kdub)]

I agree that the way we determine whether to recomposite is pretty fragile. We have a signal which tells compositing us to composite, and then we tell a function to composite, while at the same time asking that function if it thinks that we should recomposite again. I'd like to mop that up when we improve the scene/compositor composition signal.

At any rate, at least we have a test that will catch us when we move this bug-fixing nugget of code around unknowningly.

[Kevin DuBois (kdub)]

oops, topapproved, but forgot this was dependent on another branch. switching back to 'needs review'

[Alan Griffiths (alan-griffiths)]

> oops, topapproved, but forgot this was dependent on another branch. switching
> back to 'needs review'

Prerequisite has landed - top-approving

=== modified file 'include/test/mir_test_doubles/mock_renderable.h'
--- include/test/mir_test_doubles/mock_renderable.h	2014-03-26 05:48:59 +0000
+++ include/test/mir_test_doubles/mock_renderable.h	2014-03-27 17:30:53 +0000
@@ -37,7 +37,16 @@
             .WillByDefault(testing::Return(geometry::Rectangle{{},{}}));
         ON_CALL(*this, buffer(testing::_))
             .WillByDefault(testing::Return(std::make_shared<StubBuffer>()));
+        ON_CALL(*this, buffers_ready_for_compositor())
+            .WillByDefault(testing::Return(1));
+        ON_CALL(*this, alpha())
+            .WillByDefault(testing::Return(1.0f));
+        ON_CALL(*this, transformation())
+            .WillByDefault(testing::Return(glm::mat4{}));
+        ON_CALL(*this, should_be_rendered_in(testing::_))
+            .WillByDefault(testing::Return(true));
     }
+
     MOCK_CONST_METHOD1(buffer, std::shared_ptr<graphics::Buffer>(void const*));
     MOCK_CONST_METHOD0(alpha_enabled, bool());
     MOCK_CONST_METHOD0(screen_position, geometry::Rectangle());
@@ -45,7 +54,7 @@
     MOCK_CONST_METHOD0(transformation, glm::mat4());
     MOCK_CONST_METHOD1(should_be_rendered_in, bool(geometry::Rectangle const& rect));
     MOCK_CONST_METHOD0(shaped, bool());
-    int buffers_ready_for_compositor() const override { return 1; }
+    MOCK_CONST_METHOD0(buffers_ready_for_compositor, int());
 };
 }
 }

=== modified file 'src/server/compositor/default_display_buffer_compositor.cpp'
--- src/server/compositor/default_display_buffer_compositor.cpp	2014-03-27 17:30:53 +0000
+++ src/server/compositor/default_display_buffer_compositor.cpp	2014-03-27 17:30:53 +0000
@@ -129,6 +129,10 @@
         auto const& view_area = display_buffer.view_area();
         auto renderable_list = scene->generate_renderable_list();
         mc::filter_occlusions_from(renderable_list, view_area);
+
+        for(auto const& renderable : renderable_list)
+            uncomposited_buffers |= (renderable->buffers_ready_for_compositor() > 1);
+
         renderer->set_rotation(display_buffer.orientation());
         renderer->begin();
         mc::RenderingOperator applicator(*renderer);
@@ -138,15 +142,11 @@
 
         display_buffer.post_update();
 
-        uncomposited_buffers |= applicator.uncomposited_buffers();
-
         // This is a frig to avoid lp:1286190
-        if (last_pass_rendered_anything && !applicator.anything_was_rendered())
-        {
+        if (last_pass_rendered_anything && renderable_list.empty())
             uncomposited_buffers = true;
-        }
 
-        last_pass_rendered_anything = applicator.anything_was_rendered();
+        last_pass_rendered_anything = !renderable_list.empty();
         // End of frig
     }
 

=== modified file 'src/server/compositor/rendering_operator.cpp'
--- src/server/compositor/rendering_operator.cpp	2014-03-26 05:48:59 +0000
+++ src/server/compositor/rendering_operator.cpp	2014-03-27 17:30:53 +0000
@@ -24,26 +24,14 @@
 
 mc::RenderingOperator::RenderingOperator(
     Renderer& renderer)
-    : renderer(renderer),
-      uncomposited_buffers_{false}
+    : renderer(renderer)
 {
 }
 
 void mc::RenderingOperator::operator()(graphics::Renderable const& renderable)
 {
-    uncomposited_buffers_ |= renderable.buffers_ready_for_compositor() > 1;
     auto compositor_buffer = renderable.buffer(&renderer);
     // preserves buffers used in rendering until after post_update()
     saved_resources.push_back(compositor_buffer);
     renderer.render(renderable, *compositor_buffer);
 }
-
-bool mc::RenderingOperator::uncomposited_buffers() const
-{
-    return uncomposited_buffers_;
-}
-
-bool mc::RenderingOperator::anything_was_rendered() const
-{
-    return !saved_resources.empty();
-}

=== modified file 'src/server/compositor/rendering_operator.h'
--- src/server/compositor/rendering_operator.h	2014-03-26 05:48:59 +0000
+++ src/server/compositor/rendering_operator.h	2014-03-27 17:30:53 +0000
@@ -38,13 +38,10 @@
     ~RenderingOperator() = default;
 
     void operator()(graphics::Renderable const&);
-    bool uncomposited_buffers() const;
-    bool anything_was_rendered() const;
 
 private:
     Renderer& renderer;
     std::vector<std::shared_ptr<void>> saved_resources;
-    bool uncomposited_buffers_;
 };
 
 }

=== modified file 'tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp'
--- tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2014-03-27 17:30:53 +0000
+++ tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2014-03-27 17:30:53 +0000
@@ -136,9 +136,9 @@
 {
     using namespace testing;
     mtd::NullDisplayBuffer display_buffer;
-    auto mock_renderable1 = std::make_shared<mtd::MockRenderable>();
-    auto mock_renderable2 = std::make_shared<mtd::MockRenderable>();
-    auto mock_renderable3 = std::make_shared<mtd::MockRenderable>();
+    auto mock_renderable1 = std::make_shared<NiceMock<mtd::MockRenderable>>();
+    auto mock_renderable2 = std::make_shared<NiceMock<mtd::MockRenderable>>();
+    auto mock_renderable3 = std::make_shared<NiceMock<mtd::MockRenderable>>();
 
     auto buf = std::make_shared<mtd::StubBuffer>();
     EXPECT_CALL(*mock_renderable1, buffer(_))
@@ -513,3 +513,44 @@
         mr::null_compositor_report());
     compositor.composite();
 }
+
+//test associated with lp:1290306, 1293896, 1294048, 1294051, 1294053
+TEST_F(DefaultDisplayBufferCompositor, decides_whether_to_recomposite_before_rendering)
+{
+    using namespace testing;
+    ON_CALL(display_buffer, view_area())
+        .WillByDefault(Return(screen));
+    ON_CALL(display_buffer, orientation())
+        .WillByDefault(Return(mir_orientation_normal));
+    ON_CALL(display_buffer, can_bypass())
+        .WillByDefault(Return(false));
+
+    auto mock_renderable = std::make_shared<NiceMock<mtd::MockRenderable>>();
+    ON_CALL(*mock_renderable, screen_position())
+        .WillByDefault(Return(geom::Rectangle{{0,0},{200,200}})); 
+
+    //check for how many buffers should come before accessing the buffers.
+    Sequence seq;
+    EXPECT_CALL(*mock_renderable, buffers_ready_for_compositor())
+        .InSequence(seq)
+        .WillOnce(Return(2));
+    EXPECT_CALL(*mock_renderable, buffer(_))
+        .InSequence(seq); 
+    EXPECT_CALL(*mock_renderable, buffers_ready_for_compositor())
+        .InSequence(seq)
+        .WillOnce(Return(1));
+    EXPECT_CALL(*mock_renderable, buffer(_))
+        .InSequence(seq); 
+
+    mg::RenderableList list({mock_renderable});
+    FakeScene scene(list);
+
+    mc::DefaultDisplayBufferCompositor compositor(
+        display_buffer,
+        mt::fake_shared(scene),
+        mt::fake_shared(mock_renderer),
+        mr::null_compositor_report());
+
+    EXPECT_TRUE(compositor.composite());
+    EXPECT_FALSE(compositor.composite());
+}