1083530 - Refactor GeckoTouchDispatcher to use mozilla::TimeStamp instead of nsecs_t for vsync times

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Mason Chang [Inactive] [:mchang]]

+++ This bug was initially created as a clone of Bug #1069037 +++

For Project Silk, we'd like to use 1 timestamp and pass it around to the three components required to listen to vsync: The compositor, touch input, and refresh driver. The compositor and refresh driver both use the mozilla::TimeStamp as scheduling mechanisms for animations and requestAnimationFrames. Instead of having the VsyncFramework pass two timestamps, the android input vsync timestamp and the mozilla::TimeStamp around, let's simplify the framework so we can just use the mozilla::TImeStamp. The only user of the android nsecs_t timestamp is the GeckoTouchDispatcher. Change it to use mozilla::TimeStamps.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Use mozilla::TimeStamp in GeckoTouchDispatcher instead of nsecs_t

Mostly a global rename from int64_t to mozilla::Timestamp. No logic changes in resampling, that will come in another bug. I sync android nsecs_t time and mozilla::TimeStamp in both the HwcComposer and GeckoInputDispatcher so that we can have mozilla::Timestamps that are aligned with andriod nsecs_t times.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Use mozilla::TimeStamp in GeckoTouchDispatcher instead of nsecs_t

Rebased now that bug 1069037 landed. No logic changes, just mostly renaming nsecs_t to TimeStamps.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Use mozilla::TimeStamp in GeckoTouchDispatcher instead of nsecs_t

Rebased on master. Also fixed a small bug in the VsyncDispatcher to ensure that we correctly notify the GeckoTouchDispatcher.

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 8511149 [details] [diff] [review]
Use mozilla::TimeStamp in GeckoTouchDispatcher instead of nsecs_t

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

I'm OK with switching to TimeStamp, but don't pretend that the moz timestamp and system timestamp are different things. You can do a sanity check to make sure they're close enough, but don't try to translate one to another - just convert the system time into a moz timestamp.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Create mozilla::TimeStamp constructor with system time argument

Hi Roc,

:mwu thinks it would be better to enable mozilla::TimeStamp to be created from the android system time since they both use clock_gettime and are actually the same time systems. This patch enables the creation of a mozilla::TimeStamp from an android system time on b2g. 

The TimeStamp.h comment says this type of behavior is explicitly disabled (http://dxr.mozilla.org/mozilla-central/source/xpcom/ds/TimeStamp.h#363), but can we make an exception for this since they are both the same units?

Thanks!

Comment 6

[Michael Wu [:mwu]]

Comment on attachment 8512872 [details] [diff] [review]
Create mozilla::TimeStamp constructor with system time argument

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

::: xpcom/ds/TimeStamp.h
@@ +389,5 @@
>  
> +#ifdef MOZ_WIDGET_GONK
> +  TimeStamp(int64_t aAndroidTime) : mValue(aAndroidTime)
> +  {
> +    MOZ_RELEASE_ASSERT(sizeof(aAndroidTime) == sizeof(TimeStampValue));

I think you want MOZ_STATIC_ASSERT here.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: Refactor GeckoTouchDispatcher to use mozilla::TimeStamp instead of nsecs_t for vsync times

Fixed to create mozilla::TimeStamps based off android system times.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Create mozilla::TimeStamp constructor with system time argument

Carrying r+. Use static_assert, MOZ_STATIC_ASSERT only works in C code.

Comment 9

[Michael Wu [:mwu]]

Comment on attachment 8512994 [details] [diff] [review]
Refactor GeckoTouchDispatcher to use mozilla::TimeStamp instead of nsecs_t for vsync times

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

::: widget/gonk/GeckoTouchDispatcher.cpp
@@ +320,5 @@
>    }
>  
>    // This has to be signed int since it is negative
> +  int64_t frameDiff = (currentTouchTime - aSampleTime).ToMicroseconds();
> +  ResampleTouch(aOutTouch, currentTouch, prevTouch, frameDiff, mTouchTimeDiff.ToMicroseconds(), false);

Why not also switch ResampleTouch and Interpolate to using TouchDuration? Then we don't need to convert here.

::: widget/gonk/HwcComposer2D.cpp
@@ +74,5 @@
>  
>  namespace mozilla {
>  
>  #if ANDROID_VERSION >= 17
>  nsecs_t sAndroidInitTime = 0;

This can be removed now, right?

@@ +233,5 @@
>          }
>      }
>  }
>  
> +nsecs_t sLastVsyncTime;

Make this a member of HwcComposer2D.

Comment 10

[Mason Chang [Inactive] [:mchang]]

Attachment: Refactor GeckoTouchDispacher to use mozilla::Timestamp instead of nsecs_t

Updated with comment 9.

Comment 11

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/907d4f91d07c
https://hg.mozilla.org/integration/b2g-inbound/rev/a9748c48d9bd

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/907d4f91d07c
https://hg.mozilla.org/mozilla-central/rev/a9748c48d9bd