1223204 - Compositor animation indication not shown when there is a delay

Status: RESOLVED DUPLICATE of bug 1223658

(Core :: DOM: Animation, defect)

Description

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

Attachment: Test case

See attached test case. The same animation is run twice: one with a delay and once without. When there is a delay there is no "compositor animation" (⚡) mark.

The 'isRunningOnCompositor' member of animations is a live property. It becomes true when the animation actually starts playing (i.e. after its delay has finished) and goes false after it finishes.

I think what's happening is that we fail to dispatch an animation mutation record when this member changes so if it is not true when the animation is created (e.g. because it has a delay) then the indication never gets set.

This is ultimately a platform bug since I think we should be sending that notification, but some work on the DevTools side is probably also required since I think DevTools wants to display, "Is this animation currently running on the compositor OR did it run on the compositor in the past". That is, if we dispatch a change notification when the animation finished and is no longer on the compositor, DevTools probably needs to ignore that notification.

If need be, DevTools could work around this for now but re-checking the state of each animation with a delay once it finishes its delay.

Long-term we will likely expand this API as follows:
* Expose which properties are running on the compositor (bug 1205704)
* Expose *why* animations are *not* running on the compositor (e.g. 'layer too large', 'that property can't run on the compositor', 'can't animate transform on compositor if geometric properties are also being animated' etc.)

Comment 1

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

(In reply to Brian Birtles (:birtles, busy 6-17 Nov) from comment #0)
> Long-term we will likely expand this API as follows:
> * Expose which properties are running on the compositor (bug 1205704)

That should be bug 1196114.

Comment 2

[Patrick Brosset <:pbro>]

(In reply to Brian Birtles (:birtles, busy 6-17 Nov) from comment #0)
> This is ultimately a platform bug since I think we should be sending that
> notification, but some work on the DevTools side is probably also required
> since I think DevTools wants to display, "Is this animation currently
> running on the compositor OR did it run on the compositor in the past". That
> is, if we dispatch a change notification when the animation finished and is
> no longer on the compositor, DevTools probably needs to ignore that
> notification.
Correct, some devtools work is required here. Especially because for now, animation timelines UIs do not get refreshed unless you press play or pause (or when new animations are added ore removed).

> Long-term we will likely expand this API as follows:
> * Expose which properties are running on the compositor (bug 1205704)
> * Expose *why* animations are *not* running on the compositor (e.g. 'layer
> too large', 'that property can't run on the compositor', 'can't animate
> transform on compositor if geometric properties are also being animated'
> etc.)
This would be really really awesome!

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Use Animation::AnimationTimeToTimeStamp instead of timeline->ToTimeStamp

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

> We need here to divide the mDelay by playback rate just like
> Animation::AnimationTimeToTimeStamp does[3]. Or use AnimationTimeToTimeStamp
> here too.

This patch took the latter to avoid similar mistake elsewhere in future.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8685840 [details] [diff] [review]
Use Animation::AnimationTimeToTimeStamp instead of timeline->ToTimeStamp

I did upload this patch to wrong bug....

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Notify isRunningOnCompositor change to MutationObserver

This is the part of fix for platform.
This patch does not notify after the animation is finished because the notification is sent if the animation is current state.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 2: Update isRunningOnCompositor state when it changes

This is an incomplete patch.
The indicator for compositor is surely shown up after the animation with delay is started. But in the mean time the vertical red bar which indicates the current time (or something else?) has disappeared.

Patrick, do you have any idea why the red bar disappears?

Comment 7

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

Do we need to fix this? If we fix bug 1223658 then it should fix this problem.

Also, if we fix this bug directly, we'll make more work for DevTools because they'll have to ignore the update when we set isRunningOnCompositor to false.

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Created attachment 8686036 [details] [diff] [review]
> Part 2: Update isRunningOnCompositor state when it changes
> 
> This is an incomplete patch.
> The indicator for compositor is surely shown up after the animation with
> delay is started. But in the mean time the vertical red bar which indicates
> the current time (or something else?) has disappeared.
> 
> Patrick, do you have any idea why the red bar disappears?
Not immediately no, but for now, the UI handling for mutation events is quite simple and doesn't expect animations' states to change while they are playing. This will require a bit more work than this unfortunately.
First of all, instead of refreshing the whole timeline whenever something changes in onAnimationStateChanged here https://dxr.mozilla.org/mozilla-central/source/devtools/client/animationinspector/components.js#841 , we'd need to refresh each animation individually when needed only. I have never tested this so far, so things may break.
I think this change is quite involved, I can probably mentor you through it if you want, or I can do it if you prefer.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Thanks, Patrick.

After I and Brian discussed about this issue, we'd take the approach described in bug 1223658. 
If there is still something errors of this compositor indication once bug 1223658 is fixed, I will ask you to mentor me!

Comment 10

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

I think bug 1223658 might not land for a couple of months still. I think we should revisit doing this since this makes DevTools much less useful. The patches here probably need updating to reflect the more detailed API we now expose through getPropertyState() (bug 1196114).

Comment 11

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

Doesn't strictly block bug 1254408, but bug 1254408 is going to be less useful and harder to test without this.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Part 1: Notify isRunningOnCompositor change to MutationObserver v2

Here is an updated patch, but I am now inclined to fix bug 1223658 first.
That's because I was overlooking that changing currentTime resets isRunningOnCompositor flag.  As a result, this patch breaks lots of tests in test_animation_observers.html.  I will try to fix bug 1223658 for a couple of days.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Fixed by bug 1223658.