Mir

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

Proposed by Alexandros Frantzis on 2013-08-02
Status: Merged
Approved by: Alan Griffiths on 2013-08-05
Approved revision: 920
Merged at revision: 923
Proposed branch: lp:~afrantzis/mir/multimonitor-misc-fixes
Merge into: lp:~mir-team/mir/trunk
Diff against target: 151 lines (+63/-18)
5 files modified
include/server/mir/compositor/gl_renderer_factory.h (+5/-0)
src/server/compositor/gl_renderer_factory.cpp (+8/-0)
src/server/graphics/gbm/gbm_cursor.cpp (+29/-18)
src/server/graphics/gbm/gbm_cursor.h (+2/-0)
tests/unit-tests/graphics/gbm/test_gbm_cursor.cpp (+19/-0)
To merge this branch: bzr merge lp:~afrantzis/mir/multimonitor-misc-fixes
Reviewer Review Type Date Requested Status
Alan Griffiths 2013-08-02 Approve on 2013-08-05
PS Jenkins bot (community) continuous-integration Approve on 2013-08-02
Review via email: mp+178219@code.launchpad.net

Commit message

gbm,compositor: Misc multimonitor related fixes

Description of the change

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.

To post a comment you must log in.
Alan Griffiths (alan-griffiths) wrote :

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) wrote :

            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) wrote :

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

review: Approve
920. By Alexandros Frantzis on 2013-08-02

gbm: Deduplicate GBM cursor internal code

Alexandros Frantzis (afrantzis) wrote :

> 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) wrote :
review: Needs Fixing (continuous-integration)
Alexandros Frantzis (afrantzis) wrote :

Spurious failure... resubmitting.

Alan Griffiths (alan-griffiths) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/compositor/gl_renderer_factory.h'
2--- include/server/mir/compositor/gl_renderer_factory.h 2013-07-22 10:44:21 +0000
3+++ include/server/mir/compositor/gl_renderer_factory.h 2013-08-02 10:56:25 +0000
4@@ -21,6 +21,8 @@
5
6 #include "mir/compositor/renderer_factory.h"
7
8+#include <mutex>
9+
10 namespace mir
11 {
12 namespace compositor
13@@ -30,6 +32,9 @@
14 {
15 public:
16 std::unique_ptr<Renderer> create_renderer_for(geometry::Rectangle const& rect);
17+
18+private:
19+ std::mutex renderer_mutex;
20 };
21
22 }
23
24=== modified file 'src/server/compositor/gl_renderer_factory.cpp'
25--- src/server/compositor/gl_renderer_factory.cpp 2013-07-25 10:25:58 +0000
26+++ src/server/compositor/gl_renderer_factory.cpp 2013-08-02 10:56:25 +0000
27@@ -26,6 +26,14 @@
28 std::unique_ptr<mc::Renderer>
29 mc::GLRendererFactory::create_renderer_for(geom::Rectangle const& rect)
30 {
31+ /*
32+ * We need to serialize renderer creation because some GL calls used
33+ * during renderer construction that create unique resource ids
34+ * (e.g. glCreateProgram) are not thread-safe when the threads are
35+ * have the same or shared EGL contexts.
36+ */
37+ std::lock_guard<std::mutex> lg{renderer_mutex};
38+
39 auto raw = new GLRenderer(rect);
40 return std::unique_ptr<mc::Renderer>(raw);
41 }
42
43=== modified file 'src/server/graphics/gbm/gbm_cursor.cpp'
44--- src/server/graphics/gbm/gbm_cursor.cpp 2013-07-30 14:23:10 +0000
45+++ src/server/graphics/gbm/gbm_cursor.cpp 2013-08-02 10:56:25 +0000
46@@ -88,28 +88,12 @@
47
48 void mgg::GBMCursor::move_to(geometry::Point position)
49 {
50- for_each_used_output([&](KMSOutput& output, geom::Rectangle const& output_rect)
51- {
52- if (output_rect.contains(position))
53- {
54- auto dp = position - output_rect.top_left;
55- output.move_cursor({dp.dx.as_int(), dp.dy.as_int()});
56- if (!output.has_cursor())
57- output.set_cursor(buffer);
58- }
59- else
60- {
61- if (output.has_cursor())
62- output.clear_cursor();
63- }
64- });
65-
66- current_position = position;
67+ place_cursor_at(position, UpdateState);
68 }
69
70 void mgg::GBMCursor::show_at_last_known_position()
71 {
72- move_to(current_position);
73+ place_cursor_at(current_position, ForceState);
74 }
75
76 void mgg::GBMCursor::hide()
77@@ -140,3 +124,30 @@
78 });
79 });
80 }
81+
82+void mgg::GBMCursor::place_cursor_at(
83+ geometry::Point position,
84+ ForceCursorState force_state)
85+{
86+ for_each_used_output([&](KMSOutput& output, geom::Rectangle const& output_rect)
87+ {
88+ if (output_rect.contains(position))
89+ {
90+ auto dp = position - output_rect.top_left;
91+ output.move_cursor({dp.dx.as_int(), dp.dy.as_int()});
92+ if (force_state || !output.has_cursor())
93+ {
94+ output.set_cursor(buffer);
95+ }
96+ }
97+ else
98+ {
99+ if (force_state || output.has_cursor())
100+ {
101+ output.clear_cursor();
102+ }
103+ }
104+ });
105+
106+ current_position = position;
107+}
108
109=== modified file 'src/server/graphics/gbm/gbm_cursor.h'
110--- src/server/graphics/gbm/gbm_cursor.h 2013-07-30 14:23:10 +0000
111+++ src/server/graphics/gbm/gbm_cursor.h 2013-08-02 10:56:25 +0000
112@@ -72,7 +72,9 @@
113 void hide();
114
115 private:
116+ enum ForceCursorState { UpdateState, ForceState };
117 void for_each_used_output(std::function<void(KMSOutput&, geometry::Rectangle const&)> const& f);
118+ void place_cursor_at(geometry::Point position, ForceCursorState force_state);
119
120 KMSOutputContainer& output_container;
121 geometry::Point current_position;
122
123=== modified file 'tests/unit-tests/graphics/gbm/test_gbm_cursor.cpp'
124--- tests/unit-tests/graphics/gbm/test_gbm_cursor.cpp 2013-07-30 14:23:10 +0000
125+++ tests/unit-tests/graphics/gbm/test_gbm_cursor.cpp 2013-08-02 10:56:25 +0000
126@@ -215,6 +215,25 @@
127 , std::logic_error);
128 }
129
130+TEST_F(GBMCursorTest, forces_cursor_state_on_construction)
131+{
132+ using namespace testing;
133+
134+ EXPECT_CALL(*output_container.outputs[10], move_cursor(geom::Point{0,0}));
135+ EXPECT_CALL(*output_container.outputs[10], set_cursor(_));
136+ EXPECT_CALL(*output_container.outputs[11], clear_cursor());
137+
138+ /* No checking of existing cursor state */
139+ EXPECT_CALL(*output_container.outputs[10], has_cursor()).Times(0);
140+ EXPECT_CALL(*output_container.outputs[11], has_cursor()).Times(0);
141+
142+ mgg::GBMCursor cursor_tmp{mock_gbm.fake_gbm.device, output_container,
143+ std::make_shared<StubCurrentConfiguration>()};
144+
145+ output_container.verify_and_clear_expectations();
146+}
147+
148+
149 TEST_F(GBMCursorTest, move_to_sets_clears_cursor_if_needed)
150 {
151 using namespace testing;

Subscribers

People subscribed via source and target branches