Closed Bug 1348310 Opened 6 years ago Closed 6 years ago

Wayland - don't use CurrentX11TimeGetter

Categories

(Core :: Widget: Gtk, enhancement, P2)

Product:
Core
Component:
Widget: Gtk
Platform:
All
Linux
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox55 --- affected
firefox58 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug)

Details

(Whiteboard: tpi:+)

Attachments

(1 file)

Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,
59 bytes, text/x-review-board-request
karlt
: review+
Details 
      No description provided.
We need to update class CurrentX11TimeGetter for Wayland.
Summary: Wayland - update CurrentX11TimeGetter → Wayland - don't use CurrentX11TimeGetter
property_notify_event_cb() is never called on Wayland (there's no such concept of window properties) so the whole mechanism of getting X11 system timestamp does not work.

I don't know why it the gdk_x11_get_server_time() emulation used at CurrentX11TimeGetter (optimization?) but we can use clock_gettime(CLOCK_MONOTONIC) directly on Wayland/Linux.

Katl, you mentioned connection between the time stamps and XRemote. The XRemote is also unsupported on Wayland and we may need to implement the inter-process communication by our own, perhaps by D-BUS.
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review125792

::: widget/gtk/nsWindow.cpp:293
(Diff revision 1)
>  public:
>      explicit CurrentX11TimeGetter(GdkWindow* aWindow)
>          : mWindow(aWindow)
>          , mAsyncUpdateStart()
>      {
> +        mIsX11Display = GDK_IS_X11_DISPLAY(gdk_window_get_display(aWindow));

Isn't CurrentX11TimeGetter used only with X11?

If so, then there is no need to modify CurrentX11TimeGetter.

::: widget/gtk/nsWindow.cpp:2984
(Diff revision 1)
> -    MOZ_ASSERT(getCurrentTime,
> +        MOZ_ASSERT(getCurrentTime,
> -               "Null current time getter despite having a window");
> +                   "Null current time getter despite having a window");
> -    return TimeConverter().GetTimeStampFromSystemTime(aEventTime,
> +        return TimeConverter().GetTimeStampFromSystemTime(aEventTime,
> -                                                      *getCurrentTime);
> +                                                          *getCurrentTime);
> +    } else {
> +        //TODO - do we need to implement the CurrentX11TimeGetter for Wayland?

Instead of something like CurrentX11TimeGetter(), see the Mac and Android implementations, where the relationship between event timestamp and TimeStamp is known.
Attachment #8850458 - Flags: review?(karlt)
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review125792

> Instead of something like CurrentX11TimeGetter(), see the Mac and Android implementations, where the relationship between event timestamp and TimeStamp is known.

Android had used TimeStamp::Now() directly for the timestamp untill TextEventDispatcher was implemented (Bug 1137567). AFAIK we don't use the TextEventDispatcher in Gtk so we may go with simple TimeStamp::Now() call.
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review125792

> Android had used TimeStamp::Now() directly for the timestamp untill TextEventDispatcher was implemented (Bug 1137567). AFAIK we don't use the TextEventDispatcher in Gtk so we may go with simple TimeStamp::Now() call.

Ahh, I see TextEventDispatcher is actually used in Gtk...so we need to redesign it then.
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review125900
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review125902
Attachment #8850458 - Flags: review-
Attachment #8850458 - Flags: review?(karlt)
Attachment #8850458 - Flags: review-
Priority: -- → P2
Whiteboard: tpi:+
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review138008

::: widget/gtk/nsWindow.cpp:2966
(Diff revision 3)
> +        // Wayland uses SYSTEM_TIME_MONOTONIC.
> +        // Our posix implemententaion of TimeStamp::Now uses SYSTEM_TIME_MONOTONIC

SYSTEM_TIME_MONOTONIC is an Android concept AFAIK.
I think you mean CLOCK_MONOTONIC here and in the commit message.

::: widget/gtk/nsWindow.cpp:2966
(Diff revision 3)
>          // that they are synthetic events. Some input method editors do this.
>          // In this case too, just return the current timestamp.
>          return TimeStamp::Now();
>      }
> +    if (!mIsX11Display) {
> +        // Wayland uses SYSTEM_TIME_MONOTONIC.

https://wayland.freedesktop.org/docs/html/ch04.html#sect-Protocol-Input
says "Input events also carry timestamps with millisecond granularity. Their base is undefined, so they can't be compared against system time (as obtained with clock_gettime or gettimeofday)."

Hmmm.  If the base is undefined, then a variant of CurrentX11TimeGetter will be needed for Wayland.

Perhaps you mean that Weston uses CLOCK_MONOTONIC?

I had a quick look at Weston, and it looks like timestamps might be straight from libinput.

Is it reasonable to assume Weston and libinput?
Does libinput define its time base?

Is there someone more familiar with Wayland/Weston/libinput who can review the suitability of these assumptions?

::: widget/gtk/nsWindow.cpp:2969
(Diff revision 3)
> +        int64_t tick =
> +           BaseTimeDurationPlatformUtils::TicksFromMilliseconds(aEventTime);

aEventTime has been truncated to 32 bits, and so needs to be extended using a time that has not been truncated, before it can be converted to a tick, if the CLOCK_MONOTONIC assumption is valid.
Attachment #8850458 - Flags: review?(karlt)
Jan is going to look at it.
Assignee: nobody → jhorak
Taking back as Jan has plenty of reviews now :)
Assignee: jhorak → stransky
(In reply to Karl Tomlinson (:karlt) from comment #11)
> Comment on attachment 8850458 [details]
> Bug 1348310 - Use SYSTEM_TIME_MONOTONIC for nsWindow::GetEventTimeStamp() on
> Wayland,
> 
> https://reviewboard.mozilla.org/r/123054/#review138008
> 
> ::: widget/gtk/nsWindow.cpp:2966
> (Diff revision 3)
> > +        // Wayland uses SYSTEM_TIME_MONOTONIC.
> > +        // Our posix implemententaion of TimeStamp::Now uses SYSTEM_TIME_MONOTONIC
> 
> SYSTEM_TIME_MONOTONIC is an Android concept AFAIK.
> I think you mean CLOCK_MONOTONIC here and in the commit message.
> 
> ::: widget/gtk/nsWindow.cpp:2966
> (Diff revision 3)
> >          // that they are synthetic events. Some input method editors do this.
> >          // In this case too, just return the current timestamp.
> >          return TimeStamp::Now();
> >      }
> > +    if (!mIsX11Display) {
> > +        // Wayland uses SYSTEM_TIME_MONOTONIC.
> 
> https://wayland.freedesktop.org/docs/html/ch04.html#sect-Protocol-Input
> says "Input events also carry timestamps with millisecond granularity. Their
> base is undefined, so they can't be compared against system time (as
> obtained with clock_gettime or gettimeofday)."
> 
> Hmmm.  If the base is undefined, then a variant of CurrentX11TimeGetter will
> be needed for Wayland.
> 
> Perhaps you mean that Weston uses CLOCK_MONOTONIC?
> 
> I had a quick look at Weston, and it looks like timestamps might be straight
> from libinput.
> 
> Is it reasonable to assume Weston and libinput?
> Does libinput define its time base?
> 
> Is there someone more familiar with Wayland/Weston/libinput who can review
> the suitability of these assumptions?

We actually does not interact with Wayland but rather with Gdk backend for Wayland. I looked at gdkdevice-wayland.c and it uses just plain (guint32)(g_get_monotonic_time () / 1000) to set event time so we can for Gtk/Wayland combo.

> ::: widget/gtk/nsWindow.cpp:2969
> (Diff revision 3)
> > +        int64_t tick =
> > +           BaseTimeDurationPlatformUtils::TicksFromMilliseconds(aEventTime);
> 
> aEventTime has been truncated to 32 bits, and so needs to be extended using
> a time that has not been truncated, before it can be converted to a tick, if
> the CLOCK_MONOTONIC assumption is valid.
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review196334

::: widget/gtk/nsWindow.cpp:2966
(Diff revision 3)
>          // that they are synthetic events. Some input method editors do this.
>          // In this case too, just return the current timestamp.
>          return TimeStamp::Now();
>      }
> +    if (!mIsX11Display) {
> +        // Wayland uses SYSTEM_TIME_MONOTONIC.

Actually we don't need to care about Wayland itself as we communicate with Gtk/Gdk only and gdkdevice-wayland.c uses (guint32)(g_get_monotonic_time() / 1000) for the event timestamp.

::: widget/gtk/nsWindow.cpp:2966
(Diff revision 3)
> +        // Wayland uses SYSTEM_TIME_MONOTONIC.
> +        // Our posix implemententaion of TimeStamp::Now uses SYSTEM_TIME_MONOTONIC

Yes, CLOCK_MONOTONIC.

::: widget/gtk/nsWindow.cpp:2969
(Diff revision 3)
> +        int64_t tick =
> +           BaseTimeDurationPlatformUtils::TicksFromMilliseconds(aEventTime);

As Gdk uses the g_get_monotonic_time() / 1000 formula we don't have to extend that.
Attachment #8850458 - Flags: review-
Karl, I assigned you here as you did the last review...please let me know if you can't do that. Thanks!
Flags: needinfo?(karlt)
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review196350


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: widget/gtk/nsWindow.cpp:2994
(Diff revision 4)
> +        // gdkdevice-wayland.c uses (guint32)(g_get_monotonic_time() / 1000)
> +        // to set timestamps.
> +        int64_t tick =
> +            BaseTimeDurationPlatformUtils::TicksFromMilliseconds(aEventTime);
> +        return TimeStamp::FromSystemTime(tick);
> +    } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review196334

