1223658 - Compositor animations with delay arrive at the compositor too late

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

If I create a looping frame-based animation (test case forthcoming) that toggles images opacity on and off alternatively I notice that there is considerable flicker during the first run of the animation.

The problem, I think, is that we don't upload animations to the compositor during their delay phase. As a result, we wait until we get a tick on the main thread that occurs *after* the delay phase has ended and only then do we upload the animation. As a result, by the time the layer transaction finishes and the compositor applies the new animation we can easily miss a frame. There could be more going on--I assume we've already uploaded the texture for the layer but I haven't checked.

If that analysis is correct, then we could fix this by uploading animations to the compositor during their delay phase.

I don't think we save a lot by keeping animations off the compositor during that phase since it's not common to have a lot of animations scheduled far into the future. (On the other hand it is common to have a lot of paused animations or animations that have completed in the past--so it makes sense to keep those animations off the compositor.)

For anyone trying to do these kind of frame-based animations as well as anyone trying to sequence animations, having animations start at the correct moment when running on the compositor seems important.

Incidentally, uploading animations during the delay phase would also fix bug 1223204 (and without requiring changes on the DevTools side).

Comment 1

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

Test case: http://people.mozilla.org/~bbirtles/bugs/1223658/wavey-davey.html

This test case runs the same animation on the main thread and on the compositor. It forces it to the main thread in the first case by including a 'filter' property in the keyframes rule which is enough to trick our "can run on compositor" heuristics into running the animation on the main thread.

For me, the second case which runs on the compositor flickers for the first iteration of the animation.

Comment 2

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

I think I want to land the first part of bug 1190235 before attempting this, however, since that will significantly simplify the compositor decision logic.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Very incomplete patch

This patch releases various checks that we don't send animations to compositor until the animations are in the active phase.

This patch works only if the animation property is opacity.  E.g., the example in comment #1.  This patch does not work if the animation property is transform because I don't know how the identity matrix is set to yet.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: A patch works in most cases

This patch works in most cases except some test cases in test_animations_omta.html.  I am missing something under test control.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

It turns out that the approach of attachment 8731576 [details] is not good.  We should resolve the transform/opacity style in before phase anyway.  I'd want to depend this bug on bug 1245748 for now.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

A try with patches that CSS animations/transitions send to compositor in before phase.  This should work as expected.  To send script animations to compositor in before phase, we should land bug 1245748 first, I think.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89e3c677b980

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1223658 - Part 4: Add a function to check active duration is zero.

CommonAnimationManager::ExtractComputedValueForTransition does not involve
any members of CommonAnimationManager.

Review commit: https://reviewboard.mozilla.org/r/41599/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41599/

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1223658 - Part 8: Reftests for checking stacking context of transform animations.

To send transform animations in before phase to compositor we need any
transforms in the before phase even though there is no transform on the
target element.  SetIdentityTransform is for that purporse.

Review commit: https://reviewboard.mozilla.org/r/41601/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41601/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: MozReview Request: Bug 1223658 - Part 5: Add a function to check active duration is zero. r?birtles

If the animation property is opacity or transform and the animation has
a positive delay, we need to set opacity or transform property to send
the animation to compositor.  If the target element has no transform,
identity transform is set for it.  In case of opacity
ResolveStyleWithoutAnimation returns opacity:1 if the target element has no
opacity property.

Review commit: https://reviewboard.mozilla.org/r/41603/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41603/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1223658 - Part 2: Pass delay property to compositor.

The check of negative elapsedDuration is no longer valid since
animation delay is not factored into start time any more.

Review commit: https://reviewboard.mozilla.org/r/41605/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41605/

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

Review commit: https://reviewboard.mozilla.org/r/41607/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/41607/

Comment 12

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

Hi Hiro, I'm really sorry but I wonder if it would be possible to hold off on this until after bug 1245748 is done? That bug changes a lot of the same code, is quite difficult, and blocks shipping Element.animate. I think making it work with these patches at the same time could make that bug even harder and I think we should prioritize that bug ahead of this.

In particular, that bug makes a distinction between building the frames when we setup an animation, and turning those frames into properties (which happens whenever the style context changes). Part 3 in this patch series looks up the current style context to get the effective backwards fill. That part should happen at the same time as we resolve frames into properties but in this patch it happens in CSSAnimationBuilder. So at least that will need to change. Maybe that's not such a big change though?

Also, one minor point of feedback on part 3, we probably shouldn't use ExtractComputedValueForTransition. That method just does special handling of visibility (that we didn't want to also apply to SMIL animation). Since we don't do visibility on the compositor it doesn't seem necessary and we could simply use StyleAnimationValue::ExtractComputedValue here.

Apart from that, it looks reasonable. Part 5 is the most tricky bit though! I'll have to look at that carefully.

What do you think about waiting for bug 1245748?

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Sure.  Following bug 1245748 will not be hard.  Never mind!

Thanks for the feedback against part 3 patch!

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8733198 [details]
Bug 1223658 - Part 8: Reftests for checking stacking context of transform animations.

https://reviewboard.mozilla.org/r/41601/#review38303

::: layout/style/StyleAnimationValue.h:401
(Diff revision 1)
>    void SetAndAdoptCSSValueListValue(nsCSSValueList *aValue, Unit aUnit);
>    void SetAndAdoptCSSValuePairListValue(nsCSSValuePairList *aValue);
>  
>    void SetTransformValue(nsCSSValueSharedList* aList);
> +  // Set identity transform to send animations in before phase to compositor.
> +  void SetIdentityTransform();

Do we really need a distinct setter for this?

Instead of growing the StyleAnimationValue API, I suspect it'd be cleaner for the callsite (or callsites) to have a static "MakeIdentityTransform()" function declared nearby (which would look very much like the function you're adding in this patch).  And then the caller would just combine the existing SetTransformValue() API with that static function -- i.e. it'd call `SetTransformValue(MakeIdentityTransform())` or something. Would that work?

