Closed Bug 1256565 Opened 8 years ago Closed 7 years ago

Convert native event times to TimeStamps for Android

Categories

(Core Graveyard :: Widget: Android, defect)

Product:
Core Graveyard
Component:
Widget: Android
Platform:
Unspecified
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox48 affected, firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox48 --- affected
firefox54 --- fixed

People

(Reporter: birtles, Assigned: m_kato)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1256565 - Part 1. Implement GetEventTimeStamp().
59 bytes, text/x-review-board-request
jchen
: review+
karlt
: review+
Details
Bug 1256565 - Part 2. Use GetEventTimeStamp() for timestamp.
59 bytes, text/x-review-board-request
jchen
: review+
Details
Bug 1256565 - Part 3. Turn on highrestimestamp on Android Nightly.
59 bytes, text/x-review-board-request
smaug
: review+
Details 
As with bug 1026803 and bug 77992, we need to convert events times provided by the OS to corresponding TimeStamp values.

I'm not sure what is needed for Android, if anything.
Flags: needinfo?(ajones)
Component: DOM: Events → Widget: Android
Removing stale needinfo.

It looks like right now a lot of the events dispatched by the Android widget don't have mTime values, and none of them seem to have mTimeStamp values. The mTime values are mostly on the APZ input events, and come from the Android MotionEvent instances, using the code at [1]. I don't know what the requirements are on mTime and mTimeStamp (are they meant to be based on device time, or uptime? are they allowed to be not-strictly-increasing?) but there might need to be some additional processing to satisfy those requirements.

[1] http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoEvent.java#154
Flags: needinfo?(ajones)
DOM will convert mTimeStamp to a value in the same coordinate as Performance.now().
If events can get out of order, then I guess values could decrease, but ideally events would not get out of order.  Multiple events relating to the same instant of user action would have the same value (not increasing).
mTime is not well defined.
So it looks like the current mTime timestamps on the events created by Android widgetry are mostly a mess. We should just set mTimestamp to TimeStamp::Now() when we create the event in widget code. Performance.now() also looks to be based on TimeStamp::Now() so that should make it consistent.
James - we need to figure out if we need to do anything on Android. Who is the expert in Android widgetry?
Flags: needinfo?(snorp)
See Also: → 1261894
Jim probably knows the most at this point.
Flags: needinfo?(snorp) → needinfo?(nchen)
1) What happens if we don't do anything for Android? Do we just fall back to TimeStamp::Now() for mTimeStamp?

2) Do we require high-res times? A lot of event times we do get on Android (e.g. KeyEvent and MotionEvent) are in milliseconds, so not really high-res.

3) In addition to OS events, the Android widget code also synthesizes some events (e.g. key events). Should synthesized events just use TimeStamp::Now()? Or use the time for whatever action led to the events being synthesized? Do we have to use the same clock source for both synthesized events and OS events, so they have matching timing characteristics?

Thanks!
Flags: needinfo?(nchen) → needinfo?(bbirtles)
(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> 1) What happens if we don't do anything for Android? Do we just fall back to
> TimeStamp::Now() for mTimeStamp?

Yes.

> 2) Do we require high-res times? A lot of event times we do get on Android
> (e.g. KeyEvent and MotionEvent) are in milliseconds, so not really high-res.

The idea is just that converting a timestamp from a UI event which was set at the time the UI event was originally created to a TimeStamp is likely to be more accurate than using TimeStamp::Now() at the time when we receive the event in Gecko. I suspect that main situation where that matters is when we're flooded with events or are otherwise under load such that we can't promptly pass on each event. In that case I suppose even millisecond precision would be better than TimeStamp::Now().

> 3) In addition to OS events, the Android widget code also synthesizes some
> events (e.g. key events). Should synthesized events just use
> TimeStamp::Now()? Or use the time for whatever action led to the events
> being synthesized? Do we have to use the same clock source for both
> synthesized events and OS events, so they have matching timing
> characteristics?

