From 89ad66fbbd38c1ba750a1243ef2ae533e656e278 Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Tue, 9 Jan 2024 10:48:03 +1100 Subject: [PATCH] Fix #1092563: invalid event timestamps on X11 after 49 days uptime Regression from [0] based on the incorrect assumption that X11's Time was a uint64. Despite the `Time` type being 8 bytes, the value wraps at UINT32_MAX. Details: - GHOST_SystemX11::m_start_time now uses CLOCK_MONOTONIC instead of gettimeofday(..) since event times should always increase. - Event timestamps now accumulate uint32 rollover. [0]: efef709ec73ce95817a82e4e26cf7ec0bad8eca0 --- intern/ghost/intern/GHOST_SystemX11.cc | 78 +++++++++++++++++++------- intern/ghost/intern/GHOST_SystemX11.hh | 4 +- 2 files changed, 59 insertions(+), 23 deletions(-) diff --git a/intern/ghost/intern/GHOST_SystemX11.cc b/intern/ghost/intern/GHOST_SystemX11.cc index 66e1d1d81ac..b6917d028ae 100644 --- a/intern/ghost/intern/GHOST_SystemX11.cc +++ b/intern/ghost/intern/GHOST_SystemX11.cc @@ -107,7 +107,6 @@ GHOST_SystemX11::GHOST_SystemX11() : GHOST_System(), m_xkb_descr(nullptr), m_start_time(0), - m_start_time_monotonic(0), m_keyboard_vector{0}, #ifdef WITH_X11_XINPUT m_last_key_time(0), @@ -176,22 +175,12 @@ GHOST_SystemX11::GHOST_SystemX11() m_last_release_keycode = 0; m_last_release_time = 0; - /* Compute the initial times. */ - { - timeval tv; - if (gettimeofday(&tv, nullptr) != 0) { - GHOST_ASSERT(false, "Could not instantiate timer!"); - } - /* Taking care not to overflow the `tv.tv_sec * 1000`. */ - m_start_time = uint64_t(tv.tv_sec) * 1000 + tv.tv_usec / 1000; - } - { timespec ts = {0, 0}; if (clock_gettime(CLOCK_MONOTONIC, &ts) != 0) { GHOST_ASSERT(false, "Could not instantiate monotonic timer!"); } - m_start_time_monotonic = (uint64_t(ts.tv_sec) * 1000) + uint64_t(ts.tv_nsec / 1000000); + m_start_time = (uint64_t(ts.tv_sec) * 1000) + uint64_t(ts.tv_nsec / 1000000); } /* Use detectable auto-repeat, mac and windows also do this. */ @@ -283,24 +272,73 @@ GHOST_TSuccess GHOST_SystemX11::init() uint64_t GHOST_SystemX11::getMilliSeconds() const { - timeval tv; - if (gettimeofday(&tv, nullptr) != 0) { - GHOST_ASSERT(false, "Could not compute time!"); + timespec ts = {0, 0}; + if (clock_gettime(CLOCK_MONOTONIC, &ts) != 0) { + GHOST_ASSERT(false, "Could not instantiate monotonic timer!"); } - /* Taking care not to overflow the tv.tv_sec * 1000 */ - return uint64_t(tv.tv_sec) * 1000 + tv.tv_usec / 1000 - m_start_time; + const uint64_t time = (uint64_t(ts.tv_sec) * 1000) + uint64_t(ts.tv_nsec / 1000000); + GHOST_ASSERT(m_start_time <= time, "Monotonic time unexpectedly went backwards!"); + return time - m_start_time; } -uint64_t GHOST_SystemX11::ms_from_input_time(const Time timestamp) const +uint64_t GHOST_SystemX11::ms_from_input_time(Time timestamp) const { - GHOST_ASSERT(timestamp >= m_start_time_monotonic, "Invalid time-stamp"); /* NOTE(@ideasman42): Return a time compatible with `getMilliSeconds()`, * this is needed as X11 time-stamps use monotonic time. * The X11 implementation *could* use any basis, in practice though we are supporting * XORG/LIBINPUT which uses time-stamps based on the monotonic time, * Needed to resolve failure to detect double-clicking, see: #40009. */ - return uint64_t(timestamp) - m_start_time_monotonic; + + /* Accumulate time rollover (as well as store the initial delta from `m_start_time`). */ + static uint64_t timestamp_offset = 0; + + /* The last event time (to detect rollover). */ + static uint32_t timestamp_prev = 0; + /* Causes the X11 time-stamp to be zero based. */ + static uint32_t timestamp_start = 0; + + static bool is_time_init = false; + +#if 0 + /* Force rollover after 2 seconds (for testing). */ + { + const uint32_t timestamp_wrap_ms = 2000; + static uint32_t timestamp_offset_fake = 0; + if (!is_time_init) { + timestamp_offset_fake = UINT32_MAX - (timestamp + timestamp_wrap_ms); + } + timestamp = uint32_t(timestamp + timestamp_offset_fake); + } +#endif + + if (!is_time_init) { + /* Store the initial delta in the rollover. */ + const uint64_t current_time = getMilliSeconds(); + timestamp_offset = current_time; + timestamp_start = timestamp; + } + + /* Always remove the start time. + * This changes the point where `uint32_t` rolls over, but that's OK. */ + timestamp = uint32_t(timestamp) - timestamp_start; + + if (!is_time_init) { + is_time_init = true; + timestamp_prev = timestamp; + } + + if (UNLIKELY(timestamp < timestamp_prev)) { + /* Only rollover if this is within a reasonable range. */ + if (UNLIKELY(timestamp_prev - timestamp > UINT32_MAX / 2)) { + timestamp_offset += uint64_t(UINT32_MAX) + 1; + } + } + timestamp_prev = timestamp; + + uint64_t timestamp_final = (uint64_t(timestamp) + timestamp_offset); + + return timestamp_final; } uint8_t GHOST_SystemX11::getNumDisplays() const diff --git a/intern/ghost/intern/GHOST_SystemX11.hh b/intern/ghost/intern/GHOST_SystemX11.hh index 7358c169d40..87f689982a3 100644 --- a/intern/ghost/intern/GHOST_SystemX11.hh +++ b/intern/ghost/intern/GHOST_SystemX11.hh @@ -348,10 +348,8 @@ class GHOST_SystemX11 : public GHOST_System { /** The vector of windows that need to be updated. */ std::vector m_dirty_windows; - /** Start time at initialization. */ + /** Start time at initialization (using `CLOCK_MONOTONIC`). */ uint64_t m_start_time; - /** Start time at initialization (using `CLOCK_MONOTONIC`). */ - uint64_t m_start_time_monotonic; /** A vector of keyboard key masks. */ char m_keyboard_vector[32];