Mir

Merge lp:~alan-griffiths/mir/dont-handle-nested-input-on-rpc-thread into lp:mir

Proposed by Alan Griffiths on 2017-06-23
Status: Merged
Approved by: Brandon Schaefer on 2017-06-26
Approved revision: 4201
Merged at revision: 4197
Proposed branch: lp:~alan-griffiths/mir/dont-handle-nested-input-on-rpc-thread
Merge into: lp:mir
Diff against target: 193 lines (+82/-12)
2 files modified
src/server/graphics/nested/input_platform.cpp (+77/-12)
tests/unit-tests/input/test_nested_input_platform.cpp (+5/-0)
To merge this branch: bzr merge lp:~alan-griffiths/mir/dont-handle-nested-input-on-rpc-thread
Reviewer Review Type Date Requested Status
Brandon Schaefer (community) 2017-06-23 Approve on 2017-06-26
Andreas Pokorny (community) Approve on 2017-06-25
Mir CI Bot continuous-integration Approve on 2017-06-23
Review via email: mp+326234@code.launchpad.net

Commit message

We recently changes the input transport to arrive on the RPC Thread of clients.

Mir-on-Mir therefore needs another thread for processing input as the handling of input can reasonably make blocking client API calls.

To post a comment you must log in.
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:4201
https://mir-jenkins.ubuntu.com/job/mir-ci/3461/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4723
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/4881
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/4870
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/4870
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/4870
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4760
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4760/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4760
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4760/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4760
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4760/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4760
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4760/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4760
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4760/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4760
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4760/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4760
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4760/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4760
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4760/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/3461/rebuild

review: Approve (continuous-integration)
Andreas Pokorny (andreas-pokorny) wrote :

I think this is reasonable to do - but there might be acceptance test troubles because now there are less order guarantees between certain state changes and input dispatching within the nested server.

Lgtm

review: Approve
Brandon Schaefer (brandontschaefer) wrote :