I'm not familiar with which events we synthesize and for what reasons but I think the main quality we want to preserve is ordering of events. If we have a series of events of the same logical source (e.g. all keyboard events), some of which are synthesized and some of which are generated from OS events, then ideally the timestamps should correspond to the order in which the events are queued (i.e. increasing order). Is that do-able?

I think that's right but Karl would probably remember better than I do.
Flags: needinfo?(bbirtles)
Assignee: nobody → m_kato
Comment on attachment 8821944 [details]
Bug 1256565 - Part 2. Use GetEventTimeStamp() for timestamp.

https://reviewboard.mozilla.org/r/101016/#review101614
Attachment #8821944 - Flags: review?(nchen) → review+
Comment on attachment 8821943 [details]
Bug 1256565 - Part 1. Implement GetEventTimeStamp().

https://reviewboard.mozilla.org/r/101014/#review101616

I think Brian will know better whether to implement TimeGetter or not.
Attachment #8821943 - Flags: review?(nchen) → review+
Flags: needinfo?(bbirtles)
Comment on attachment 8821945 [details]
Bug 1256565 - Part 3. Turn on highrestimestamp on Android Nightly.

https://reviewboard.mozilla.org/r/101018/#review101626

Do we have a bug for enabling this stuff on beta/release?
IIRC, Chrome has been shipping highres timestamps for several months. Would be good to verify our implementation is compatible with theirs.
Attachment #8821945 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #14)
> Do we have a bug for enabling this stuff on beta/release?

That would be bug 1026804.
> On Nexus 5X with Android 7.1, delta will be 5-10ms.  So, if it is OK not to implement TimeGetter, I will return TimeStamp() instead.  Jim and Brian, how do you think?

That's a pretty big delta. But the real question is, does that delta grow over time? The SystemTimeConverter machinery is supposed to detect gradual skew between time sources.

By not implementing the TimeGetter do you mean, not doing an async time get? Is there no way to request the time asynchronously, e.g. by posting a message and checking its event time?
Flags: needinfo?(bbirtles)
Comment on attachment 8821943 [details]
Bug 1256565 - Part 1. Implement GetEventTimeStamp().

https://reviewboard.mozilla.org/r/101014/#review102748

