Merge #2590

2590: Fix timestamp calibration  r=AlanGriffiths a=wmww

Fixes #2577

Prevent timestamps in the future ever being sent, and use unmodified timestamps if they're close to Mir's internal timestamps.

Co-authored-by: Sophie Winter <[email protected]>

src/server/input/default_event_builder.cpp

@@ -28,13 +28,11 @@ namespace mi = mir::input;
 mi::DefaultEventBuilder::DefaultEventBuilder(
     MirInputDeviceId device_id,
     std::shared_ptr<time::Clock> const& clock,
-    std::shared_ptr<mir::cookie::Authority> const& cookie_authority,
-    std::shared_ptr<mi::Seat> const& seat)
+    std::shared_ptr<mir::cookie::Authority> const& cookie_authority)
     : device_id(device_id),
       clock(clock),
       timestamp_offset(Timestamp::max()),
-      cookie_authority(cookie_authority),
-      seat(seat)
+      cookie_authority(cookie_authority)
 {
 }
 
@@ -203,16 +201,32 @@ mir::EventUPtr mi::DefaultEventBuilder::touch_event(
 
 auto mi::DefaultEventBuilder::calibrate_timestamp(std::optional<Timestamp> timestamp) -> Timestamp
 {
+    using namespace std::chrono_literals;
+
+    auto const now = clock->now().time_since_epoch();
+    auto offset = timestamp_offset.load();
     if (!timestamp)
     {
-        return clock->now().time_since_epoch();
+        return now;
     }
-    auto offset = timestamp_offset.load();
-    if (offset == Timestamp::max())
+    else
     {
-        // If used from multiple threads this could happen multiple times at once, but that's not a problem
-        offset = clock->now().time_since_epoch() - timestamp.value();
-        timestamp_offset.store(offset);
+        // If the offset has not been set yet or timestamp + offset is in the future, the offset needs to be set
+        if (offset == Timestamp::max() || timestamp.value() + offset > now)
+        {
+            // If the timestamp is in the future or more than 1 second in the past, the platform must be using a
+            // different time base
+            if (timestamp.value() > now || timestamp.value() < now - 1s)
+            {
+                // Therefore the offset should be set to give the event the current time
+                timestamp_offset = offset = now - timestamp.value();
+            }
+            else
+            {
+                // Otherwise, the platform is using the same time base so it's timestamps should be unchanged
+                timestamp_offset = offset = 0s;
+            }
+        }
+        return timestamp.value() + offset;
     }
-    return timestamp.value() + offset;
 }

src/server/input/default_event_builder.h

@@ -41,8 +41,7 @@ class DefaultEventBuilder : public EventBuilder
     explicit DefaultEventBuilder(
         MirInputDeviceId device_id,
         std::shared_ptr<time::Clock> const& clock,
-        std::shared_ptr<cookie::Authority> const& cookie_authority,
-        std::shared_ptr<Seat> const& seat);
+        std::shared_ptr<cookie::Authority> const& cookie_authority);
 
     EventUPtr key_event(
         std::optional<Timestamp> source_timestamp,
@@ -113,7 +112,6 @@ class DefaultEventBuilder : public EventBuilder
     /// Added to input timestams to get calibrated timestamps for events. Is Timestamp::max() until initial event.
     std::atomic<Timestamp> timestamp_offset;
     std::shared_ptr<cookie::Authority> const cookie_authority;
-    std::shared_ptr<Seat> const seat;
 };
 }
 }

src/server/input/default_input_device_hub.cpp

@@ -325,7 +325,7 @@ void mi::DefaultInputDeviceHub::RegisteredDevice::start(std::shared_ptr<Seat> co
     multiplexer->add_watch(queue);
 
     this->seat = seat;
-    builder = std::make_unique<DefaultEventBuilder>(device_id, clock, cookie_authority, seat);
+    builder = std::make_unique<DefaultEventBuilder>(device_id, clock, cookie_authority);
     device->start(this, builder.get());
 }
 

