Merge lp:~afrantzis/mir/multimonitor-misc-fixes into lp:~mir-team/mir/trunk

[Alexandros Frantzis]

gbm,compositor: Misc multimonitor related fixes

gbm,compositor: Misc multimonitor related fixes

This MP forces a reset of the cursor position and associated state when starting, and disallows concurrent creation of GLRenderers. See individual commits for more info.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:919
http://jenkins.qa.ubuntu.com/job/mir-ci/1187/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1565
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1450
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/425
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/425/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1187/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

show_at_last_known_position() looks horribly like duplicated code with move_to() - there must be a more expressive way to differentiate the logic.

[Alan Griffiths (alan-griffiths)]

            auto dp = position - output_rect.top_left;
            output.move_cursor({dp.dx.as_int(), dp.dy.as_int()});

This (Displacement->Point) conversion looks like a common requirement. Maybe we should have a geom::change_origin() function to do it.

BTW Another way to write it is:

    auto displacement_of_output = geom::Point{} - output_rect.top_left;
    output.move_cursor(position + displacement_of_output);

Which keeps things at a higher level of abstraction.

[Alan Griffiths (alan-griffiths)]

I don't think either of the above are blockers - but won't top-approve as someone else might.

review: Approve

[Alexandros Frantzis (afrantzis)]

> show_at_last_known_position() looks horribly like duplicated code with move_to() - there must be a more expressive way to differentiate the logic.

Fixed.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:920
http://jenkins.qa.ubuntu.com/job/mir-ci/1196/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1576
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1461
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/434/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1196/rebuild

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Spurious failure... resubmitting.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:920
http://jenkins.qa.ubuntu.com/job/mir-ci/1197/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1578
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1463
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/435
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/435/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1197/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

90 + auto dp = position - output_rect.top_left;
91 + output.move_cursor({dp.dx.as_int(), dp.dy.as_int()});

I still prefer something like:

    auto change_of_origin = geom::Point{} - output_rect.top_left;
    output.move_cursor(position + change_of_origin);

But top-approving as no-one else cares.

review: Approve

=== modified file 'include/server/mir/compositor/gl_renderer_factory.h'
--- include/server/mir/compositor/gl_renderer_factory.h	2013-07-22 10:44:21 +0000
+++ include/server/mir/compositor/gl_renderer_factory.h	2013-08-02 10:56:25 +0000
@@ -21,6 +21,8 @@
 
 #include "mir/compositor/renderer_factory.h"
 
+#include <mutex>
+
 namespace mir
 {
 namespace compositor
@@ -30,6 +32,9 @@
 {
 public:
     std::unique_ptr<Renderer> create_renderer_for(geometry::Rectangle const& rect);
+
+private:
+    std::mutex renderer_mutex;
 };
 
 }

=== modified file 'src/server/compositor/gl_renderer_factory.cpp'
--- src/server/compositor/gl_renderer_factory.cpp	2013-07-25 10:25:58 +0000
+++ src/server/compositor/gl_renderer_factory.cpp	2013-08-02 10:56:25 +0000
@@ -26,6 +26,14 @@
 std::unique_ptr<mc::Renderer>
 mc::GLRendererFactory::create_renderer_for(geom::Rectangle const& rect)
 {
+    /*
+     * We need to serialize renderer creation because some GL calls used
+     * during renderer construction that create unique resource ids
+     * (e.g. glCreateProgram) are not thread-safe when the threads are
+     * have the same or shared EGL contexts.
+     */
+    std::lock_guard<std::mutex> lg{renderer_mutex};
+
     auto raw = new GLRenderer(rect);
     return std::unique_ptr<mc::Renderer>(raw);
 }

=== modified file 'src/server/graphics/gbm/gbm_cursor.cpp'
--- src/server/graphics/gbm/gbm_cursor.cpp	2013-07-30 14:23:10 +0000
+++ src/server/graphics/gbm/gbm_cursor.cpp	2013-08-02 10:56:25 +0000
@@ -88,28 +88,12 @@
 
 void mgg::GBMCursor::move_to(geometry::Point position)
 {
-    for_each_used_output([&](KMSOutput& output, geom::Rectangle const& output_rect)
-    {
-        if (output_rect.contains(position))
-        {
-            auto dp = position - output_rect.top_left;
-            output.move_cursor({dp.dx.as_int(), dp.dy.as_int()});
-            if (!output.has_cursor())
-                output.set_cursor(buffer);
-        }
-        else
-        {
-            if (output.has_cursor())
-                output.clear_cursor();
-        }
-    });
-
-    current_position = position;
+    place_cursor_at(position, UpdateState);
 }
 
 void mgg::GBMCursor::show_at_last_known_position()
 {
-    move_to(current_position);
+    place_cursor_at(current_position, ForceState);
 }
 
 void mgg::GBMCursor::hide()
@@ -140,3 +124,30 @@
             });
         });
 }
+
+void mgg::GBMCursor::place_cursor_at(
+    geometry::Point position,
+    ForceCursorState force_state)
+{
+    for_each_used_output([&](KMSOutput& output, geom::Rectangle const& output_rect)
+    {
+        if (output_rect.contains(position))
+        {
+            auto dp = position - output_rect.top_left;
+            output.move_cursor({dp.dx.as_int(), dp.dy.as_int()});
+            if (force_state || !output.has_cursor())
+            {
+                output.set_cursor(buffer);
+            }
+        }
+        else
+        {
+            if (force_state || output.has_cursor())
+            {
+                output.clear_cursor();
+            }
+        }
+    });
+
+    current_position = position;
+}

=== modified file 'src/server/graphics/gbm/gbm_cursor.h'
--- src/server/graphics/gbm/gbm_cursor.h	2013-07-30 14:23:10 +0000
+++ src/server/graphics/gbm/gbm_cursor.h	2013-08-02 10:56:25 +0000
@@ -72,7 +72,9 @@
     void hide();
 
 private:
+    enum ForceCursorState { UpdateState, ForceState };
     void for_each_used_output(std::function<void(KMSOutput&, geometry::Rectangle const&)> const& f);
+    void place_cursor_at(geometry::Point position, ForceCursorState force_state);
 
     KMSOutputContainer& output_container;
     geometry::Point current_position;

=== modified file 'tests/unit-tests/graphics/gbm/test_gbm_cursor.cpp'
--- tests/unit-tests/graphics/gbm/test_gbm_cursor.cpp	2013-07-30 14:23:10 +0000
+++ tests/unit-tests/graphics/gbm/test_gbm_cursor.cpp	2013-08-02 10:56:25 +0000
@@ -215,6 +215,25 @@
     , std::logic_error);
 }
 
+TEST_F(GBMCursorTest, forces_cursor_state_on_construction)
+{
+    using namespace testing;
+
+    EXPECT_CALL(*output_container.outputs[10], move_cursor(geom::Point{0,0}));
+    EXPECT_CALL(*output_container.outputs[10], set_cursor(_));
+    EXPECT_CALL(*output_container.outputs[11], clear_cursor());
+
+    /* No checking of existing cursor state */
+    EXPECT_CALL(*output_container.outputs[10], has_cursor()).Times(0);
+    EXPECT_CALL(*output_container.outputs[11], has_cursor()).Times(0);
+
+    mgg::GBMCursor cursor_tmp{mock_gbm.fake_gbm.device, output_container,
+                              std::make_shared<StubCurrentConfiguration>()};
+
+    output_container.verify_and_clear_expectations();
+}
+
+
 TEST_F(GBMCursorTest, move_to_sets_clears_cursor_if_needed)
 {
     using namespace testing;