::: widget/android/nsWindow.cpp:127
(Diff revision 1)
> +
> +    uint64_t GetCurrentTime() const
> +    {
> +        int64_t uptime = 0;
> +        if (NS_FAILED(java::sdk::SystemClock::UptimeMillis(&uptime))) {
> +            MOZ_CRASH("Canot get uptimeMillis via JNI");

Nit: s/Canot/Cannot/
(In reply to Brian Birtles (:birtles) from comment #16)
> > On Nexus 5X with Android 7.1, delta will be 5-10ms.  So, if it is OK not to implement TimeGetter, I will return TimeStamp() instead.  Jim and Brian, how do you think?
> 
> That's a pretty big delta. But the real question is, does that delta grow
> over time? The SystemTimeConverter machinery is supposed to detect gradual
> skew between time sources.

delta won't grow. current Andoid implementation uses JNI directory now, so until dispatching to Gecko's thread, we don't use event queue.

> By not implementing the TimeGetter do you mean, not doing an async time get?
> Is there no way to request the time asynchronously, e.g. by posting a
> message and checking its event time?

It is no way to use via async and calculation of time delta is Java's UI thread, not Gecko thread.  If we need async implementation, we must create worker thread for this.
Comment on attachment 8821943 [details]
Bug 1256565 - Part 1. Implement GetEventTimeStamp().

brian requests additional review to karl.
Attachment #8821943 - Flags: review?(karlt)
Comment on attachment 8821943 [details]
Bug 1256565 - Part 1. Implement GetEventTimeStamp().

(In reply to Makoto Kato [:m_kato] from comment #18)
> delta won't grow. current Andoid implementation uses JNI directory now, so
> until dispatching to Gecko's thread, we don't use event queue.

If delta won't grow, then I expect SystemTimeConverter is not necessary.

(In reply to Makoto Kato [:m_kato] from comment #9)
> Android uses android.os.SystemClock.uptimeMilles for event time.

uptimeMillis sounds like CLOCK_MONOTONIC, and the source code confirms this:
https://android.googlesource.com/platform/frameworks/native/+/jb-dev/libs/utils/SystemClock.cpp#100
https://android.googlesource.com/platform/frameworks/native/+/jb-dev/libs/utils/Timers.cpp#40

Timestamp also uses CLOCK_MONOTONIC, so I expect widget/android can use a simple implementation like Mac, which would also be more precise.  See what landed for bug 1256562.
Attachment #8821943 - Flags: review?(karlt)
Attachment #8821943 - Flags: review+ → review?(nchen)
Comment on attachment 8821943 [details]
Bug 1256565 - Part 1. Implement GetEventTimeStamp().

https://reviewboard.mozilla.org/r/101014/#review107568
Attachment #8821943 - Flags: review?(nchen) → review+
Karl, I think part 1 might be still waiting on review. It's very simple so would you be able to give it a quick look?

I think this is the last thing blocking us from switch Event.timestamp over (not sure we need to wait on bug 1261894) so I'd like to tie this up.
Flags: needinfo?(karlt)
Comment on attachment 8821943 [details]
Bug 1256565 - Part 1. Implement GetEventTimeStamp().

https://reviewboard.mozilla.org/r/101014/#review105868

::: widget/android/nsWindow.cpp:108
(Diff revision 2)
> +    // Android's event time is SystemClock.uptimeMillis.
> +    // SystemClock.uptimeMillis uses SYSTEM_TIME_MONOTONIC and so it is safe
> +    // to use event time.

The thing to explain here is that uptimeMillis and POSIX Timestamp both use CLOCK_MONOTONIC.

SYSTEM_TIME_MONOTONIC is an Android concept IIUC.  It's fine to mention SYSTEM_TIME_MONOTONIC here if you like, but it doesn't explain why it is safe to use aEventTime.

::: widget/android/nsWindow.cpp:111
(Diff revision 2)
> +GetEventTimeStamp(int64_t aEventTime)
> +{
> +    // Android's event time is SystemClock.uptimeMillis.
> +    // SystemClock.uptimeMillis uses SYSTEM_TIME_MONOTONIC and so it is safe
> +    // to use event time.
> +    return TimeStamp::FromSystemTime(aEventTime);

aEventTime is from CLOCK_MONOTONIC converted to milliseconds, but FromSystemTime() is not expecting milliseconds.  BaseTimeDurationPlatformUtils is needed to reverse the conversion, as in the Mac implementation.
Attachment #8821943 - Flags: review?(karlt) → review-
Is there a manual testcase that can be used to check that the results are close to correct (at least within a factor of 10^6)?

I guess there's not much point adding a timestamp to SynthesizeNative* methods because there's just as likely to be a similar bug in its implementation, and I don't know how one would trigger native events on Android.
Flags: needinfo?(karlt) → needinfo?(bbirtles)
No, none that I'm aware of. When I did the initial Windows/Linux work, I used a bunch of logging, JS to artificially hold up the main thread, and changed window focus too to generate the different types of lag and check the result.
Flags: needinfo?(bbirtles)
Comment on attachment 8821943 [details]
Bug 1256565 - Part 1. Implement GetEventTimeStamp().

https://reviewboard.mozilla.org/r/101014/#review115332

When pushing updated patches to mozreview, please push the changes on the same base revision where practical, so that interdiffs are helpful.

If this is not practical, then please first push the rebase of the original patch and then push the modified patch, so that interdiff shows the patch changes separately from other changes to the file.
Attachment #8821943 - Flags: review?(karlt) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/48ea57cefa17
Part 1. Implement GetEventTimeStamp(). r=jchen,karlt
https://hg.mozilla.org/integration/autoland/rev/9c07eb353a54
Part 2. Use GetEventTimeStamp() for timestamp. r=jchen
https://hg.mozilla.org/integration/autoland/rev/4e1d46e028bf
Part 3. Turn on highrestimestamp on Android Nightly. r=smaug
I am hoping this fixes bug 1336274.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: