Devicemotion event timestamp should return values from Android sensor API and not Gecko.

RESOLVED FIXED in Firefox 48

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Casey Yee, Assigned: droeh)

Tracking

Trunk
Firefox 48
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox48 fixed)

Details

(Whiteboard: [webvr], URL)

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8683780 [details]
Firefox - DeviceMotionEvent.timestamp delta

This is leading to time deltas that are very erratic:

caseyyee.github.io/DevicemotionTest
(Reporter)

Comment 1

3 years ago
Created attachment 8683781 [details]
Chrome - DeviceMotionEvent.timestamp delta

Comparing with Chrome.
(Reporter)

Updated

3 years ago
Flags: needinfo?(snorp)
Created attachment 8683878 [details] [diff] [review]
Use Android event time for DOM motion event timestamp

This looks like it should work, but somehow the test page shows a negative min delta. I don't
see negative deltas in nsDeviceSensors, so I'm not sure what's going on. Kyle, does this patch seem reasonable to you?
See anything obvious?
Attachment #8683878 - Flags: feedback?(khuey)
Comment on attachment 8683878 [details] [diff] [review]
Use Android event time for DOM motion event timestamp

Review of attachment 8683878 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not familiar with this code at all. I don't see anything obviously wrong, but I couldn't tell you e.g. why WidgetEvent has a time and timestamp ...
Attachment #8683878 - Flags: feedback?(khuey)
Dylan, can you finish this up?
Assignee: nobody → droeh
Flags: needinfo?(snorp)
(Reporter)

Comment 5

3 years ago
Just as a note, I do divide the timestamp that we get back by 1000 so that the timestamp was at a comparable to what we see from chrome:

https://github.com/caseyyee/DevicemotionTest/blob/master/index.html#L112

If the timestamp being returned is not in ns, we might get a weird number.

The devicemotion spec http://www.w3.org/TR/orientation-event/  does not say anything about timestamp, so maybe for this case, we should be consistent with what is being returned in Chrome?
(Assignee)

Comment 6

3 years ago
Created attachment 8687964 [details] [diff] [review]
bug-1222098-fix.patch

I rebased snorp's patch and it seems to be working correctly now; min/avg/max deltas are all around 4ms on my Nexus 6.
Attachment #8683878 - Attachment is obsolete: true
Attachment #8687964 - Flags: review?(bugs)

Comment 7

3 years ago
(In reply to Casey Yee from comment #5)
> Just as a note, I do divide the timestamp that we get back by 1000 so that
> the timestamp was at a comparable to what we see from chrome:
> 
> https://github.com/caseyyee/DevicemotionTest/blob/master/index.html#L112
> 
> If the timestamp being returned is not in ns, we might get a weird number.
timestamp is in ms, but We've been in process to convert timestamp handling to 
highrestimestamp. Brian should know its current state.


> 
> The devicemotion spec http://www.w3.org/TR/orientation-event/ 
please don't look at deprecated specs. The latest one is
http://w3c.github.io/deviceorientation/spec-source-orientation.html

> does not say
> anything about timestamp,
because timestamp is defined in DOM spec.
Flags: needinfo?(bbirtles)

Comment 8

3 years ago
Comment on attachment 8687964 [details] [diff] [review]
bug-1222098-fix.patch

>                                          aRotationRate.mGamma);
>   mInterval = aInterval;
>+  if (!timeStamp.IsNull()) {
>+    mEvent->time = timeStamp.Value();
>+  }
> }
So DOM spec says "When an event is created the attribute must be initialized to", and this is kind of against that.
But, I guess we can pretend that the event is actually created earlier, at the time of timeStamp, and js just gets access to the DOM event later.

argument names should be in form aName.