> Actually we don't need to care about Wayland itself as we communicate with Gtk/Gdk only and gdkdevice-wayland.c uses (guint32)(g_get_monotonic_time() / 1000) for the event timestamp.

pointer_handle_enter()/_leave() use (guint32)(g_get_monotonic_time () / 1000),
but other functions and events, including pointer_handle_motion() use the time
from Wayland.

emulate_crossing() uses times passed to gdk_pointer_grab() sometimes and times
from zwp_tablet_tool_v2 at other times.

You may like to argue that GDK is assuming that Wayland timestamps use the
same base as g_get_monotonic_time(), and so Gecko follows the example of GDK.

However, it is not correct to claim that all or even most timestamps are from
g_get_monotonic_time() calls in GDK.

> As Gdk uses the g_get_monotonic_time() / 1000 formula we don't have to extend that.

The purpose of this code is to generate a TimeStamp that can be compared with
other TimeStamps.  To get such a Timestamp from ticks, TicksFromMilliseconds()
must use milliseconds from the same base as Timestamp.  Truncated milliseconds
from GDK will give a different tick count and so different TimeStamp.
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review196566

::: commit-message-7513b:1
(Diff revision 4)
> +Bug 1348310 - Use CLOCK_MONOTONIC for Gtk/Wayland, r?karlt

