Merge lp:~kdub/mir/share-occlusion-with-examples into lp:mir

[Kevin DuBois]

share occlusion detection logic with the examples/, and allow for different screen coverage areas to be computed based on a renderable.

share occlusion detection logic with the examples/, and allow for different screen coverage areas to be computed based on a renderable.

note:
I'm aiming for this MP to be preparation for fixing lp: #1299977. It raises a sticky question of how to share useful src/ code with examples/ though, so I'd like to tackle that question in this MP.

I've been trying to read the team's temperature on the question 'how to share production code with examples', and the way this MP shares the code seems the most agreeable. Here are the options I considered for how to do this:

1) place the header file in the public folders (as was done with gl_renderer.h). This has the big disadvantage that these privately-intended classes become public API, so I'd like to shy away from this.

2) have the example code duplicate functionality where it needs to. This is actually what I prefer the most. If my intention is to use the mir API to rewrite the compositor, I should expect to write an occlusion algorithm and a renderer from scratch. This does have the downside of we'd have to maintain one algorithm in the examples/ and one in src/, but the upside that the demos only depend on the major version number and are not locked into the specific mirserver version.

3) allow the examples to access the src/ headers, which is what this MP implements. This keeps the private stuff (like occlusion and renderer) private, but has the downside that examples/ is tied to the specific version of libmirserver.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1692
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1846/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/498
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/499
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/497
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/367
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/367/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/364
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/364/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/1439
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/1439/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/1704
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/8265

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

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

Perhaps whatever shared code examples need could be factored out of the private area and put under a util dir. Both the private headers and the examples can then include what they need from util.

[Andreas Pokorny (andreas-pokorny)]

My cents on this topic:

If it is a useful utility that mir users are likely to use then we should provide it as an official API. Maybe it is so self contained that it is possible to provide that as a header only solutions to avoid having a larger ABI? So if that is the case 1) might be a possibility.

But I share your preference here: If it is a case of the examples should show how things are done properly or in a specific way based on shell decisions or should show how to do it efficiently, we should duplicate the code inside the examples and have a version with elaborate inline comments or unit tests that tell a good story.

[Alan Griffiths (alan-griffiths)]

If I were an end user looking for an example of how to "filter occlusions" and saw this use of a private API then I'd be unimpressed. And rightly so.

The examples show end-users how to write code that works with Mir. That means they must only access facilities that are are available to users (and should use them correctly).

If there is a conflict between "it should be private" and "it is useful for the examples" then there is a design issue to be resolved about what the requirement is and how to support it.

In other words, find a way to publish the facility that's needed so end users can access it too. I can believe that you want to offer a different (more restricted or more stable) interface than the internal one, but this MP is wrong.

Even duplicating code is better than telling the user "don't do what the example shows you how to do".

review: Disapprove

[Kevin DuBois (kdub)]

I agree that its strange to let the examples access internals in src/ that other users don't have access to. So this leaves us with:

1) reimplement common graphics algorithms (drawing, occlusion scanning) in the examples. This has the downside that we'd have somewhat-duplicated code in examples/ and src/
2) provide an API for common composition (and really, common GL operations) for the future compositor writers. This has the downside that its a pretty sizable undertaking (nux/qml/etc)
3) provide a utility class that's private, but contains only common GL operations. (eg, things anyone drawing with opengl would be expected to know how to write)

Our current two users (USC, unity8) have elected to use the default implementation (usc) or rewrite everything (unity8). I'd expect a future compositor writer to not want to use a mir-made drawing utility class (mir-nux).

The middle route seems to be 3). The utility folder would only have common graphics utilities in it. It wouldn't be a public API. In the current code, this is occlusion.h and Renderer/GLRenderer. I'd expect it to grow to include an animation framework and other stuff like that. If any of these become really awesome interfaces every user wants to use, we can make it a public API at that point. Now we just need a good name for that folder...

[Kevin DuBois (kdub)]

> 3) provide a utility class that's private, but contains only common GL
clarification: "utility classes in a utility folder" is a clearer

[Cemil Azizoglu (cemil-azizoglu)]