>+++ b/dom/webidl/DeviceMotionEvent.webidl
>@@ -48,10 +48,10 @@ dictionary DeviceMotionEventInit : Event
> // Mozilla extensions.
> partial interface DeviceMotionEvent {
>   void initDeviceMotionEvent(DOMString type,
>                              boolean canBubble,
>                              boolean cancelable,
>                              DeviceAccelerationInit acceleration,
>                              DeviceAccelerationInit accelerationIncludingGravity,
>                              DeviceRotationRateInit rotationRate,
>-                             double? interval);
>+                             double? interval, double? timeStamp);
> };
We should not change legacy init*Event methods in .webidl, (but try to get rid of them if possible).
If you need this all for cpp only usage, please change only C++ side.
Attachment #8687964 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #7)
> (In reply to Casey Yee from comment #5)
> > Just as a note, I do divide the timestamp that we get back by 1000 so that
> > the timestamp was at a comparable to what we see from chrome:
> > 
> > https://github.com/caseyyee/DevicemotionTest/blob/master/index.html#L112
> > 
> > If the timestamp being returned is not in ns, we might get a weird number.
> timestamp is in ms, but We've been in process to convert timestamp handling
> to 
> highrestimestamp. Brian should know its current state.

We've made Windows and Linux convert native event times to TimeStamp values (but we only use that value to return a DOMHighResTimeStamp in Nightly and Aurora channels on Win/Linux). I was told in about September that someone from Anthony's team would do the work for Mac. No-one's looked into Android however. Once we have all platforms converted we'll switch the pref on for release channels so that Event.timeStamp always returns a DOMHighResTimeStamp.
Flags: needinfo?(bbirtles)
(Assignee)

Comment 10

3 years ago
Created attachment 8692030 [details] [diff] [review]
Updated patch

Updated with suggested fixes.
Attachment #8687964 - Attachment is obsolete: true
Attachment #8692030 - Flags: review?(bugs)
Comment on attachment 8692030 [details] [diff] [review]
Updated patch


>                      const DeviceAccelerationInit& aAcceleration,
>                      const DeviceAccelerationInit& aAccelIncludingGravity,
>                      const DeviceRotationRateInit& aRotationRate,
>                      Nullable<double> aInterval)
> {
>+  InitDeviceMotionEvent(aType, aCanBubble, aCancelable, aAcceleration,
>+                        aAccelIncludingGravity, aRotationRate, aInterval,
>+                        Nullable<double>(PR_Now()));
>+}
event.init*Event called by JS shouldn't change event's timestamp.
So you want to pass Nullabl<double>() here, I think


