1036287 - Make animations get their time from their timeline

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

As of bug 1032573, animations now have a pointer to the timeline from which they get the time to use for each sample.

They don't currently, however, actually use that time. In order to be able to support other types of timelines we should actually make animations get the current time from their timeline rather than asking the refresh driver directly.

The one complication is that timelines can potentially be in an unresolved state in which case they pass null to their subscribers. As a result, part of this work is making the animation methods deal with this "null" state.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Add a null animation phase

In order to support arbitrary timelines which may provide a "null" current time,
we need a suitable value to return from GetComputedTimingAt for the animation's
phase when the timeline time is null.

This patch introduces a null animation phase for this purpose.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Make GetComputedTimingAt take a nullable local time

As part of supporting arbitrary timelines, we'd like to pass null times to the
function that calculates computed timing. Incidentally, this also provides
a means for evaluating calculating timing parameters that are independent of the
current time (currently on the active duration) without requiring a valid time.

This patch updates the signature of ElementAnimation::GetComputedTimingAt to
take a nullable time duration. Many of the call sites are longer as a result but
this is addressed in a subsequent patches.

We use the Nullable wrapper to represent null TimeDurations since, unlike,
TimeStamp, TimeDuration does not include a null state.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Make GetLocalTime(At) get the current time automatically from the timeline

This patch changes ElementAnimation::GetLocalTimeAt so that instead of taking
the current time as input, it uses the animation's mTimeline member to look up
the current time of the associated timeline. As a result of this, it is possible
to remove a few instances of querying the refresh driver for the current time.
Further instances are removed in subsequent patches.

Furthermore, in order to keep the use of time sources consistent, the mStartTime
of new transitions and animations is initialized with the current time from the
animation's timeline rather than with the latest refresh driver tick.
Since this time could, in future, be null, GetLocalTime(At) is updated to check
for a null start time.

GetLocalTimeAt is also renamed to GetLocalTime in the process.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Make GetLocalTimeAt return a nullable time duration

Once we support arbitrary timelines which can return null current time values,
the local time of an animation can also become null so this patch updates
ElementAnimation::GetLocalTimeAt to return a Nullable<TimeDuration>.
Doing this also allows us to pass the result of GetLocalTimeAt directly to
GetComputedTimingAt.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 5 - Make IsRunning(At) and IsCurrent(At) use the current timeline time

Similar to the changes made to GetLocalTime(At) in a previous patch in this
series, this patch makes IsRunningAt and IsCurrentAt automatically use the
current time from their animation timeline. This ensures that each animation
gets sampled with the correct timeline once we introduce setting arbitrary
timelines.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 6 - Add a GetComputedTiming shortcut that uses the current animation timeline time

This patch introduces a method GetComputedTiming that calls GetComputedTimingAt
supplying the current time of the animation's timeline.

We still keep the GetComputedTimingAt static method since it is used for
off-main thread animation. Furthermore, we keep the second argument to
GetComputedTiming--the animation's timing properties--since on some occasions we
want to override those properties (ElementPropertyTransition::ValuePortionFor
does this). We could also add another overload that also supplies the
animation's timing properties but that can happen as a separate step.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

One concern with these patches is that they mean we end up querying the refresh driver for its most recent refresh somewhat more frequently. This method (MostRecentRefresh) doesn't appear to be too expensive but it might be better if we cached the current sample time in the timeline and just return that, perhaps as extension of what we do in bug 1002332.

The only thing is I'm not sure how to hook up the timeline to the refresh driver and ensure it gets updated before any of the animation code that queries the timeline which is also triggered by the refresh driver.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Make GetLocalTime(At) get the current time automatically from the timeline

Remove an unreferenced variable

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8452913 [details] [diff] [review]
part 2 - Make GetComputedTimingAt take a nullable local time

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

::: layout/style/AnimationCommon.cpp
@@ +455,5 @@
>      return false;
>    }
>  
>    ComputedTiming computedTiming =
> +    GetComputedTimingAt(Nullable<TimeDuration>(GetLocalTimeAt(aTime)), mTiming);