> > 3) provide a utility class that's private, but contains only common GL
> clarification: "utility classes in a utility folder" is a clearer

+1 if you mean "utility classes in a utility folder that both Mir proper and examples make use of".

[Alan Griffiths (alan-griffiths)]

> > > 3) provide a utility class that's private, but contains only common GL
> > clarification: "utility classes in a utility folder" is a clearer
>
> +1 if you mean "utility classes in a utility folder that both Mir proper and
> examples make use of".

I have two concerns:

1. "Utility" is a terrible name - are other folders non-utility (= useless)
2. We need to make this folder available to people trying to reproduce the examples.

[Kevin DuBois (kdub)]

> > > > 3) provide a utility class that's private, but contains only common GL
> > > clarification: "utility classes in a utility folder" is a clearer
> >
> > +1 if you mean "utility classes in a utility folder that both Mir proper and
> > examples make use of".
>
> I have two concerns:
>
> 1. "Utility" is a terrible name - are other folders non-utility (= useless)
agree

> 2. We need to make this folder available to people trying to reproduce the
> examples.

I disagree here, as this introduces an API to support, and the goal is to only have only well-known GL stuff in this folder. Someone who wants to replace an OpenGL compositor object can reasonably be expected to develop their own vertex/shader/etc OpenGL stuff. I'll grant that this is a moderate chunk of code (100 lines maybe), but people are used to writing it.

[Alan Griffiths (alan-griffiths)]

> > 2. We need to make this folder available to people trying to reproduce the
> > examples.
>
> I disagree here, as this introduces an API to support, and the goal is to only
> have only well-known GL stuff in this folder. Someone who wants to replace an
> OpenGL compositor object can reasonably be expected to develop their own
> vertex/shader/etc OpenGL stuff. I'll grant that this is a moderate chunk of
> code (100 lines maybe), but people are used to writing it.

The point of providing examples is that our users can take them, *build* them and experiment with changing them.

I don't understand why you are so averse to either duplicating or sharing a bit of "well-known GL stuff" that you want to make things difficult for users to use the example code.

[Alexandros Frantzis (afrantzis)]

My preference would also be to duplicate the code. The examples (an example shell in this case) and the core code will likely have different needs that change for different reasons. We ideally don't want the internal needs of the examples affecting core internal code. The change proposed in this MP is an example of this happening: our core code doesn't currently need the "coverage_of" parameter; it's introduced only for the benefit of the examples (if understood things correctly).

review: Disapprove

[Alberto Aguirre (albaguirre)]

I concur that a "private" api/util library is not what the examples should use.

Given that we lack test coverage on compositor/rendering output howerver, I hesitate on duplicating code. If we can make a version of the demo shell with all the current features (except I guess decorations) so that the default mir renderer/compositor is used then I would be ok. Basically a shell that we can use to manual test for rendering artifacts - at least until we have proper test coverage for that.

review: Disapprove

=== modified file 'examples/CMakeLists.txt'
--- examples/CMakeLists.txt	2014-06-02 17:07:02 +0000
+++ examples/CMakeLists.txt	2014-06-09 22:29:32 +0000
@@ -100,6 +100,10 @@
   ${PROJECT_SOURCE_DIR}/include/client
   ${PROJECT_SOURCE_DIR}/include/platform
   ${GLESv2_INCLUDE_DIRS}