>+  if (!aTimeStamp.IsNull()) {
>+    mEvent->time = aTimeStamp.Value();
>+  }
So we want to set also timeStamp here, since that is what we'll hopefully use everywhere in the future
(and we'll drop 'time', since our .time handling has been totally broken forever)

You may play with dom.event.highrestimestamp.enabled pref
Attachment #8692030 - Flags: review?(bugs) → review-
(Assignee)

Comment 12

3 years ago
Created attachment 8693805 [details] [diff] [review]
Updated patch

Updated to address the above.

Also sets (low res) event time in milliseconds rather than microseconds as it was doing previously, which I think is correct.
Attachment #8692030 - Attachment is obsolete: true
Attachment #8693805 - Flags: review?(bugs)
Comment on attachment 8693805 [details] [diff] [review]
Updated patch

+  void InitDeviceMotionEvent(
+         const nsAString& aType,
+         bool aCanBubble,
+         bool aCancelable,
+         const DeviceAccelerationInit& aAcceleration,
+         const DeviceAccelerationInit& aAccelerationIncludingGravity,
+         const DeviceRotationRateInit& aRotationRate,
+         Nullable<double> aInterval,
+         Nullable<double> aTimeStamp);
Why double? Shouldn't the type be Nullable<uint64_t>
(Sorry, didn't think this before)


>+  static mozilla::TimeStamp ts(mozilla::TimeStamp::Now());
I don't understand this.
We record some random time and then later add x microseconds to it.

>+
>   mAcceleration = new DeviceAcceleration(this, aAcceleration.mX,
>                                          aAcceleration.mY,
>                                          aAcceleration.mZ);
> 
>   mAccelerationIncludingGravity =
>     new DeviceAcceleration(this, aAccelIncludingGravity.mX,
>                            aAccelIncludingGravity.mY,
>                            aAccelIncludingGravity.mZ);
> 
>   mRotationRate = new DeviceRotationRate(this, aRotationRate.mAlpha,
>                                          aRotationRate.mBeta,
>                                          aRotationRate.mGamma);
>   mInterval = aInterval;
>+  if (!aTimeStamp.IsNull()) {
>+    // aTimeStamp is in microseconds, Event time should be milliseconds.
>+    mEvent->time = aTimeStamp.Value() / 1000.0;
This is right per spec. Unfortunately our .time handling is messy. Elsewhere we use microseconds.
But I don't have better suggestions (expect perhaps waiting for timeStamp to be used everywhere)


>+    mEvent->timeStamp = ts + mozilla::TimeDuration::FromMicroseconds(aTimeStamp.Value());
So this can't be right, or am I missing something here?
ts is some timestamp and aTimeStamp is microseconds from epoch.
Did you try what kinds of values you get when dom.event.highrestimestamp.enabled is true?
Or what is s.timestamp returning to Gecko? Is that time from the launch of application or time from epoch or what?
Don't we more like want TimeStamp::Now() - (currentTime - s.timestamp)? So we'd get some timestamp in the past.
Attachment #8693805 - Flags: review?(bugs) → review-
(Assignee)

Comment 14

3 years ago
Created attachment 8695295 [details] [diff] [review]
Updated patch

So, to explain the previous patch: The values passed to InitDeviceMotionEvent as aTimeStamp are already effectively arbitrary in that they do not use the same origin as anything in Gecko. Given that, I just wrote the patch that way as a simple way of preserving the differences between successive timestamps.

I think this version of the patch handles it better for both low and high-res timestamps, however. The first time InitDeviceMotionEvent is called, we establish low-res and high-res origins (using PR_Now and TimeStamp::Now, respectively), and an aTimeStamp origin. Successive calls use the difference between aTimeStamp and the aTimeStamp origin to get a duration in microseconds, and add that to the low-res and high-res timestamp origins. This places the timestamps as correctly as we could hope for using PR_Now and TimeStamp::Now, but preserves the differences between successive events as reported by the device.

Alternately, we could just use FromSystemTime to create the TimeStamp directly -- but I think the solution I have here is better.
Attachment #8693805 - Attachment is obsolete: true
Attachment #8695295 - Flags: review?(bugs)
Just curious, what is the time origin of timestamps from the HAL sensor code?

It's seems a shame we can't re-use SystemTimeConverter[1] since it should handle timestamps wrapping, clock skew etc. (As it stands, SystemTimeConverter expects the "native time" to be in milliseconds whereas the HAL sensor API seems to use nanoseconds, but I'm sure we could fix that.)

[1] https://dxr.mozilla.org/mozilla-central/source/widget/SystemTimeConverter.h
(Assignee)

Comment 16

3 years ago
The origin of the sensor timestamps is device dependent, unfortunately. I'll take a closer look at SystemTimeConverter tomorrow and see if it would be usable anyways.
(Assignee)

Comment 17

3 years ago
So, if I understand SystemTimeConverter correctly, I don't think it quite fits. The CurrentTimeGetter needs to (as the name suggests) be able to get the current time in the sensor clock. This is made very difficult by the aforementioned device-dependency of the timestamps -- there's no way of just querying the sensor timestamp directly, as far as I know, so you need to figure out the origin of those timestamps before you can do anything.

It would be possible, I suppose, to write a CurrentTimeGetter that simply returns the timestamp of the most recent DeviceMotionEvent as the "current" time -- they happen quite frequently (roughly every 4ms on my Nexus 6, for example) -- but I don't know if that level of error will have an impact on the skew-handling, etc. Also, it seems like an ugly workaround.
Fair enough. It seems like SystemTimeConverter isn't really helpful here. The skew detection probably isn't going to help if you can't get a current time.
The reason why I haven't reviewed this yet is Bug 1231619 which may affect to how we want to deal with timestamps.
(Assignee)

Comment 20

3 years ago
Is Bug 1231619 strictly about high-res timestamps? If so, I suggest we land just the low-res timestamp part of this and file a separate bug to land the high-res timestamp portion after bug 1231619 is resolved. If that sounds good, let me know and I'll put up an updated patch.
Flags: needinfo?(bugs)
Comment on attachment 8695295 [details] [diff] [review]
Updated patch

Yeah, ok fine. Looks like getting highres timestamps is taking quite a bit time
(so is my review :/ ). So want to update the patch and I'll review.
Flags: needinfo?(bugs)
Attachment #8695295 - Flags: review?(bugs)
(Assignee)

Comment 22

2 years ago
Created attachment 8737819 [details] [diff] [review]
Proposed patch (updated)

Here's an updated version without any high-res timestamp handling.
Attachment #8695295 - Attachment is obsolete: true
Attachment #8737819 - Flags: review?(bugs)
Comment on attachment 8737819 [details] [diff] [review]
Proposed patch (updated)

>+
>+        // SensorEvent timestamp is in nanoseconds, Gecko expects micorseconds.
micro

>+        event.mTime = s.timestamp / 1000;
Ok, the testcase does have / 1000 for FF, leading to ms as expected.

Please file a followup bug and make it block bug 1026804 to sort this all out for highres timestamps.
Attachment #8737819 - Flags: review?(bugs) → review+

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bcff3c3da698
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.