Merge lp:~vanvugt/mir/reverse-scene-traversal into lp:mir

[Daniel van Vugt]

Add support for traversing the Scene from front surface to back.
This is required for occlusion detection at least (coming soon).

Prerequisite for fixing bug 1227739.

[Alexandros Frantzis (afrantzis)]

43 + for ( auto layer = layers_by_depth.rbegin()
44 + ; layer != layers_by_depth.rend()
45 + ; ++layer
46 + )

Although I don't find anything in our coding standard about this, we tend to break lines after punctuation marks.

Use std::for_each() instead?

49 + for (auto it = surfaces.rbegin(); it != surfaces.rend(); ++it)
50 + {

Use std::for_each() instead?

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Fixed semicolons.

I disagree with for_each() here because it would force me to either add more functions or use lambdas. Both would reduce readability (and efficiency) a bit.

[Alan Griffiths (alan-griffiths)]

*Needs discussion*

I think we need a serious rethink of the interface Scene provides. It is evolving into a bad case of coupling.

lock()/unlock() are a problem because they imply locking the whole of the data model, not just the part relevant to the current operation.

Locking the whole data model is problematic if, for example, you want to composit different screens on different threads (which we attempt).

This MP adding forward and reverse traversals and expecting the results to make sense requires maintaining a lock across connected traversals. That will make it difficult to reduce the scope of locking to affected parts of the data.

I know at present there's no way to lock just a subset of the current model - but this interface makes that hard to change. The lock()/unlock() were originally a hack around the lack of a post-traversal call on OperatorForScene.

I think we want something like:

  auto surfaces = scene->select(filter);

  for (auto x : subscene) ...

and/or

  for (auto it = surfaces.rbegin(); it != surfaces.rend(); ++it)

With "~surfaces" releasing the lock.

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1127
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/144/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2315
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2200
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/141
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/141/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/144/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1128
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/145/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2317
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2202
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/142
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/142/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/145/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Alan,

As it happens, I too decided just this week I would like to see:
    auto surfaces = scene->select(filter);
or
    auto surfaces = scene->snapshot();

However I am sticking with the current design at present, in an effort to make small incremental steps that are still potentially saucy-compatible. My immediate goal is to resolve bug 1227739 in or soon-after saucy is released. That means using existing interfaces.

[Daniel van Vugt (vanvugt)]

Although "filtering" can be complex, requiring multiple passes in varying directions. So I don't endorse:
    auto surfaces = scene->select(filter);
That would break down for what I have planned. I would however endorse something like:
    auto renderables = scene->snapshot();
The locking is now atomic, non-blocking, and allows for arbitrary filtering algorithms including culling of non-visible renderables before the list is sent to the renderer.

[Kevin DuBois (kdub)]

so we always traverse layers forward, and then have a forward/reverse for surfaces? I agree with Alan's "need information", if we have enough time. If this needs to get in before ffe, though, I guess my review is an approve. Hope we can reinvent the scenegraph soon though

review: Approve

[Alan Griffiths (alan-griffiths)]

OK we all seem to agree that this adds to the technical debt we'll need to sort out (and have some tentative agreement on the right approach to doing that).

review: Abstain

[Alexandros Frantzis (afrantzis)]

> If this needs to get in before ffe, though, I guess my review is an approve.
> Hope we can reinvent the scenegraph soon though.

+1

review: Approve

=== modified file 'include/server/mir/compositor/scene.h'
--- include/server/mir/compositor/scene.h	2013-08-28 03:41:48 +0000
+++ include/server/mir/compositor/scene.h	2013-10-10 09:44:21 +0000
@@ -66,8 +66,13 @@
 public:
     virtual ~Scene() {}
 
+    // Back to front; normal rendering order
     virtual void for_each_if(FilterForScene& filter, OperatorForScene& op) = 0;
 
+    // Front to back; as used when scanning for occlusions
+    virtual void reverse_for_each_if(FilterForScene& filter,
+                                     OperatorForScene& op) = 0;
+
     /**
      * Sets a callback to be called whenever the state of the
      * Scene changes.
 
=== modified file 'include/server/mir/surfaces/surface_stack.h'
--- include/server/mir/surfaces/surface_stack.h	2013-08-28 03:41:48 +0000
+++ include/server/mir/surfaces/surface_stack.h	2013-10-10 09:44:21 +0000
@@ -67,6 +67,8 @@
 
     // From Scene
     virtual void for_each_if(compositor::FilterForScene &filter, compositor::OperatorForScene &op);
+    virtual void reverse_for_each_if(compositor::FilterForScene& filter,
+                                     compositor::OperatorForScene& op);
     virtual void set_change_callback(std::function<void()> const& f);
     
     // From InputTargets
 
=== modified file 'src/server/surfaces/surface_stack.cpp'
--- src/server/surfaces/surface_stack.cpp	2013-10-03 06:01:11 +0000
+++ src/server/surfaces/surface_stack.cpp	2013-10-10 09:44:21 +0000
@@ -66,6 +66,24 @@
     }
 }
 
+void ms::SurfaceStack::reverse_for_each_if(mc::FilterForScene& filter,
+                                           mc::OperatorForScene& op)
+{
+    std::lock_guard<std::recursive_mutex> lg(guard);
+    for (auto layer = layers_by_depth.rbegin();
+         layer != layers_by_depth.rend();
+         ++layer)
+    {
+        auto surfaces = layer->second;
+        for (auto it = surfaces.rbegin(); it != surfaces.rend(); ++it)
+        {
+            mc::CompositingCriteria& info = *((*it)->compositing_criteria());
+            ms::BufferStream& stream = *((*it)->buffer_stream());
+            if (filter(info)) op(info, stream);
+        }
+    }
+}
+
 void ms::SurfaceStack::set_change_callback(std::function<void()> const& f)
 {
     std::lock_guard<std::mutex> lg{notify_change_mutex};
 
=== modified file 'tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp'
--- tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/compositor/test_default_display_buffer_compositor.cpp	2013-10-10 09:44:21 +0000
@@ -47,6 +47,8 @@
 struct MockScene : mc::Scene
 {
     MOCK_METHOD2(for_each_if, void(mc::FilterForScene&, mc::OperatorForScene&));
+    MOCK_METHOD2(reverse_for_each_if, void(mc::FilterForScene&,
+                                           mc::OperatorForScene&));
     MOCK_METHOD1(set_change_callback, void(std::function<void()> const&));
     MOCK_METHOD0(lock, void());
     MOCK_METHOD0(unlock, void());
@@ -76,6 +78,8 @@
         }
     }
 
+    void reverse_for_each_if(mc::FilterForScene&, mc::OperatorForScene&) {}
+
     void set_change_callback(std::function<void()> const&) {}
 
     void change(const std::vector<mc::CompositingCriteria*> &surfs)
 
=== modified file 'tests/unit-tests/compositor/test_multi_threaded_compositor.cpp'
--- tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/compositor/test_multi_threaded_compositor.cpp	2013-10-10 09:44:21 +0000
@@ -86,6 +86,8 @@
     {
     }
 
+    void reverse_for_each_if(mc::FilterForScene&, mc::OperatorForScene&) {}
+
     void set_change_callback(std::function<void()> const& f)
     {
         std::lock_guard<std::mutex> lock{callback_mutex};
 
=== modified file 'tests/unit-tests/surfaces/test_surface_stack.cpp'
--- tests/unit-tests/surfaces/test_surface_stack.cpp	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/surfaces/test_surface_stack.cpp	2013-10-10 09:44:21 +0000
@@ -256,6 +256,88 @@
     stack.for_each_if(filter, renderable_operator);
 }
 
+TEST_F(SurfaceStack, skips_surface_that_is_filtered_out_reverse)
+{
+    using namespace ::testing;
+
+    EXPECT_CALL(mock_surface_allocator, create_surface(_,_))
+        .WillOnce(Return(stub_surface1))
+        .WillOnce(Return(stub_surface2))
+        .WillOnce(Return(stub_surface3));
+
+    ms::SurfaceStack stack(mt::fake_shared(mock_surface_allocator),
+                           mt::fake_shared(input_registrar));
+    auto s1 = stack.create_surface(default_params);
+    auto criteria1 = s1.lock()->compositing_criteria();
+    auto stream1 = s1.lock()->buffer_stream();
+    auto s2 = stack.create_surface(default_params);
+    auto criteria2 = s2.lock()->compositing_criteria();
+    auto stream2 = s2.lock()->buffer_stream();
+    auto s3 = stack.create_surface(default_params);
+    auto criteria3 = s3.lock()->compositing_criteria();
+    auto stream3 = s3.lock()->buffer_stream();
+
+    MockFilterForScene filter;
+    MockOperatorForScene renderable_operator;
+
+    Sequence seq1, seq2;
+    EXPECT_CALL(filter, filter(Ref(*criteria3)))
+        .InSequence(seq1)
+        .WillOnce(Return(true));
+    EXPECT_CALL(filter, filter(Ref(*criteria2)))
+        .InSequence(seq1)
+        .WillOnce(Return(false));
+    EXPECT_CALL(filter, filter(Ref(*criteria1)))
+        .InSequence(seq1)
+        .WillOnce(Return(true));
+    EXPECT_CALL(renderable_operator,
+                renderable_operator(Ref(*criteria3), Ref(*stream3)))
+        .InSequence(seq2);
+    EXPECT_CALL(renderable_operator,
+                renderable_operator(Ref(*criteria1), Ref(*stream1)))
+        .InSequence(seq2);
+
+    stack.reverse_for_each_if(filter, renderable_operator);
+}
+
+TEST_F(SurfaceStack, stacking_order_reverse)
+{
+    using namespace ::testing;
+
+    EXPECT_CALL(mock_surface_allocator, create_surface(_,_))
+        .WillOnce(Return(stub_surface1))
+        .WillOnce(Return(stub_surface2))
+        .WillOnce(Return(stub_surface3));
+
+    ms::SurfaceStack stack(mt::fake_shared(mock_surface_allocator),
+                           mt::fake_shared(input_registrar));
+
+    auto s1 = stack.create_surface(default_params);
+    auto criteria1 = s1.lock()->compositing_criteria();
+    auto stream1 = s1.lock()->buffer_stream();
+    auto s2 = stack.create_surface(default_params);
+    auto criteria2 = s2.lock()->compositing_criteria();
+    auto stream2 = s2.lock()->buffer_stream();
+    auto s3 = stack.create_surface(default_params);
+    auto criteria3 = s3.lock()->compositing_criteria();
+    auto stream3 = s3.lock()->buffer_stream();
+
+    StubFilterForScene filter;
+    MockOperatorForScene renderable_operator;
+    Sequence seq;
+    EXPECT_CALL(renderable_operator,
+                renderable_operator(Ref(*criteria3), Ref(*stream3)))
+        .InSequence(seq);
+    EXPECT_CALL(renderable_operator,
+                renderable_operator(Ref(*criteria2), Ref(*stream2)))
+        .InSequence(seq);
+    EXPECT_CALL(renderable_operator,
+                renderable_operator(Ref(*criteria1), Ref(*stream1)))
+        .InSequence(seq);
+
+    stack.reverse_for_each_if(filter, renderable_operator);
+}
+
 TEST_F(SurfaceStack, stacking_order)
 {
     using namespace ::testing;