+  #mir has some useful private code that we don't want to duplicate in our examples.
+  #it is not part of our public api, but useful nonetheless. Users of the api will
+  #have to write their own implementations
+  ${PROJECT_SOURCE_DIR}/src/server/compositor
 )
 
 add_executable(mir_demo_standalone_render_to_fb

=== modified file 'src/server/compositor/default_display_buffer_compositor.cpp'
--- src/server/compositor/default_display_buffer_compositor.cpp	2014-06-05 20:35:46 +0000
+++ src/server/compositor/default_display_buffer_compositor.cpp	2014-06-09 22:29:32 +0000
@@ -52,7 +52,7 @@
 
     auto const& view_area = display_buffer.view_area();
     auto renderable_list = scene->renderable_list_for(this);
-    mc::filter_occlusions_from(renderable_list, view_area);
+    mc::filter_occlusions_from(renderable_list, view_area, simple_renderable_rect);
 
     //TODO: the DisplayBufferCompositor should not have to figure out if it has to force
     //      a subsequent compositon. The MultiThreadedCompositor should be smart enough to 

=== modified file 'src/server/compositor/occlusion.cpp'
--- src/server/compositor/occlusion.cpp	2014-04-17 01:34:35 +0000
+++ src/server/compositor/occlusion.cpp	2014-06-09 22:29:32 +0000
@@ -27,8 +27,9 @@
 {
 bool renderable_is_occluded(
     Renderable const& renderable, 
-    Rectangle const& area,
-    std::vector<Rectangle>& coverage)
+    Rectangle const& display_area,
+    Rectangle const& coverage_area,
+    std::vector<Rectangle>& coverage_list)
 {
     static const glm::mat4 identity;
     if (renderable.transformation() != identity)
@@ -41,14 +42,13 @@
         return true;  //invisible; definitely occluded.
 
     // Not weirdly transformed but also not on this monitor? Don't care...
-    if (!area.overlaps(renderable.screen_position()))
+    if (!display_area.overlaps(coverage_area))
         return true;  // Not on the display; definitely occluded.
 
     bool occluded = false;
-    Rectangle const& window = renderable.screen_position();
-    for (const auto &r : coverage)
+    for (const auto &r : coverage_list)
     {
-        if (r.contains(window))
+        if (r.contains(coverage_area))
         {
             occluded = true;
             break;
@@ -56,21 +56,27 @@
     }
 
     if (!occluded && renderable.alpha() == 1.0f && !renderable.shaped())
-        coverage.push_back(window);
+        coverage_list.push_back(coverage_area);
 
     return occluded;
 }
 }
 
+Rectangle mir::compositor::simple_renderable_rect(Renderable const& renderable)
+{
+    return renderable.screen_position();
+}
+
 void mir::compositor::filter_occlusions_from(
     RenderableList& list,
-    Rectangle const& area)
+    geometry::Rectangle const& area,
+    std::function<Rectangle(Renderable const&)> coverage_of)
 {
     std::vector<Rectangle> coverage;
     auto it = list.rbegin();
     while (it != list.rend())
     {
-        if (renderable_is_occluded(**it, area, coverage))
+        if (renderable_is_occluded(**it, area, coverage_of(**it), coverage))
             list.erase(std::prev(it.base()));
         else
             it++;

=== modified file 'src/server/compositor/occlusion.h'
--- src/server/compositor/occlusion.h	2014-03-26 16:59:32 +0000
+++ src/server/compositor/occlusion.h	2014-06-09 22:29:32 +0000
@@ -29,8 +29,12 @@
 namespace compositor
 {
 
-void filter_occlusions_from(graphics::RenderableList& list, geometry::Rectangle const& area);
+geometry::Rectangle simple_renderable_rect(graphics::Renderable const& renderable);
 
+void filter_occlusions_from(
+    graphics::RenderableList& list,
+    geometry::Rectangle const& area,
+    std::function<geometry::Rectangle(graphics::Renderable const&)> filter_fn);
 } // namespace compositor
 } // namespace mir
 

=== modified file 'tests/unit-tests/compositor/test_occlusion.cpp'
--- tests/unit-tests/compositor/test_occlusion.cpp	2014-06-03 19:16:54 +0000
+++ tests/unit-tests/compositor/test_occlusion.cpp	2014-06-09 22:29:32 +0000
@@ -45,7 +45,7 @@
     auto window = std::make_shared<mtd::FakeRenderable>(12, 34, 56, 78);
     mg::RenderableList list{window};
  
-    filter_occlusions_from(list, monitor_rect);
+    filter_occlusions_from(list, monitor_rect, simple_renderable_rect);
     ASSERT_EQ(1u, list.size());
     EXPECT_EQ(window, list.front());
 }