::: layout/style/StyleAnimationValue.cpp:3986
(Diff revision 1)
> +StyleAnimationValue::SetIdentityTransform()
> +{
> +  RefPtr<nsCSSValue::Array> arr = AppendFunction(eCSSKeyword_matrix);
> +
> +  nsCSSValue zero(0.0f, eCSSUnit_Pixel);
> +  nsCSSValue one(1.0f, eCSSUnit_Number);

Please declare "one" and "two" here as "static const".

::: layout/style/StyleAnimationValue.cpp:3993
(Diff revision 1)
> +  arr->Item(1) = one;
> +  arr->Item(2) = zero;
> +  arr->Item(3) = zero;
> +  arr->Item(4) = one;
> +  arr->Item(5) = zero;
> +  arr->Item(6) = zero;

Not knowing exactly how this matrix is structured, I initially read this series of assignments (1, 0, 0, 1, 0, 0) as producing a matrix like this:

    1 0 0
    1 0 0

...which doesn't look very "identity-ish".

I assume they're really intended to produce:

    1 0
    0 1
    0 0

If this is correct, consider including a code-comment with that ^^ visual representation of the matrix just before your assignments, so that it's clearer what you're doing here.

::: layout/style/StyleAnimationValue.cpp:3995
(Diff revision 1)
> +  arr->Item(3) = zero;
> +  arr->Item(4) = one;
> +  arr->Item(5) = zero;
> +  arr->Item(6) = zero;
> +
> +  nsCSSValueList *item = new nsCSSValueList;

Please use MakeUnique instead of this first "new", and then use "release()" to transfer ownership, like so:

    auto item = MakeUnique<nsCSSValueList>();
    [...]
    SetTransformValue(new nsCSSValueSharedList(item.release()));

(The "release" function here is the UniquePtr version of nsAutoPtr/nsRefPtr "forget()" -- it lets us transfer ownership.  (Someday we should probably make the nsCSSValueSharedList just take a UniquePtr<>&& or something, so that it can directly steal the value; then we can replace this release() with Move() and be even more explicit/safe.))

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Comment on attachment 8733198 [details]
> MozReview Request: Bug 1223658 - Part 2: Add SetIdentityTransform to be used
> for sending animations in before phase. r?dholbert
> 
> https://reviewboard.mozilla.org/r/41601/#review38303
> 
> ::: layout/style/StyleAnimationValue.h:401
> (Diff revision 1)
> >    void SetAndAdoptCSSValueListValue(nsCSSValueList *aValue, Unit aUnit);
> >    void SetAndAdoptCSSValuePairListValue(nsCSSValuePairList *aValue);
> >  
> >    void SetTransformValue(nsCSSValueSharedList* aList);
> > +  // Set identity transform to send animations in before phase to compositor.
> > +  void SetIdentityTransform();
> 
> Do we really need a distinct setter for this?
> 
> Instead of growing the StyleAnimationValue API, I suspect it'd be cleaner
> for the callsite (or callsites) to have a static "MakeIdentityTransform()"
> function declared nearby (which would look very much like the function
> you're adding in this patch).  And then the caller would just combine the
> existing SetTransformValue() API with that static function -- i.e. it'd call
> `SetTransformValue(MakeIdentityTransform())` or something. Would that work?

The reason why SetIdentityTransform is in StyleAnimationValue is that it needs AppendFunction in StyleAnimationValue.  We need to make AppendFunction in StyleAnimationValue public in order to define MakeIdentityTransform in somewhere else.  Is that OK with you?

Comment 16

[Daniel Holbert [:dholbert]]

Ah, I missed that.  Given that, I agree that this setter (SetIdentityTransform()) makes sense to have [assuming it's needed by later patches]. Thanks!

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8733198 [details]
Bug 1223658 - Part 8: Reftests for checking stacking context of transform animations.

https://reviewboard.mozilla.org/r/41601/#review38325

I'll mark this as r=me with the other nits addressed, then.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

https://reviewboard.mozilla.org/r/41599/#review38329

::: layout/style/StyleAnimationValue.h:217
(Diff revision 1)
>    static bool ExtractComputedValue(nsCSSProperty aProperty,
>                                     nsStyleContext* aStyleContext,
>                                     StyleAnimationValue& aComputedValue);
>  
> +  static bool
> +    ExtractComputedValueForTransition(nsCSSProperty aProperty,

Everthing around this has solid documentation, so this function probably should too, to fit in better here. (and to explain how it differs from the "ExtractComputedValue" function that's declared/documented directly above it).

So: please add some documentation for this function, briefly explaining why it exists & when it should be called.

(It's not immediately obvious to me when it should/shouldn't be used, based on the callsites & implementation.)

Comment 19

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

(In reply to Daniel Holbert [:dholbert] from comment #18)
> Everthing around this has solid documentation, so this function probably
> should too, to fit in better here. (and to explain how it differs from the
> "ExtractComputedValue" function that's declared/documented directly above
> it).
> 
> So: please add some documentation for this function, briefly explaining why
> it exists & when it should be called.
> 
> (It's not immediately obvious to me when it should/shouldn't be used, based
> on the callsites & implementation.)

I was doing some code archaeology on this the other day and I believe it goes back to bug 531942 where we introduced special handling for visibility for transitions only.

Along the way, we decided that should apply to CSS animations too but presumably not to SMIL animations. I think this should apply to Web Animations too since it says, "The animation behavior of CSS properties is defined by the "Animatable:" line in the summary of the property’s definition or in [CSS3-TRANSITIONS] for properties that lack a such a line."[1]

[1] http://w3c.github.io/web-animations/#specific-animation-behaviors

So we should probably rename this but I don't have any good suggestions. Perhaps we could even make ExtractComputedValue take an enum param specifying how we should handle visibility? VisibilityHandling::Clamp or something like that?

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #19)
> So we should probably rename this but I don't have any good suggestions.
> Perhaps we could even make ExtractComputedValue take an enum param
> specifying how we should handle visibility? VisibilityHandling::Clamp or
> something like that?

I like this enum-param idea, I think.

Probably best to sort this out as part of this bug, in any case... Feels a little silly to be moving code around when we're pretty sure (I think?) that it should be replaced.

(Though, if that tangent ends up blocking things too much here, I'd also be OK with making the switch to an enum-param in a followup bug, if you drop an XXX/TODO comment code alongside this function with the followup bug number.)

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1223658 - Part 3: Consider fillMode in compositor thread as well.

Before this patch we've been using fill mode both on the compositor.  Once we
send animations to the compositor in before phase, we have to send fill mode
to know correct state in the before phase.

Review commit: https://reviewboard.mozilla.org/r/45375/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45375/

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

We are going to pass animations to the compositor in before phase but not to
pass when active duration is zero.  To distinguish this state we need a new
function to check that the active duration is zero.

Review commit: https://reviewboard.mozilla.org/r/45377/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45377/

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review commit: https://reviewboard.mozilla.org/r/45385/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45385/

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41599/diff/1-2/

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733198 [details]
Bug 1223658 - Part 8: Reftests for checking stacking context of transform animations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41601/diff/1-2/

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733199 [details]
MozReview Request: Bug 1223658 - Part 5: Add a function to check active duration is zero. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41603/diff/1-2/

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41605/diff/1-2/

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41607/diff/1-2/

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Part 6 patch depends on part 4 patch for bug 1263063.

Comment 30

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

Comment on attachment 8733199 [details]
MozReview Request: Bug 1223658 - Part 5: Add a function to check active duration is zero. r?birtles

https://reviewboard.mozilla.org/r/41603/#review38063

Cancelling this review for the time being while we see if it's possible to implement this without adding the extra initial style value.

::: dom/animation/KeyframeEffect.h:181
(Diff revision 1)
>    bool operator!=(const AnimationProperty& aOther) const {
>      return !(*this == aOther);
>    }
> +
> +  // Set mInitialStyleOnCompositor if and only if |mProperty| can be
> +  // running on compositor.

Nit: run

::: dom/animation/KeyframeEffect.cpp:2378
(Diff revision 1)
> +  RefPtr<nsStyleContext> styleWithoutAnimation =
> +    aStyleContext->PresContext()->StyleSet()->AsGecko()->
> +      ResolveStyleWithoutAnimation(aElement, aStyleContext,
> +                                   eRestyle_AllHintsWithAnimations);
> +  StyleAnimationValue initialStyle;
> +  StyleAnimationValue::ExtractComputedValueForTransition(mProperty,

We shouldn't use ExtractComputedValueForTransition. That method just does special handling of visibility (that we didn't want to also apply to SMIL animation). Since we don't do visibility on the compositor it doesn't seem necessary and we could simply use StyleAnimationValue::ExtractComputedValue here.

::: dom/animation/KeyframeEffect.h:154
(Diff revision 2)
> +  // Represents opacity or transform style without animating style.
> +  // This style is used for sending animations to compositor even though
> +  // it's in the before phase.  To send animations to compositor, we need
> +  // any opacity or transform style at any point.  But in the before phase
> +  // there are some cases the target element has no transform or opacity
> +  // property, in that case this style has identity transform or opacity:1.

As discussed this morning, I wonder if we can get away without adding this? It seems like that might be more simple if it's possible?

::: dom/animation/KeyframeEffect.h:178
(Diff revision 2)
>    }
>    bool operator!=(const AnimationProperty& aOther) const {
>      return !(*this == aOther);
>    }
> +
> +  // Set mInitialStyleOnCompositor if and only if |mProperty| can be

s/can be running/is one of the property we can run/

Comment 31

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

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

https://reviewboard.mozilla.org/r/41605/#review42189

This looks good but I want to find out about the assertion first and whether we can simplify the computation of the start time.

::: layout/base/nsDisplayList.cpp
(Diff revision 2)
> -  MOZ_ASSERT(!aAnimation->GetCurrentOrPendingStartTime().IsNull() ||
> -             (aAnimation->GetTimeline() &&
> -              aAnimation->GetTimeline()->TracksWallclockTime()),
> -             "Animation should either have a resolved start time or "
> -             "a timeline that tracks wallclock time");

Why is it ok to remove this?

::: layout/base/nsDisplayList.cpp:393
(Diff revision 2)
>      aAnimation->GetEffect()->GetComputedTiming();
>    Nullable<TimeDuration> startTime = aAnimation->GetCurrentOrPendingStartTime();
>    animation->startTime() = startTime.IsNull()
>                             ? TimeStamp()
>                             : aAnimation->AnimationTimeToTimeStamp(
> -                              StickyTimeDuration(timing.mDelay));
> +                              StickyTimeDuration(0));

Firstly, this feels a bit weird to pass 0 here.

But secondly, looking at the existing code there seems to be something amiss here. We call GetCurrentOrPendingStartTime but we never use the result except to check if it is null or not.

That's probably just as well because GetCurrentOrPendingStartTime doesn't appear to factor in the playbackRate like it is supposed to. But that's ok because no-one ever uses the result of GetCurrentOrPendingStartTime anymore (except here).

In effect, we're just using Animation::mStartTime and never Animation::mPendingReadyTime. I'd like to understand if that's ok. If it is, we should just drop that function altogether.

Otherwise, I think what we might want to do here is:

* Keep the assertion that we have a timeline or a current/pending start time
* Fix GetCurrentOrPendingStartTime to incorporate the playbackRate
* Check for a playbackRate of zero (we should probably check that elsewhere and never enter this codepath for an animation with playbackRate == 0, then just assert here that playbackRate != 0)
* Change this line to:

  animation->startTime() = startTime.IsNull()
                           ? TimeStamp()
                           : aAnimation->GetTimeline()->ToTimeStamp(startTime);

(If startTime() initializes to a null TimeStamp() then we could simplify this further still I suppose.)

What do you think?

::: layout/base/nsDisplayList.cpp:395
(Diff revision 2)
>    animation->startTime() = startTime.IsNull()
>                             ? TimeStamp()
>                             : aAnimation->AnimationTimeToTimeStamp(
> -                              StickyTimeDuration(timing.mDelay));
> -  animation->initialCurrentTime() = aAnimation->GetCurrentTime().Value()
> -                                    - timing.mDelay;
> +                              StickyTimeDuration(0));
> +  animation->delay() = timing.mDelay;
> +  animation->initialCurrentTime() = aAnimation->GetCurrentTime().Value();

Depending on what we do with part 3, I'm not sure if we need this.

Comment 32

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

Comment on attachment 8739818 [details]
Bug 1223658 - Part 3: Consider fillMode in compositor thread as well.

https://reviewboard.mozilla.org/r/45375/#review42223

r=birtles with comments addressed

::: gfx/layers/ipc/LayersMessages.ipdlh:197
(Diff revision 1)
>    // This uses the NS_STYLE_ANIMATION_DIRECTION_* constants.
>    int32_t direction;
> +  int32_t fillMode;

I wonder if we should use uint32_t for both of these?

The generated type for the members we use to fill in these are:

  enum class PlaybackDirection : uint32_t \{ ... \}
  enum class FillMode : uint32_t \{ ... \}

(i.e. the comment "This uses the NS_STYLE_ANIMATION_DIRECTION_* constants" is no longer true. We should probably fix that comment at the same time.)

::: layout/base/nsDisplayList.cpp:400
(Diff revision 1)
>    animation->initialCurrentTime() = aAnimation->GetCurrentTime().Value();
>    animation->duration() = computedTiming.mDuration;
>    animation->iterations() = computedTiming.mIterations;
>    animation->iterationStart() = computedTiming.mIterationStart;
>    animation->direction() = static_cast<uint32_t>(timing.mDirection);
> +  animation->fillMode() = static_cast<int32_t>(timing.mFill);

We should probably make this uint32_t based on the comment above.

Comment 33

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

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

https://reviewboard.mozilla.org/r/45377/#review42229

r=birtles with comments addressed.

::: dom/animation/Animation.h:267
(Diff revision 1)
> +  bool IsActiveDurationZero() const
> +  {
> +    return GetEffect() && GetEffect()->IsActiveDurationZero();
> +  }

See my comment in the next patch but I actually wonder if we need this. We only use it one place and in that place we already know that GetEffect() is not null. So we could just write:

  GetEffect()->IsActiveDurationZero()

I think that's probably better because strictly speaking *animations* don't have an active duration--their *effects* do. So I think writing the above might be better?

::: dom/animation/TimingParams.h:100
(Diff revision 1)
> +  bool IsDurationOrIterationsZero() const
> +  {
> +    static const StickyTimeDuration zeroDuration;
> +
> +    return (!mDuration || *mDuration == zeroDuration || mIterations == 0.0);
> +  }

This is really pedantic of me, but I think this code would be more clear without this method?

What if we just made KeyframeEffectReadOnly::IsActiveDurationZero do:

  SpecifiedTiming().ActiveDuration() == StickyTimeDuration(0)?

::: dom/animation/TimingParams.h:115
(Diff revision 1)
>    {
>      // If either the iteration duration or iteration count is zero,
>      // Web Animations says that the active duration is zero. This is to
>      // ensure that the result is defined when the other argument is Infinity.
>      static const StickyTimeDuration zeroDuration;
> -    if (!mDuration || *mDuration == zeroDuration || mIterations == 0.0) {
> +    if (IsDurationOrIterationsZero()) {

i.e. we could just leave this line as is.

Comment 34

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

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

https://reviewboard.mozilla.org/r/41607/#review42233

This is looking good but I think I have to see what happens with part 3 and look at it again.

::: dom/animation/Animation.h:273
(Diff revision 2)
> -   * "Playing" is different to "running". An animation in its delay phase is
> -   * still running but we only consider it playing when it is in its active
> -   * interval. This definition is used for fetching the animations that are
> +   * "Playing" is different to "running". An animation in its end delay phase
> +   * is still running but we only consider it playing when its effect is
> +   * current. This definition is used for fetching the animations that are
>     * candidates for running on the compositor (since we don't ship animations
> -   * to the compositor when they are in their delay phase or paused including
> -   * being effectively paused due to having a zero playback rate).
> +   * to the compositor when they are in their end delay phase, zero active
> +   * duration or paused including being effectively paused due to having a zero
> +   * playback rate).

We'll need to rewrite this comment somewhat but first I'd like to see the other changes.

::: dom/animation/Animation.h:281
(Diff revision 2)
>     * candidates for running on the compositor (since we don't ship animations
> -   * to the compositor when they are in their delay phase or paused including
> -   * being effectively paused due to having a zero playback rate).
> +   * to the compositor when they are in their end delay phase, zero active
> +   * duration or paused including being effectively paused due to having a zero
> +   * playback rate).
>     */
>    bool IsPlaying() const

We should probably rename this to something like "IsPlayableOnCompositor" since it has quite a different meaning now that it can return true during the delay phase.

::: dom/animation/Animation.h:283
(Diff revision 2)
> -   * being effectively paused due to having a zero playback rate).
> +   * duration or paused including being effectively paused due to having a zero
> +   * playback rate).
>     */
>    bool IsPlaying() const
>    {
> -    // We need to have an effect in its active interval, and
> +    // We need to have an effect in its before phase or in play, and

"We need to have an effect in its before or active phase, and be either running or waiting to run with
a non-zero playback rate. Also, we don't bother sending animations to the compositor whose target effect has an active duration of zero."

::: dom/animation/Animation.h:285
(Diff revision 2)
> -    return HasInPlayEffect() &&
> +    return HasCurrentEffect() &&
> +           !IsActiveDurationZero() &&

We should drop HasInPlayEffect() and IsInPlay().

::: dom/animation/Animation.h:286
(Diff revision 2)
>    bool IsPlaying() const
>    {
> -    // We need to have an effect in its active interval, and
> +    // We need to have an effect in its before phase or in play, and
>      // be either running or waiting to run with a non-zero playback rate.
> -    return HasInPlayEffect() &&
> +    return HasCurrentEffect() &&
> +           !IsActiveDurationZero() &&

As per my feedback on the previous part, I think it would be ok to make this:

  return HasCurrentEffect() &&
         !GetEffect()->IsActiveDurationZero() &&
         ...

We could put that check last too in order to match the comment above and since it will rarely be true so putting it early won't give us a significant performance improvement.

i.e.

    return HasCurrentEffect() &&
           mPlaybackRate != 0.0 &&
           (PlayState() == AnimationPlayState::Running ||
            mPendingState == PendingState::PlayPending) &&
           !GetEffect()->IsActiveDurationZero();

::: dom/animation/Animation.cpp:763
(Diff revision 2)
>  {
>    if (!mEffect) {
>      return;
>    }
>  
> -  if (!IsInEffect()) {
> +  if (!IsInEffect() && !HasCurrentEffect()) {

Why is this needed?

::: dom/animation/EffectCompositor.cpp:663
(Diff revision 2)
>  
>    // Iterate from highest to lowest composite order.
>    for (KeyframeEffectReadOnly* effect : Reversed(sortedEffectList)) {
>      MOZ_ASSERT(effect->GetAnimation(),
>                 "Effects on a target element should have an Animation");
> -    bool inEffect = effect->IsInEffect();
> +    bool isCurrentOrInEffect = effect->IsCurrent() || effect->IsInEffect();

Why is this needed? Maybe it's not depending on what we do with part 3?

::: dom/animation/KeyframeEffect.cpp:562
(Diff revision 2)
> +    if (!aStyleRule) {
> +      // Allocate the style rule now that we know we have animation data.
> +      aStyleRule = new AnimValuesStyleRule();
> +    }
> +
> +    if (computedTiming.mProgress.IsNull()) {
> +      if (!nsCSSProps::PropHasFlags(prop.mProperty,
> +                                    CSS_PROPERTY_CAN_ANIMATE_ON_COMPOSITOR) ||
> +          !nsLayoutUtils::AreAsyncAnimationsEnabled()) {
> +        continue;
> +      }
> +
> +      MOZ_ASSERT(prop.mInitialStyleOnCompositor);
> +      StyleAnimationValue *val = aStyleRule->AddEmptyValue(prop.mProperty);
> +      *val = *prop.mInitialStyleOnCompositor;
> +      continue;
> +    }

I suppose this change won't be needed depending on what we do for part 3?

::: dom/animation/test/chrome/test_running_on_compositor.html:434
(Diff revision 2)
> +      return addDiv(t, { style: 'animation: anim 100s 100s' })
> +        .getAnimations()[0];
> +    },
> +  },
> +  {
> +    desc: 'CSS transision of opacity',

transition

::: dom/animation/test/chrome/test_running_on_compositor.html:444
(Diff revision 2)
> +      div.style.opacity = 0;
> +      return div.getAnimations()[0];
> +    },
> +  },
> +  {
> +    desc: 'CSS transision of transform',

transition

::: layout/style/test/test_animations_omta.html:265
(Diff revision 2)
> -    omta_is("transform", { tx: 30 }, RunningOn.MainThread,
> +    // We can't get correct styles on compositor during before phase if the
> +    // animation's fill mode is not 'backwards' because we skip applying styles
> +    // during the before phase on the compositor.
> +    omta_is("transform", { tx: 30 }, RunningOn.Either,

I don't understand this comment or why 'Either' is correct here. Is it because the compositor style is not the *animated* value. (omta_is only returns the *animated* style value for 'transform' but for 'opacity' it returns either animated or un-animated.)

Comment 35

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

Comment on attachment 8739820 [details]
Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

https://reviewboard.mozilla.org/r/45385/#review42235

Thanks for doing this. As with my comments on part 5, I think we should actually make this uint32_t and update the comment for this member.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #30)
> ::: dom/animation/KeyframeEffect.h:154
> (Diff revision 2)
> > +  // Represents opacity or transform style without animating style.
> > +  // This style is used for sending animations to compositor even though
> > +  // it's in the before phase.  To send animations to compositor, we need
> > +  // any opacity or transform style at any point.  But in the before phase
> > +  // there are some cases the target element has no transform or opacity
> > +  // property, in that case this style has identity transform or opacity:1.
> 
> As discussed this morning, I wonder if we can get away without adding this?
> It seems like that might be more simple if it's possible?

Thanks for suggesting.  I have succeeded in removal of the initial style.  But we still need to return an identity transform from KeyframeEffectReadOnly::ComposeStyle() if the target element has no transform style initially because nsDisplayTransform is not built until when the associated nsIFrame has NS_FRAME_MAY_BE_TRANSFORMED.  To do this I've added mHasNoTransformStyle into KeyframeEffectReadOnly and set it true when the target element has no transform style in KeyframeEffectReadOnly::UpdateProperties(), and return the identity transform depends on the flag in ComposeStyle().

Comment 37

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> (In reply to Brian Birtles (:birtles) from comment #30)
> > ::: dom/animation/KeyframeEffect.h:154
> > (Diff revision 2)
> > > +  // Represents opacity or transform style without animating style.
> > > +  // This style is used for sending animations to compositor even though
> > > +  // it's in the before phase.  To send animations to compositor, we need
> > > +  // any opacity or transform style at any point.  But in the before phase
> > > +  // there are some cases the target element has no transform or opacity
> > > +  // property, in that case this style has identity transform or opacity:1.
> > 
> > As discussed this morning, I wonder if we can get away without adding this?
> > It seems like that might be more simple if it's possible?
> 
> Thanks for suggesting.  I have succeeded in removal of the initial style. 
> But we still need to return an identity transform from
> KeyframeEffectReadOnly::ComposeStyle() if the target element has no
> transform style initially because nsDisplayTransform is not built until when
> the associated nsIFrame has NS_FRAME_MAY_BE_TRANSFORMED.  To do this I've
> added mHasNoTransformStyle into KeyframeEffectReadOnly and set it true when
> the target element has no transform style in
> KeyframeEffectReadOnly::UpdateProperties(), and return the identity
> transform depends on the flag in ComposeStyle().

For (my own) references the check to build display items is in nsIFrame::BuildDisplayListForChild.
https://dxr.mozilla.org/mozilla-central/rev/1f16d3da9280e40ada252acf8110b91ee1edbb08/layout/generic/nsFrame.cpp#2737
isStackingContext there depends on its nsIFrame.  isVisuallyAtomic which is a component of isStackingContext is particularly important for our animation works.
https://dxr.mozilla.org/mozilla-central/rev/1f16d3da9280e40ada252acf8110b91ee1edbb08/layout/generic/nsFrame.cpp#2681
isVisuallyAtomic checks HasOpacity or IsTransformed there.  The difference between HasOpacity and IsTransformed affects that we need to use the identity transform in case of transform animations.

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41599/diff/2-3/

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733198 [details]
Bug 1223658 - Part 8: Reftests for checking stacking context of transform animations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41601/diff/2-3/

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41605/diff/2-3/

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739818 [details]
Bug 1223658 - Part 3: Consider fillMode in compositor thread as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45375/diff/1-2/

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733199 [details]
MozReview Request: Bug 1223658 - Part 5: Add a function to check active duration is zero. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41603/diff/2-3/

Comment 43

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41607/diff/2-3/

Comment 44

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45377/diff/1-2/

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739820 [details]
Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45385/diff/1-2/

Comment 46

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/41607/#review42233

> We should drop HasInPlayEffect() and IsInPlay().

Dropped in part 7.

> Why is this needed?

This is needed especially for transform animations whose target element has no transform style initially.  Without this nsDisplayTransform is not built during before phase at all.

Should we check that the animation has transform animating property and no transform style here?

> Why is this needed? Maybe it's not depending on what we do with part 3?

Yes, right.  It depends on part 3.  So it still needed here.

> I don't understand this comment or why 'Either' is correct here. Is it because the compositor style is not the *animated* value. (omta_is only returns the *animated* style value for 'transform' but for 'opacity' it returns either animated or un-animated.)

Ah, I was misunderstanding the purpose of omta_is. Thanks!

Comment 47

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/41605/#review42189

> Why is it ok to remove this?

This assertion had been removed when I had been struggling to make incomplete patch work somehow.  And *no*, the assertion should be there.  Now it checks GetStartTime() instead of GetCurrentOrPendingStartTime() though.

> Firstly, this feels a bit weird to pass 0 here.
> 
> But secondly, looking at the existing code there seems to be something amiss here. We call GetCurrentOrPendingStartTime but we never use the result except to check if it is null or not.
> 
> That's probably just as well because GetCurrentOrPendingStartTime doesn't appear to factor in the playbackRate like it is supposed to. But that's ok because no-one ever uses the result of GetCurrentOrPendingStartTime anymore (except here).
> 
> In effect, we're just using Animation::mStartTime and never Animation::mPendingReadyTime. I'd like to understand if that's ok. If it is, we should just drop that function altogether.
> 
> Otherwise, I think what we might want to do here is:
> 
> * Keep the assertion that we have a timeline or a current/pending start time
> * Fix GetCurrentOrPendingStartTime to incorporate the playbackRate
> * Check for a playbackRate of zero (we should probably check that elsewhere and never enter this codepath for an animation with playbackRate == 0, then just assert here that playbackRate != 0)
> * Change this line to:
> 
>   animation->startTime() = startTime.IsNull()
>                            ? TimeStamp()
>                            : aAnimation->GetTimeline()->ToTimeStamp(startTime);
> 
> (If startTime() initializes to a null TimeStamp() then we could simplify this further still I suppose.)
> 
> What do you think?

Thanks.  I did totally forget about what I was doing in bug 1223255.  Now we can use DocumentTimeline::ToTimeStamp directly as you suggested.

> Depending on what we do with part 3, I'm not sure if we need this.

I am not sure I understand the purpose of initialCurrentTime completely, so I'd like to leave it there.
There is a case that start time is not correctly resolved on the compositor (bug 1264107),  the case might be unrelated to initialCurrentTime but I'd like to drop initialCurrentTime once we understand the reason of bug 1264107.

Comment 48

[Hiroyuki Ikezoe (:hiro)]

https://reviewboard.mozilla.org/r/45375/#review42223

> I wonder if we should use uint32_t for both of these?
> 
> The generated type for the members we use to fill in these are:
> 
>   enum class PlaybackDirection : uint32_t \{ ... \}
>   enum class FillMode : uint32_t \{ ... \}
> 
> (i.e. the comment "This uses the NS_STYLE_ANIMATION_DIRECTION_* constants" is no longer true. We should probably fix that comment at the same time.)

Thanks! Fixed fill mode in this patch.  Fixed the playback direction in part 8.

Comment 49

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

Requesting review because MozReview broke something.

Comment 50

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #47)
> > Firstly, this feels a bit weird to pass 0 here.
> > 
> > But secondly, looking at the existing code there seems to be something amiss here. We call GetCurrentOrPendingStartTime but we never use the result except to check if it is null or not.
> > 
> > That's probably just as well because GetCurrentOrPendingStartTime doesn't appear to factor in the playbackRate like it is supposed to. But that's ok because no-one ever uses the result of GetCurrentOrPendingStartTime anymore (except here).
> > 
> > In effect, we're just using Animation::mStartTime and never Animation::mPendingReadyTime. I'd like to understand if that's ok. If it is, we should just drop that function altogether.
> > 
> > Otherwise, I think what we might want to do here is:
> > 
> > * Keep the assertion that we have a timeline or a current/pending start time
> > * Fix GetCurrentOrPendingStartTime to incorporate the playbackRate
> > * Check for a playbackRate of zero (we should probably check that elsewhere and never enter this codepath for an animation with playbackRate == 0, then just assert here that playbackRate != 0)
> > * Change this line to:
> > 
> >   animation->startTime() = startTime.IsNull()
> >                            ? TimeStamp()
> >                            : aAnimation->GetTimeline()->ToTimeStamp(startTime);
> > 
> > (If startTime() initializes to a null TimeStamp() then we could simplify this further still I suppose.)
> > 
> > What do you think?
> 
> Thanks.  I did totally forget about what I was doing in bug 1223255.  Now we
> can use DocumentTimeline::ToTimeStamp directly as you suggested.

I'm concerned that https://hg.mozilla.org/mozilla-central/rev/272af1e271a4 (bug 1223255) was not correct. I'm afraid we might have regressed bug 1123196.

Comment 51

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #50)
> > Thanks.  I did totally forget about what I was doing in bug 1223255.  Now we
> > can use DocumentTimeline::ToTimeStamp directly as you suggested.
> 
> I'm concerned that https://hg.mozilla.org/mozilla-central/rev/272af1e271a4
> (bug 1223255) was not correct. I'm afraid we might have regressed bug
> 1123196.

I will look into bug 1123196. Cleared all review request since looking bug 1123196 will take a while since there is no concrete test cases.

Comment 52

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #51)
> (In reply to Brian Birtles (:birtles) from comment #50)
> > > Thanks.  I did totally forget about what I was doing in bug 1223255.  Now we
> > > can use DocumentTimeline::ToTimeStamp directly as you suggested.
> > 
> > I'm concerned that https://hg.mozilla.org/mozilla-central/rev/272af1e271a4
> > (bug 1223255) was not correct. I'm afraid we might have regressed bug
> > 1123196.
> 
> I will look into bug 1123196. Cleared all review request since looking bug
> 1123196 will take a while since there is no concrete test cases.

Thank you! Yes, I noticed that too. I think bug 1123196 comment 16 is probably the best start. I wonder if it is possible now to create an automated test for this somehow.

Comment 53

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

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

https://reviewboard.mozilla.org/r/45377/#review44063

I'm so confused by what MozReview is doing right now, but I'm pretty sure this is r=me.

Comment 54

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #52)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #51)
> > (In reply to Brian Birtles (:birtles) from comment #50)
> > > > Thanks.  I did totally forget about what I was doing in bug 1223255.  Now we
> > > > can use DocumentTimeline::ToTimeStamp directly as you suggested.
> > > 
> > > I'm concerned that https://hg.mozilla.org/mozilla-central/rev/272af1e271a4
> > > (bug 1223255) was not correct. I'm afraid we might have regressed bug
> > > 1123196.
> > 
> > I will look into bug 1123196. Cleared all review request since looking bug
> > 1123196 will take a while since there is no concrete test cases.
> 
> Thank you! Yes, I noticed that too. I think bug 1123196 comment 16 is
> probably the best start. I wonder if it is possible now to create an
> automated test for this somehow.

I am now trying to understand bug 1123196, but it gets complicated than I thought initially.  So I filed bug 1266254 as an interim report for now.

Comment 55

[Hiroyuki Ikezoe (:hiro)]

I've decided to take the approach which retains GetCurrentOrPendingStartTime() because bug 1266254 will take more time and some of patches here go rotten, actually part 1 has not been valid.

Comment 56

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41599/diff/3-4/

Comment 57

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733198 [details]
Bug 1223658 - Part 8: Reftests for checking stacking context of transform animations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41601/diff/3-4/

Comment 58

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41605/diff/3-4/

Comment 59

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739818 [details]
Bug 1223658 - Part 3: Consider fillMode in compositor thread as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45375/diff/2-3/

Comment 60

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733199 [details]
MozReview Request: Bug 1223658 - Part 5: Add a function to check active duration is zero. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41603/diff/3-4/

Comment 61

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41607/diff/3-4/

Comment 62

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45377/diff/2-3/

Comment 63

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739820 [details]
Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45385/diff/2-3/

Comment 64

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

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

https://reviewboard.mozilla.org/r/41605/#review45091

Looks good! Thank you!

r=birtles with comments addressed.

::: dom/animation/Animation.cpp:581
(Diff revision 4)
> -  // need to incorporate the playbackRate when implemented (bug 1127380).
> -  result.SetValue(mPendingReadyTime.Value() - mHoldTime.Value());
> +  if (mPlaybackRate != 0) {
> +    result.SetValue(mPendingReadyTime.Value() -
> +        (mHoldTime.Value().MultDouble(1 / mPlaybackRate)));
> +  } else {
> +    result.SetValue(mPendingReadyTime.Value());
> +  }
>    return result;

I wonder if we could factor out a common method to use here and ResumeAt. Something that looks like:

/**
 * Calculates the corresponding start time to use for an animation that is
 * currently pending with current time |mHoldTime| but should behave
 * as if it began or resumed playback at timeline time |aReadyTime|.
 */
TimeDuration
Animation::StartTimeFromReadyTime(const TimeDuration& aReadyTime) const
{
  MOZ_ASSERT(!mHoldTime.IsNull(), "Hold time should be set in order to"
                                  " convert a ready time to a start time");
  ...
}

We'd still need to check for 'mPlaybackRate != 0' and reset mHoldTime in Animation::ResumeAt but at least it would reduce some duplicated code?

Then we could remove the second line of the comment before this block.

::: layout/base/nsDisplayList.cpp:383
(Diff revision 4)
>               (aAnimation->GetTimeline() &&
>                aAnimation->GetTimeline()->TracksWallclockTime()),
>               "Animation should either have a resolved start time or "
>               "a timeline that tracks wallclock time");
> +  MOZ_ASSERT(aAnimation->PlaybackRate() != 0.0,
> +             "Animation which has playbackRate of zero should not send to "

Nit: s/send/be sent/

Comment 65

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

Comment on attachment 8733199 [details]
MozReview Request: Bug 1223658 - Part 5: Add a function to check active duration is zero. r?birtles

https://reviewboard.mozilla.org/r/41603/#review45101

Comment 66

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

Comment on attachment 8739820 [details]
Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

https://reviewboard.mozilla.org/r/45385/#review45099

::: gfx/layers/ipc/LayersMessages.ipdlh:197
(Diff revision 3)
>    // Number of times to repeat the animation, including positive infinity.
>    // Values <= 0 mean the animation will not play (although events are still
>    // dispatched on the main thread).
>    float iterations;
>    float iterationStart;
> -  // This uses the NS_STYLE_ANIMATION_DIRECTION_* constants.
> +  // This uses the dom::PlaybackDirection.

Nit: This uses the dom::PlaybackDirection enumeration values.

(We could update the comment about dom::FillMode too below.)

Comment 67

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

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

https://reviewboard.mozilla.org/r/41599/#review45103

Thanks for doing this!

Comment 68

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

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

https://reviewboard.mozilla.org/r/41607/#review45097

I don't want to cancel review, I just want to add a comment but MozReview doesn't seem to allow me to do that...

::: dom/animation/Animation.h:270
(Diff revision 4)
> -   * still running but we only consider it playing when it is in its active
> -   * interval. This definition is used for fetching the animations that are
> +   * We send animations to the compositor if the animation is in before or
> +   * active phase but not paused or active duration of zero.

Nit: We could make this sentence a little easier to parse if we say something like:

"We send animations to the compositor if they are in the before or active phase, have a non-zero active duration, and are not paused (including being effectively paused due to having zero playback rate)."

::: dom/animation/Animation.cpp:767
(Diff revision 4)
>  {
>    if (!mEffect) {
>      return;
>    }
>  
> -  if (!IsInEffect()) {
> +  if (!IsInEffect() && !HasCurrentEffect()) {

As discussed on IRC, I'm concerned about this part: making us run ComposeStyle while in the before phase.

I'm concerned that it could be a little slower since it affects *all* animations in their before phase--for most animations we would do a lot of unnecessary work.

I'm also concerned that it makes this code harder to understand. It seems surprising to me that we would run through this when we are in the before phase without any backwards fill. At very least we need a comment here explaining why we continue going in that case.

Most of all, however, I think we shouldn't have to do tricks here in animation code to get the display list generation to do the right thing. We should try to decouple those concerns if we can.

So, I'd prefer we work out how to tweak display-list generation to create transform display items when we have an upcoming transform animation.

Comment 69

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add nsIFrame::IsOrWillBeTransformed() to build nsDisplayTransform even when transform animations are in the delay phase

Matt, I need your help.
In this bug, we are going to send transform animations to the compositor even though the animations are in the delay phase.  To archive this I did initially some tweaks in animation code, but as Brian noted in comment 68 it affects all animations.  To archive this in building display lists code, we need some tweaks in nsIframe::IsTransformed() and its callers. Currently nsIFrame::IsTransformed() checks NS_FRAME_MAY_BE_TRANSFORMED bit, but the bit checks only *current* style, not incoming one.  So I'd like to introduce a similar function which does not check NS_FRAME_MAY_BE_TRANSFORMED.

Here is a patch of proof of the concept.  Does this make sense?

Comment 70

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8744209 [details] [diff] [review]
Add nsIFrame::IsOrWillBeTransformed() to build nsDisplayTransform even when transform animations are in the delay phase

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #69)
> Created attachment 8744209 [details] [diff] [review]
> Add nsIFrame::IsOrWillBeTransformed() to build nsDisplayTransform even when
> transform animations are in the delay phase
> 
> Matt, I need your help.
> In this bug, we are going to send transform animations to the compositor
> even though the animations are in the delay phase.  To archive this I did
> initially some tweaks in animation code, but as Brian noted in comment 68 it
> affects all animations.  To archive this in building display lists code, we
> need some tweaks in nsIframe::IsTransformed() and its callers. Currently
> nsIFrame::IsTransformed() checks NS_FRAME_MAY_BE_TRANSFORMED bit, but the
> bit checks only *current* style, not incoming one.  So I'd like to introduce
> a similar function which does not check NS_FRAME_MAY_BE_TRANSFORMED.
> 
> Here is a patch of proof of the concept.  Does this make sense?

NS_FRAME_MAY_BE_TRANSFORM is meant to be just an optimization to avoid needing to check all the style properties if a frame definitely doesn't have a transform.

You should try get that bit set on the frames you want to build transforms for, rather than skipping the check.

Returning true for IsTransformed() is also going to force the element to build a stacking context. Is this the correct behaviour for an animation in the delay phase?

Comment 71

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #70)
> Returning true for IsTransformed() is also going to force the element to
> build a stacking context. Is this the correct behaviour for an animation in
> the delay phase?

I think no matter what approach we use for this bug, we're going to end up building a stacking context during the delay phase. I guess we need to test what other browsers do here and then possibly follow up on www-style.

Comment 72

[Hiroyuki Ikezoe (:hiro)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #70)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #69)
> > Created attachment 8744209 [details] [diff] [review]
> > Add nsIFrame::IsOrWillBeTransformed() to build nsDisplayTransform even when
> > transform animations are in the delay phase
> > 
> > Matt, I need your help.
> > In this bug, we are going to send transform animations to the compositor
> > even though the animations are in the delay phase.  To archive this I did
> > initially some tweaks in animation code, but as Brian noted in comment 68 it
> > affects all animations.  To archive this in building display lists code, we
> > need some tweaks in nsIframe::IsTransformed() and its callers. Currently
> > nsIFrame::IsTransformed() checks NS_FRAME_MAY_BE_TRANSFORMED bit, but the
> > bit checks only *current* style, not incoming one.  So I'd like to introduce
> > a similar function which does not check NS_FRAME_MAY_BE_TRANSFORMED.
> > 
> > Here is a patch of proof of the concept.  Does this make sense?
> 
> NS_FRAME_MAY_BE_TRANSFORM is meant to be just an optimization to avoid
> needing to check all the style properties if a frame definitely doesn't have
> a transform.
> 
> You should try get that bit set on the frames you want to build transforms
> for, rather than skipping the check.

Thank you, Matt.  One thing what I was concerned about NS_FRAME_MAY_BE_TRANSFORMED is that 
the comment of NS_FRAME_MAY_BE_TRANSFORMED in nsFrameStateBits.h.
https://dxr.mozilla.org/mozilla-central/rev/fc15477ce628599519cb0055f52cc195d640dc94/layout/generic/nsFrameStateBits.h#147

It says that "This is used primarily in GetTransformMatrix to optimize for the common case".  Once we set NS_FRAME_MAY_BE_TRANSFORMED in delay phase, GetTransformMatrix calculates matrix for the transform.  Is that OK with you?  Or should we do some tweaks there in case of delay phase?

Comment 73

[Matt Woodrow (:mattwoodrow)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #72)

> It says that "This is used primarily in GetTransformMatrix to optimize for
> the common case".  Once we set NS_FRAME_MAY_BE_TRANSFORMED in delay phase,
> GetTransformMatrix calculates matrix for the transform.  Is that OK with
> you?  Or should we do some tweaks there in case of delay phase?

As long as it computes the identity transform then that's ok.

(In reply to Brian Birtles (:birtles) from comment #71) 
> I think no matter what approach we use for this bug, we're going to end up
> building a stacking context during the delay phase. I guess we need to test
> what other browsers do here and then possibly follow up on www-style.

That seems fine, I just didn't see anything in the spec about it.

It's not what we do currently, so it seems plausible that there could be web compat issues.

Comment 74

[Hiroyuki Ikezoe (:hiro)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #73)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #72)
> 
> > It says that "This is used primarily in GetTransformMatrix to optimize for
> > the common case".  Once we set NS_FRAME_MAY_BE_TRANSFORMED in delay phase,
> > GetTransformMatrix calculates matrix for the transform.  Is that OK with
> > you?  Or should we do some tweaks there in case of delay phase?
> 
> As long as it computes the identity transform then that's ok.

Thanks, I will try.

Brian, I think KeyframeEffectReadOnly::UpdateProperties is the right place to set NS_FRAME_MAY_BE_TRANSFORMED bit but I have no idea where we unset the bit.  We should wait for bug 1067769?

Comment 75

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #74)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #73)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #72)
> > 
> > > It says that "This is used primarily in GetTransformMatrix to optimize for
> > > the common case".  Once we set NS_FRAME_MAY_BE_TRANSFORMED in delay phase,
> > > GetTransformMatrix calculates matrix for the transform.  Is that OK with
> > > you?  Or should we do some tweaks there in case of delay phase?
> > 
> > As long as it computes the identity transform then that's ok.
> 
> Thanks, I will try.
> 
> Brian, I think KeyframeEffectReadOnly::UpdateProperties is the right place
> to set NS_FRAME_MAY_BE_TRANSFORMED bit but I have no idea where we unset the
> bit.  We should wait for bug 1067769?

I'm not sure that's the right place. For example, you could do:

  var effect = new KeyframeEffect(div,
                                  { transform: [ 'translate(0px)', 'translate(100px)' ] },
                                  { delay: 1000, duration: 1000 });

We will call UpdateProperties as part of the constructor (inside SetFrames) even though we are not playing the effect at that time. And, ideally, I don't think we'll call UpdateProperties again even once we start playing the effect.

Perhaps we can just do a check in Animation::ComposeStyle for an animation with a transform (perhaps even a transform that we're able to run on the compositor?) and either set the frame bit there if we are in the before/active phase or clear it if we are not?

We could do that check before the check for IsInEffect() and then we can still keep the optimization that animations in their before phase return early from ComposeStyle?

Most of all, however, I think we need to check when other browsers create a stacking context. If there are Web compatibility issues with creating a stacking context during the delay phase then we might have to fix this bug a completely different way, if at all.

Comment 76

[Hiroyuki Ikezoe (:hiro)]

Attachment: An example to check creating stacking context in delay phase

Chrome does not change stack context in delay phase.

Comment 77

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #76)
> Created attachment 8745192 [details]
> An example to check creating stacking context in delay phase
> 
> Chrome does not change stack context in delay phase.

But it appears that IE and Edge do! Safari does not.

Comment 78

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

(In reply to Brian Birtles (:birtles) from comment #77)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #76)
> > Created attachment 8745192 [details]
> > An example to check creating stacking context in delay phase
> > 
> > Chrome does not change stack context in delay phase.
> 
> But it appears that IE and Edge do! Safari does not.

Followed up on www-style: https://lists.w3.org/Archives/Public/www-style/2016Apr/0422.html

It seems like Simon Fraser from WebKit was proposing something similar in Sep 2015 so WebKit may be open to this change as well.

Comment 79

[Hiroyuki Ikezoe (:hiro)]

Thank you, Brian.  I have suddenly come to like IE and Edge!

Comment 80

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

Based on discussion on www-style, I've updated CSS Animations and Web Animations to mention that a stacking context is created so long as there is a current/in-effect animation on a relevant property:

  https://github.com/w3c/csswg-drafts/commit/d107d5e7b14fd944378da5664394380537b088f3
  https://github.com/w3c/web-animations/commit/e9fed4ba00659a96bb76745937a24664f33f03da

Comment 81

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41599/diff/4-5/

Comment 82

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733198 [details]
Bug 1223658 - Part 8: Reftests for checking stacking context of transform animations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41601/diff/4-5/

Comment 83

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41605/diff/4-5/

Comment 84

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739818 [details]
Bug 1223658 - Part 3: Consider fillMode in compositor thread as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45375/diff/3-4/

Comment 85

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733199 [details]
MozReview Request: Bug 1223658 - Part 5: Add a function to check active duration is zero. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41603/diff/4-5/

Comment 86

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41607/diff/4-5/

Comment 87

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45377/diff/3-4/

Comment 88

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739820 [details]
Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45385/diff/3-4/

Comment 89

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #75)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #74)
> > (In reply to Matt Woodrow (:mattwoodrow) from comment #73)
> > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #72)
> > > 
> > > > It says that "This is used primarily in GetTransformMatrix to optimize for
> > > > the common case".  Once we set NS_FRAME_MAY_BE_TRANSFORMED in delay phase,
> > > > GetTransformMatrix calculates matrix for the transform.  Is that OK with
> > > > you?  Or should we do some tweaks there in case of delay phase?
> > > 
> > > As long as it computes the identity transform then that's ok.
> > 
> > Thanks, I will try.
> > 
> > Brian, I think KeyframeEffectReadOnly::UpdateProperties is the right place
> > to set NS_FRAME_MAY_BE_TRANSFORMED bit but I have no idea where we unset the
> > bit.  We should wait for bug 1067769?
> 
> I'm not sure that's the right place. For example, you could do:
> 
>   var effect = new KeyframeEffect(div,
>                                   { transform: [ 'translate(0px)',
> 'translate(100px)' ] },
>                                   { delay: 1000, duration: 1000 });
> 
> We will call UpdateProperties as part of the constructor (inside SetFrames)
> even though we are not playing the effect at that time. And, ideally, I
> don't think we'll call UpdateProperties again even once we start playing the
> effect.
> 
> Perhaps we can just do a check in Animation::ComposeStyle for an animation
> with a transform (perhaps even a transform that we're able to run on the
> compositor?) and either set the frame bit there if we are in the
> before/active phase or clear it if we are not?
> 
> We could do that check before the check for IsInEffect() and then we can
> still keep the optimization that animations in their before phase return
> early from ComposeStyle?

As discussed this morning, there are some cases we don't have corresponding nsIFrame in ComposeStyle().   So we'd take an approach to set an identity transform in ComposeStyle() before IsInEffect() check.

Comment 90

[Hiroyuki Ikezoe (:hiro)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c91560afef557b3e4d6bce1461c7bb5573e40a
Bug 1223658 - Part 1: Drop ExtractComputedValueForTransition. r=birtles,dholbert

Comment 91

[Hiroyuki Ikezoe (:hiro)]

Landed part 1 patch since Brian wants to do it.  See bug 1271526 comment 3.

Comment 92

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e8c91560afef

Comment 94

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

I've been going through the review for part 6, and I'm a bit concerned about the way we update KeyframeEffectReadOnly::mNeedsIdentityTransformStyle from KeyframeEffectReadOnly::UpdateProperties.

I'm afraid this bit of state will fail to get updated leading to bugs. For example, in future we hope to call UpdateProperties less often. We currently update it when the StyleContext changes but we should only really do that if there's something in the keyframes that depends on the style context (e.g. em-based units etc.). If we fix that, then I'm afraid this will break.

Is it possible to do this without adding extra state?

I guess the tricky bit is we don't have a style context at this point and we shouldn't add anything to the style rule if there is already a transform specified elsewhere?

Is it possible to set a flag on the node instead? We could clear/set it in EffectCompositor::ComposeAnimationRule, perhaps?

Comment 95

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

Thinking about this a bit more, I think part of the challenge is we want the check for transforms to be fast in the common case (no transforms). I wonder if we can use a combination of NS_FRAME_MAY_BE_TRANSFORMED and the MayHaveAnimations flag on the node to quickly filter out animations that have neither transforms nor animations -- then, if MayHaveAnimations is true, we could do the check to see if we have a transform animation at that point (rather than storing state). Just a thought.

Comment 96

[Hiroyuki Ikezoe (:hiro)]

Thanks for re-thinking about this.

(In reply to Brian Birtles (:birtles) from comment #95)
> Thinking about this a bit more, I think part of the challenge is we want the
> check for transforms to be fast in the common case (no transforms). I wonder
> if we can use a combination of NS_FRAME_MAY_BE_TRANSFORMED and the
> MayHaveAnimations flag on the node to quickly filter out animations that
> have neither transforms nor animations -- then, if MayHaveAnimations is
> true, we could do the check to see if we have a transform animation at that
> point (rather than storing state). Just a thought.

If you mean that this check is in nsIFrame::IsTransformed() and the check is something like below, it sounds work to me.

 if ((mState & ~NS_FRAME_MAY_BE_TRANSFORMED) &&
     (!mContent || !mContent->MayHaveAnimations()) {
    return false;
 }
 if (StyleDisplay()->HasTransform(this) || ...

Before I try this, I will re-consider adding NS_FRAME_MAY_BE_TRANSFORMED flag somehow in all cases.

Comment 97

[Hiroyuki Ikezoe (:hiro)]

I've now managed to add NS_FRAME_MAY_BE_TRANSFORMED flag in all cases.  We need to set NS_FRAME_MAY_BE_TRANSFORMED in two different places.  Animation::ComposeStyle (or EffectCompositor::ComposeAnimationRule) and nsIContent::SetPrimaryFrame().

Setting the flag in Animation::ComposeStyle() works fine in most cases, but as I explained in comment 89 there are some cases that we have no nsIFrame there.  On the other hand, NS_FRAME_MAY_BE_TRANSFORMED is usually set in nsIFrame::Init(), but we can't set the flag when we have animations which are during delay phase because in nsIFrame::Init() the frame is not yet associated with content so we have no way to know whether we have animations or not there.  In my conclusion, we need to set the flag when the frame is associated with the content. i.e, SetPrimaryFrame(). 

Brian, what do you think?  And I think we should get feedback from an expert of relationship between our frame constructing process and nsIContent.

Comment 98

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #97)
> I've now managed to add NS_FRAME_MAY_BE_TRANSFORMED flag in all cases.  We
> need to set NS_FRAME_MAY_BE_TRANSFORMED in two different places. 
> Animation::ComposeStyle (or EffectCompositor::ComposeAnimationRule) and
> nsIContent::SetPrimaryFrame().
> 
> Setting the flag in Animation::ComposeStyle() works fine in most cases, but
> as I explained in comment 89 there are some cases that we have no nsIFrame
> there.  On the other hand, NS_FRAME_MAY_BE_TRANSFORMED is usually set in
> nsIFrame::Init(), but we can't set the flag when we have animations which
> are during delay phase because in nsIFrame::Init() the frame is not yet
> associated with content so we have no way to know whether we have animations
> or not there.  In my conclusion, we need to set the flag when the frame is
> associated with the content. i.e, SetPrimaryFrame(). 
> 
> Brian, what do you think?  And I think we should get feedback from an expert
> of relationship between our frame constructing process and nsIContent.

Thanks for looking into this.

Like you said, I'm not sure if SetPrimaryFrame is suitable. It doesn't seem to do anything else like at the moment.

My other concern is where do we clear the flag? e.g., if a transform animation is in its delay phase but then gets cancelled?

Comment 99

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #98)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #97)
> > I've now managed to add NS_FRAME_MAY_BE_TRANSFORMED flag in all cases.  We
> > need to set NS_FRAME_MAY_BE_TRANSFORMED in two different places. 
> > Animation::ComposeStyle (or EffectCompositor::ComposeAnimationRule) and
> > nsIContent::SetPrimaryFrame().
> > 
> > Setting the flag in Animation::ComposeStyle() works fine in most cases, but
> > as I explained in comment 89 there are some cases that we have no nsIFrame
> > there.  On the other hand, NS_FRAME_MAY_BE_TRANSFORMED is usually set in
> > nsIFrame::Init(), but we can't set the flag when we have animations which
> > are during delay phase because in nsIFrame::Init() the frame is not yet
> > associated with content so we have no way to know whether we have animations
> > or not there.  In my conclusion, we need to set the flag when the frame is
> > associated with the content. i.e, SetPrimaryFrame(). 
> > 
> > Brian, what do you think?  And I think we should get feedback from an expert
> > of relationship between our frame constructing process and nsIContent.
> 
> Thanks for looking into this.
> 
> Like you said, I'm not sure if SetPrimaryFrame is suitable. It doesn't seem
> to do anything else like at the moment.
> 
> My other concern is where do we clear the flag? e.g., if a transform
> animation is in its delay phase but then gets cancelled?

I am suspecting our current code does not clear the flag at all something like below case.

 div.animate({transform: ['translateX(0px)', 'translateX(100px)']}, 1000);
 // after this animation is finished, the flag may　keep on being set.

I should check it and fix it first if that's true.

Comment 100

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #99)
> (In reply to Brian Birtles (:birtles) from comment #98)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #97)
> > > I've now managed to add NS_FRAME_MAY_BE_TRANSFORMED flag in all cases.  We
> > > need to set NS_FRAME_MAY_BE_TRANSFORMED in two different places. 
> > > Animation::ComposeStyle (or EffectCompositor::ComposeAnimationRule) and
> > > nsIContent::SetPrimaryFrame().
> > > 
> > > Setting the flag in Animation::ComposeStyle() works fine in most cases, but
> > > as I explained in comment 89 there are some cases that we have no nsIFrame
> > > there.  On the other hand, NS_FRAME_MAY_BE_TRANSFORMED is usually set in
> > > nsIFrame::Init(), but we can't set the flag when we have animations which
> > > are during delay phase because in nsIFrame::Init() the frame is not yet
> > > associated with content so we have no way to know whether we have animations
> > > or not there.  In my conclusion, we need to set the flag when the frame is
> > > associated with the content. i.e, SetPrimaryFrame(). 
> > > 
> > > Brian, what do you think?  And I think we should get feedback from an expert
> > > of relationship between our frame constructing process and nsIContent.
> > 
> > Thanks for looking into this.
> > 
> > Like you said, I'm not sure if SetPrimaryFrame is suitable. It doesn't seem
> > to do anything else like at the moment.
> > 
> > My other concern is where do we clear the flag? e.g., if a transform
> > animation is in its delay phase but then gets cancelled?
> 
> I am suspecting our current code does not clear the flag at all something
> like below case.
> 
>  div.animate({transform: ['translateX(0px)', 'translateX(100px)']}, 1000);
>  // after this animation is finished, the flag may　keep on being set.
> 
> I should check it and fix it first if that's true.

Confirmed and filed bug 1272495.  I am convinced that once we fix bug 1272495, we will have a way to set/clear NS_FRAME_MAY_BE_TRANSFORMED property from code in dom/animation (or some other place)

Comment 101

[Hiroyuki Ikezoe (:hiro)]

I noticed below case has the same issues with regard to stacking context and isRunningOnCompositor flag.

@keyframes move {
  0% { transform: none; }
 50% { transform: none; }
100% { transform: translateX(100px); }
}

It might be better to just modify the check at https://dxr.mozilla.org/mozilla-central/rev/1f1a8b96d5167153d1f750439ba6a1063155a4bc/layout/generic/nsFrame.cpp#557

Comment 102

[Hiroyuki Ikezoe (:hiro)]

I've decided to set NS_FRAME_MAY_BE_TRANSFORMED here and to leave the problem when the flag is cleared as bug 1272495.  As of current implementation, the flag is cleared while reframing process, so I think it will become a big problem.

I've also decided to leave some CSS animations cases as bug 1273042.  The problem case is that CSS animation which sets the animation property right after creating an element.  In such case, the target element does not have the primary frame when we add KeyframeEffect to EffectSet. 

I will upload new patch set with stacking context tests from now on.

Comment 103

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41605/diff/5-6/

Comment 104

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739818 [details]
Bug 1223658 - Part 3: Consider fillMode in compositor thread as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45375/diff/4-5/

Comment 105

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41599/diff/5-6/

Comment 106

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41607/diff/5-6/

Comment 107

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45377/diff/4-5/

Comment 108

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739820 [details]
Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45385/diff/4-5/

Comment 109

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733198 [details]
Bug 1223658 - Part 8: Reftests for checking stacking context of transform animations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41601/diff/5-6/

Comment 110

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #102)
> I've also decided to leave some CSS animations cases as bug 1273042.  The
> problem case is that CSS animation which sets the animation property right
> after creating an element.  In such case, the target element does not have
> the primary frame when we add KeyframeEffect to EffectSet. 

Why does using the API not hit that case?

Comment 111

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #110)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #102)
> > I've also decided to leave some CSS animations cases as bug 1273042.  The
> > problem case is that CSS animation which sets the animation property right
> > after creating an element.  In such case, the target element does not have
> > the primary frame when we add KeyframeEffect to EffectSet. 
> 
> Why does using the API not hit that case?

I have not investigated the problem free case. I just know the problem CSS animation case.  The actual problem is that UpdateAnimations() is called during frame construction process.  I am going to investigate it and the reason why web animation does not hit that case further, but I'd like to do so in bug 1273042.

Comment 112

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #111)
> (In reply to Brian Birtles (:birtles) from comment #110)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #102)
> > > I've also decided to leave some CSS animations cases as bug 1273042.  The
> > > problem case is that CSS animation which sets the animation property right
> > > after creating an element.  In such case, the target element does not have
> > > the primary frame when we add KeyframeEffect to EffectSet. 
> > 
> > Why does using the API not hit that case?
> 
> I have not investigated the problem free case. I just know the problem CSS
> animation case.  The actual problem is that UpdateAnimations() is called
> during frame construction process.  I am going to investigate it and the
> reason why web animation does not hit that case further, but I'd like to do
> so in bug 1273042.

The problem also happens in a case of web animations;

  var div = document.createElement("div");
  div.animate({ transform: [ "translateX(0x)", "translateX(100px)" ] }, 1000);
  document.body.appendChild(div);

In above case, we should also set the flag nsIFrame::Init() I think.

Unfortunately there is another bug in the above case.  the animation is not shown up in devtools at all.

Comment 113

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

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

https://reviewboard.mozilla.org/r/41607/#review48609

This looks good but there are a few parts I don't understand.

::: dom/animation/Animation.h:276
(Diff revision 5)
> -   * "Playing" is different to "running". An animation in its delay phase is
> -   * still running but we only consider it playing when it is in its active
> -   * interval. This definition is used for fetching the animations that are
> -   * candidates for running on the compositor (since we don't ship animations
> -   * to the compositor when they are in their delay phase or paused including
> -   * being effectively paused due to having a zero playback rate).
> -   */
> -  bool IsPlaying() const
> -  {
> -    // We need to have an effect in its active interval, and
> +   * Returns true if this animation can be sent to the compositor.
> +   * We send animations to the compositor if the animation is in before or
> +   * active phase but not paused or active duration of zero.
> +   */
> +  bool IsPlayableOnCompositor() const
> +  {
> +    // We need to have an effect in its before or active phase, and be either
> +    // running or waiting to run with a non-zero playback rate.  Also, we don't
> +    // bother sending animations to the compositor whose target effect has an
> +    // active duration of zero.

These two comments are redundant. I think the first comment is enough. Perhaps we could make it slightly more clear as follows:

"Returns true if this animation's playback state makes it a candidate for running on the compositor.
We send animations to the compositor when their target effect is 'current' (a definition that is roughly equivalent to when they are in their before or active phase). However, we don't send animations to the compositor when they are paused/pausing (including being effectively paused due to having a zero playback rate), have a zero-duration active interval, or have no target effect at all."

::: dom/animation/Animation.cpp:770
(Diff revision 5)
> +             "ComposeStyle should be called only when the animation have a "
> +             "target effect");

"ComposeStyle is only expected to be called on animations with a target effect"

::: dom/animation/EffectSet.h:68
(Diff revision 6)
>    static EffectSet* GetOrCreateEffectSet(dom::Element* aElement,
>                                           CSSPseudoElementType aPseudoType);
>    static void DestroyEffectSet(dom::Element* aElement,
>                                 CSSPseudoElementType aPseudoType);
>  
> -  void AddEffect(dom::KeyframeEffectReadOnly& aEffect);
> +  // Return false if we already have the effect.

Nit: Returns false if the effect is already in the EffectSet.

::: dom/animation/KeyframeEffect.cpp:793
(Diff revision 6)
> +      // Set NS_FRAME_MAY_BE_TRANSFORMED to the associated frame.
> +      // Unfortunately we can't set NS_FRAME_MAY_BE_TRANSFORMED for some CSS
> +      // animations, e.g., set CSS animation right after creating an element,
> +      // since the primary frame is not associated with the target element
> +      // at this point (In case of such CSS animations, this function is
> +      // called during frame construction.).
> +      // For such CSS animations we need to set NS_FRAME_MAY_BE_TRANSFORMED
> +      // in nsFrame::Init(). We will handle the case in bug 1273042.

// Set NS_FRAME_MAY_BE_TRANSFORMED on the associated frame if one is
      // available.
      //
      // Bug 1273042: Set NS_FRAME_MAY_BE_TRANSFORMED for effects when the
      // frame is created/updated after adding to the EffectSet.

?

::: dom/animation/KeyframeEffect.cpp:801
(Diff revision 6)
> +      // since the primary frame is not associated with the target element
> +      // at this point (In case of such CSS animations, this function is
> +      // called during frame construction.).
> +      // For such CSS animations we need to set NS_FRAME_MAY_BE_TRANSFORMED
> +      // in nsFrame::Init(). We will handle the case in bug 1273042.
> +      for (auto property : mProperties) {

Does this work when we change the target?

We are iterating over mProperties but mProperties may not be set until after this is called? For example, SetTarget calls UpdateTargetRegistration *then* MaybeUpdateProperties, so if the animation has a newly-added target element we won't set the flag.

Should be call this at a different point?

::: dom/animation/KeyframeEffect.cpp:802
(Diff revision 6)
> +        // We don't check mWinsInCascade flag here because, at this point,
> +        // UpdateCascadeResults is not proccessed yet.
> +        // FIXME: Bug 1272495: In case of losing in cascading rules,
> +        // NS_FRAME_MAY_BE_TRANSFORMED flag should be removed when the
> +        // animation will be removed from effect set.

I think the comment should probably be something like:

        // We don't check mWinsInCascade flag here because, at this point,
        // UpdateCascadeResults has not yet run.
        //
        // FIXME: Bug 1272495: If this effect does not win in the cascade,
        // the NS_FRAME_MAY_BE_TRANSFORMED flag should be removed when the
        // animation will be removed from effect set.

But that seems odd. If the animation *loses* we need to clear the flag when it is removed?

Is this explaining why we don't just call GetAnimationOfProperty?

Maybe we should pass a flag to GetAnimationOfProperty to indicate if we care about mWinsInCascade or not?

::: dom/animation/test/chrome/test_running_on_compositor.html:465
(Diff revision 6)
> +  {
> +    desc: 'CSS animation of opacity',
> +    setupAnimation: function(t) {
> +      return addDiv(t, { style: 'animation: opacity 100s 100s' })
> +        .getAnimations()[0];
> +    },
> +  },

Just curious: is there a reason we don't have an test for CSS animation of transform (like we do for script animations and transitions?)

::: dom/animation/test/chrome/test_running_on_compositor.html:494
(Diff revision 6)
> +      return div.getAnimations()[0];
> +    },
> +  },
> +];
> +
> +var delayPhaseWithTransformStyleTests = [

What the significant of this set of tests. What are we trying to check that's different to the first set of tests? Perhaps we can add a comment explaining the purpose of the different test groups?

Comment 114

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

Comment on attachment 8733198 [details]
Bug 1223658 - Part 8: Reftests for checking stacking context of transform animations.

https://reviewboard.mozilla.org/r/41601/#review51690

::: layout/reftests/css-animations/stacking-context-opacity-1-animation.html:3
(Diff revision 6)
> +<title>Opacity animation creates a stacking context even the animation is delay phase</title>
> +<style>
> +span {
> +  height: 100px;
> +  width: 100px;
> +  position: fixed;
> +  background: green;
> +  top: 50px;
> +}
> +@keyframes opacity {
> +  from { opacity: 1 }
> +  to { opacity: 1 }
> +}
> +#test {
> +  width: 100px; height: 100px;
> +  background: blue;
> +  animation: opacity 100s;

This says it tests the delay phase but it seems like the animation has no delay? I guess the comment is supposed to say "even if it only has 100% opacity in its keyframes" or something like that?

::: layout/reftests/css-animations/stacking-context-opacity-in-delay.html:3
(Diff revision 6)
> +<!DOCTYPE html>
> +<html>
> +<title>Opacity animation creates a stacking context even the animation is delay phase</title>

Nit: even *if* the

Likewise for the other titles in this patch.

::: layout/reftests/css-animations/stacking-context-transform-in-delay.html:27
(Diff revision 6)
> +  var anim = div.getAnimations()[0];
> +  anim.ready.then(function() {
> +    document.documentElement.classList.remove("reftest-wait");
> +  });

(We could probably just wait a frame if we want to avoid the dependency on Web Animations API?)

We should probably add a comment saying why we need to wait a frame for transform but not for opacity?

Comment 115

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #113)
> ::: dom/animation/KeyframeEffect.cpp:801
> (Diff revision 6)
> > +      // since the primary frame is not associated with the target element
> > +      // at this point (In case of such CSS animations, this function is
> > +      // called during frame construction.).
> > +      // For such CSS animations we need to set NS_FRAME_MAY_BE_TRANSFORMED
> > +      // in nsFrame::Init(). We will handle the case in bug 1273042.
> > +      for (auto property : mProperties) {
> 
> Does this work when we change the target?
> 
> We are iterating over mProperties but mProperties may not be set until after
> this is called? For example, SetTarget calls UpdateTargetRegistration *then*
> MaybeUpdateProperties, so if the animation has a newly-added target element
> we won't set the flag.

Oh! Thanks!  I did overlook that MaybeUpdateProperties is called after UpdateTargetRegistration.  But I think it does not matter here because changing target does not mean we drop transform property, right? We should perhaps check mKeyframes instead?

> Should be call this at a different point?

I can not find any better place to call this for now.

> ::: dom/animation/KeyframeEffect.cpp:802
> (Diff revision 6)
> > +        // We don't check mWinsInCascade flag here because, at this point,
> > +        // UpdateCascadeResults is not proccessed yet.
> > +        // FIXME: Bug 1272495: In case of losing in cascading rules,
> > +        // NS_FRAME_MAY_BE_TRANSFORMED flag should be removed when the
> > +        // animation will be removed from effect set.
> 
> I think the comment should probably be something like:
> 
>         // We don't check mWinsInCascade flag here because, at this point,
>         // UpdateCascadeResults has not yet run.
>         //
>         // FIXME: Bug 1272495: If this effect does not win in the cascade,
>         // the NS_FRAME_MAY_BE_TRANSFORMED flag should be removed when the
>         // animation will be removed from effect set.
> 
> But that seems odd. If the animation *loses* we need to clear the flag when
> it is removed?
> 
> Is this explaining why we don't just call GetAnimationOfProperty?

Yes, that's right.

> Maybe we should pass a flag to GetAnimationOfProperty to indicate if we care
> about mWinsInCascade or not?

Good idea!  I will do so, if checking mKeyframes does not work as I expected.

> ::: dom/animation/test/chrome/test_running_on_compositor.html:465
> (Diff revision 6)
> > +  {
> > +    desc: 'CSS animation of opacity',
> > +    setupAnimation: function(t) {
> > +      return addDiv(t, { style: 'animation: opacity 100s 100s' })
> > +        .getAnimations()[0];
> > +    },
> > +  },
> 
> Just curious: is there a reason we don't have an test for CSS animation of
> transform (like we do for script animations and transitions?)

The test is split out another test at the end of the file. (CSS animation of transform on painted element in the delay phase)

> ::: dom/animation/test/chrome/test_running_on_compositor.html:494
> (Diff revision 6)
> > +      return div.getAnimations()[0];
> > +    },
> > +  },
> > +];
> > +
> > +var delayPhaseWithTransformStyleTests = [
> 
> What the significant of this set of tests. What are we trying to check
> that's different to the first set of tests? Perhaps we can add a comment
> explaining the purpose of the different test groups?

The purpose is that the flag, NS_FRAME_MAY_BE_TRANSFORMED, is not cleared by setting transform:none style to the element.  I will add the comment there.

Thanks.

Comment 116

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #113)
> ::: dom/animation/KeyframeEffect.cpp:801
> (Diff revision 6)
> > +      // since the primary frame is not associated with the target element
> > +      // at this point (In case of such CSS animations, this function is
> > +      // called during frame construction.).
> > +      // For such CSS animations we need to set NS_FRAME_MAY_BE_TRANSFORMED
> > +      // in nsFrame::Init(). We will handle the case in bug 1273042.
> > +      for (auto property : mProperties) {
> 
> Does this work when we change the target?
> 
> We are iterating over mProperties but mProperties may not be set until after
> this is called? For example, SetTarget calls UpdateTargetRegistration *then*
> MaybeUpdateProperties, so if the animation has a newly-added target element
> we won't set the flag.

The flag is certainly updated in case of changing the target.  But a test for changing target made me realize that current approach relies on SchedulePaint call in PendingAnimationTracker[1].  That's because we don't return any styles from ComposeStyle() in the delay phase,  the paint process in the delay phase is kicked by the SchedulePaint calls.

[1] https://dxr.mozilla.org/mozilla-central/rev/8d0aadfe7da782d415363880008b4ca027686137/dom/animation/PendingAnimationTracker.cpp#121

I will post an additional patch here or open a new bug.  It will depend on the patch size.

Comment 117

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #116)
> (In reply to Brian Birtles (:birtles) from comment #113)
> > ::: dom/animation/KeyframeEffect.cpp:801
> > (Diff revision 6)
> > > +      // since the primary frame is not associated with the target element
> > > +      // at this point (In case of such CSS animations, this function is
> > > +      // called during frame construction.).
> > > +      // For such CSS animations we need to set NS_FRAME_MAY_BE_TRANSFORMED
> > > +      // in nsFrame::Init(). We will handle the case in bug 1273042.
> > > +      for (auto property : mProperties) {
> > 
> > Does this work when we change the target?
> > 
> > We are iterating over mProperties but mProperties may not be set until after
> > this is called? For example, SetTarget calls UpdateTargetRegistration *then*
> > MaybeUpdateProperties, so if the animation has a newly-added target element
> > we won't set the flag.
> 
> The flag is certainly updated in case of changing the target.  But a test
> for changing target made me realize that current approach relies on
> SchedulePaint call in PendingAnimationTracker[1].  That's because we don't
> return any styles from ComposeStyle() in the delay phase,  the paint process
> in the delay phase is kicked by the SchedulePaint calls.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 8d0aadfe7da782d415363880008b4ca027686137/dom/animation/
> PendingAnimationTracker.cpp#121
> 
> I will post an additional patch here or open a new bug.  It will depend on
> the patch size.

I meant we need to call SchedulePaint() in case of changing the target in the delay phase.

Comment 118

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, high review load) from comment #113)
> ::: dom/animation/KeyframeEffect.cpp:801
> (Diff revision 6)
> > +      // since the primary frame is not associated with the target element
> > +      // at this point (In case of such CSS animations, this function is
> > +      // called during frame construction.).
> > +      // For such CSS animations we need to set NS_FRAME_MAY_BE_TRANSFORMED
> > +      // in nsFrame::Init(). We will handle the case in bug 1273042.
> > +      for (auto property : mProperties) {
> 
> Does this work when we change the target?
> 
> We are iterating over mProperties but mProperties may not be set until after
> this is called? For example, SetTarget calls UpdateTargetRegistration *then*
> MaybeUpdateProperties, so if the animation has a newly-added target element
> we won't set the flag.
> 
> Should be call this at a different point?

It turned out changing keyframes does not pass AddEffect like this;

 div.animate({ width: ['100px', '200px'] }, { duration: 1000, delay: 1000 });
 anim.ready.then(function() {
   anim.keyframes({ transform: [ 'translateX(100px)', 'translateX(200px)' ]);
 });

Setting the flag should be moved in UpdateProperties().

Comment 119

[Hiroyuki Ikezoe (:hiro)]

I've decided that the part of calling SchedulePaint() integrates into part 5 patch because splitting this part became hard to be reviewed.  The part of reftests for changing keyframe/target is splitted as part 9 patch.  Brian, I will ask to review next week.  Thank you for your patient review!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e14d2e3e5cfb

Comment 120

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1223658 - Part 9: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review commit: https://reviewboard.mozilla.org/r/57340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57340/

Comment 121

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41605/diff/6-7/

Comment 122

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739818 [details]
Bug 1223658 - Part 3: Consider fillMode in compositor thread as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45375/diff/5-6/

Comment 123

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41599/diff/6-7/

Comment 124

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41607/diff/6-7/

Comment 125

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45377/diff/5-6/

Comment 126

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739820 [details]
Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45385/diff/5-6/

Comment 127

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733198 [details]
Bug 1223658 - Part 8: Reftests for checking stacking context of transform animations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41601/diff/6-7/

Comment 128

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

https://reviewboard.mozilla.org/r/41607/#review54220

MozReview has changed today so I hope this works. There are a few things I wanted to ask while I continue to review the rest of the patch -- hopefully this doesn't clear the review request.

::: dom/animation/Animation.h:282
(Diff revision 7)
> -   * "Playing" is different to "running". An animation in its delay phase is
> -   * still running but we only consider it playing when it is in its active
> -   * interval. This definition is used for fetching the animations that are
> -   * candidates for running on the compositor (since we don't ship animations
> -   * to the compositor when they are in their delay phase or paused including
> -   * being effectively paused due to having a zero playback rate).
> +   * Returns true if this animation's playback state makes it a candidate for
> +   * running on the compositor.
> +   * We send animations to the compositor when their target effect is 'current'
> +   * (a definition that is roughly equivalent to when they are in their before
> +   * or active phase). However, we don't send animations to the compositor when
> +   * they are in paused/pausing (including being effectively paused dues to

Nit:
s/they are in/they are/
s/paused dues/paused due/

::: dom/animation/KeyframeEffect.cpp:522
(Diff revision 7)
>                                                           mTarget->mElement,
>                                                           mTarget->mPseudoType,
>                                                           mKeyframes);
>    }
>  
> +  // If we are here when the effect is not active and the effect has properties

I think we mean "not in effect" as opposed to "not active"? Is that right?

::: dom/animation/KeyframeEffect.cpp:526
(Diff revision 7)
> +  // Note that this function should be called before equivalent checking against
> +  // the previous properties below, because changing target does not cause any
> +  // property changes.

Nit: "Note that we need to perform this check before checking if the properties have changed, below, since when the target is updated the properties won't change but we may need to update the bits on the new target frame."

However, having said that, why don't we just also run this function when we update the target and then only do it here when the properties have changed? It seems like MaybeUpdateFrameForCompositor performs some operations that we don't necessarily need to do every time the style context changes (e.g. calling SchedulePaint).

At any rate, it seems odd to do this check in between calculating the new set of properties and comparing them with the old set.

::: dom/animation/KeyframeEffect.cpp:1376
(Diff revision 7)
> +  // We can't check AnimationPhase::Before here since this function is called
> +  // when the current time is not resolved.
> +  MOZ_ASSERT(!IsInEffect(),
> +             "We don't need to call this function in effect state");
> +

I don't understand this comment. Does this mean we might send animations to the compositor that are in their after phase? Or idle animations?

I presume not because that is checked elsewhere, and if so, we should probably mention that in this comment.

::: dom/animation/KeyframeEffect.cpp:1411
(Diff revision 7)
> +
> +  // We need to call SchedulePaint directly because we don't return any
> +  // effective styles from ComposeStyle() in the delay phase, therefore
> +  // any paint process for the frame will not be done without this call.
> +  frame->SchedulePaint();

Do we need to do this even when we don't have a transform? Perhaps this is needed for the opacity case?

If not, can we just check the state bits on the frame at the start of the function and avoid doing any work if the appropriate bits are set.

I'm concerned about calling SchedulePaint any more than absolutely necessary.

Comment 129

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

https://reviewboard.mozilla.org/r/41607/#review54224

Looks good but I'd like to find out about if there are any cases where we can avoid calling SchedulePaint, and why we are running a paused animation on the compositor, before finishing the review.

::: dom/animation/test/chrome/test_running_on_compositor.html:505
(Diff revision 7)
> +
> +var delayPhaseWithTransformStyleTests = [
> +  {
> +    desc: 'script animation of transform with transform style',
> +    setupAnimation: function(t) {
> +      return addDiv(t, { style: 'transform: translateX(10px)' }).animate(

Nit: Would -100px be more useful for the underlying style (to make it obvious. if it fails, that it's the underlying style and not an interpolated result from the animation).

::: dom/animation/test/chrome/test_running_on_compositor.html:549
(Diff revision 7)
> +          .getAnimations()[0];
> +    },
> +  },
> +];
> +
> +delayPhaseTests.forEach(function(test) {

Nit: It seems like it would make more sense to move this to directly after where delayPhaseTests is defined? Likewise for the other test sets.

::: dom/animation/test/chrome/test_running_on_compositor.html:562
(Diff revision 7)
> +// The purpose of thie test cases is to check that
> +// NS_FRAME_MAY_BE_TRANSFORMED flag on the associated nsIFrame persists
> +// after transform style on the frame is removed.

Perhaps this comment should go before the definition of delayPhaseWithTransformStyleTests ?

::: dom/animation/test/chrome/test_running_on_compositor.html:616
(Diff revision 7)
> +      'Animations reports that it is running on the compositor after '
> +      + 'changing the target element to another elemenent having no '
> +      + 'opacity style');

Nice!

Do we have an equivalent test for if we clear the opacity style on this element, that it is sent to the compositor?

::: dom/animation/test/chrome/test_running_on_compositor.html:621
(Diff revision 7)
> +      'Animations reports that it is running on the compositor after '
> +      + 'changing the target element to another elemenent having no '
> +      + 'opacity style');
> +  });
> +}, 'Changing target element whose property can be run on the compositor ' +
> +   'sends the animation to the compositor even the animation is in the ' +

Nit: s/even the/even if the/

::: layout/style/test/test_animations_omta.html:1382
(Diff revision 7)
> -  omta_is_approx("opacity", gTF.ease_out(0.1), 0.01, RunningOn.Either,
> -                 "delay and play-state delay test at 1300ms");
> +  omta_is_approx("opacity", gTF.ease_out(0.1), 0.01, RunningOn.Compositor,
> +                 "delay and play-state delay test at 1300ms"); // paused.

Why is the animation running on the compositor if it is paused?

Comment 130

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

Comment on attachment 8759387 [details]
Bug 1223658 - Part 9: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

https://reviewboard.mozilla.org/r/57340/#review54226

r=me with comments addressed

Nit: s/chaging/changing/ in commit message

::: layout/reftests/web-animations/stacking-context-opacity-changing-keyframe.html:25
(Diff revision 1)
> +  var anim = document.getElementById("test")
> +    .animate({ width: ['100px', '100px'] },
> +             { delay: 100000, duration: 100000 });
> +  anim.ready.then(function() {
> +    anim.effect.setKeyframes({ opacity: [0, 1] });
> +    anim.ready.then(function() {

setKeyframes doesn't actually generate a new ready promise (perhaps it should?). So, as far as I can tell, this is equivalent to simply scheduling a new microtask, which shouldn't actually give painting a chance to run (although it might in our implementation). Perhaps it's better to just use rAF?

::: layout/reftests/web-animations/stacking-context-opacity-changing-target.html:3
(Diff revision 1)
> +<!DOCTYPE html>
> +<html class="reftest-wait">
> +<title>Changing target to transparent allowed element creates a stacking context even if the animation is delay phase</title>

Nit: "Changing target to an element that does not override opacity animations creates ..."

Also, can we wrap to 80 chars? (Likewise for other titles in this patch)

::: layout/reftests/web-animations/stacking-context-opacity-changing-target.html:31
(Diff revision 1)
> +  var anim = document.getElementById("first")
> +    .animate({ opacity: [0, 1] },
> +             { delay: 100000, duration: 100000 });
> +  anim.ready.then(function() {
> +    anim.effect.target = document.getElementById("second");
> +    anim.ready.then(function() {

Likewise here, setting the target doesn't generate a new ready promise.

::: layout/reftests/web-animations/stacking-context-transform-changing-keyframe.html:26
(Diff revision 1)
> +    .animate({ width: ['100px', '100px'] },
> +             { delay: 100000, duration: 100000 });
> +  anim.ready.then(function() {
> +    anim.effect.setKeyframes(
> +      { transform: ['translate(0px)', 'translate(0px)'] });
> +    anim.ready.then(function() {

As above, ready will already be resolved.

::: layout/reftests/web-animations/stacking-context-transform-changing-target.html:3
(Diff revision 1)
> +<!DOCTYPE html>
> +<html class="reftest-wait">
> +<title>Changing target to transform allowed element creates a stacking context even if the animation is delay phase</title>

As before, "Changing target to an element that does not override transform animations creates ..."

::: layout/reftests/web-animations/stacking-context-transform-changing-target.html:29
(Diff revision 1)
> +<div id="second"></div>
> +<script>
> +  var anim = document.getElementById("first")
> +    .animate({ transform: ['translateX(0px)', 'translateX(100px)'] },
> +             { delay: 100000, duration: 100000 });
> +  anim.ready.then(function() {

And here.

Comment 131

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #128)

> ::: dom/animation/KeyframeEffect.cpp:522
> (Diff revision 7)
> >                                                           mTarget->mElement,
> >                                                           mTarget->mPseudoType,
> >                                                           mKeyframes);
> >    }
> >  
> > +  // If we are here when the effect is not active and the effect has properties
> 
> I think we mean "not in effect" as opposed to "not active"? Is that right?

Ah, nice catch.  I am little bit concerned that, as I comment below, we should send the animation in after phase.  This comment shows my anxious..  We should use "not in effect" here?  Sorry for the confusion.

> ::: dom/animation/KeyframeEffect.cpp:526
> (Diff revision 7)
> > +  // Note that this function should be called before equivalent checking against
> > +  // the previous properties below, because changing target does not cause any
> > +  // property changes.
> 
> Nit: "Note that we need to perform this check before checking if the
> properties have changed, below, since when the target is updated the
> properties won't change but we may need to update the bits on the new target
> frame."
> 
> However, having said that, why don't we just also run this function when we
> update the target and then only do it here when the properties have changed?
> It seems like MaybeUpdateFrameForCompositor performs some operations that we
> don't necessarily need to do every time the style context changes (e.g.
> calling SchedulePaint).

Yes, I did initially call the function in SetTarget() as well.  I will change it.

> At any rate, it seems odd to do this check in between calculating the new
> set of properties and comparing them with the old set.
> 
> ::: dom/animation/KeyframeEffect.cpp:1376
> (Diff revision 7)
> > +  // We can't check AnimationPhase::Before here since this function is called
> > +  // when the current time is not resolved.
> > +  MOZ_ASSERT(!IsInEffect(),
> > +             "We don't need to call this function in effect state");
> > +
> 
> I don't understand this comment. Does this mean we might send animations to
> the compositor that are in their after phase? Or idle animations?

Yes unfortunately, I think so.  For example;

 var anim = div.animate({color: ['red', 'blue'], {duration: 100, endDelay: 100});
 ...
 anim.effect.setKeyframes({ opacity: [1, 0]});  // while in end delay phase;

I think we should send this animation to the compositor as well.  (Actually I am not a big fan of end delay though).  I think fill:forwards case is also the same even if its style is transform:none.  Honestly it's little bit weird to me.  This is my understanding. *But* current patch does not send such animations (at least I am not intend to do so because it's out of scope of this bug). 

> ::: dom/animation/KeyframeEffect.cpp:1411
> (Diff revision 7)
> > +
> > +  // We need to call SchedulePaint directly because we don't return any
> > +  // effective styles from ComposeStyle() in the delay phase, therefore
> > +  // any paint process for the frame will not be done without this call.
> > +  frame->SchedulePaint();
> 
> Do we need to do this even when we don't have a transform? Perhaps this is
> needed for the opacity case?

Yes, we do call it for opacity too.  

> If not, can we just check the state bits on the frame at the start of the
> function and avoid doing any work if the appropriate bits are set.
> 
> I'm concerned about calling SchedulePaint any more than absolutely necessary.

Actually it might be. In case of animations can not send to the compositor because of our restrictions, e.g, too small content or too large one.  I have no clear idea to avoid it for now because the content size check is processed in building display list.

Comment 132

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #129)

> ::: layout/style/test/test_animations_omta.html:1382
> (Diff revision 7)
> > -  omta_is_approx("opacity", gTF.ease_out(0.1), 0.01, RunningOn.Either,
> > -                 "delay and play-state delay test at 1300ms");
> > +  omta_is_approx("opacity", gTF.ease_out(0.1), 0.01, RunningOn.Compositor,
> > +                 "delay and play-state delay test at 1300ms"); // paused.
> 
> Why is the animation running on the compositor if it is paused?

Ooo?  I thought this test actually passed.  I will check it.

Comment 133

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #132)
> (In reply to Brian Birtles (:birtles) from comment #129)
> 
> > ::: layout/style/test/test_animations_omta.html:1382
> > (Diff revision 7)
> > > -  omta_is_approx("opacity", gTF.ease_out(0.1), 0.01, RunningOn.Either,
> > > -                 "delay and play-state delay test at 1300ms");
> > > +  omta_is_approx("opacity", gTF.ease_out(0.1), 0.01, RunningOn.Compositor,
> > > +                 "delay and play-state delay test at 1300ms"); // paused.
> > 
> > Why is the animation running on the compositor if it is paused?
> 
> Ooo?  I thought this test actually passed.  I will check it.

Ah, I understood now.  We don't pull the animation back to the main-thread when calling pause() originally.  I will leave it as 'RunningOn.Either'

Comment 134

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #131)
> (In reply to Brian Birtles (:birtles) from comment #128)
> > ::: dom/animation/KeyframeEffect.cpp:526
> > (Diff revision 7)
> > > +  // Note that this function should be called before equivalent checking against
> > > +  // the previous properties below, because changing target does not cause any
> > > +  // property changes.
> > 
> > Nit: "Note that we need to perform this check before checking if the
> > properties have changed, below, since when the target is updated the
> > properties won't change but we may need to update the bits on the new target
> > frame."
> > 
> > However, having said that, why don't we just also run this function when we
> > update the target and then only do it here when the properties have changed?
> > It seems like MaybeUpdateFrameForCompositor performs some operations that we
> > don't necessarily need to do every time the style context changes (e.g.
> > calling SchedulePaint).
> 
> Yes, I did initially call the function in SetTarget() as well.  I will
> change it.

Ok, I will cancel the review request for now since it seems like there will be an updated patch to come.

Also, over the weekend I was thinking about this, and instead of SchedulePaint(), I was wondering why we can't use RequestRestyle(Layer)? That should work, right?

Comment 135

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

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

https://reviewboard.mozilla.org/r/41607/#review54638

Comment 136

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #134)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #131)
> > (In reply to Brian Birtles (:birtles) from comment #128)
> > > ::: dom/animation/KeyframeEffect.cpp:526
> > > (Diff revision 7)
> > > > +  // Note that this function should be called before equivalent checking against
> > > > +  // the previous properties below, because changing target does not cause any
> > > > +  // property changes.
> > > 
> > > Nit: "Note that we need to perform this check before checking if the
> > > properties have changed, below, since when the target is updated the
> > > properties won't change but we may need to update the bits on the new target
> > > frame."
> > > 
> > > However, having said that, why don't we just also run this function when we
> > > update the target and then only do it here when the properties have changed?
> > > It seems like MaybeUpdateFrameForCompositor performs some operations that we
> > > don't necessarily need to do every time the style context changes (e.g.
> > > calling SchedulePaint).
> > 
> > Yes, I did initially call the function in SetTarget() as well.  I will
> > change it.
> 
> Ok, I will cancel the review request for now since it seems like there will
> be an updated patch to come.
> 
> Also, over the weekend I was thinking about this, and instead of
> SchedulePaint(), I was wondering why we can't use RequestRestyle(Layer)?
> That should work, right?

Unfortunately, we can't use RequestRestyle(Layer).  Initially I was wondering exactly the same thing.  The RequestRestyle(Layer) is actually called, but animations are not painted.  That was because we don't return any valid styles from ComposeStyle() in the delay phase.  Returning an identity transform is an approach what I did the past patch in previous comments will not need SchedulePaint() at all.  To paint (precisely to run paint process) without returning valid styles from ComposeStyle, we need to call SchedulePaint().

Comment 137

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #136)
> (In reply to Brian Birtles (:birtles) from comment #134)
> > Also, over the weekend I was thinking about this, and instead of
> > SchedulePaint(), I was wondering why we can't use RequestRestyle(Layer)?
> > That should work, right?
> 
> Unfortunately, we can't use RequestRestyle(Layer).  Initially I was
> wondering exactly the same thing.  The RequestRestyle(Layer) is actually
> called, but animations are not painted.  That was because we don't return
> any valid styles from ComposeStyle() in the delay phase.  Returning an
> identity transform is an approach what I did the past patch in previous
> comments will not need SchedulePaint() at all.  To paint (precisely to run
> paint process) without returning valid styles from ComposeStyle, we need to
> call SchedulePaint().

I'd be interested in knowing more specifics. What check is failing?

I suppose ElementRestyler::AddLayerChangesForAnimation[1] doesn't help us because we don't have the appropriate layers?

https://dxr.mozilla.org/mozilla-central/rev/3e8ee3599a67edd971770af4982ad4b0fe77f073/layout/base/RestyleManager.cpp#2726

Comment 138

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #137)
> > To paint (precisely to run
> > paint process) without returning valid styles from ComposeStyle, we need to
> > call SchedulePaint().
> 
> I'd be interested in knowing more specifics. What check is failing?

I will check it.

> I suppose ElementRestyler::AddLayerChangesForAnimation[1] doesn't help us
> because we don't have the appropriate layers?

Hmm,  I don't think it's related to layering process since the problem what we are talking here is before layer is created.

Comment 139

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #138)
> (In reply to Brian Birtles (:birtles) from comment #137)
> > I suppose ElementRestyler::AddLayerChangesForAnimation[1] doesn't help us
> > because we don't have the appropriate layers?
> 
> Hmm,  I don't think it's related to layering process since the problem what
> we are talking here is before layer is created.

Yes, that's what I mean. We don't have layers yet so comparing the animation generation won't help us.

Comment 140

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #139)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #138)
> > (In reply to Brian Birtles (:birtles) from comment #137)
> > > I suppose ElementRestyler::AddLayerChangesForAnimation[1] doesn't help us
> > > because we don't have the appropriate layers?
> > 
> > Hmm,  I don't think it's related to layering process since the problem what
> > we are talking here is before layer is created.
> 
> Yes, that's what I mean. We don't have layers yet so comparing the animation
> generation won't help us.

Ah, right. This might be one reason.  But I guess a main reason is no returning style does not produce any change hint or before that.

Comment 141

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41605/diff/7-8/

Comment 142

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739818 [details]
Bug 1223658 - Part 3: Consider fillMode in compositor thread as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45375/diff/6-7/

Comment 143

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41599/diff/7-8/

Comment 144

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41607/diff/7-8/

Comment 145

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45377/diff/6-7/

Comment 146

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739820 [details]
Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45385/diff/6-7/

Comment 147

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733198 [details]
Bug 1223658 - Part 8: Reftests for checking stacking context of transform animations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41601/diff/7-8/

Comment 148

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8759387 [details]
Bug 1223658 - Part 9: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57340/diff/1-2/

Comment 149

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

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

https://reviewboard.mozilla.org/r/41607/#review57560

I only skimmed most of this patch (which looks good!) because the "wins in cascade" handling seems like a significant problem so it's probably not worth reviewing the rest of the patch until we solve that (unless I've misunderstood and it's not a problem!).

::: dom/animation/EffectCompositor.cpp:670
(Diff revision 8)
> -    bool inEffect = effect->IsInEffect();
> +    bool isCurrentOrInEffect = effect->IsCurrent() || effect->IsInEffect();
>      for (AnimationProperty& prop : effect->Properties()) {
>  
>        bool winsInCascade = !animatedProperties.HasProperty(prop.mProperty) &&
> -                           inEffect;
> +                           isCurrentOrInEffect;

This doesn't seem right. If an animation is in its delay phase with no backwards fill, it shouldn't win in the cascade. Or am I missing something?

This might be hard to solve but we need to do it for additive animations anyway.

(For additive animations, we'll need to completely rework the "wins in cascade" boolean flag since we'll need to send animations to the compositor that do *not* win in the cascade once we are able to add them on the compositor.)

Comment 150

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #149)
> ::: dom/animation/EffectCompositor.cpp:670
> (Diff revision 8)
> > -    bool inEffect = effect->IsInEffect();
> > +    bool isCurrentOrInEffect = effect->IsCurrent() || effect->IsInEffect();
> >      for (AnimationProperty& prop : effect->Properties()) {
> >  
> >        bool winsInCascade = !animatedProperties.HasProperty(prop.mProperty) &&
> > -                           inEffect;
> > +                           isCurrentOrInEffect;
> 
> This doesn't seem right. If an animation is in its delay phase with no
> backwards fill, it shouldn't win in the cascade. Or am I missing something?

I am sorry I did not read this comment carefully. And now I am surprised.  I thought the animation in delay phase wins.

Let me clarify. For example;

@keyframes move {
  from { transform: translateX(0px); }
  to { transform: translateX(100px); }
}

div.style.animation = 'move 1s';
div.animate({ transform: [ 'translateX(0px)', 'translateX(100px)' ]}, { duration: 1000, delay: 500 });

In the above case, Does the web animation lose during delay phase?  If so, we need more work on compositor side to check which animations win in cascade.

> (For additive animations, we'll need to completely rework the "wins in
> cascade" boolean flag since we'll need to send animations to the compositor
> that do *not* win in the cascade once we are able to add them on the
> compositor.)

For the above case, we should implement additive animations first.

Comment 151

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #150)
> (In reply to Brian Birtles (:birtles) from comment #149)
> > This doesn't seem right. If an animation is in its delay phase with no
> > backwards fill, it shouldn't win in the cascade. Or am I missing something?
> 
> I am sorry I did not read this comment carefully. And now I am surprised.  I
> thought the animation in delay phase wins.
> 
> Let me clarify. For example;
> 
> @keyframes move {
>   from { transform: translateX(0px); }
>   to { transform: translateX(100px); }
> }
> 
> div.style.animation = 'move 1s';
> div.animate({ transform: [ 'translateX(0px)', 'translateX(100px)' ]}, {
> duration: 1000, delay: 500 });
> 
> In the above case, Does the web animation lose during delay phase?  If so,
> we need more work on compositor side to check which animations win in
> cascade.

Yes. You don't even need to combine CSS animations and Web animations -- you could simply have two CSS animations or two Web animations. If the higher-priority animation is in the delay phase with no backwards fill, the lower-priority one should still take effect.

That's why we need to send animations to the compositor even if they don't win in the cascade at the current time. On the compositor we'll need to determine which animation to apply.

There are probably a number of different approaches we could take to this but I think it will take some work--however it's work we need to do for additive animations anyway.

Comment 152

[Hiroyuki Ikezoe (:hiro)]

Note that this bug is still valid but the test case in comment 1 does not cause a flicker on each image changes.

Comment 153

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #152)
> Note that this bug is still valid but the test case in comment 1 does not
> cause a flicker on each image changes.

Yeah, I see flicker in both cases now towards the end of the animation.

Comment 154

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41605/diff/8-9/

Comment 155

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739818 [details]
Bug 1223658 - Part 3: Consider fillMode in compositor thread as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45375/diff/7-8/

Comment 156

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41599/diff/8-9/

Comment 157

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41607/diff/8-9/

Comment 158

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45377/diff/7-8/

Comment 159

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739820 [details]
Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45385/diff/7-8/

Comment 160

[Hiroyuki Ikezoe (:hiro)]

Markus, I have a question similar to bug 1292441 comment 2.

After this bug, transform and opacity animations will be sent from their delay phase.  background-position animation will be also affected by this change, that means an active layer for background-position animation is created from its delay phase, because ActiveLayerTracker::IsBackgroundPositionAnimated uses also ActiveLayerTracker::IsStyleAnimated().  If this impact is not preferable, I will post a following patch to avoid this.  What do you think?

Comment 161

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

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

https://reviewboard.mozilla.org/r/41607/#review82730

Yay! r=me with the following comments/questions addressed.

::: dom/animation/KeyframeEffectReadOnly.cpp:1137
(Diff revision 9)
>    // We currently only expect this method to be called when this effect
>    // is attached to a playing Animation. If that ever changes we'll need
>    // to update this to only return true when that is the case since paused,
>    // filling, cancelled Animations etc. shouldn't stop other Animations from
>    // running on the compositor.

This comment should probably be updated to something like:

  // We currently only expect this to be called for effects whose animations are
  // eligible for the compositor since. Animations that are paused,
  // zero-duration, finished etc. should not block other animations from running
  // on the compositor.

::: dom/animation/test/chrome/test_running_on_compositor.html:13
(Diff revision 9)
> +@keyframes transform-starts-with-none {
> +    0% { transform: none }
> +   99% { transform: none }
> +  100% { transform: translate(100px) }
> +}
> +@keyframes opacity {
> +  to { opacity: 0 }
> +}
>  @keyframes background_and_translate {

(Minor nit: We have transform-starts-with-none and background_and_translate. We should probably be consistent. Personally I prefer '-' to separate words in CSS but it's up to you.)

::: dom/animation/test/chrome/test_running_on_compositor.html:728
(Diff revision 9)
> +      return addDiv(t, { style: 'animation: opacity 100s 100s' })
> +        .getAnimations()[0];

Nice!

::: dom/animation/test/chrome/test_running_on_compositor.html:812
(Diff revision 9)
> +delayPhaseTests.forEach(function(test) {
> +  promise_test(function(t) {
> +    var animation = test.setupAnimation(t);
> +
> +    return animation.ready.then(function() {
> +      assert_animation_is_running_on_compositor(animation,
> +         test.desc + ' reports that it is running on the '
> +         + 'compositor even though it is in the delay phase');
> +    });
> +  }, 'isRunningOnCompositor for ' + test.desc + ' is true even though ' +
> +     'it is in the delay phase');
> +});

Can we move this to directly after we define delayPhaseTests?

(Better still just do [ ... ].forEach(...) -- i.e. skip using a local variable altogether -- and add a comment at the top describing the category of things you're testing?)

::: dom/animation/test/chrome/test_running_on_compositor.html:825
(Diff revision 9)
> +    });
> +  }, 'isRunningOnCompositor for ' + test.desc + ' is true even though ' +
> +     'it is in the delay phase');
> +});
> +
> +delayPhaseWithTransformStyleTests.forEach(function(test) {

(Likewise, move this up to directly after we initialize delayPhaseWithTransformStyleTests... or better still just have an array literal with .forEach attached directly to the end of it.)

::: dom/animation/test/chrome/test_running_on_compositor.html:859
(Diff revision 9)
> +promise_test(function(t) {
> +  var div = addDiv(t, { style: 'opacity: 1 ! important' });
> +
> +  var animation = div.animate(
> +    { opacity: [0, 1] },
> +    { delay: 100 * MS_PER_SEC, duration: 100 * MS_PER_SEC });
> +
> +  return animation.ready.then(function() {
> +    assert_animation_is_not_running_on_compositor(animation,
> +      'Opacity animation on an element which has opacity:1 important style'
> +      + 'reports that it is not running on the compositor');
> +    // Clear the opacity style on the target element.
> +    div.style.setProperty("opacity", "1", "");
> +    return waitForFrame();
> +  }).then(function() {
> +    assert_animation_is_running_on_compositor(animation,
> +      'Opacity animations reports that it is running on the compositor after '
> +      + 'clearing the opacity style on the element');
> +  });
> +}, 'Clearing *important* opacity style on the target element sends the ' +
> +   'animation to the compositor even if the animation is in the delay phase');

I really like this kind of test. Very simple, very clear and with minimal external dependencies.

::: dom/animation/test/chrome/test_running_on_compositor.html:927
(Diff revision 9)
> +}, 'Changing target element whose property can be run the compositor ' +
> +   'sends the animation to the compositor even if the animation is in the ' +
> +   'delay phase');

This test has the same description as the last one. I believe the difference is one is for opacity and one is for transform---perhaps we should mention that in the test description so it's easy to tell from the logs which one failed.

(Also, in general, if the code being tested is the same for both transform and opacity then we should only add one test. In future we will likely have many other properties that can be run on the compositor and I don't expect we'll add a new test for each property. We should try to have the minimum number of tests that covers everything we need.)

::: layout/base/ActiveLayerTracker.cpp:449
(Diff revision 9)
>      }
>    }
>    if (aProperty == eCSSProperty_transform && aFrame->Combines3DTransformWithAncestors()) {
>      return IsStyleAnimated(aBuilder, aFrame->GetParent(), aProperty);
>    }
> -  return nsLayoutUtils::HasActiveAnimationOfProperty(aFrame, aProperty);
> +  return nsLayoutUtils::HasEffectiveAnimation(aFrame, aProperty);

What was the difference between these two? I'm lost in all these patches at the moment!

Is it possible to delete HasActiveAnimationOfProperty?

::: layout/reftests/css-animations/reftest.list:40
(Diff revision 9)
>  == stacking-context-transform-none-with-fill-forwards.html stacking-context-animation-ref.html
>  == stacking-context-opacity-1-in-delay.html stacking-context-animation-ref.html
>  == stacking-context-opacity-removing-important-in-delay.html stacking-context-animation-ref.html
>  == stacking-context-transform-none-in-delay.html stacking-context-animation-ref.html
>  == stacking-context-transform-removing-important-in-delay.html stacking-context-animation-ref.html
> -== background-position-in-delay.html background-position-ref.html
> +fails == background-position-in-delay.html background-position-ref.html # This test fails the reftest-opaque-layer check since animating background-position currently creates an active layer from its delay phse, and reftest-opaque-layer only handles items assigned to PaintedLayers.

Bug number to fix this?

::: layout/style/test/test_animations_omta.html:1306
(Diff revision 9)
> +  // NOTE: getOMTAStyle() can't detect the animation is running on the
> +  // compositor during delay phase, since on the compositor any opacty style
> +  // during delay phase are not applied at all.

"NOTE: getOMTAStyle() can't detect if the animation is running on the compositor or not during the delay phase since no opacity style is applied during the delay phase."

(Is this only because we don't have a backwards fill?)

::: layout/style/test/test_animations_omta.html:1324
(Diff revision 9)
> +  // NOTE: getOMTAStyle() can't detect the animation is running on the
> +  // compositor during delay phase, since on the compositor any opacty style
> +  // during delay phase are not applied at all.

As with the previous comment.

::: layout/style/test/test_animations_omta.html:1329
(Diff revision 9)
> +  // NOTE: getOMTAStyle() can't detect the animation is running on the
> +  // compositor during delay phase, since on the compositor any opacty style
> +  // during delay phase are not applied at all.
>    omta_is("opacity", 0, RunningOn.Either, "dynamic delay delay test at 0ms");
>    advance_clock(400);
>    omta_is("opacity", 0, RunningOn.Either, 

(Do you mind dropping the trailing space on this line while you're here?)

::: layout/style/test/test_animations_omta.html:1360
(Diff revision 9)
> +  // NOTE: getOMTAStyle() can't detect the animation is running on the
> +  // compositor during delay phase, since on the compositor any opacty style
> +  // during delay phase are not applied at all.

As with the other comments... perhaps it's enough to have the comment once then just say:

NOTE: As noted above, getOMTAStyle() can't detect if the animation is running on the compositor during the delay phase.

::: layout/style/test/test_animations_omta.html:2366
(Diff revision 9)
> +  omta_is("transform", { tx: 100 }, RunningOn.Either,
> +          "transition runs on compositor thread during delay");

The assertion description here seems to contradict the comment above it. Which is correct?

The assertion says we are testing that the transition runs on the compositor but the comment says we can't tell.

Comment 162

[Hiroyuki Ikezoe (:hiro)]

Thank you Brian.  Unfortunately part 2 patch makes layout/reftests/css-transitions/stacking-context-opacity-wins-over-important-style.html intermittent failure because of dropping 'elapsedDuration.ToSeconds() < 0' check[1].

https://treeherder.mozilla.org/logviewer.html#?job_id=28769921&repo=try#L4476

[1] https://hg.mozilla.org/try/rev/3287be7112ef618245ec8417fb774771b90e150d#l3.21

After landing fix for bug 1208411, sometimes time stamp on the compositor becomes smaller than animation's start time.

In the failure test, stacking-context-opacity-wins-over-important-style.html, steps(1, start) transition is created to make the first composed value the final keyframe value of the transition on the first composition.  In this test case if the time stamp is smaller than animation's start time, the first composed value becomes the initial value of the transition because transition's fill mode is 'backwards'[2].

[2] http://hg.mozilla.org/mozilla-central/file/7be6b348c431/layout/style/nsTransitionManager.cpp#l775

I think this behavior basically fine because it will not happen frequently in real world, it will happen only on very light weight html just like the test case, and even if it happened, people hardly notice that starting transition is slightly delayed.  *BUT* in this test case, this is very problematic behavior. We have no way to tell whether composed result on the compositor is surely the value for a time stamp after animation's start time.
Using a negative delay in the test case avoids this intermittent failure, but I am still thinking it's a good way.

Comment 163

[Hiroyuki Ikezoe (:hiro)]

OK, we should skip composing if elapsedDuration is negative AND animation's delay is positive (or zero). In such case we should not compose any value on the compositor.

Comment 164

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #163)
> OK, we should skip composing if elapsedDuration is negative AND animation's
> delay is positive (or zero). In such case we should not compose any value on
> the compositor.

This did not work.  We've met the condition in test_animations_omta.html[1], and unfortunately this case we should compose values..

[1] http://hg.mozilla.org/mozilla-central/file/723c2e894079/layout/style/test/test_animations_omta.html#l265

Comment 165

[Hiroyuki Ikezoe (:hiro)]

I came up with another idea.  The idea is that using steps(1, end) instead of steps(1, start). in case of opacity: 1 -> 0 transition with steps(1, end), the opacity in before phase is 1!

Comment 166

[Markus Stange [:mstange]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #160)
> Markus, I have a question similar to bug 1292441 comment 2.
> 
> After this bug, transform and opacity animations will be sent from their
> delay phase.  background-position animation will be also affected by this
> change, that means an active layer for background-position animation is
> created from its delay phase, because
> ActiveLayerTracker::IsBackgroundPositionAnimated uses also
> ActiveLayerTracker::IsStyleAnimated().  If this impact is not preferable, I
> will post a following patch to avoid this.  What do you think?

I think it doesn't really matter. I don't think many people use actual CSS animations / transitions on background-position. What I care about most is that script changes to background-position still cause layerization, so that for example on https://www.amazon.ca/clouddrive/home we don't have to repaint the image on every scroll.

Comment 167

[Hiroyuki Ikezoe (:hiro)]

(In reply to Markus Stange [:mstange] from comment #166)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #160)
> > Markus, I have a question similar to bug 1292441 comment 2.
> > 
> > After this bug, transform and opacity animations will be sent from their
> > delay phase.  background-position animation will be also affected by this
> > change, that means an active layer for background-position animation is
> > created from its delay phase, because
> > ActiveLayerTracker::IsBackgroundPositionAnimated uses also
> > ActiveLayerTracker::IsStyleAnimated().  If this impact is not preferable, I
> > will post a following patch to avoid this.  What do you think?
> 
> I think it doesn't really matter. I don't think many people use actual CSS
> animations / transitions on background-position. What I care about most is
> that script changes to background-position still cause layerization, so that
> for example on https://www.amazon.ca/clouddrive/home we don't have to
> repaint the image on every scroll.

Thank you Markus, then I'd leave it as part I did in part 5 patch.

Comment 168

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1223658 - Part 8: Drop nsLayoutUtils::HasActiveAnimationOfProperty.

Review commit: https://reviewboard.mozilla.org/r/85778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85778/

Comment 169

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41605/diff/9-10/

Comment 170

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739818 [details]
Bug 1223658 - Part 3: Consider fillMode in compositor thread as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45375/diff/8-9/

Comment 171

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41599/diff/9-10/

Comment 172

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41607/diff/9-10/

Comment 173

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45377/diff/8-9/

Comment 174

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739820 [details]
Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45385/diff/8-9/

Comment 175

[Hiroyuki Ikezoe (:hiro)]

Brian, can you please take a look part 2 and part 8?

Part 2 has a fix for the intermittent test failure I described in comment 162, and take the approach in comment 165.
A try still in progress but the failure test already passed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5db0036eb74c04b25c4303182df5595208be672&exclusion_profile=false&selectedJob=29153627

(Part 2 has also an important change against nsDisplayList.cpp that I did accidentally drop in the middle of rebasing!)

Comment 176

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

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

https://reviewboard.mozilla.org/r/41605/#review84416

::: dom/animation/Animation.cpp:680
(Diff revision 10)
> +  MOZ_ASSERT(!mHoldTime.IsNull(), "Hold time should be set in order to"
> +                                  " convert a ready time to a start time");
> +  if (mPlaybackRate == 0) {
> +    return aReadyTime;
> +  }
> +  return aReadyTime - (mHoldTime.Value().MultDouble(1 / mPlaybackRate));

Nit: Unnecessary braces here

Comment 177

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

Comment on attachment 8800988 [details]
Bug 1223658 - Part 8: Drop nsLayoutUtils::HasActiveAnimationOfProperty.

https://reviewboard.mozilla.org/r/85778/#review84418

Comment 178

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733200 [details]
Bug 1223658 - Part 2: Pass delay property to compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41605/diff/10-11/

Comment 179

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739818 [details]
Bug 1223658 - Part 3: Consider fillMode in compositor thread as well.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45375/diff/9-10/

Comment 180

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733197 [details]
Bug 1223658 - Part 4: Add a function to check active duration is zero.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41599/diff/10-11/

Comment 181

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8733201 [details]
Bug 1223658 - Part 5: Send animations to compositor even though it's in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41607/diff/10-11/

Comment 182

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739819 [details]
Bug 1223658 - Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45377/diff/9-10/

Comment 183

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8739820 [details]
Bug 1223658 - Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45385/diff/9-10/

Comment 184

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8800988 [details]
Bug 1223658 - Part 8: Drop nsLayoutUtils::HasActiveAnimationOfProperty.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85778/diff/1-2/

Comment 185

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b66b75c2d042
Part 2: Pass delay property to compositor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/00799db70975
Part 3: Consider fillMode in compositor thread as well. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a523efc645d0
Part 4: Add a function to check active duration is zero. r=birtles,dholbert
https://hg.mozilla.org/integration/autoland/rev/7ee13a1fd665
Part 5: Send animations to compositor even though it's in delay phase. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ad518d220cd2
Part 6: Remove Animation::HasInPlayEffect and AnimationEffectReadOnly::IsInPlay. r=birtles
https://hg.mozilla.org/integration/autoland/rev/e2bf40358de2
Part 7: Reftests for checking stacking context when changing keyframe and target directly in delay phase. r=birtles
https://hg.mozilla.org/integration/autoland/rev/6a000381265d
Part 8: Drop nsLayoutUtils::HasActiveAnimationOfProperty. r=birtles

Comment 186

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b66b75c2d042
https://hg.mozilla.org/mozilla-central/rev/00799db70975
https://hg.mozilla.org/mozilla-central/rev/a523efc645d0
https://hg.mozilla.org/mozilla-central/rev/7ee13a1fd665
https://hg.mozilla.org/mozilla-central/rev/ad518d220cd2
https://hg.mozilla.org/mozilla-central/rev/e2bf40358de2
https://hg.mozilla.org/mozilla-central/rev/6a000381265d