lgtm

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/graphics/nested/input_platform.cpp'
2--- src/server/graphics/nested/input_platform.cpp 2017-05-08 03:04:26 +0000
3+++ src/server/graphics/nested/input_platform.cpp 2017-06-23 16:26:45 +0000
4@@ -33,6 +33,9 @@
5 #include "mir/events/event_builders.h"
6
7 #include <chrono>
8+#include <condition_variable>
9+#include <thread>
10+#include <queue>
11
12 namespace mi = mir::input;
13 namespace mgn = mir::graphics::nested;
14@@ -52,9 +55,68 @@
15 return mgn::UniqueInputConfig(nullptr, [](MirInputConfig const*){});
16 }
17
18-}
19-
20-struct mgn::InputPlatform::InputDevice : mi::InputDevice
21+class Postman
22+{
23+public:
24+
25+ void start_work();
26+ void post(std::shared_ptr<MirEvent> const& event);
27+ void stop_work();
28+
29+ mi::InputSink* destination{nullptr};
30+
31+private:
32+ using EventQueue = std::queue<std::shared_ptr<MirEvent>>;
33+
34+ std::thread worker_thread;
35+ std::mutex mutable mutex;
36+ std::condition_variable work_cv;
37+ EventQueue events;
38+
39+ void do_work();
40+
41+ std::shared_ptr<MirEvent> next_event();
42+};
43+
44+void Postman::do_work()
45+{
46+ while (auto const event = next_event())
47+ destination->handle_input(*event);
48+}
49+
50+std::shared_ptr<MirEvent> Postman::next_event()
51+{
52+ EventQueue::value_type event;
53+ {
54+ std::unique_lock<decltype(mutex)> lock{mutex};
55+ work_cv.wait(lock, [this] { return !events.empty(); });
56+ event.swap(events.front());
57+ events.pop();
58+ }
59+ return event;
60+}
61+
62+void Postman::post(std::shared_ptr<MirEvent> const& event)
63+{
64+ std::lock_guard<decltype(mutex)> lock{mutex};
65+ events.push(event);
66+ work_cv.notify_one();
67+}
68+
69+void Postman::start_work()
70+{
71+ worker_thread = std::thread([this] { do_work(); });
72+}
73+
74+void Postman::stop_work()
75+{
76+ // We use a null event as a sentinel
77+ post({});
78+ worker_thread.join();
79+}
80+}
81+
82+struct mgn::InputPlatform::InputDevice : mi::InputDevice, Postman
83 {
84 public:
85 InputDevice(MirInputDevice* dev, std::function<void()> config_change)
86@@ -119,6 +181,12 @@
87 {
88 this->destination = destination;
89 this->builder = builder;
90+ start_work();
91+ }
92+
93+ void post_event(std::shared_ptr<MirEvent> const event)
94+ {
95+ post(event);
96 }
97
98 void emit_event(MirInputEvent const* event, mir::geometry::Rectangle const& frame)
99@@ -147,20 +215,19 @@
100 contact.touch_major = mir_touch_event_axis_value(touch_event, i, mir_touch_axis_touch_major);
101 contact.touch_minor = mir_touch_event_axis_value(touch_event, i, mir_touch_axis_touch_minor);
102 }
103- destination->handle_input(*builder->touch_event(event_time, contacts));
104+ post_event(builder->touch_event(event_time, contacts));
105 break;
106 }
107 case mir_input_event_type_key:
108 {
109 auto const* key_event = mir_input_event_get_keyboard_event(event);
110
111- auto new_kev = builder->key_event(
112+ post_event(builder->key_event(
113 event_time,
114 mir_keyboard_event_action(key_event),
115 mir_keyboard_event_key_code(key_event),
116 mir_keyboard_event_scan_code(key_event)
117- );
118- destination->handle_input(*new_kev);
119+ ));
120 break;
121 }
122 case mir_input_event_type_pointer:
123@@ -168,7 +235,7 @@
124 auto const* pointer_event = mir_input_event_get_pointer_event(event);
125 auto x = mir_pointer_event_axis_value(pointer_event, mir_pointer_axis_x);
126 auto y = mir_pointer_event_axis_value(pointer_event, mir_pointer_axis_y);
127- auto new_event = builder->pointer_event(
128+ post_event(builder->pointer_event(
129 event_time,
130 filter_pointer_action(mir_pointer_event_action(pointer_event)),
131 mir_pointer_event_buttons(pointer_event),
132@@ -178,9 +245,7 @@
133 mir_pointer_event_axis_value(pointer_event, mir_pointer_axis_vscroll),
134 mir_pointer_event_axis_value(pointer_event, mir_pointer_axis_relative_x),
135 mir_pointer_event_axis_value(pointer_event, mir_pointer_axis_relative_y)
136- );
137-
138- destination->handle_input(*new_event);
139+ ));
140 break;
141 }
142 default:
143@@ -190,6 +255,7 @@
144
145 void stop() override
146 {
147+ stop_work();
148 this->destination = nullptr;
149 this->builder = nullptr;
150 }
151@@ -272,7 +338,6 @@
152
153 MirInputDeviceId device_id;
154 MirInputDevice* device{nullptr};
155- mi::InputSink* destination{nullptr};
156 mi::EventBuilder* builder{nullptr};
157 mi::InputDeviceInfo device_info;
158 mir::optional_value<mi::PointerSettings> pointer_settings;
159
160=== modified file 'tests/unit-tests/input/test_nested_input_platform.cpp'
161--- tests/unit-tests/input/test_nested_input_platform.cpp 2017-05-08 03:04:26 +0000
162+++ tests/unit-tests/input/test_nested_input_platform.cpp 2017-06-23 16:26:45 +0000
163@@ -153,6 +153,8 @@
164 mock_host_connection.event_callback(*mev::make_event(a_mouse.id(), 12ns, cookie, mir_input_event_modifier_none,
165 mir_pointer_action_motion, mir_pointer_button_primary, 23, 42,
166 0, 0, 12, 10), source_surface);
167+
168+ nested_input_device->stop();
169 }
170
171 TEST_F(TestNestedInputPlatform, devices_forward_key_events)
172@@ -172,6 +174,7 @@
173
174 mock_host_connection.event_callback(*mev::make_event(a_keyboard.id(), 141ns, cookie, mir_keyboard_action_down, 0,
175 scan_code, mir_input_event_modifier_none), source_surface);
176+ nested_input_device->stop();
177 }
178
179 TEST_F(TestNestedInputPlatform, replaces_enter_events_as_motion_event)
180@@ -199,6 +202,7 @@
181 60.0f, 35.0f);
182
183 mock_host_connection.event_callback(*event, source_surface);
184+ nested_input_device->stop();
185 }
186
187 TEST_F(TestNestedInputPlatform, device_configurations_are_forwarded_to_host_connection)
188@@ -216,4 +220,5 @@
189 EXPECT_CALL(mock_host_connection, apply_input_configuration(_));
190
191 nested_input_device->apply_settings(left_handed);
192+ nested_input_device->stop();
193 }

Subscribers

People subscribed via source and target branches