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

[Alan Griffiths]

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.

[Mir CI Bot (mir-ci-bot)]

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)]

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)]

lgtm

review: Approve

=== modified file 'src/server/graphics/nested/input_platform.cpp'
--- src/server/graphics/nested/input_platform.cpp	2017-05-08 03:04:26 +0000
+++ src/server/graphics/nested/input_platform.cpp	2017-06-23 16:26:45 +0000
@@ -33,6 +33,9 @@
 #include "mir/events/event_builders.h"
 
 #include <chrono>
+#include <condition_variable>
+#include <thread>
+#include <queue>
 
 namespace mi = mir::input;
 namespace mgn = mir::graphics::nested;
@@ -52,9 +55,68 @@
     return mgn::UniqueInputConfig(nullptr, [](MirInputConfig const*){});
 }
 
-}
-
-struct mgn::InputPlatform::InputDevice : mi::InputDevice
+class Postman
+{
+public:
+
+    void start_work();
+    void post(std::shared_ptr<MirEvent> const& event);
+    void stop_work();
+
+    mi::InputSink* destination{nullptr};
+
+private:
+    using EventQueue = std::queue<std::shared_ptr<MirEvent>>;
+
+    std::thread worker_thread;
+    std::mutex mutable mutex;
+    std::condition_variable work_cv;
+    EventQueue events;
+
+    void do_work();
+
+    std::shared_ptr<MirEvent> next_event();
+};
+
+void Postman::do_work()
+{
+    while (auto const event = next_event())
+        destination->handle_input(*event);
+}
+
+std::shared_ptr<MirEvent> Postman::next_event()
+{
+    EventQueue::value_type event;
+    {
+        std::unique_lock<decltype(mutex)> lock{mutex};
+        work_cv.wait(lock, [this] { return !events.empty(); });
+        event.swap(events.front());
+        events.pop();
+    }
+    return event;
+}
+
+void Postman::post(std::shared_ptr<MirEvent> const& event)
+{
+    std::lock_guard<decltype(mutex)> lock{mutex};
+    events.push(event);
+    work_cv.notify_one();
+}
+
+void Postman::start_work()
+{
+    worker_thread = std::thread([this] { do_work(); });
+}
+
+void Postman::stop_work()
+{
+    // We use a null event as a sentinel
+    post({});
+    worker_thread.join();
+}
+}
+
+struct mgn::InputPlatform::InputDevice : mi::InputDevice, Postman
 {
 public:
     InputDevice(MirInputDevice* dev, std::function<void()> config_change)
@@ -119,6 +181,12 @@
     {
         this->destination = destination;
         this->builder = builder;
+        start_work();
+    }
+
+    void post_event(std::shared_ptr<MirEvent> const event)
+    {
+        post(event);
     }
 
     void emit_event(MirInputEvent const* event, mir::geometry::Rectangle const& frame)
@@ -147,20 +215,19 @@
                     contact.touch_major = mir_touch_event_axis_value(touch_event, i, mir_touch_axis_touch_major);
                     contact.touch_minor = mir_touch_event_axis_value(touch_event, i, mir_touch_axis_touch_minor);
                 }
-                destination->handle_input(*builder->touch_event(event_time, contacts));
+                post_event(builder->touch_event(event_time, contacts));
                 break;
             }
         case mir_input_event_type_key:
             {
                 auto const* key_event = mir_input_event_get_keyboard_event(event);
 
-                auto new_kev = builder->key_event(
+                post_event(builder->key_event(
                         event_time,
                         mir_keyboard_event_action(key_event),
                         mir_keyboard_event_key_code(key_event),
                         mir_keyboard_event_scan_code(key_event)
-                        );
-                destination->handle_input(*new_kev);
+                        ));
                 break;
             }
         case mir_input_event_type_pointer:
@@ -168,7 +235,7 @@
                 auto const* pointer_event = mir_input_event_get_pointer_event(event);
                 auto x = mir_pointer_event_axis_value(pointer_event, mir_pointer_axis_x);
                 auto y = mir_pointer_event_axis_value(pointer_event, mir_pointer_axis_y);
-                auto new_event = builder->pointer_event(
+                post_event(builder->pointer_event(
                         event_time,
                         filter_pointer_action(mir_pointer_event_action(pointer_event)),
                         mir_pointer_event_buttons(pointer_event),
@@ -178,9 +245,7 @@
                         mir_pointer_event_axis_value(pointer_event, mir_pointer_axis_vscroll),
                         mir_pointer_event_axis_value(pointer_event, mir_pointer_axis_relative_x),
                         mir_pointer_event_axis_value(pointer_event, mir_pointer_axis_relative_y)
-                        );
-
-                destination->handle_input(*new_event);
+                        ));
                 break;
             }
         default:
@@ -190,6 +255,7 @@
 
     void stop() override
     {
+        stop_work();
         this->destination = nullptr;
         this->builder = nullptr;
     }
@@ -272,7 +338,6 @@
 
     MirInputDeviceId device_id;
     MirInputDevice* device{nullptr};
-    mi::InputSink* destination{nullptr};
     mi::EventBuilder* builder{nullptr};
     mi::InputDeviceInfo device_info;
     mir::optional_value<mi::PointerSettings> pointer_settings;

=== modified file 'tests/unit-tests/input/test_nested_input_platform.cpp'
--- tests/unit-tests/input/test_nested_input_platform.cpp	2017-05-08 03:04:26 +0000
+++ tests/unit-tests/input/test_nested_input_platform.cpp	2017-06-23 16:26:45 +0000
@@ -153,6 +153,8 @@
     mock_host_connection.event_callback(*mev::make_event(a_mouse.id(), 12ns, cookie, mir_input_event_modifier_none,
                                                          mir_pointer_action_motion, mir_pointer_button_primary, 23, 42,
                                                          0, 0, 12, 10), source_surface);
+
+    nested_input_device->stop();
 }
 
 TEST_F(TestNestedInputPlatform, devices_forward_key_events)
@@ -172,6 +174,7 @@
 
     mock_host_connection.event_callback(*mev::make_event(a_keyboard.id(), 141ns, cookie, mir_keyboard_action_down, 0,
                                                          scan_code, mir_input_event_modifier_none), source_surface);
+    nested_input_device->stop();
 }
 
 TEST_F(TestNestedInputPlatform, replaces_enter_events_as_motion_event)
@@ -199,6 +202,7 @@
                                  60.0f, 35.0f);
 
     mock_host_connection.event_callback(*event, source_surface);
+    nested_input_device->stop();
 }
 
 TEST_F(TestNestedInputPlatform, device_configurations_are_forwarded_to_host_connection)
@@ -216,4 +220,5 @@
     EXPECT_CALL(mock_host_connection, apply_input_configuration(_));
 
     nested_input_device->apply_settings(left_handed);
+    nested_input_device->stop();
 }