tests/include/mir/test/doubles/advanceable_clock.h

@@ -18,8 +18,6 @@
 #define MIR_TEST_DOUBLES_ADVANCEABLE_CLOCK_H_
 
 #include "mir/time/steady_clock.h"
-#include <functional>
-#include <list>
 #include <mutex>
 
 namespace mir
@@ -32,6 +30,16 @@ namespace doubles
 class AdvanceableClock : public mir::time::Clock
 {
 public:
+    AdvanceableClock()
+        : current_time{mir::time::SteadyClock{}.now()}
+    {
+    }
+
+    AdvanceableClock(mir::time::Timestamp start_time)
+        : current_time{start_time}
+    {
+    }
+
     mir::time::Timestamp now() const override
     {
         std::lock_guard lock{mutex};
@@ -50,14 +58,8 @@ class AdvanceableClock : public mir::time::Clock
     }
 
 private:
-    mutable std::recursive_mutex mutex;
-    mir::time::Timestamp current_time{
-        []
-        {
-           mir::time::SteadyClock clock;
-           return clock.now();
-        }()
-        };
+    mutable std::mutex mutex;
+    mir::time::Timestamp current_time;
 };
 
 }

tests/unit-tests/input/CMakeLists.txt

@@ -18,6 +18,7 @@ list(APPEND UNIT_TEST_SOURCES
   ${CMAKE_CURRENT_SOURCE_DIR}/test_idle_poking_dispatcher.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_validator.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_buffer_keymap.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_default_event_builder.cpp
 )
 
 list(APPEND UMOCK_UNIT_TEST_SOURCES

tests/unit-tests/input/evdev/test_libinput_device.cpp

@@ -80,8 +80,7 @@ struct MockEventBuilder : mi::EventBuilder
     mi::DefaultEventBuilder builder{
         MirInputDeviceId{3},
         mt::fake_shared(clock),
-        cookie_authority,
-        mt::fake_shared(seat)};
+        cookie_authority};
     MockEventBuilder()
     {
         ON_CALL(*this, key_event(_,_,_,_)).WillByDefault(

tests/unit-tests/input/test_default_event_builder.cpp

@@ -0,0 +1,97 @@
+/*
+ * Copyright © 2022 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "src/server/input/default_event_builder.h"
+#include "mir/cookie/authority.h"
+
+#include "mir/test/doubles/advanceable_clock.h"
+#include "mir/test/fake_shared.h"
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+namespace mi = mir::input;
+namespace mev = mir::events;
+namespace mt = mir::test;
+namespace mtd = mt::doubles;
+
+using namespace ::testing;
+using namespace std::chrono_literals;
+
+namespace
+{
+
+struct DefaultEventBuilder : public Test
+{
+    mtd::AdvanceableClock clock{{}};
+    mir::input::DefaultEventBuilder builder{
+        0,
+        mt::fake_shared(clock),
+        mir::cookie::Authority::create()};
+
+    auto event_timestamp(std::optional<std::chrono::nanoseconds> timestamp) -> std::chrono::nanoseconds
+    {
+        auto const ev = builder.key_event(timestamp, mir_keyboard_action_down, 0, 0);
+        return std::chrono::nanoseconds{mir_input_event_get_event_time(mir_event_get_input_event(ev.get()))};
+    }
+};
+}
+
+TEST_F(DefaultEventBuilder, when_no_timestamp_is_given_then_clock_timestamp_is_used)
+{
+    clock.advance_by(12s);
+    EXPECT_THAT(event_timestamp(std::nullopt), Eq(12s));
+}
+
+TEST_F(DefaultEventBuilder, when_no_timestamp_is_given_then_clock_timestamp_is_updated_and_used)
+{
+    clock.advance_by(12s);
+    event_timestamp(std::nullopt);
+    clock.advance_by(2s);
+    EXPECT_THAT(event_timestamp(std::nullopt), Eq(14s));
+}
+
+TEST_F(DefaultEventBuilder, when_timestamp_matches_clock_then_correct_timestamp_used)
+{
+    clock.advance_by(12s);
+    EXPECT_THAT(event_timestamp(12s), Eq(12s));
+}
+
+TEST_F(DefaultEventBuilder, when_timestamp_is_slightly_older_than_clock_then_given_timestamp_is_used)
+{
+    clock.advance_by(12s);
+    EXPECT_THAT(event_timestamp(12s - 10ms), Eq(12s - 10ms));
+}
+
+TEST_F(DefaultEventBuilder, when_timestamp_is_slightly_newer_than_clock_then_clock_timestamp_is_used)
+{
+    clock.advance_by(12s);
+    EXPECT_THAT(event_timestamp(12s + 10ms), Eq(12s));
+}
+
+TEST_F(DefaultEventBuilder, when_timestamp_is_much_older_than_clock_then_clock_timestamp_is_used)
+{
+    clock.advance_by(400s);
+    EXPECT_THAT(event_timestamp(20s), Eq(400s));
+}
+
+TEST_F(DefaultEventBuilder, when_timestamp_is_much_older_than_clock_then_offset_persists)
+{
+    clock.advance_by(400s);
+    event_timestamp(20s);
+    clock.advance_by(2s);
+    EXPECT_THAT(event_timestamp(22s - 10ms), Eq(402s - 10ms));
+}

tests/unit-tests/input/test_seat_input_device_tracker.cpp

@@ -21,7 +21,6 @@
 #include "mir/test/doubles/mock_input_dispatcher.h"
 #include "mir/test/doubles/mock_cursor_listener.h"
 #include "mir/test/doubles/mock_touch_visualizer.h"
-#include "mir/test/doubles/mock_input_seat.h"
 #include "mir/test/doubles/mock_seat_report.h"
 #include "mir/test/doubles/advanceable_clock.h"
 #include "mir/test/event_matchers.h"
@@ -51,7 +50,6 @@ struct SeatInputDeviceTracker : ::testing::Test
     Nice<mtd::MockInputDispatcher> mock_dispatcher;
     Nice<mtd::MockCursorListener> mock_cursor_listener;
     Nice<mtd::MockTouchVisualizer> mock_visualizer;
-    Nice<mtd::MockInputSeat> mock_seat;
     Nice<mtd::MockSeatObserver> mock_seat_report;
     MirInputDeviceId some_device{8712};
     MirInputDeviceId another_device{1246};
@@ -62,18 +60,15 @@ struct SeatInputDeviceTracker : ::testing::Test
     mi::DefaultEventBuilder some_device_builder{
         some_device,
         mt::fake_shared(clock),
-        cookie_factory,
-        mt::fake_shared(mock_seat)};
+        cookie_factory};
     mi::DefaultEventBuilder another_device_builder{
         another_device,
         mt::fake_shared(clock),
-        cookie_factory,
-        mt::fake_shared(mock_seat)};
+        cookie_factory};
     mi::DefaultEventBuilder third_device_builder{
         third_device,
         mt::fake_shared(clock),
-        cookie_factory,
-        mt::fake_shared(mock_seat)};
+        cookie_factory};
     mi::receiver::XKBMapper mapper;
     mi::SeatInputDeviceTracker tracker{
         mt::fake_shared(mock_dispatcher), mt::fake_shared(mock_visualizer), mt::fake_shared(mock_cursor_listener),

tests/unit-tests/input/test_x11_platform.cpp

@@ -57,8 +57,7 @@ struct X11PlatformTest : ::testing::Test
     mir::input::DefaultEventBuilder builder{
         0,
         mt::fake_shared(clock),
-        mir::cookie::Authority::create(),
-        mt::fake_shared(mock_seat)};
+        mir::cookie::Authority::create()};
 
     mir::input::X::XInputPlatform x11_platform{
         mt::fake_shared(mock_registry),