@@ -58,7 +58,7 @@
     auto bottom = std::make_shared<mtd::FakeRenderable>(200, 1000, 100, 1000);
     mg::RenderableList list{left, right, top, bottom};
  
-    filter_occlusions_from(list, monitor_rect);
+    filter_occlusions_from(list, monitor_rect, simple_renderable_rect);
     EXPECT_EQ(4u, list.size());
 }
 
@@ -68,7 +68,7 @@
     auto bottom = std::make_shared<mtd::FakeRenderable>(12, 12, 5, 5);
     mg::RenderableList list{bottom, top};
 
-    filter_occlusions_from(list, monitor_rect);
+    filter_occlusions_from(list, monitor_rect, simple_renderable_rect);
 
     ASSERT_EQ(1u, list.size());
     EXPECT_EQ(top, list.front());
@@ -80,7 +80,7 @@
     auto bottom = std::make_shared<mtd::FakeRenderable>(Rectangle{{12, 12}, {5, 5}}, 1.0f);
     mg::RenderableList list{bottom, top};
 
-    filter_occlusions_from(list, monitor_rect);
+    filter_occlusions_from(list, monitor_rect, simple_renderable_rect);
 
     ASSERT_EQ(2u, list.size());
     EXPECT_EQ(bottom, list.front());
@@ -92,7 +92,7 @@
     auto window = std::make_shared<mtd::FakeRenderable>(Rectangle{{10, 10}, {10, 10}}, 1.0f, true, false);
     mg::RenderableList list{window};
 
-    filter_occlusions_from(list, monitor_rect);
+    filter_occlusions_from(list, monitor_rect, simple_renderable_rect);
 
     EXPECT_EQ(0u, list.size());
 }
@@ -103,7 +103,7 @@
     auto bottom = std::make_shared<mtd::FakeRenderable>(12, 12, 5, 5);
     mg::RenderableList list{bottom, top};
 
-    filter_occlusions_from(list, monitor_rect);
+    filter_occlusions_from(list, monitor_rect, simple_renderable_rect);
 
     ASSERT_EQ(1u, list.size());
     EXPECT_EQ(bottom, list.front());
@@ -115,7 +115,7 @@
     auto bottom = std::make_shared<mtd::FakeRenderable>(12, 12, 5, 5);
     mg::RenderableList list{bottom, top};
 
-    filter_occlusions_from(list, monitor_rect);
+    filter_occlusions_from(list, monitor_rect, simple_renderable_rect);
 
     ASSERT_EQ(2u, list.size());
     EXPECT_EQ(bottom, list.front());
@@ -128,7 +128,7 @@
     auto bottom = std::make_shared<mtd::FakeRenderable>(10, 10, 10, 10);
     mg::RenderableList list{bottom, top};
 
-    filter_occlusions_from(list, monitor_rect);
+    filter_occlusions_from(list, monitor_rect, simple_renderable_rect);
 
     ASSERT_EQ(1u, list.size());
     EXPECT_EQ(top, list.front());
@@ -140,7 +140,7 @@
     auto bottom = std::make_shared<mtd::FakeRenderable>(9, 9, 12, 12);
     mg::RenderableList list{bottom, top};
 
-    filter_occlusions_from(list, monitor_rect);
+    filter_occlusions_from(list, monitor_rect, simple_renderable_rect);
 
     ASSERT_EQ(2u, list.size());
     EXPECT_EQ(bottom, list.front());
@@ -154,7 +154,7 @@
     for (auto x = 0u; x < num_windows; x++)
         list.push_back(std::make_shared<mtd::FakeRenderable>(x, x, 200, 100));
 
-    filter_occlusions_from(list, monitor_rect);
+    filter_occlusions_from(list, monitor_rect, simple_renderable_rect);
     EXPECT_EQ(num_windows, list.size());
 }
 
@@ -175,7 +175,7 @@
         window0  //not occluded
     };
 
-    filter_occlusions_from(list, monitor_rect);
+    filter_occlusions_from(list, monitor_rect, simple_renderable_rect);
 
     auto expected_size = 3u;
     ASSERT_EQ(expected_size, list.size());