> 80 characters, so needs some wrapping.

BUT -- instead of changing all these callsites, I'd rather we just create a convenience GetComputedTimingAt() wrapper-function, with the old function-signature (except maybe taking a const TimeDuration& instead of a TimeDuration), to keep these callers from needing to change. This would just be a one-liner function, which would invoke the *real* GetComputedTimingAt() method by wrapping its arg in a Nullable<>.  (This wrapper could probably even be an inline function defined in the header-file, which should remove any overhead associated with the extra layer of function-call.)

That way, we won't need to change any existing callers. That lets us revert most of this patch, since a lot of this patch is just adding the Nullable<> wrapper in callers.

@@ +485,5 @@
>    return false;
>  }
>  
>  ComputedTiming
> +ElementAnimation::GetComputedTimingAt(Nullable<TimeDuration> aLocalTime,

This arg should probably be a const ref.

@@ +508,5 @@
> +  // values consistent with an animation that has not been sampled.
> +  if (aLocalTime.IsNull()) {
> +    return result;
> +  }
> +  TimeDuration localTime = aLocalTime.Value();

This, too, should probably be a const ref. ("const TimeDuration& localTime")

(That would avoid needlessly copying, & more clearly tie this variable to the aLocalTime arg)

::: layout/style/AnimationCommon.h
@@ +386,5 @@
>    // mTimeFraction member of the return value if the animation should not be
>    // run (because it is not currently active and is not filling at this time).
> +  static ComputedTiming
> +  GetComputedTimingAt(Nullable<mozilla::TimeDuration> aLocalTime,
> +                      const AnimationTiming& aTiming);

Can you update the documentation for this function to reflect the Nullable change? (and document what it means if a caller passes in an IsNull value)

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> BUT -- instead of changing all these callsites, I'd rather we just create a
> convenience GetComputedTimingAt() wrapper-function

(I'm aware that a lot of these callsites are changing back in the next patch, when GetLocalTimeAt()'s return-type changes.  With the convenience wrapper, those callsites will just be able to stay untouched (no need for the code-churn on their lines), and they'll just be implicitly changing which version of the function they call into.)

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8452920 [details] [diff] [review]
part 3 - Make GetLocalTimeAt return a nullable time duration

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

::: layout/style/AnimationCommon.cpp
@@ +429,5 @@
>  double
>  ElementAnimation::CurrentTime() const
>  {
> +  // In Web Animations AnimationPlayers have a *current* time and Animations
> +  // have a *local* time. However, since we have a 1:1 correspondance between

Add comma after "Web Animations"

Also, I believe you need s/a/e/ in "correspondance" (should be "correspondence", unless maybe the "a" spelling is a British-ism)

::: layout/style/AnimationCommon.h
@@ +365,5 @@
>  
>    // Return the duration at aTime (or, if paused, mPauseStart) since
>    // the start of the delay period.  May be negative.
> +  Nullable<mozilla::TimeDuration>
> +  GetLocalTimeAt(mozilla::TimeStamp aTime) const {

Update the documentation here, since there's a new flavor of value we can return.

e.g. maybe add something to the effect of:
// Returns a "Null" value IFF the passed-in TimeStamp IsNull().

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> Comment on attachment 8452920 [details] [diff] [review]
> part 3 - Make GetLocalTimeAt return a nullable time duration
[...]
> > +  Nullable<mozilla::TimeDuration>
> > +  GetLocalTimeAt(mozilla::TimeStamp aTime) const {
> 
> Update the documentation here, since there's a new flavor of value we can
> return.
> 
> e.g. maybe add something to the effect of:
> // Returns a "Null" value IFF the passed-in TimeStamp IsNull().

er, I guess the passed-in TimeStamp is going away in the next patch (replaced w/ timestamp from the timeline). So, never mind about this comment -- let's just update the documentation in the next patch. I'll note that in my review notes for that patch.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8453498 [details] [diff] [review]
part 4 - Make GetLocalTime(At) get the current time automatically from the timeline

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

::: layout/style/AnimationCommon.h
@@ +364,5 @@
>    bool IsCurrentAt(mozilla::TimeStamp aTime) const;
>  
> +  // Return the duration since the start of the delay period taking into
> +  // account the pause state.  May be negative.
> +  Nullable<mozilla::TimeDuration> GetLocalTime() const {

Add comma between "period" and "taking".

Also, per previous comment, let's update the documentation here to indicate the significance of an IsNull return value.  (Based on the impl, it looks like this returns an IsNull value if either timelineTime is null or mStartTime is null, though I don't fully grok the significance of those conditions.)

@@ +365,5 @@
>  
> +  // Return the duration since the start of the delay period taking into
> +  // account the pause state.  May be negative.
> +  Nullable<mozilla::TimeDuration> GetLocalTime() const {
> +    mozilla::TimeStamp timelineTime = mTimeline->GetCurrentTimeStamp();

Observation: here and in other callers of GetCurrentTimeStamp(), we should perhaps be sticking the return value into a const reference, to avoid a needless copy (and for better const-correctness).  (since C++ delays destruction of a returned local-var if the caller sticks it into a const reference)

(I suppose Return Value Optimization gets you the same copy-avoidance, so maybe it doesn't make a difference... but probably better to give the compiler more ways to see that no copying need be done, since TimeStamp is a somewhat-bulky object, at least on Windows where we use TimeStamp_windows.h)

::: layout/style/nsAnimationManager.cpp
@@ +30,5 @@
>                                           TimeStamp aRefreshTime,
>                                           EnsureStyleRuleFlags aFlags)
>  {
>    aCollection->EnsureStyleRuleFor(aRefreshTime, aFlags);
> +  GetEventsAt(aCollection, mPendingEvents);

Looks like these two statements could now potentially be acting based on different timestamps. Is that bad?

Also, GetEventsAt() seems like it needs a rename, now that it no longer takes a timestamp arg.  (Without the timestamp-arg, it's unclear what the "At" is referring to now -- I think it's vestigial.) In other functions where you're dropping the timestamp, you're dropping the "At", but maybe that doesn't work here since "GetEvents()" might be too vague... maybe "GetCurrentEvents" or "GetEventsForCurTime", or something along those lines?

@@ +46,2 @@
>      ComputedTiming computedTiming =
>        ElementAnimation::GetComputedTimingAt(localTime, anim->mTiming);

Let's just ditch the 'localTime' variable here, and replace its usage with a direct call to anim->GetLocalTime() (now that it's a shorter expression, w/out the aRefreshTime arg)

@@ +305,5 @@
>              } else {
>                // Handle change in pause state by adjusting start
>                // time to unpause.
> +              newAnim->mStartTime += timeline->GetCurrentTimeStamp()
> +                                     - oldAnim->mPauseStart;

Two things:
 (1) Do we need to check timeline->GetCurrentTimeStamp().IsNull() here before using it in arithmetic?
   (Or, if we know it's guaranteed to be non-null here, maybe we should be asserting that?)

 (2) This leaves only one usage of the local-var 'refreshTime', a bit further down.  Let's move the refreshTime decl down to be right next to that one remaining usage of it. (or maybe even just drop the variable altogether and fold it into that one usage, if you like)

@@ -392,5 @@
>  
>    ResolvedStyleCache resolvedStyles;
>  
>    const nsStyleDisplay *disp = aStyleContext->StyleDisplay();
> -  TimeStamp now = mPresContext->RefreshDriver()->MostRecentRefresh();

[see below]

@@ +427,3 @@
>      dest->mPlayState = src.GetPlayState();
>      if (dest->IsPaused()) {
> +      dest->mPauseStart = aTimeline->GetCurrentTimeStamp();

This will make a lot of redundant calls to GetCurrentTimeStamp() -- two here, multiplied by the number of times we run this loop.

That function-call is cheap, but not free -- it does a few checks before returning its result (based on attachment 8448511 [details] [diff] [review] at least).  So, we should only call it once, if possible.

Can't we just keep this as it was before, with "now" declared at the top of the function? (initialized from aTimeline->GetCurrentTimeStamp() instead of mPresContext->RefreshDriver()->MostRecentRefresh())

::: layout/style/nsTransitionManager.cpp
@@ +36,5 @@
>  using namespace mozilla::layers;
>  using namespace mozilla::css;
>  
>  double
> +ElementPropertyTransition::ValuePortionFor() const

As above with GetEventsAt(), this probably needs a rename -- the "For" suffix is now vague/meaningless. (since it no longer has an argument to be "for")

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8452926 [details] [diff] [review]
part 5 - Make IsRunning(At) and IsCurrent(At) use the current timeline time

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

One nit on the commit message -- so, it currently says "Make IsRunning(At) and IsCurrent(At) use the current timeline time" (and has similar language in the explanatory bonus-commit-message-lines).

This is not quite correct. It looks to me like the *previous* patch (part 4) is *really* what makes these functions use the current timeline time. This patch just adjusts their function-signatures to match that reality, by removing the already-unused parameter "aTime".

So, something like this would be clearer, I think:
 "Drop aTime param from IsRunning(At) and IsCurrent(At), since they now use the current timeline time."

::: content/base/src/Element.cpp
@@ -2871,5 @@
> -  mozilla::TimeStamp now = OwnerDoc()->Timeline()->GetCurrentTimeStamp();
> -  if (now.IsNull()) {
> -    // If the timeline doesn't have a current time, return an empty list.
> -    return;
> -  }

Maybe it'd still be worth keeping this early-return (in Element::GetAnimationPlayers) as an optimization, since it lets us skip some hash-table operations (calls to GetProperty()) that are doomed to be useless?

(I'm fine either way.)

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> Comment on attachment 8453498 [details] [diff] [review]
> part 4 - Make GetLocalTime(At) get the current time automatically from the
> timeline
[...]
> @@ +46,2 @@
> >      ComputedTiming computedTiming =
> >        ElementAnimation::GetComputedTimingAt(localTime, anim->mTiming);
> 
> Let's just ditch the 'localTime' variable here, and replace its usage with a
> direct call to anim->GetLocalTime() (now that it's a shorter expression,
> w/out the aRefreshTime arg)

(Er, never mind about this part -- it looks like patch 6 condenses this in a different way.)

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Make GetLocalTime(At) get the current time automatically from the timeline

This patch changes ElementAnimation::GetLocalTimeAt so that instead of taking
the current time as input, it uses the animation's mTimeline member to look up
the current time of the associated timeline. As a result of this, it is possible
to remove a few instances of querying the refresh driver for the current time.
Further instances are removed in subsequent patches.

Furthermore, in order to keep the use of time sources consistent, the mStartTime
of new transitions and animations is initialized with the current time from the
animation's timeline rather than with the latest refresh driver tick.
Since this time could, in future, be null, GetLocalTime(At) is updated to check
for a null start time.

GetLocalTimeAt is also renamed to GetLocalTime in the process.

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> Comment on attachment 8453498 [details] [diff] [review]
> part 4 - Make GetLocalTime(At) get the current time automatically from the
> timeline
...
> ::: layout/style/nsAnimationManager.cpp
> @@ +30,5 @@
> >                                           TimeStamp aRefreshTime,
> >                                           EnsureStyleRuleFlags aFlags)
> >  {
> >    aCollection->EnsureStyleRuleFor(aRefreshTime, aFlags);
> > +  GetEventsAt(aCollection, mPendingEvents);
> 
> Looks like these two statements could now potentially be acting based on
> different timestamps. Is that bad?

I think it's ok. Within a collection of animations, each animation can have a different timeline (in the future, anyway) we don't use their timeline time here. Instead we use the refresh driver time to record whether we've updated styles for the current refresh driver tick. Then we use the timeline time to know where we are within each animation.

> @@ +305,5 @@
> >              } else {
> >                // Handle change in pause state by adjusting start
> >                // time to unpause.
> > +              newAnim->mStartTime += timeline->GetCurrentTimeStamp()
> > +                                     - oldAnim->mPauseStart;
> 
> Two things:
>  (1) Do we need to check timeline->GetCurrentTimeStamp().IsNull() here
> before using it in arithmetic?
>    (Or, if we know it's guaranteed to be non-null here, maybe we should be
> asserting that?)

Added.

>  (2) This leaves only one usage of the local-var 'refreshTime', a bit
> further down.  Let's move the refreshTime decl down to be right next to that
> one remaining usage of it. (or maybe even just drop the variable altogether
> and fold it into that one usage, if you like)

Fixed.

> @@ -392,5 @@
> >  
> >    ResolvedStyleCache resolvedStyles;
> >  
> >    const nsStyleDisplay *disp = aStyleContext->StyleDisplay();
> > -  TimeStamp now = mPresContext->RefreshDriver()->MostRecentRefresh();
> 
> [see below]
> 
> @@ +427,3 @@
> >      dest->mPlayState = src.GetPlayState();
> >      if (dest->IsPaused()) {
> > +      dest->mPauseStart = aTimeline->GetCurrentTimeStamp();
> 
> This will make a lot of redundant calls to GetCurrentTimeStamp() -- two
> here, multiplied by the number of times we run this loop.
> 
> That function-call is cheap, but not free -- it does a few checks before
> returning its result (based on attachment 8448511 [details] [diff] [review]
> at least).  So, we should only call it once, if possible.
> 
> Can't we just keep this as it was before, with "now" declared at the top of
> the function? (initialized from aTimeline->GetCurrentTimeStamp() instead of
> mPresContext->RefreshDriver()->MostRecentRefresh())

Fixed.

I've addressed all the other feedback locally. Thanks Daniel!

Comment 18

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Comment on attachment 8452926 [details] [diff] [review]
> part 5 - Make IsRunning(At) and IsCurrent(At) use the current timeline time
 current timeline time."
... 
> ::: content/base/src/Element.cpp
> @@ -2871,5 @@
> > -  mozilla::TimeStamp now = OwnerDoc()->Timeline()->GetCurrentTimeStamp();
> > -  if (now.IsNull()) {
> > -    // If the timeline doesn't have a current time, return an empty list.
> > -    return;
> > -  }
> 
> Maybe it'd still be worth keeping this early-return (in
> Element::GetAnimationPlayers) as an optimization, since it lets us skip some
> hash-table operations (calls to GetProperty()) that are doomed to be useless?
> 
> (I'm fine either way.)

In future it should be possible that all the animations targeting an element have a timeline that is not the document timeline. In that case, even if the document timeline returned null we should still return the list of players. I'm not sure if that's ever going to happen (since the document timeline should only rarely return null, especially after bug 1002332 lands) but in terms of correctness it's better without this check. Also, since the document timeline is so rarely null, this extra check is almost never going to help us.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8455964 [details] [diff] [review]
part 4 - Make GetLocalTime(At) get the current time automatically from the timeline

Thanks! r=me

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c52984dbf1be
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac5d63123ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a342071fd1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d6e547234ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/d159f3b81265
https://hg.mozilla.org/integration/mozilla-inbound/rev/713c2e0870f5

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c52984dbf1be
https://hg.mozilla.org/mozilla-central/rev/8ac5d63123ae
https://hg.mozilla.org/mozilla-central/rev/4a342071fd1a
https://hg.mozilla.org/mozilla-central/rev/3d6e547234ac
https://hg.mozilla.org/mozilla-central/rev/d159f3b81265
https://hg.mozilla.org/mozilla-central/rev/713c2e0870f5