It was helpful to include nsWindow::GetEventTimeStamp() in the first line to understand for what CLOCK_MONOTONIC was used.

::: commit-message-7513b:3
(Diff revision 4)
> +Emulate how gdkdevice-wayland.c sets timestamps for events on Wayland platform
> +instead of CurrentX11TimeGetter.

This code is not really emulating how gdkdevice-wayland.c sets timestamps but converting GDK event times by assuming they are derived from CLOCK_MONOTONIC due to similar assumptions about Wayland times implied by code in gdkdevice-wayland.c.

::: mozglue/misc/TimeStamp.h:425
(Diff revision 4)
>     * on platforms that support vsync aligned refresh drivers / compositors
>     * Verified true as of Jan 31, 2015: B2G and OS X
>     * False on Windows 7
>     * Android's event time uses CLOCK_MONOTONIC via SystemClock.uptimeMilles.
>     * So it is same value of TimeStamp posix implementation.
>     * UNTESTED ON OTHER PLATFORMS

The comment about GDK on Wayland was helpful to distinguish from X11.
Attachment #8850458 - Flags: review?(karlt) → review-
Flags: needinfo?(karlt)
(In reply to Karl Tomlinson (:karlt) from comment #19)
> Comment on attachment 8850458 [details]
> Bug 1348310 - Use CLOCK_MONOTONIC for Gtk/Wayland,
>
> https://reviewboard.mozilla.org/r/123054/#review196334
>
> > Actually we don't need to care about Wayland itself as we communicate with Gtk/Gdk only and gdkdevice-wayland.c uses (guint32)(g_get_monotonic_time() / 1000) for the event timestamp.
>
> pointer_handle_enter()/_leave() use (guint32)(g_get_monotonic_time () /
> 1000),
> but other functions and events, including pointer_handle_motion() use the
> time
> from Wayland.
>
> emulate_crossing() uses times passed to gdk_pointer_grab() sometimes and
> times
> from zwp_tablet_tool_v2 at other times.
>
> You may like to argue that GDK is assuming that Wayland timestamps use the
> same base as g_get_monotonic_time(), and so Gecko follows the example of GDK.
>
> However, it is not correct to claim that all or even most timestamps are from
> g_get_monotonic_time() calls in GDK.

Note that the "Wayland" is actually only a library which sits between Mutter/Clutter/Weston compositor and Gdk/Firefox so it's not much interesting for us here. We're generally getting event timestamps from kernel (fetched by libinput/evdev) or directly from compositor (the window events).

- sync events gdkwindow-wayland.c:fill_presentation_time_from_frame_time() clams usage of clock_gettime (CLOCK_MONOTONIC) for Weston/DRM (although Gtk+ uses mutter/clutter as compositor).

- plain mouse/keyboard event Gdk gets from wl_seat interface through callback, by wl_keyboard_listener/wl_pointer_listener interfaces. Those interfaces are fed by compositor so it depends on the compositor here (look for wl_keyboard_send_key() for keys for instance). As you said Weston uses libinput directly, mutter uses clutter with libinput/evdev as a backend here.

- it's not completely clear to me how timestamps for window event are generated by mutter. I suspect mutter-master-clock is involved here but haven't check that yet.

> > As Gdk uses the g_get_monotonic_time() / 1000 formula we don't have to extend that.
>
> The purpose of this code is to generate a TimeStamp that can be compared with
> other TimeStamps.  To get such a Timestamp from ticks,
> TicksFromMilliseconds()
> must use milliseconds from the same base as Timestamp.  Truncated
> milliseconds
> from GDK will give a different tick count and so different TimeStamp.

Fine, and how to extend the event time to get the correct timestamp when it's already truncated? I admin I didn't look at the CurrentX11TimeGetter() very deeply if that's the solution.
(In reply to Martin Stránský [:stransky] from comment #21)
> (In reply to Karl Tomlinson (:karlt) from comment #19)
> > The purpose of this code is to generate a TimeStamp that can be compared
> > with other TimeStamps.  To get such a Timestamp from ticks,
> > TicksFromMilliseconds() must use milliseconds from the same base as
> > Timestamp.  Truncated milliseconds from GDK will give a different tick
> > count and so different TimeStamp.
> 
> Fine, and how to extend the event time to get the correct timestamp when
> it's already truncated?

Assume that the event time was within the last 2^32 milliseconds, or a shorter
period of time if the event time may be in the future.  Let |refTime| be the
number of milliseconds from the base time to the current time.  Truncating
this gives a number that can be compared with the event time to find a
difference.  That difference can then be applied to |refTime| to give an
un-truncated number of milliseconds corresponding to the event time.
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review198932


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: widget/gtk/nsWindow.cpp:3002
(Diff revision 5)
> +        int64_t timestampTime =
> +            refTime - (refTimeTruncated - int64_t(aEventTime));
> +        int64_t tick =
> +            BaseTimeDurationPlatformUtils::TicksFromMilliseconds(timestampTime);
> +        return TimeStamp::FromSystemTime(tick);
> +    } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review199842

::: widget/gtk/nsWindow.cpp:2996
(Diff revisions 4 - 5)
> +        int64_t refTimeTruncated = guint32(refTime);
> +        int64_t timestampTime =
> +            refTime - (refTimeTruncated - int64_t(aEventTime));

Make refTimeTruncated guint32 and subtract the guint32 aEventTime so that the difference is performed using modulo arithmetic to appropriately handle when refTimeTruncated < aEventTime (when refTimeTruncated has wrapped one more time than aEventTime).
Attachment #8850458 - Flags: review?(karlt) → review-
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review199842

> Make refTimeTruncated guint32 and subtract the guint32 aEventTime so that the difference is performed using modulo arithmetic to appropriately handle when refTimeTruncated < aEventTime (when refTimeTruncated has wrapped one more time than aEventTime).

Good point, Thanks!
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review199968


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: widget/gtk/nsWindow.cpp:3002
(Diff revision 6)
> +
> +        timestampTime -= refTimeTruncated - aEventTime;
> +        int64_t tick =
> +            BaseTimeDurationPlatformUtils::TicksFromMilliseconds(timestampTime);
> +        return TimeStamp::FromSystemTime(tick);
> +    } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review200226

Please remove the else after return flagged by clang-tidy.  IMO it doesn't
make a big difference to readability when the blocks both return at the end of
a function, but addressing the automated review now will mean that it doesn't
flag an issue on the next person to change these lines.

https://clang.llvm.org/extra/clang-tidy/checks/readability-else-after-return.html
Attachment #8850458 - Flags: review?(karlt) → review+
Comment on attachment 8850458 [details]
Bug 1348310 - Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland,

https://reviewboard.mozilla.org/r/123054/#review200226

Fixed.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/99f78ebf8d65
Use CLOCK_MONOTONIC as a base for nsWindow::GetEventTimeStamp() on Wayland, r=karlt
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/99f78ebf8d65
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.