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

RESOLVED FIXED in mozilla36

Status

defect
RESOLVED FIXED
5 years ago
9 months ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

26 Branch
mozilla36
x86
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

+++ 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.
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.
Attachment #8505834 - Flags: review?(mwu)
Rebased now that bug 1069037 landed. No logic changes, just mostly renaming nsecs_t to TimeStamps.
Attachment #8505834 - Attachment is obsolete: true
Attachment #8505834 - Flags: review?(mwu)
Attachment #8507992 - Flags: review?(mwu)
Blocks: 1085512
Blocks: 1087048
Rebased on master. Also fixed a small bug in the VsyncDispatcher to ensure that we correctly notify the GeckoTouchDispatcher.
Attachment #8507992 - Attachment is obsolete: true
Attachment #8507992 - Flags: review?(mwu)
Attachment #8511149 - Flags: review?(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.
Attachment #8511149 - Flags: review?(mwu)
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!
Attachment #8512872 - Flags: review?(roc)
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.
Fixed to create mozilla::TimeStamps based off android system times.
Attachment #8511149 - Attachment is obsolete: true
Attachment #8512994 - Flags: review?(mwu)
Attachment #8512994 - Attachment description: Create mozilla::TimeStamp constructor with system time argument → Refactor GeckoTouchDispatcher to use mozilla::TimeStamp instead of nsecs_t for vsync times
Carrying r+. Use static_assert, MOZ_STATIC_ASSERT only works in C code.
Attachment #8512872 - Attachment is obsolete: true
Attachment #8512995 - Flags: review+
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.
Attachment #8512994 - Flags: review?(mwu)
Attachment #8513030 - Flags: review?(mwu) → review+
https://hg.mozilla.org/mozilla-central/rev/907d4f91d07c
https://hg.mozilla.org/mozilla-central/rev/a9748c48d9bd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1094058
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.