927349 - Delay effective start of CSS animations and transitions till after the rendering of their first frame

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

When a CSS animation or transition starts, we often have to redraw stuff to layerize the animated content. Currently this easily causes us to skip frames at the start of the animation. We should instead do the drawing and then display the whole animation.

Comment 1

[Chris Lord [:cwiiis]]

Cc'ing some people that may be interested in working on this.

This is easily the biggest issue when trying to implement smooth animations on FirefoxOS transitions/animations at the moment, and a major point of differentiation in behaviour between Gecko and WebKit/Blink/Trident.

Comment 2

[Chris Lord [:cwiiis]]

I think this should be a blocking bug for 2.1 - Pretty much every b2g app that has an animation or transition has serious jank at the start of said transition, often causing all but the last couple of frames to be skipped (for example, moving icons on the home-screen, or panels sliding out in the e-mail app).

Fixing this would help most of those cases and also allow us to save some memory by not having to over-use the will-change property to force layerisation.

Comment 3

[Chris Lord [:cwiiis]]

Brian, you expressed some interest/willingness to work on this before, is this something you'll have time for any time soon?

And if not, could you perhaps dump some information here for other people that might want to tackle it?

Comment 4

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

(In reply to Chris Lord [:cwiiis] from comment #3)
> Brian, you expressed some interest/willingness to work on this before, is
> this something you'll have time for any time soon?

I plan to work on this in Q3. Is that soon enough?

Comment 5

[Chris Lord [:cwiiis]]

If we wanted this in b2g 2.1 without it blocking, this would need to land before 1st September (feature-landing cut-off).

If this is accepted as a 2.1 blocker, that adds an extra 6 weeks, but it's uncertain as to whether other people consider this a blocker (to be honest, I don't think we *need* to block on this, but it'd be a huge improvement and is currently a major disparity between Gecko and other browser engines).

We would be able to nominate this for approval beyond feature landing, but again, whether it's accepted isn't a certainty.

How does that sound? 1st September is a fair way into Q3, does this tally with your schedule?

Comment 6

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

(In reply to Chris Lord [:cwiiis] from comment #5)
> How does that sound? 1st September is a fair way into Q3, does this tally
> with your schedule?

I have a full load until end Aug but I'll try to shuffle things and tackle this this month. If I don't make any progress by end July then we can start looking for other people to help.

Comment 7

[Chris Lord [:cwiiis]]

(In reply to Brian Birtles (:birtles) from comment #6)
> I have a full load until end Aug but I'll try to shuffle things and tackle
> this this month. If I don't make any progress by end July then we can start
> looking for other people to help.

Sounds great, thanks a lot :) I can probably help out (with guidance) if you get short on time.

Comment 8

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

I had a look at this but I'm afraid I'm going away in a few hours' time and won't be able to pick it up again for a while. Here is what I think needs to be done:

Part 1: Fix up AnimationPlayer a bit

* I've recently updated the Web Animations spec specifically to address this usage:
  http://dev.w3.org/fxtf/web-animations/
  
  It now has the concept of hitting 'play' on an animation such that initially its start time becomes null and when the animation is 'ready' (e.g. first frame drawn) then the start time gets set to the current time and animation begins from that point. A similar thing happens if you resume after pausing mid-way.
  
  We don't need all the complexity of those algorithms yet (although dzbarsky may add some other parts over in bug 1033114) but I think it would be good to align with the terminology / rough approach there.
  
* Bug 1040543 splits AnimationPlayer into its own object which should make this a lot easier to work with. I hope to land bug 1040543 as soon as possible (I'll be checking bugmail while I'm away just so I can try to get this landed).

* We should extend AnimationPlayer in the following ways:
  - Make startTime and currentTime nullable (this simplifies some things a bit. The current implementation was trying to implement things like the "effective current time" which has now been removed from the spec)
  - Rename mPauseStart to mHoldTime and make it a Nullable<TimeDuration>
  - Optional: Rename mPlayState to a boolean member mPaused (better matches the spec)
  - Optional: Possibly expose PlayState() (started in bug 1037321) -- may help with testing but not really necessary
  - Later: Make mPaused private (currently all the member variables are public but we will probably gradually encapsulate them as we add complexity to these classes) and introduce Play() / Pause() methods to set them (probably following the algorithms in Web Animations to some degree)
  
Part 2: Add a method for getting back the start time of an animation on the compositor

Part 3: Add a method to AnimationPlayer to let it know when the animation has drawn its first frame

* This would then update mStartTime
  - If the animation is running on the compositor (we have mIsRunningOnCompositor to tell us this), we'd call across and get the start time from there (we'd probably have to subtract the delay however since we don't send the delay to the compositor).
    Otherwise we'd just calculate from the current time -- assuming that we get called pretty-much immediately after drawing finishes this should be ok
  It would also set mHoldTime to null
  (These steps more-or-less line up with the description of the task to run when the player's source content is ready in "3.5.6 Playing a player" of the Web Animations spec)
  
Part 4: Pass both the hold time and start time to the compositor

* This would let the compositor know what time to use initially when the start time is null.
* If the start time is null, the compositor would understand that it should sample the animation at the hold time and then update the start time.
(If we ever want to send animations to the compositor whilst still in their delay phase then we'd need to send the delay across too or else they'd start too soon.)
* Currently we do addition on mStartTime in AddAnimationForProperty (nsDisplayList.cpp) but we want to allow mStartTime to be null so we need to handle that.

Part 5: Work out how to determine when the first frame has been drawn

* I don't know the best way to do this. We need something that:
- Fires pretty much immediately when we do the first paint after a main-thread animation
- Fires soonish after we do a paint on the compositor for off-main thread animations
- Still fires when we have an empty animation or an animation that is off-screen etc. and doesn't actually paint anything

Maybe roc and Matt Woodrow can suggest the best place to hook into this.

Part 6: Store that task from part 5 on the AnimationPlayer

* If we pause before we get the callback from part 5 we'll want to cancel that callback. Also, if it's recurring callback we'll want to unregister after the first one. That suggests we might want to store the task / event listener on the animation player.

* The method from part 3 would probably do the cancelling, or, if the member wasn't set, ignore the callback.

* Originally I was thinking that the nsAnimationManager and nsTransitionManager would create this task and set it, but on further thought I think AnimationPlayer should create it since we'll want this same behavior in future for animations created from script that bypass the nsAnimationManager and nsTransitionManager.

Part 7: Adjust nsAnimationManager and nsTransitionManager

* They basically need to make new animations that have a null start time and a zero hold time. Then they need to get the AnimationPlayer to set up the task from part 6 and run it.

* Care needs to be taken however when updating existing animations since we won't want to set up a task in that case unless we're resuming the animation.

* This is probably best done by pushing more of the play/pause/resume logic into the AnimationPlayer as suggested towards the end of Part 1 above.

Comment 9

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

Attachment: experiment part 1 - Update the AnimationPlayer interface so it's a little closer to the spec

Comment 10

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

Attachment: experiment part 2 - Replace mPauseStart on AnimationPlayer with a nullable TimeDuration: mHoldTime

Comment 11

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

Attachment: experiment part 3 - Add a NotifyDrawingReady method to AnimationPlayer

Comment 12

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

Attachment: experiment part 4 - Add paint event listener

Comment 13

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

Attachment: experiment part 5 - Use the paint event listener in nsAnimationManager

Comment 14

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

Attachment: experiment part 6 - Fixes to OMTA

Comment 15

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

The above patches are the result of me blindly hacking away at this for a couple of hours. They're all wrong and break animations badly (OMTA sort of works but keeps resetting, main-thread animation doesn't work at all). I'm attaching them in the hope they illustrate some of what I was trying to explain in comment 8 or at least point to the relevant files.

They should apply on top of the patches from bug 1040543 (not that I expect anyone to actually apply them!).

Comment 16

[Chris Lord [:cwiiis]]

Brian and I discussed this this afternoon, and came up with a preliminary plan. I'll let him fill in the details, but the basics are this:

1. Record a list of animations on the (DOM) document when they're created, with a callback to set the start time.
2. Set the start time of the animations after the EndTransaction in nsDisplayList.cpp's PaintForFrame and empty the list.
3. If a new animation doesn't trigger a paint (and nothing else triggers a paint in that same style computation/reflow(?)), set the start time of the animation as the current time and empty the list.
4. For async animations, only record the animation delay and not the start time (we can use a negative delay for resuming paused animations/setting animations position/etc.).
5. At the compositor, after receiving a transaction with a new async animation, set the start time of the animation as the current frame start time.

This should have the effect that for non-async animations, their start will happen after their first frame has been painted, and for async animations, their start will happen on composition of the first frame. If an animation doesn't cause a paint, it will behave as it currently does.

We expect this to have a dramatic effect on perceived performance on mobile.

Comment 17

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

(In reply to Chris Lord [:cwiiis] from comment #16)
> Brian and I discussed this this afternoon, and came up with a preliminary
> plan. I'll let him fill in the details, but the basics are this:
> ...

That matches my notes pretty well.

For my own reference:

* We decided not to wait for the compositor to set the start time and then read it back but just to use the time from when content finishes painting.

* We discussed iterating over all the layers after painting and filling in their start times before sending the IPC transaction. We were concerned there might be a cyclic dependency there and decided it's probably just easier to leave the start times in the IPC transaction as null and let the compositor fill them in. This will lead to some drift between the main thread start time and the compositor start time but is probably acceptable as a first pass. Also, some concern about possibly performance of iterating over all animations in the transaction.

* We decided that the best place to update start times was probably LayerManager::EndTransaction after the call to PaintRoot. We talked about doing this in the base layer manger rather than in each different implementation.

* We looked into how to navigate from LayerManager::EndTransaction to actual animation objects. We looked at going LayerManager -> Root layer -> (traverse layers) -> Display item -> content node but decided it's probably just easier and more performant to stick a hash of pending animations on the document and in EndTransaction update all of them with the current time and clear the list.

For ClientLayerManager::EndTransaction we might just leave the times as null and let the compositor update it when it receives the transaction?

* During style resolution, need to check if a paint is scheduled. If not, need to resolve start times straight away. This is necessary for animations with an empty keyframes rule or that animate a property that doesn't trigger a paint.

Comment 18

[Dietrich Ayala (:dietrich)]

Blocking request from June, patches not updated since July. Seems high risk at this point for 2.1, so this can ride the trains.

Comment 19

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

I have a prototype of this coming along fairly well. I haven't done an opt build yet to be able to see what effect it has in real-life but by tying up the main thread with script I can see that animations now start from the beginning rather than part-way through. I plan to apply the same handling to animations that are resumed after pausing.

I came across a few different approaches for handling compositor animations that I want to document here for my own reference.

Approach A) Main thread and compositor resolve start time independently

- Main thread passes an animation with a null start time to the compositor
- The compositor sees this as an indication that it should fill the start time in itself
- On the next composite, it uses the composite time as the start time and stores in on the animation from thereon
- The main thread resolves the start time independently at *roughly* the same time introducing some drift but hopefully not enough to matter

Basically, this approach doesn't work. We don't just send animations to the compositor when they're created, but when we do a layer transaction, we send the whole list of current animations and the compositor swaps its list for the one it's given.

As a result, this approach works when we send a new animation, but the second time we do a layer transaction we'll send the same animation, but now with a *filled-in* start time. The compositor will blindly swap out the animation with the start time that it filled in, with the animation with the start time that the main thread filled-in. This could cause the animation jump forwards or backwards.

We could try to ameliorate this in Layer::SetAnimations by trying to match animations up and not overwrite the start time of animations already running on the compositor. However two issues arise: (i) how to match them up reliably -- we don't have any unique identifiers on them, (ii) how to differentiate between a start time we should preserve because it only differs due to drift between the main thread and compositor, and a start time we should overwrite because the main thread updated the start time (e.g. using the animation API)

Approach B) Compositor resolves start times and main thread applies them

- Main thread passes an animation with a possibly null start time to the compositor
- The compositor, during sampling, detects such animations and fills in the start time with the current compose time
- The main thread reads back this time and sets it on all animations with a null start time

The main problem with this is that the compositing happens asynchronously. We don't fill in mLastCompose on CompositorParent until then so the main thread would either have to force that to happen synchronously (we do this when testing in CompositorParent::ShadowLayersUpdated) or wait on some async event; neither are particularly appealing.

We could perhaps just get the compositor to store the time when it received the transaction but then:
i) How do we get it back -- passing it through all the return value machinery seems somewhat clumsy and adding a synchronous getter to PLayerTransaction and doing a separate IPC call seems suboptimal
ii) Why bother? It doesn't seem like it would be a whole lot more accurate than just storing the time on the main thread

Approach C) Resolve the start time on the main thread and send to the compositor

- At the latest possible moment, and after painting, record the current time
- Walk through all the animations in the current transaction and update any with a null start time
- After finishing the transaction, go through all animations in content and update them with the same start time

In principle, this is close to what we currently do. The main difference is that we defer when we resolve the start time until the latest possible moment.


I've implemented (C). In order to do this after painting I've added steps to EndTransactionInternal in CompositorLayerManager and BasicLayerManager which record the time then update the animations in the layer transaction. Then I've added a getter to LayerManager to get back that time for updating the content side of things.

I'll post some patches next week (hopefully) after applying the same handling to animations that are resumed after pausing. Also, there will be some work to get tests passing with this change in behaviour. The Web Animations API has a 'ready' promise for this purpose (i.e. it resolves when the animation actually starts running) which I might need to implement as well in order to get those tests working.

Also, I need to confirm that animations with no effect (e.g. empty keyframes) still start with this approach since they won't trigger a paint. Detecting that might be a bit involved too.

Comment 20

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

Oh, and the other case to handle is pausing. Currently we have a situation where if you pause an animation and the main thread is busy, the compositor can get ahead and when the pause actually kicks in, the animation jumps backwards. I'd like to fix that at the same time unless I can find a good way to split that part off.

Comment 21

[Chris Lord [:cwiiis]]

Good to hear the progress on this - seems compositor animations aren't implemented as one might've assumed... I wonder how tricky it would be to introduce a unique ID for animations? (do we have a unique object content-side whose pointer value could be used as an ID?)

It does sound like ideally the main thread would be able to read values back from the compositor, but this does complicate things significantly... I don't know how the content-side works to know how easy that would be, but we have a similar situation with progressive tile rendering that might be worth looking at, where the compositor continuously updates some metrics in an area of shared memory and content takes a lock and reads them when needed (https://mxr.mozilla.org/mozilla-central/ident?i=LookupCompositorFrameMetrics)

Comment 22

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

(In reply to Chris Lord [:cwiiis] from comment #21)
> It does sound like ideally the main thread would be able to read values back
> from the compositor, but this does complicate things significantly... I
> don't know how the content-side works to know how easy that would be, but we
> have a similar situation with progressive tile rendering that might be worth
> looking at, where the compositor continuously updates some metrics in an
> area of shared memory and content takes a lock and reads them when needed
> (https://mxr.mozilla.org/mozilla-central/
> ident?i=LookupCompositorFrameMetrics)

I think we probably need to measure this to know if it's worthwhile. I think what I'll do is go ahead with my implementation of (C) and see if it makes a noticeable difference on a B2G device. If it does, it might be enough to land this as a first-pass then investigate ways of making it better still.

Suppose we do decide to read back time values from the compositor, we'd need a way to be notified when the compositor completed its first composite after a transaction. Matt, is there already a mechanism for doing this?

Comment 23

[Matt Woodrow (:mattwoodrow)]

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

> 
> I think we probably need to measure this to know if it's worthwhile. I think
> what I'll do is go ahead with my implementation of (C) and see if it makes a
> noticeable difference on a B2G device. If it does, it might be enough to
> land this as a first-pass then investigate ways of making it better still.
> 
> Suppose we do decide to read back time values from the compositor, we'd need
> a way to be notified when the compositor completed its first composite after
> a transaction. Matt, is there already a mechanism for doing this?

We have a 'DidComposite' message sent from the compositor back to the main thread.

This is received as CompositorChild::RecvDidComposite.

Comment 24

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

I got a prototype of this running and did a side-by-side comparison on two Flame devices with memory set to 256Mb. There's not an obvious difference. Performing the same actions on both devices you occasionally notice some that with the prototype patches for this bug the transitions occur whereas without them the animation jumps straight to the end (e.g. the app just appears instead of zooming in). It's not a massive difference though and occasionally the reverse is true. But typically you see a few more transitions.

I really don't know how much difference it would make if we delayed until after the first composite.

Comment 25

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

Attachment: Recording of prototype

Here's a very poor quality video of an unpatched trunk build (LHS) vs a patched build (RHS).

You'll see a few more transitions taking place on the right. Ignore the part about the blue box moving, that's just a test case.

Comment 26

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

Since it no longer seems like we're pushing to get this in before the next uplift I'd like to do this properly.

First there's some groundwork:

* Bug 1070745 Implement play / pause (in progress, hopefully landing next week)
* Bug 1037321 Implement getPlayState (ready to land)
* Bug 1073336 Provide a way for changes to AnimationPlayer / Animation objects to update style
  (This might not be strictly needed, but involves some refactoring that will hopefully make this easier)
* Bug 1073379 Make startTime writeable
  (This also might not be strictly needed but would probably simplify implementation and testing)

After that, (for my own reference since it might take a while to get back to this), some of the steps are:

* Add hash for storing pending players
* Add players to the hash when Play() is called
* Make setting the player start time remove player from hash
  (Assuming we have bug 1073379)
* Add methods for resolving start times to PendingAnimationTracker (or whatever we call it)
  - Add stub method to AnimationPlayer to resolve the start time
* Add methods to Layer for updating the time of animations after painting has finished
* Add methods to nsDisplayList.cpp to call the pending animation trackers with the animation start time
  (all tests should still pass at this point)
* Inside AnimationPlayer::Play set hold time and leave start time null
  (add tests for this)
  (may need another patch that temporarily disables other tests)
* Get tests to pass
 - Make nsDOMWindowUtils::AdvanceTimeAndRefresh time resolve pending players
 - Implement ready promise so that tests that need to wait for the animation to start can wait on it
 - Get empty animations to start -- not sure what will be needed here
* Adjust when events are queued (animationstart should probably not be dispatched until we *actually* start)
 - Get tests to pass
* Add additional tests for this behavior
  (test it doesn't do anything stupid in the pause case)

Follow-up bugs:
* Do the same sort of thing for pausing (the upcoming prototype patch already covers this)
* Try to actually wait until the first composite, then read the start time from shared memory

Comment 27

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

Attachment: WIP patch

Just a rough experiment of this. Depends on bug 1067701 but probably shouldn't (we won't need it once bug 1073336 is fixed I think).

Comment 28

[Chris Lord [:cwiiis]]

(In reply to Brian Birtles (:birtles) from comment #25)
> Created attachment 8495097 [details]
> Recording of prototype
> 
> Here's a very poor quality video of an unpatched trunk build (LHS) vs a
> patched build (RHS).
> 
> You'll see a few more transitions taking place on the right. Ignore the part
> about the blue box moving, that's just a test case.

These are definitely not the animations to look at to test this feature - in Gaia master, enable app-grouping in the developer settings and try expanding and collapsing groups on the homescreen. I've tested myself and it makes a *huge* difference :)

As long as this can land in 2.2, that's fine, but this honestly makes a huge difference!

Do bear in mind that a lot of the animations in Gaia have been crafted and worked on specifically to counteract this bug (where as the app-grouping ones I mention, I've specifically not done that so as not to complicate the code when I knew that this was being worked on).

Comment 29

[Chris Lord [:cwiiis]]

An issue I see with this WIP is that if elements involved in an animation change during the animation, it causes the animation to stutter as I assume content sends updates to the compositor. This may well be a pre-existing bug however that I've not noticed because there are way more frames in my animations now :)

Comment 30

[Chris Lord [:cwiiis]]

Another place where this patch makes a dramatic, obvious difference, is switching between panes in the Settings app. You see the whole animation now, and it's much more pleasant.

I also notice the starting frames of the home->app transition now which I definitely didn't see before, as I think the start of that animation needs some work :)

Comment 31

[Chris Lord [:cwiiis]]

Attachment: app-grouping-animation.webm

Video of grouping animations without patch on left, with patch on right.

Excuse the quality, I only had my Open C to record, and I had to use Linux video editing tools to get it down to a size that Bugzilla would accept. No idea what happened to the aspect ratio.

Comment 32

[Chris Lord [:cwiiis]]

Just to note, the flickering seems to happen when the overflow changes due to the animation - again, I assume this is down to layer transactions coming in mid-animation and causing animations to reset to whatever the value is that comes in via the transaction.

If this is a correct assumption, I would suppose that we need to be able to not do that unless the animation has been paused or reset in some other similar way.

Comment 33

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #32)
> Just to note, the flickering seems to happen when the overflow changes due
> to the animation - again, I assume this is down to layer transactions coming
> in mid-animation and causing animations to reset to whatever the value is
> that comes in via the transaction.
> 
> If this is a correct assumption, I would suppose that we need to be able to
> not do that unless the animation has been paused or reset in some other
> similar way.

(or is this down to the animation start time being different on content vs. gecko and the jitter being the drift between the two?)

Comment 34

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

(In reply to Chris Lord [:cwiiis] from comment #33)
> (In reply to Chris Lord [:cwiiis] from comment #32)
> > Just to note, the flickering seems to happen when the overflow changes due
> > to the animation - again, I assume this is down to layer transactions coming
> > in mid-animation and causing animations to reset to whatever the value is
> > that comes in via the transaction.

In theory, that shouldn't happen. This patch should mean we end up with the exact same start time on both the compositor and main thread so that even if there is a layer transaction it doesn't change.

It might be that I'm doing something wrong to the delay. Just a look at the code suggests it's wrong.

Alternatively, if you're waiting on events at all, they will probably fire before the animation actually starts with the current patch.

Comment 35

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

(In reply to Brian Birtles (:birtles) from comment #34)
> It might be that I'm doing something wrong to the delay. Just a look at the
> code suggests it's wrong.

Unfortunately that's not the issue. For the app grouping animations we send a 0 delay for all layer transactions.

Comment 36

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

Attachment: WIP patch v2

Turns out the issue was repeat transactions. We would resolve the animation start time, forward the transaction, then run the repeat transaction and re-resolve the animation start before updating the content side. Then, on the next transaction, we'd push the slightly later start time (as recorded in content) across causing the animation to jump backwards.

This patch adds a check for mIsRepeatTransaction and hopefully also fixes some bitrot (but it still requires the patch from bug 1067701).

Comment 37

[Chris Lord [:cwiiis]]

(In reply to Brian Birtles (:birtles) from comment #36)
> Created attachment 8496674 [details] [diff] [review]
> WIP patch v2
> 
> Turns out the issue was repeat transactions. We would resolve the animation
> start time, forward the transaction, then run the repeat transaction and
> re-resolve the animation start before updating the content side. Then, on
> the next transaction, we'd push the slightly later start time (as recorded
> in content) across causing the animation to jump backwards.
> 
> This patch adds a check for mIsRepeatTransaction and hopefully also fixes
> some bitrot (but it still requires the patch from bug 1067701).

Excellent work :) I'd love for a version of this to get in sooner rather than later, if it's feasible - this makes a dramatic difference to the home-screen and makes it much easier to design animations and not have to spend hours/days finding the perfect work-around.

Comment 38

[Chris Lord [:cwiiis]]

Something odd I'm seeing in v2 of the patch, sometimes animations seem to get re-triggered from the beginning.

You can duplicate this by long-pressing and holding on an icon on the homescreen (to trigger edit mode) and dragging it back and forth between the same two positions without letting go. So, for example, on a default setup, dragging messages to switch place with contacts, then back again.

It feels like some animations aren't getting cleared on completion/replacement(?)

Comment 39

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

(In reply to Chris Lord [:cwiiis] from comment #38)
> Something odd I'm seeing in v2 of the patch, sometimes animations seem to
> get re-triggered from the beginning.
> 
> You can duplicate this by long-pressing and holding on an icon on the
> homescreen (to trigger edit mode) and dragging it back and forth between the
> same two positions without letting go. So, for example, on a default setup,
> dragging messages to switch place with contacts, then back again.

I can't seem to reproduce this. I pulled gaia just yesterday but Gecko is about a week or two old.

(In reply to Chris Lord [:cwiiis] from comment #37)
> Excellent work :) I'd love for a version of this to get in sooner rather
> than later, if it's feasible - this makes a dramatic difference to the
> home-screen and makes it much easier to design animations and not have to
> spend hours/days finding the perfect work-around.

Alright, I'll do my best to get this moving but it's going to take some time simply because it will cause a lot of tests (that expect animations to begin immediately) to fail. If you have any particular deadlines, let me know.

Comment 40

[Chris Lord [:cwiiis]]

So I can no longer reproduce the weirdness I was seeing, it must've been something I did in the Gaia code I was working on at the time, so sorry about that!

It's sounding like 2.2 is going to ship with Gecko 39, so there's quite a lot of time to get this done... I don't know if that's a solid plan yet or not though(?), given that previously I'd seen feature freeze for 2.2 being end of October or something like that... Sorry to be so flakey, I wish I knew more myself!

Is it only potential test failures (and review) that are holding this up? If so, I guess we should push a try build and see what breaks. Hopefully I can at least help fix some of the test failures.

Comment 41

[Chris Lord [:cwiiis]]

So, confirmed that 2.2 will be using Gecko 39 - 2.2 QA sign-off begins 23/3/2015, so it'd be great to have this before then. Gaia master branches to 2.3 on 6/4/2015, so it'd be good to know we can rely on this and perhaps remove some of the work-arounds we have and simplify some of the code (as well as not introduce any work-arounds for app-grouping).

Comment 42

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

(In reply to Chris Lord [:cwiiis] from comment #41)
> So, confirmed that 2.2 will be using Gecko 39 - 2.2 QA sign-off begins
> 23/3/2015, so it'd be great to have this before then. Gaia master branches
> to 2.3 on 6/4/2015, so it'd be good to know we can rely on this and perhaps
> remove some of the work-arounds we have and simplify some of the code (as
> well as not introduce any work-arounds for app-grouping).

That should be no problem. I hope to be getting stuck into this bug by mid next week. (This week I'm focussing on the groundwork for this bug and already have about a dozen patches together.) I think it's safe to assume this will land in Q4.

(In reply to Chris Lord [:cwiiis] from comment #40)
> Is it only potential test failures (and review) that are holding this up? If
> so, I guess we should push a try build and see what breaks. Hopefully I can
> at least help fix some of the test failures.

It's mostly that, although there are a few other things to fix like dispatching events at the right time. For the tests, one big piece is just getting nsDOMWindowUtils::AdvanceTimeAndRefresh to basically override the behavior. That should fix a big bunch of tests in one hit.

We could push to get something landed quickly (e.g. 2~3 week timeline) but I think it'll still be a big set of patches to uplift to 2.1 and quite risky. (When we initially looked up the B2G dates I think we confused the feature-complete date for the branch date hence why we thought we had more time for this.)

Comment 43

[Maria Sandberg [:mushi]]

Hi there,

This same bug is causing a bad janky behavior for app grouping on the vertical home screen described in bug 1077169. Grouping is slotted for 2.2 so no real rush there but it would be great to have this out of the way well before then to test the usability. 

Given all the current constraints, is there an estimated timeline for when this may land just so I can get an idea?

Comment 44

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

(In reply to Maria Sandberg [:mushi] from comment #43)
> Given all the current constraints, is there an estimated timeline for when
> this may land just so I can get an idea?

Hi Maria, this is my top priority at the moment. I'm currently clearing out a lot of dependent bugs to enable this to happen. I'd estimate this will land in early November, with best case scenario being within October and worst case being end of November.

Comment 45

[Wilson Page [:wilsonpage]]

Wow, I just applied a WIP patch from Cwiiis and the difference is incredible! We're seeing all transitions go from inconsistent frame rates to super smooth.

We'll need this patch to land for v2.2 to get the performance we need for the new web-component control's transitions and animations.

Comment 46

[Chris Lord [:cwiiis]]

A bug I've noticed this does seem to cause, similar to what I alluded to in comment #38;

If you launch some applications, then open the task switcher (long-press the home button). Navigate to the far left of the switcher (by making rightwards swiping motions). You'll probably see the bug just doing this, but if not, wait until you get to the left-most app. Then drag it towards the right until the app on the right of it is far off-screen, then let go.

You'll see the app in question animate back into place with a blank area to the right. The app that was next to it will load back in and the entire animation will repeat, but faster.

Comment 47

[Stephany Wilkes]

Strongly suggest we ship this in 2.2. It fixes myriad performance issues that are pretty critical.

Comment 48

[Chris Lord [:cwiiis]]

Requesting blocking for 2.2 - this now blocks bug 1067435 (which will be a blocker), but I'd like to track this specifically as it would be good for overall perception of smoothness in Gaia and generally on the web, and isn't limited to that bug alone.

Comment 49

[Maria Sandberg [:mushi]]

Adding Wilfred as an FYI that we are noming this a blocking for 2.2

Comment 50

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

I've rebased and rewritten the WIP patch as a series of smaller patches. This try run should identify what mochitests this breaks:

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

Based on the results of that I'll probably need to add some or all of the following before this is ready for review:
* Handling for empty animations (that don't actually trigger a paint) to make sure they actually start
* Tests for play() followed by pause() before painting completes and likewise for changes to animation-play-state
* Changes to the dispatch of animationstart and transitionstart event so that it isn't fired until the animation *actually* starts.
* Tests for playState == "pending"
* The AnimationPlayer.ready promise -- there's probably a bit of work in this (and lots of testing) so if possible, it might be better to split this into a separate bug but that might not be possible since I suspect some of the tests in dom/animation/test/ won't work without it. Perhaps we could introduce a temporary hack for those tests so we can land this separately.

Comment 51

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

Next round:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a6e0fb2019a9

All the dom/animation/test/* tests will still fail, and so will layout/style/test/test_animations_pausing.html, plus there will probably a few failed assertions with OMTA but it should be closer.

Comment 52

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

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

Most of the mochitests should now pass.

Still to do:

* Work out why we resolve the 'ready' promise before actually putting the animations on the layer in some cases (I'm seeing this in test_running_on_compositor.html)

* Trigger animations that don't paint. Currently some of the tests will hang for a while until something triggers a paint in this case.
  - I'm not sure yet how to handle this. Check at the end of the refresh driver tick if we still have pending animations and resolve them then (unless paint suppression is in effect)? Empty animations we could detect upfront and resolve immediately but animations that don't paint because their target is off-screen are harder.

* A bunch of other tests that I don't expect to be too involved:
 - further tests for the ready promise and pending state (these should be ok though since other tests are already depending on them)
 - play() followed by pause() correctly resolves the promise
 - fetching .ready flushes style (it doesn't currently but that should be easy to fix)
 - animationstart/transitionstart are not triggered until the animation actually starts (this one might take a bit more work but could possibly be deferred to another bug if necessary)

Comment 53

[Chris Lord [:cwiiis]]

After much effort, rebasing, etc., I've tried the most recent version of this patch - I still see the problem I described in comment #46.

My git branch, for reference: https://github.com/Cwiiis/gecko-dev/tree/patches-i-want (still uploading as I write this)

Comment 54

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

(In reply to Chris Lord [:cwiiis] from comment #53)
> After much effort, rebasing, etc., I've tried the most recent version of
> this patch - I still see the problem I described in comment #46.

I'm happy to send you a roll-up patch to save you the effort in future. Provided I can work out what's going on with one failing assertion I should be able to land the dependent bugs today which should make applying these patches easier in future.

I'll look at comment 46 after I fix the other stack of failing tests.

Comment 55

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

I've spent most of the afternoon trying to get dom/animation/test/chrome/test_running_on_compositor.html to pass with these patches by waiting on the animation's ready promise.

It passes sometimes. If I wait until after document load it always passes.

After some digging around it appears that when it fails, it's because we haven't created the appropriate nsDisplayTransform yet. And that appears to be because we haven't attached the frames for the subdocument yet (it's a chrome mochitest so the test is loaded in an iframe), so they don't get traversed when doing a paint. Or at least I *think* that's what happens.

However, we end up resolving all the waiting animations after we've done a paint by enumerating all subdocuments on the content side and we seem to pick up the iframe document then.

Does that sound possible?

If so, I need to work out how to tell if a subdocument's frames are going to be traversed when painting so I don't mark the animations on the corresponding elements as ready to run until that's true.

(Then I'll have to work out what to do for a display:none subdocument but I already have to solve that for other kinds of animations that don't paint at all.)

Comment 56

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

On further investigation I don't know what is going on.

Comparing display list generation where we don't reach the element in question in the iframe document and when we do:
* Sometimes the nsCanvasFrame under nsSubdocumentFrame differs, sometimes it is the same
* Even when the nsCanvasFrame is the same object we sometimes don't reach the frame for the element in question.

Comment 57

[Boris Zbarsky [:bzbarsky]]

> Does that sound possible?

Yes.  Style structs are created lazily, when someone asks for them.

You can force immediate creation of a given style struct for a given element (esp. if it should have a frame) by doing elementsWindow.getComputedStyle(element).someProp, where someProp lives in the relevant style struct.   Does that help?

Comment 58

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

(In reply to Please do not ask for reviews for a bit [:bz] from comment #57)
> > Does that sound possible?
> 
> Yes.  Style structs are created lazily, when someone asks for them.
> 
> You can force immediate creation of a given style struct for a given element
> (esp. if it should have a frame) by doing
> elementsWindow.getComputedStyle(element).someProp, where someProp lives in
> the relevant style struct.   Does that help?

Thanks! However, it's more display items that I'm interested in this case.

Comment 59

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

So, there appear to be two problems blocking dom/animation/test/chrome/test_running_on_compositor.html

1) We resolve the start times before completing reflow?

In some cases we complete a paint and mark all animations as having started. However, during the paint some of the frames aren't traversed. I've noticed that when this is the case nsIPresShell::HasPendingReflows() is true (also the paint count is zero). When we succeed nsIPresShell::HasPendingReflows() is false (and the paint count is one; in both cases "is first paint" is true).

So, perhaps the issue is less to do with iframes but simply to do with interrupted reflow?

In any case, if we continue in this state, we come to check if the animation is running on the compositor but because it wasn't traversed by the paint that kick-started the animation it still says "no" and the test fails.

I can just check if nsIPresShell::HasPendingReflows() is true and not resolve animations in that case but I'm not sure if that's reasonable. I suppose there are cases where we have pending reflows for a protracted period of time and not starting animations at all in that period would be bad? Would "HasPendingFlows() && IsFirstPaint()" make sense?

Also, in the case of incremental reflow, ideally, I suppose we should resolve the start time for those frames that have been painted? Doing that would complicate the mechanism here and I'm not immediately sure how to do it but might be necessary.

Comment 60

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

The other issue blocking dom/animation/test/chrome/test_running_on_compositor.html is:

2) When we resolve start times they are ahead of the latest timeline time

Normally animation times are tied to refresh driver times via the animation timeline. When we have one of these "waiting to start" animations we essentially pause it at its time zero (i.e. its first frame for animations that don't have a delay).

However, when we resolve the start time we set it to a time that is *ahead* of the current timeline time. As a result, if we go to sample the animation *between* the moment when its start time is resolved and the next refresh driver tick we will compare the start time to the current refresh driver time and say, "Oh, this animation hasn't started yet". As a result the animation will jump from "paused at first frame" to "starting in the future" to "started".

(This, incidentally, was the cause of the assertion I mentioned in bug 1073336 comment 71. Basically we would start the animation, add a change hint to update the transform layer, then when we'd process restyles we'd get surprised, "You said to update the transform layer but there's no transform going on here.")

We could possibly work around this by putting the player in some temporary "even though it doesn't look like it, I've actually started"-state but I really don't want to add more states to the player. We'd have to test every possible action that could take place while in that weird state and make sure it did the right thing.

Rather, I'd like to force the timeline to move ahead to the timestamp when painting finished, ahead of the refresh driver. I'm not sure how we'll manage this case for non-refresh driver timelines but we can cross that bridge when we come to it.

That means adding some extra state to the timeline but I think it should be ok.

Comment 61

[Boris Zbarsky [:bzbarsky]]

> However, it's more display items that I'm interested in this case.

Waiting one requestAnimationFrame tick should work for those, right?  Modulo the interruptible reflow issue.

Comment 62

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

Attachment: WIP patch v3

Updated WIP patch.

Try run of the same: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2a84e6f255ff

Still to do:
* Properly cancel temporary/abandoned players in nsAnimationManager
* A few correctness fixes (e.g. making fetching the ready promise flush styles)
* A few more tests
* Possibly adjust dispatch of animationstart events (not sure about this)

Comment 63

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

Attachment: WIP patch v4

Two steps forward one step back: This fixes a few issues to do with cancelling but I notice that test_running_on_compositor.html now fails again because we're triggering some compositor animations too soon. Either we're simply hooking up to the wrong refresh driver (pretty sure at least this is true), or the approach to detecting empty animations just isn't going to work.

Also, I'm getting a test failure in /tests/layout/style/test/test_animations_omta_start.html that I can't reproduce locally so it might take a while to fix.

Feature-wise this should be mostly done now but it's going to take a while to fix those two test failures (especially if the approach to detecting non-painting animations proves invalid).

Try run anyway: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=99f6d6e0a8fa

Comment 64

[Chris Lord [:cwiiis]]

Seems this more recent version of the patch fixes the glitch I spoke about in previous comments :) Going to try running with this on my main phone again to see if I spot anything else.

Comment 65

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

I have this basically working but before I stick up a whole bunch of patches for review, I want to get some feedback on the overall approach.

Overall idea: When we play an animation we initially make its start time null. Then, when painting finishes and just before we finish the layer transaction, we record the current time and set that in two places: (1) the AnimationPlayer objects (these are basically the objects we use to represent animations on the main thread now), and (2) any animations on layers that have a null start time.

There are two problems with this:

Problem #1: When we finish a paint, we might not have painted everything. In cases like interrupted reflow we might have only painted some of the frames. This comes up in one test case that runs in an iframe where the frames belonging to the iframe document might not have been visited when the paint finishes.

Problem #2: Some animations don't trigger a paint. This happens for animations with empty keyframes rules or which animate something that isn't rendered. If we wait for a paint to complete, we might wait a long time and these animations won't start.

I've tested a number of approaches to solving these two but I suspect there's a better way.

The approach I've used goes like this:

* When we play an animation, we stick it on a hash of pending animations tied to the document that it is animating.
* When we finish a paint, we record TimeStamp::Now() as the "ready time" (aka "animation start time").
* We update all animations on layers with a null start time to use the "ready time".
* We then go through each subdocument and resolve animations in its pending animations hash as follows:
  - If there are *either* pending paints or reflows for that document, we set a "waiting for paint to finish" flag on the hashmap. Otherwise, we clear that flag.
    (Based on my testing we have to check both these things, i.e. nsRefreshDriver::IsWaitingForPaint() on display root refresh driver, and nsPresContext::HasPendingRestyleOrReflow() on the presentation context for the document. Often only one is set.)
  - If "waiting for paint to finish" is true we *only* set the start time for animations that are running on the compositor. We can check this easily because AnimationPlayer objects have an mIsRunningOnCompositor flag that gets set during display list generation.
    Currently this is necessary because when we are doing interrupted reflow we can end up sending some animations to the compositor while there are still pending reflows. Since we already updated the start times of animations on the layers we need to make sure their corresponding AnimationPlayer objects get the same start time.
    (An alternative approach would be to pass a flag to EndTransaction and *not* update animations on layers until all reflows/repaints were done. That might actually better because it would mean animations that are triggered at the same time start together.)
  - When we *do* have animations that are eligible for starting, we first fast-forward the corresponding timeline to the "ready time" then we simply tell the animation to set its start time to the current time of its timeline.
* When pending animations are added to a documents hash, we register a PostRefresh callback with the display root presentation context's refresh driver.
* If we complete a refresh driver tick and the "waiting for paint to finish" flag is not set, we simply start any pending animations using the current time of their timelines.

My questions are:

1) Is there are better way of detecting when things are "painted"?
2) Is it better to delay all animations from starting until they are all ready and trigger them in one go? (And, if so, if additional animations are added in the meantime should they form a separate set?)

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For problem #1, personally I think we shouldn't do anything clever with interrupted reflow. That adds a lot of complexity for not much gain. Perhaps the best way to deal with it would be to say that if there are any animations in the document, force a full reflow in the refresh driver.

For problem #2, can we just track the set of pending animations and at the next layer transaction, update their start times --- and, whenever an animation is added to the set, ensure SchedulePaint is called so there will be another layer transaction "soon"?

Comment 67

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #66)
> For problem #1, personally I think we shouldn't do anything clever with
> interrupted reflow. That adds a lot of complexity for not much gain. Perhaps
> the best way to deal with it would be to say that if there are any
> animations in the document, force a full reflow in the refresh driver.

Ok, that makes sense. So it sounds like during the Flush_Layout step of nsRefreshDriver::Tick we'd navigate presshell -> document -> pending animation set, and if there were any pending animations we'd do:

  shell->FlushPendingNotifications(ChangesToFlush(Flush_Layout, false))

instead of:

  shell->FlushPendingNotifications(ChangesToFlush(Flush_InterruptibleLayout, false))

(and probably skip setting mSuppressInterruptibleReflows to false, but I have to check that).
 
> For problem #2, can we just track the set of pending animations and at the
> next layer transaction, update their start times --- and, whenever an
> animation is added to the set, ensure SchedulePaint is called so there will
> be another layer transaction "soon"?

That sounds good. I suppose we'd call SchedulePaint on the root frame for the corresponding document (since we need to handle animations that aren't tied to any frame at all).

Comment 68

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

I tried the suggestions from comment 66 and the tests seem to pass locally. I can't believe you solved it so easily.

Let's see how try likes it: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d121b088e972

Comment 69

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

Attachment: WIP patch v5

Incorporating the feedback from comment 66

Comment 70

[Chris Lord [:cwiiis]]

"2) Is it better to delay all animations from starting until they are all ready and trigger them in one go? (And, if so, if additional animations are added in the meantime should they form a separate set?)"

I think it's important that animations start together - otherwise there will be situations where animations aren't in sync, and the reason will be totally opaque to an inexperienced web-dev. That said, if it holds things up, I think it would be fine to address it in follow-up.

Comment 71

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

(In reply to Chris Lord [:cwiiis] from comment #70)
> I think it's important that animations start together - otherwise there will
> be situations where animations aren't in sync, and the reason will be
> totally opaque to an inexperienced web-dev. That said, if it holds things
> up, I think it would be fine to address it in follow-up.

Using the approach roc suggested of forcing full reflow animations should start together. I agree that's quite important.

Comment 72

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

(In reply to Brian Birtles (:birtles) from comment #68)
> I tried the suggestions from comment 66 and the tests seem to pass locally.
> I can't believe you solved it so easily.

It turns out I was testing with OMTA turned off. With OMTA turned back on test_is_running_on_compositor.html fails again.

Adding some debugging, basically this is what happens:

  --- 18739460 Add pending player
  --> 0629B6A0 Tick
    --- Flush_Style...
    --- No Flush_Layout observers...
        (has pending reflows: 0)
    --- View manager flush...
    --- Starting pending animations
    --- 18645E50 Start pending players
    --- 18739460 Start pending players
  <-- 0629B6A0 Tick
  --> 182BBAC0 Tick
    --- Flush_Style...
    --- Flush_Layout...
  <-- 182BBAC0 Tick
  2 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/animation/test/chrome/test_running_on_compositor.html | AnimationPlayer reports that it is running on the compositor during playback - got false, expected true

The code I added ensures we turn an interruptible reflow into a full reflow when we have pending animations but in this case there are no pending reflows.

Hopefully this will be easier to work out in person next week.

Comment 73

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

Attachment: WIP patch v6

I almost completely rewrote this after discussing with roc last week. The new approach is that after we finish painting we simply mark the animation as ready to start but don't actually start it until the next refresh driver tick. This means we don't end up doing extra samples within a sample but making nsDOMWindowUtils::AdvanceTimeAndRefresh continue to behave in the same way becomes quite complicated.

I think I've fixed most of the test failures from this new approach (currently crossing my fingers while waiting for: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ca29cdae74f3).

I've yet to fix browser/devtools/storage/test/browser_storage_basic.js however.

The problem is that test tries to select stuff in trees that have an animated expansion effect. The start of the animation sets the elements to max-height: 0 but normally by the time the test goes to select them the animation is in progress and they can be selected.

With the delayed animation start behaviour, however, the animation is still on the first frame with max-height: 0 set to zero. As a result the item does not get selected and the test fails. I'll probably need to add a 'animationsComplete' to browser/devtools/shared/widgets/TreeWidget.js and get browser/devtools/storage/test/head.js::selectTreeItem to wait on it.

(The test failures in the try run around browser_wa_graph-click.js etc. are because I turned off a bunch of UI animations temporarily)

Comment 74

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

(In reply to Brian Birtles (:birtles) from comment #73)
> Created attachment 8533571 [details] [diff] [review]
> WIP patch v6
> 
> I almost completely rewrote this after discussing with roc last week. The
> new approach is that after we finish painting we simply mark the animation
> as ready to start but don't actually start it until the next refresh driver
> tick. This means we don't end up doing extra samples within a sample but
> making nsDOMWindowUtils::AdvanceTimeAndRefresh continue to behave in the
> same way becomes quite complicated.

Having implemented this and got nearly all tests to pass I did a side-by-side comparison of this vs the old approach and the old approach is clearly superior. :(

Basically the difference between the two approaches is:

A) When we finish painting record the TimeStamp and use that as the animation start time even though this will not line up with a refresh driver tick. This means that not all animation times will line up with a refresh driver tick and that means we need to do some special handling to make sure everything is in a consistent state.

B) When we finish painting we simply mark the animations as ready to start and resolve their start times on the next refresh driver tick. After doing that we still need to send the animations to the compositor. In the end, the animation ends up starting later and its quite noticeable.

So, I'm going to revert back to doing A. I'm not sure I'll be able to make everything entirely consistent but I'll probably address that is a follow up (perhaps along with bug 1085769 we can tick animations from the timeline and then we'll have a convenient place to mark them as dirty when we fast-forward the animations).

Comment 75

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

Attachment: WIP patch v7

I've reverted to the old approach and now hopefully fixed nearly all the failing tests. (I might still need to update test_popupreflows.xul, not sure).

I'll find out soon anyway:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3c316104353b

I'll tidy up these patches and put them up for review at the beginning of next week.

Comment 76

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

Attachment: part 1 - Add PendingPlayerTracker

This patch adds a hashtable to nsDocument that stores all the animation players
that are currently waiting to start. In the future it may also be used to store
players that are waiting to pause which is why the methods are called
AddPlayPending/RemovePlayPending instead of just AddPlayer/RemovePlayer.

Comment 77

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

Attachment: part 2 - Add AnimationPlayer::mIsPending and return "pending" from AnimationPlayer::PlayState()

This patch adds a member for tracking if a player is currently pending or not
and uses this to return the "pending" state from PlayState(). We don't currently
set mIsPending to true yet, however.

Additionally, this patch updates AnimationPlayer::ComposeStyle to set
aNeedsRefreshes to true when the player is pending. This is used by the
appropriate animation/transition manager to determine if it should continue to
observe the refresh driver or not.

Comment 78

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

Attachment: part 3 - Cancel a pending play when pause is called

This patch updates the pause procedure for AnimationPlayer so that if there is
a pending play it will be cancelled.

At the same it removes the existing check for a redundant call to Pause when we
are already paused. This check is not necessary since if we are already paused
the method will have no effect anyway.

Finally, this patch updates the comment about going to the pending state while
pausing since this will happen in bug 1109390.

Comment 79

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

Attachment: part 4 - Make the ready Promise be created lazily

In Bug 1104435 we made the initial ready promise be created lazily. This patch
takes this a step further and makes sure we don't create the promise at all
unless it is explicitly requested.

Comment 80

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

Attachment: part 5 - Cancel players that are no longer used

This patch adds a means of terminating an animation so that is has no effect.
The procedure is defined by Web Animations:

  http://w3c.github.io/web-animations/#cancelling-a-player-section

We don't implement all of this, however, since we don't currently support the
finished promise or custom effects.

In a later bug we will expose this as the cancel() method on AnimationPlayer.

We call this method for terminated animations in nsAnimationManager and
nsTransitionManager to ensure they get removed from the pending player tracker
and so that, for example, the ready promise of CSS Animation player objects is
rejected when the corresponding item is removed from animation-name.

Comment 81

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

Attachment: part 6 - Rename AnimationPlayer::ResolveStartTime to AnimationPlayer::StartNow

ResolveStartTime is a bit hard to understand. Eventually, once we implement
SetStartTime, we can probably remove this method altogether and just use that.
This renaming moves us closer to that direction.

This patch also adjusts a comment about the preconditions for calling StartNow.
This is because in a subsequent patch in this series we will update the
assertion at the beginning of StartNow to simply check that the player is
pending.

Comment 82

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

Attachment: part 7 - Factor out AnimationTimeline::GetRefreshDriver into a separate method

This is in preparation for adding AnimationTimeline::FastForward in the next
patch which will reuse this code.

Comment 83

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

Attachment: part 8 - Fast-forward the timeline before resolving start times

Normally animation players get times from their timeline which is based on the
refresh driver for their associated document. However, for animations that
we time from when their first frame has been rendered, we want to record the
actual time when painting finished as their start time. If we wait until the
next refresh driver tick there the delay between playing an animation and its
actual start will be too great.

In this patch, we introduce a mechanism for fast-forwarding a timeline to a
time between the current refresh driver time and the next refresh driver tick.
By adjusting the timeline rather than the player we maintain a consistent state
(in fact, if we just naively set the animation player start time to the
timestamp value we recorded when painting finished it will appear to start in
the future and the animation will temporarily from playing, to waiting to start,
then back to playing again on the next refresh driver tick).

To be completely consistent, however, when we fast-forward the timeline we
should tell all animation players listening to the timeline to mark their
target element as needing a style flush. Otherwise we may be able to observe an
inconsistency between some animation players' current time and the computed
style of their targets.

We don't, however, currently know which players are observing a given timeline.
We will likely introduce that in the near future (in order to implement
AnimationTimeline.getAnimationPlayers) and fix the inconsistency in timing then.
A test later in the patch series verifies this inconsistency so it is easy to
fix in future.

An alternative approach would be to simply record the time when animation should
start, send that time to the compositor but don't actually update the animation
start time on the main thread until the subsequent refresh driver tick. Such
an approach would be more complex as it introduces an additional state--
"finished pending but not yet started"--but might be worth attempting if the
approach of fast-forwarding the timeline proves undesirable.

Comment 84

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

Attachment: part 9 - Add PendingPlayerTracker::StartPendingPlayers

Comment 85

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

Attachment: part 10 - Record the time when animations are ready to start

Comment 86

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

Attachment: part 11 - Pass the animation ready time to the pending player tracker

Comment 87

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

Attachment: part 12 - Store the initial time of animations on layers so start times can be resolved after-the-fact

REVIEW: Is 'resume time' a better name?

Comment 88

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

Attachment: part 14 - Store reference to owning document on PendingPlayerTracker

This patch adds a reference from PendingPlayerTracker back to the document
object that owns it. This is used in the next patch in this series to find the
document's root frame for scheduling a paint.

Comment 89

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

Attachment: part 15 - Schedule a paint when a new pending animation is added

We would like to trigger animations from the point when the first frame of the
animation is painted. However, some animations will never trigger a paint (e.g.
animations with an empty keyframes rule). These animations should start start
however. To ensure this, whenever an animation is newly pending we schedule
a paint.

Comment 90

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

Attachment: part 16 - Do a full reflow when we have pending animations

We want to time animations from when their first frame is painted. However,
interruptible reflow complicates this since, for a given set of pending
animations, some may be painted whilst others are not. To simplify this we
simply force an uninterruptible reflow when we have animations that are
waiting to start.

Comment 91

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

Attachment: part 17 - Don't count native reflows in test_popupreflows.xul

test_popupreflows.xul currently expects to get 1 reflow when a popup is opened.
This is triggered by JS code. An incremental layout may be triggered by native
code but the test ignores that for unclear reasons ("Because we've simply
cargo-culted this test from browser_tabopen_reflows.js!").

In part 16 on this patch series, we force interruptible reflows to become
full reflows when we have pending animations which can cause this test to
fail since it will see 2 reflows.

The above quoted browser_tabopen_reflows.js specifically ignores reflows
triggered by native code so we adjust this test to do likewise.

Comment 92

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

Attachment: part 18 - Make nsDOMWindowUtils::AdvanceTimeAndRefresh trigger any pending animations first

Currently many tests rely on nsDOMWindowUtils::AdvanceTimeAndRefresh. These
tests assume that the animation starts from the moment it is created. In order
to allow these tests to continue to operate without change we make
AdvanceTimeAndRefresh force any pending animations to start.

Comment 93

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

Attachment: part 19 - Don't add pending animations to layers when their refresh driver is under test control

Comment 94

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

Attachment: part 20 - Update selectTreeItem to wait for tree expansion animations to finish

Currently selectTreeItem, as used by various devtools storage panel tests,
expands the tree then clicks items. However, it fails to account for the fact
that expansion animations may mean that the item that should be selected is not
yet able to be clicked. It currently happens to be lucky enough that typically
the animation is short enough that the item in question will be click-able in
time (since click() spins the event loop using executeSoon there is some time
for the animation to play).

If we make animations wait until their first frame has rendered before
beginning the chance that the tree item will become clickable in time is
reduced. This patch works around that by looking for animating branches
amongst the ancestors of the item to be selected. If one is found it waits for
the animation to end.

Unfortunately this ties the tests to the tree widget styles somewhat (in
particular the test looks for specified values of max-height).
However, these changes only delay the execution of the test so, if these styles
were to change, it is only likely that these tests would fail (and hence need to
be updated) than to mask a genuine bug. Until the Web Animations API is
available by default it is difficult to test if animations are running so this
seems like the best we can do for now.

Comment 95

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

Attachment: part 21 - Factor out AnimationPlayer::UpdateSourceContent

In the future we will want to specifically just update source content without
necessarily triggering any other actions that might take place on a tick (like
queuing events).

Comment 96

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

Attachment: part 22 - Make AnimationPlayer wait for animations to be rendered before starting

This patch (finally!) introduces the delayed start behavior. It updates
AnimationPlayer::DoPlay to put animations in the PendingPlayerTracker from
where they are triggered.

This patch also updates nsTransitionManager to set the animation's source
before calling Play as otherwise the AnimationPlayer won't be able to access
the pending player tracker (which it locates by navigating AnimationPlayer ->
Animation (source content) -> target element -> composed doc -> pending player
tracker). In future, when we support setting the AnimationPlayer.source property
we will make this more robust so that the order in which these steps are
performed doesn't matter.

This patch also updates a couple of tests to reflect the fact that
AnimationPlayer will now return the pending state.

Comment 97

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

Attachment: part 23 - Wait on player.ready on test_running_on_compositor.html

Comment 98

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

Attachment: part 24 - Add a test that empty animations still start

Comment 99

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

Attachment: part 25 - Test cancelling of players

Comment 100

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

Attachment: part 13 - Update start times on animations in layers when the animation ready time is resolved

Comment 101

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

Note that these patches should apply on top of bug 1104435.

Full try run of the above patches (plus bug 1104435):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cbd2eeb3f2ba

I still need to:

* Add a test for the inconsistency between animated time and animated style that can sometimes be observed due to fast-forwarding the timeline (I have a test for this but just need to tidy it up a bit)

* Add a test that animationstart events are dispatched while we're still pending (the reasoning being that you want to know when an animated value has been applied; also, if you're trying to gauge if there are any animations in the document to run you want to get that event as soon as possible). This should also test that cancelling doesn't trigger an end event.

Comment 102

[Chris Lord [:cwiiis]]

Comment on attachment 8536392 [details] [diff] [review]
part 13 - Update start times on animations in layers when the animation ready time is resolved

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

I don't think I'm the right reviewer for this anymore (not being on the gfx team), however, if I was, this would be an r+. Passing it on to nical.

Comment 103

[Chris Lord [:cwiiis]]

Comment on attachment 8536378 [details] [diff] [review]
part 12 - Store the initial time of animations on layers so start times can be resolved after-the-fact

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

Forwarding to nical. Not f+'ing this as though it's small, I think the comment describing initialTime could be clearer. It's a tiny change though, and this could be addressed in follow-up.

::: gfx/layers/ipc/LayersMessages.ipdlh
@@ +176,5 @@
> +  // The animation's current time at the moment it was initialized.
> +  // For animations that are waiting to start, their startTime will be null.
> +  // Once the animation is ready to start, the initialTime is used to calculate
> +  // the a value for startTime such that the animation's current time matches
> +  // initialTime.

I'm not sure I really understand this comment...

Comment 104

[Nicolas Silva [:nical]]

Comment on attachment 8536378 [details] [diff] [review]
part 12 - Store the initial time of animations on layers so start times can be resolved after-the-fact

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

::: gfx/layers/ipc/LayersMessages.ipdlh
@@ +176,5 @@
> +  // The animation's current time at the moment it was initialized.
> +  // For animations that are waiting to start, their startTime will be null.
> +  // Once the animation is ready to start, the initialTime is used to calculate
> +  // the a value for startTime such that the animation's current time matches
> +  // initialTime.

To second Chris, I had to read the comment several time, and I am not entirely sure still:
* startTime is when things actually start moving (while the animation is preparing we don't know when it'll be ready to move so until then startTime is 0).
* initialTime is when the animation would have started if "preparing" was instantaneous.
What confuses me is the last sentence because if I look at AddAnimationForProperty below, initialTime isn't used in the calculation of startTime (though there may  be other places in the code where it is?), and more importantly what does "match" mean in this context?

Comment 105

[Nicolas Silva [:nical]]

(In reply to Nicolas Silva [:nical] from comment #104)
> if I look at
> AddAnimationForProperty below, initialTime isn't used in the calculation of
> startTime (though there may  be other places in the code where it is?)

Sorry, forget that part, it makes more sense after I read some of the other patches

Comment 106

[Girish Sharma [:Optimizer]]

Comment on attachment 8536386 [details] [diff] [review]
part 20 - Update selectTreeItem to wait for tree expansion animations to finish

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

Although, I get the need for the change, but the change seems to be a more of a hack. Isn't there an easier way to do this ?

Like adding an event listener for transitionend event before calling the expand method ? (or something like that)

If no, you have my r+ on this version of the patch. Although, I am not a peer. Please ask :bgrins for an actual r+

Comment 108

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

Attachment: part 12 - Store the initial time of animations on layers so start times can be resolved after-the-fact

(In reply to Nicolas Silva [:nical] from comment #104)
> Comment on attachment 8536378 [details] [diff] [review]
> part 12 - Store the initial time of animations on layers so start times can
> be resolved after-the-fact
...
> To second Chris, I had to read the comment several time, and I am not
> entirely sure still:
> * startTime is when things actually start moving (while the animation is
> preparing we don't know when it'll be ready to move so until then startTime
> is 0).
> * initialTime is when the animation would have started if "preparing" was
> instantaneous.

I agree with you both. That was a horrible comment. I've renamed the property to
initialCurrentTime and revised the comment.

The slightly confusing part is probably that startTime is a TimeStamp--i.e.
a moment in wall-clock time. On the other hand initialCurrentTime (nee
initialTime) is an offset--a number of milliseconds since startTime representing
how far into the animation we are.

(The even more confusing part is that normally when we talk about "current time"
we're including the delay, e.g. an animation with a delay of 2s has a current
time of 2000 when it starts. But in the context of OMTA we don't put animations
on the layer until *after* the delay has finished and we never set the delay on
the layer animation. As a result, "current time" in context of layer animations
is *without* the delay. I just noticed a bug because of this and have fixed it
now. I'll add a follow-up patch with a test case to check this.)

For animations that are already playing we set both startTime and
initialCurrentTime but we never use initialCurrentTime.

For animations that are waiting to start/resume, we set startTime to a null
timestamp and fill in initialCurrentTime with whatever the animation is
currently up to minus its delay (the result of which will be 0 for animations
that are starting from the beginning, > 0 for animations that are resuming).
Then later (this is part 13), when the animation is ready to start, we calculate
startTime from initialCurrentTime.

Comment 109

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

Attachment: part 13 - Update start times on animations in layers when the animation ready time is resolved

Rebase

Comment 110

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

Attachment: Roll-up patch

(In reply to Nicolas Silva [:nical] from comment #105)
> (In reply to Nicolas Silva [:nical] from comment #104)
> > if I look at
> > AddAnimationForProperty below, initialTime isn't used in the calculation of
> > startTime (though there may  be other places in the code where it is?)
> 
> Sorry, forget that part, it makes more sense after I read some of the other
> patches

Sorry about that, there are quite a lot of patches to dig through. Here's a roll-up patch for reference.

Comment 111

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

(In reply to Girish Sharma [:Optimizer] from comment #106)
> Comment on attachment 8536386 [details] [diff] [review]
> part 20 - Update selectTreeItem to wait for tree expansion animations to
> finish
> 
> Review of attachment 8536386 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Although, I get the need for the change, but the change seems to be a more
> of a hack. Isn't there an easier way to do this ?

I'm afraid not. I tried a half a dozen different approaches.

> Like adding an event listener for transitionend event before calling the
> expand method ? (or something like that)

I'm afraid that doesn't work. We don't always animate, only sometimes (it depends if the branch has been expanded or not). We won't know if we've finished animating or not unless we track all animationstart and animationend events from the outset. Furthermore, we can (and do) get unbalanced animationstart/animationend events because if we drop a running animation by updating animation-name the animation will be cancelled and won't dispatch animationend.

Basically, until the Web Animations API is available by default we don't have a reliable way of telling if animations are running or not so we don't know if we need to wait for them or not. The best we can do is check the computed style for now.

> If no, you have my r+ on this version of the patch. Although, I am not a
> peer. Please ask :bgrins for an actual r+

Ok, will do. Thanks.

Comment 112

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

Attachment: part 24 - Add a test that empty animations still start

I've updated this test to stick the Mozilla-specific tests in a 'mozilla' folder instead of 'gecko' since 'mozilla' is the naming used in web-platform-tests

Comment 113

[Brian Grinstead [:bgrins]]

Comment on attachment 8536386 [details] [diff] [review]
part 20 - Update selectTreeItem to wait for tree expansion animations to finish

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

Looks fine, have a couple of notes inline

::: browser/devtools/storage/test/head.js
@@ +491,5 @@
> +  // If the list is still expanding we won't be able to click the requested
> +  // item. We detect if a list (or one of its ancestor lists) is animating
> +  // by checking if max-height is set. If the animation effect in
> +  // widgets.inc.css changes this will likely break but until the Web
> +  // Animations API is available there's no reliable way to detect if

Add a bug # in this comment for when we will be able to switch to the new API

@@ +504,5 @@
> +  }
> +
> +  if (animatingList) {
> +    let expanded = false;
> +    animatingList.addEventListener("animationend", function() {

You could just removeEventListener instead of tracking whether it's been called with the expanded variable:

    animatingList.addEventListener("animationend", function animationend() {
      animatingList.removeEventListener("animationend", animationend);
      click(target);
    });

Comment 114

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

Attachment: part 26 - Add a test for compositor animations with a delay

(In reply to Brian Birtles (:birtles) from comment #108)
> (The even more confusing part is that normally when we talk about "current
> time" we're including the delay, e.g. an animation with a delay of 2s has
> a current time of 2000 when it starts. But in the context of OMTA we don't
> put animations on the layer until *after* the delay has finished and we never
> set the delay on the layer animation. As a result, "current time" in context
> of layer animations is *without* the delay. I just noticed a bug because of
> this and have fixed it now. I'll add a follow-up patch with a test case to
> check this.)

Here is the test case I mentioned. Nicolas, if this isn't your area,
just let me know and I'll assign to roc or heycam.

Comment 115

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

(In reply to Brian Grinstead [:bgrins] from comment #113)
> Comment on attachment 8536386 [details] [diff] [review]
> part 20 - Update selectTreeItem to wait for tree expansion animations to
> finish
...
> ::: browser/devtools/storage/test/head.js
> @@ +491,5 @@
> > +  // If the list is still expanding we won't be able to click the requested
> > +  // item. We detect if a list (or one of its ancestor lists) is animating
> > +  // by checking if max-height is set. If the animation effect in
> > +  // widgets.inc.css changes this will likely break but until the Web
> > +  // Animations API is available there's no reliable way to detect if
> 
> Add a bug # in this comment for when we will be able to switch to the new API

Fixed locally.

> @@ +504,5 @@
> > +  }
> > +
> > +  if (animatingList) {
> > +    let expanded = false;
> > +    animatingList.addEventListener("animationend", function() {
> 
> You could just removeEventListener instead of tracking whether it's been
> called with the expanded variable:
> 
>     animatingList.addEventListener("animationend", function animationend() {
>       animatingList.removeEventListener("animationend", animationend);
>       click(target);
>     });

I was waiting for someone to teach me how to do that! Thank you! Fixed locally.

Comment 116

[Jonathan Watt [:jwatt]]

Comment on attachment 8536373 [details] [diff] [review]
part 7 - Factor out AnimationTimeline::GetRefreshDriver into a separate method

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

::: dom/animation/AnimationTimeline.h
@@ +46,5 @@
>    Nullable<TimeDuration> ToTimelineTime(const TimeStamp& aTimeStamp) const;
>    TimeStamp ToTimeStamp(const TimeDuration& aTimelineTime) const;
>  
>  protected:
> +  virtual ~AnimationTimeline() { }

My personal preference is to have ctors/dtors up at the top of a class declaration, so if you're moving this here I wouldn't mind you doing that at the same time. :)

Comment 117

[Jonathan Watt [:jwatt]]

Comment on attachment 8536374 [details] [diff] [review]
part 8 - Fast-forward the timeline before resolving start times

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

::: dom/animation/AnimationTimeline.cpp
@@ +97,5 @@
> +  // The timeline may have been fast-forwarded to account for animations
> +  // that begin playing between ticks of the refresh driver. If so, we should
> +  // use the fast-forward time unless we've already gone past that time.
> +  //
> +  // (If the refresh driver were ever to go backwards then we would need to

Maybe MOZ_ASSERT this?

@@ +104,5 @@
> +  //  refresh driver goes backwards is when it is restored from test control
> +  //  and FastForward makes sure we don't set the fast foward time when we
> +  //  are under test control.)
> +  if (result.IsNull() ||
> +       (!mFastForwardTime.IsNull() && mFastForwardTime > result)) {

Maybe add a comment about why the |mFastForwardTime > result| check is necessary if you can easily think how to write one? Otherwise not a big deal.

::: dom/animation/AnimationTimeline.h
@@ +50,5 @@
> +  //
> +  // Normally the timeline uses the refresh driver time but when we have
> +  // animations that are timed from when their first frame is rendered we need
> +  // to bring the timeline forward to that moment. If we don't, the animation
> +  // will be temporarily treated as not running until the timeline catches up.

Maybe "If we don't, calling IsRunning() on the animation will incorrectly return false (because GetCurrentTime will return a negative time) until the next refresh driver tick causes the timeline to catch up."

@@ +52,5 @@
> +  // animations that are timed from when their first frame is rendered we need
> +  // to bring the timeline forward to that moment. If we don't, the animation
> +  // will be temporarily treated as not running until the timeline catches up.
> +  //
> +  // |aTimeStamp| must be greater or equal to the current refresh driver

MOZ_ASSERT that.

Comment 118

[Jonathan Watt [:jwatt]]

Comment on attachment 8536374 [details] [diff] [review]
part 8 - Fast-forward the timeline before resolving start times

Can you file a follow-up about considering storing the start time to use but and applying it at the next refresh driver tick rather than having this fast-forwarding thing? I know you considered it, so it would be useful to have your thoughts about that written down, or at least return to consider it again at some point.

Comment 119

[Jonathan Watt [:jwatt]]

And needinfo roc on that bug?

Comment 120

[Jonathan Watt [:jwatt]]

Comment on attachment 8536374 [details] [diff] [review]
part 8 - Fast-forward the timeline before resolving start times

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

::: dom/animation/AnimationTimeline.cpp
@@ +42,5 @@
> +AnimationTimeline::FastForward(const TimeStamp& aTimeStamp)
> +{
> +  // If we have already been fast-forwarded to an equally or more
> +  // recent time, ignore this call.
> +  if (!mFastForwardTime.IsNull() && aTimeStamp <= mFastForwardTime) {

Maybe assert that |aTimeStamp >= mFastForwardTime|.

Comment 121

[Jonathan Watt [:jwatt]]

Comment on attachment 8536375 [details] [diff] [review]
part 9 - Add PendingPlayerTracker::StartPendingPlayers

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

::: dom/animation/PendingPlayerTracker.cpp
@@ +34,5 @@
>    return mPlayPendingSet.Contains(const_cast<dom::AnimationPlayer*>(&aPlayer));
>  }
>  
> +PLDHashOperator
> +StartNow(nsRefPtrHashKey<dom::AnimationPlayer>* aKey, void*)

Maybe remove this, unless it's used in a future patch.

@@ +48,5 @@
> +                  void* aReadyTime)
> +{
> +  dom::AnimationPlayer* player = aKey->GetKey();
> +
> +  // Fast-forward the player's timeline to the time when painting finished.

This function and StartPendingPlayers() isn't obviously associated with painting finishing. Maybe expand the comment a bit to explain the scenario in which we are called.

@@ +52,5 @@
> +  // Fast-forward the player's timeline to the time when painting finished.
> +  // We can't just use TimeStamp::Now() since we want to use the same time
> +  // here as we set on layer animations.
> +  dom::AnimationTimeline* timeline = player->Timeline();
> +  timeline->FastForward(*static_cast<const TimeStamp*>(aReadyTime));

When we get to the point where we can have multiple

::: dom/animation/PendingPlayerTracker.h
@@ +21,5 @@
>    void AddPlayPending(dom::AnimationPlayer& aPlayer);
>    void RemovePlayPending(dom::AnimationPlayer& aPlayer);
>    bool IsWaitingToPlay(dom::AnimationPlayer const& aPlayer) const;
>  
> +  void StartPendingPlayers(const TimeStamp& aReadyTime);

Add a comment noting that this function fast forwards the players' timeline to aReadyTime.

Comment 122

[Jonathan Watt [:jwatt]]

Comment on attachment 8536377 [details] [diff] [review]
part 11 - Pass the animation ready time to the pending player tracker

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

::: layout/base/nsDisplayList.cpp
@@ +1586,5 @@
> +    TimeStamp animationReadyTime = layerManager->GetAnimationReadyTime();
> +    MOZ_ASSERT(!animationReadyTime.IsNull(),
> +               "Using a layer manager that doesn't update the animation"
> +               " ready time");
> +    StartPendingPlayers(document, &animationReadyTime);

To minimize cluttering this already long method, maybe make StartPendingPlayers take a TimeStamp&, change this to:

StartPendingPlayers(document, layerManager->GetAnimationReadyTime());

and move the assert into that method. Since this isn't animation code, I think it would be better to rename the function to StartPendingAnimationPlayers too.

Comment 123

[Jonathan Watt [:jwatt]]

Comment on attachment 8536376 [details] [diff] [review]
part 10 - Record the time when animations are ready to start

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

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +545,5 @@
>      }
>    }
>  
> +  if (mRoot) {
> +    mAnimationReadyTime = TimeStamp::Now();

mattwoodrow noted that for BasicLayerManager you should maybe only do this if |aFlags & END_NO_COMPOSITE| since otherwise we'll update it when we composite too.

Comment 124

[Jonathan Watt [:jwatt]]

Comment on attachment 8536381 [details] [diff] [review]
part 15 - Schedule a paint when a new pending animation is added

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

::: dom/animation/PendingPlayerTracker.cpp
@@ +23,5 @@
>  {
>    mPlayPendingSet.PutEntry(&aPlayer);
> +
> +  // Schedule a paint. Otherwise animations that don't trigger a paint by
> +  // themselves won't start until something else paints.

Add a parenthetical here to give an example of when animations don't trigger paint. Maybe (for example, CSS animations without any keyframes rules).

@@ +24,5 @@
>    mPlayPendingSet.PutEntry(&aPlayer);
> +
> +  // Schedule a paint. Otherwise animations that don't trigger a paint by
> +  // themselves won't start until something else paints.
> +  nsIFrame* rootFrame = GetRootFrameForDoc();

Rather than having a GetRootFrameForDoc method, maybe add an EnsurePaintIsScheduled method.

Comment 125

[Jonathan Watt [:jwatt]]

Comment on attachment 8536367 [details] [diff] [review]
part 1 - Add PendingPlayerTracker

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

::: dom/animation/PendingPlayerTracker.h
@@ +11,5 @@
> +#include "nsTHashtable.h"
> +
> +namespace mozilla {
> +
> +class PendingPlayerTracker MOZ_FINAL

It seems likely that we'll end up with a set of 'in play' animations too, so maybe this should be named AnimationPlayerTracker?

@@ +21,5 @@
> +  void AddPlayPending(dom::AnimationPlayer& aPlayer);
> +  void RemovePlayPending(dom::AnimationPlayer& aPlayer);
> +  bool IsWaitingToPlay(dom::AnimationPlayer const& aPlayer) const;
> +
> +protected:

No subclasses, so private.

@@ +28,5 @@
> +  // REVIEW: Should we make this an array of nsWeakPtr instead?
> +  //
> +  // When we come to resolve the start times, ideally we should iterate
> +  // over animations in the order they were added so that ready promises get
> +  // resolved in the expected order.

So long as it doesn't add too much complexity I agree that we should do that.

@@ +45,5 @@
> +  // ones that already exist).
> +  //
> +  // We could avoid some uses of RemovePlayPending by storing weak pointers to
> +  // the AnimationPlayers and therefore eliminating the need for players to
> +  // explicitly remove themselves in various cases. However, we'd still need

It's not clear to me whether this depends on whether we have cycles, in which case deletion of the players would be delayed.

::: dom/base/nsDocument.h
@@ +1048,5 @@
>    // If HasAnimationController is true, this is guaranteed to return non-null.
>    nsSMILAnimationController* GetAnimationController() MOZ_OVERRIDE;
>  
> +  virtual mozilla::PendingPlayerTracker*
> +  GetPendingPlayerTracker() MOZ_OVERRIDE

You can put MOZ_FINAL on these too.

Comment 126

[Jonathan Watt [:jwatt]]

Comment on attachment 8536382 [details] [diff] [review]
part 16 - Do a full reflow when we have pending animations

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

::: dom/animation/PendingPlayerTracker.h
@@ +29,5 @@
>    void RemovePlayPending(dom::AnimationPlayer& aPlayer);
>    bool IsWaitingToPlay(dom::AnimationPlayer const& aPlayer) const;
>  
>    void StartPendingPlayers(const TimeStamp& aReadyTime);
> +  bool HasPendingPlayers() const { return !!mPlayPendingSet.Count(); }

> 0

::: layout/base/nsRefreshDriver.h
@@ +325,5 @@
>      return mFrameRequestCallbackDocs.Length() != 0;
>    }
>  
>    void FinishedWaitingForTransaction();
> +  static bool HasPendingAnimations(nsIPresShell* aShell);

If this isn't used in a later patch, it would be better if it was static is the source file rather than a static method.

Comment 127

[Jonathan Watt [:jwatt]]

Comment on attachment 8536384 [details] [diff] [review]
part 18 - Make nsDOMWindowUtils::AdvanceTimeAndRefresh trigger any pending animations first

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +2656,5 @@
>  
>    nsRefreshDriver* driver = GetPresContext()->RefreshDriver();
> +
> +  // Before we advance the time, we should trigger any animations that are
> +  // waiting to start.

Can you file a follow-up bug to consider rewriting the tests, and note here in the comment that bug number and the fact that if it's fixed we won't need this?

Comment 128

[Jonathan Watt [:jwatt]]

Comment on attachment 8536388 [details] [diff] [review]
part 22 - Make AnimationPlayer wait for animations to be rendered before starting

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

::: dom/animation/AnimationPlayer.cpp
@@ +134,5 @@
>  AnimationPlayer::Tick()
>  {
> +  // FIXME: Check if we are pending but have lost access to the pending
> +  // player tracker. If that's the case we should probably trigger the
> +  // animation now.

File bug and note the number here?

@@ +153,4 @@
>  
>    Nullable<TimeDuration> readyTime = mTimeline->GetCurrentTime();
> +
> +  // FIXME: If readyTime.IsNull(), we should return early here. This will

Again, note bug number?

::: dom/animation/AnimationPlayer.h
@@ +91,5 @@
>  
>    // Sets the start time of a player that is waiting to play to the current
>    // time of its timeline.
> +  //
> +  // This will reset the pending flag if the call succeeded but the caller is

I read this incorrectly until I read the code. make it "This will reset the pending flag if the call succeeded. The caller is responsible for removing the player from the PendingPlayerTracker though."

I think this would be a good place to add a nice big comment explaining the whole "pending" concept and how it exists due to the desire to start animations at their first paint (and why that's desirable).

Also change this to a proper Doxygen comment /** ... */

::: layout/style/nsTransitionManager.cpp
@@ +539,5 @@
>    segment.mToKey = 1;
>    segment.mTimingFunction.Init(tf);
>  
>    nsRefPtr<CSSTransitionPlayer> player = new CSSTransitionPlayer(timeline);
> +  player->SetSource(pt);

Note that the order here is significant, and why. That comment can be removed later when it's no longer the case, but I think it's good to have for now.

Comment 129

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

(In reply to Jonathan Watt [:jwatt] (vacation Dec 22 - Jan 5) from comment #125)
> Comment on attachment 8536367 [details] [diff] [review]
> part 1 - Add PendingPlayerTracker
> 
> Review of attachment 8536367 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/PendingPlayerTracker.h
> @@ +11,5 @@
> > +#include "nsTHashtable.h"
> > +
> > +namespace mozilla {
> > +
> > +class PendingPlayerTracker MOZ_FINAL
> 
> It seems likely that we'll end up with a set of 'in play' animations too, so
> maybe this should be named AnimationPlayerTracker?

As discussed on IRC, this is different to the list of players we'll store on the AnimationTimeline. This is just the players that are waiting to start.

> > +  // When we come to resolve the start times, ideally we should iterate
> > +  // over animations in the order they were added so that ready promises get
> > +  // resolved in the expected order.
> 
> So long as it doesn't add too much complexity I agree that we should do that.

As discussed on IRC, this is not easy to do at the moment so long as we are storing in a hashmap.

Actually, I think it might be better if we resolved animations in their priority order. If we store all the animations against the AnimationTimeline, and adopt the approach in bug 1112480 we could do this fairly easily I think.

> ::: dom/base/nsDocument.h
> @@ +1048,5 @@
> >    // If HasAnimationController is true, this is guaranteed to return non-null.
> >    nsSMILAnimationController* GetAnimationController() MOZ_OVERRIDE;
> >  
> > +  virtual mozilla::PendingPlayerTracker*
> > +  GetPendingPlayerTracker() MOZ_OVERRIDE
> 
> You can put MOZ_FINAL on these too.

Comment 130

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

Landed parts 1-7:
https://hg.mozilla.org/integration/mozilla-inbound/rev/323f4574fd31
https://hg.mozilla.org/integration/mozilla-inbound/rev/95eeca2c7e32
https://hg.mozilla.org/integration/mozilla-inbound/rev/df1375d0e714
https://hg.mozilla.org/integration/mozilla-inbound/rev/7441537d3f5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f47c180eb1c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/19407030d97a
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4ce2a592b8

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb9dd540956f

Comment 131

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

(In reply to Jonathan Watt [:jwatt] (vacation Dec 22 - Jan 5) from comment #118)
> Comment on attachment 8536374 [details] [diff] [review]
> part 8 - Fast-forward the timeline before resolving start times
> 
> Can you file a follow-up about considering storing the start time to use but
> and applying it at the next refresh driver tick rather than having this
> fast-forwarding thing? I know you considered it, so it would be useful to
> have your thoughts about that written down, or at least return to consider
> it again at some point.

I filed bug 1112480 for that and hope to try implementing it this week.

Comment 132

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

(In reply to Jonathan Watt [:jwatt] (vacation Dec 22 - Jan 5) from comment #127)
> Comment on attachment 8536384 [details] [diff] [review]
> part 18 - Make nsDOMWindowUtils::AdvanceTimeAndRefresh trigger any pending
> animations first
> 
> Review of attachment 8536384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsDOMWindowUtils.cpp
> @@ +2656,5 @@
> >  
> >    nsRefreshDriver* driver = GetPresContext()->RefreshDriver();
> > +
> > +  // Before we advance the time, we should trigger any animations that are
> > +  // waiting to start.
> 
> Can you file a follow-up bug to consider rewriting the tests, and note here
> in the comment that bug number and the fact that if it's fixed we won't need
> this?

Filed bug 1112957 for this.

Comment 133

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

Attachment: part 25 - Test cancelling of players

Fix bitrot and fix a bug

Comment 134

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

Attachment: part 27 - Make DevTools actor wait on ready promise when playing

Hi Patrick, does this make any sense?

Comment 135

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

(In reply to Jonathan Watt [:jwatt] (vacation Dec 22 - Jan 5) from comment #123)
> Comment on attachment 8536376 [details] [diff] [review]
> part 10 - Record the time when animations are ready to start
> 
> Review of attachment 8536376 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicLayerManager.cpp
> @@ +545,5 @@
> >      }
> >    }
> >  
> > +  if (mRoot) {
> > +    mAnimationReadyTime = TimeStamp::Now();
> 
> mattwoodrow noted that for BasicLayerManager you should maybe only do this
> if |aFlags & END_NO_COMPOSITE| since otherwise we'll update it when we
> composite too.

This didn't seem to work for me. If I do that, sometimes we would not set mAnimationReadyTime at all and another assertion would fail. Can you explain what we should be doing here?

Comment 136

[Cameron McCormack (:heycam)]

Comment on attachment 8536995 [details] [diff] [review]
part 24 - Add a test that empty animations still start

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

::: dom/animation/test/mozilla/test_deferred_start.html
@@ +35,5 @@
> +  // Before we start, wait for the document to finish loading. This is because
> +  // during loading we will have other paint events taking place which might,
> +  // by luck, happen to trigger animations that otherwise would not have been
> +  // triggered, leading to false positives.
> +	//

Stray tab.

Comment 137

[Cameron McCormack (:heycam)]

Comment on attachment 8538240 [details] [diff] [review]
part 25 - Test cancelling of players

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

Might be worth adding some tests that animations are cancelled if you set display:none on the element (assuming that's what's meant to happen).

::: dom/animation/test/css-animations/test_animation-player-ready.html
@@ +75,5 @@
>    });
>  }, 'The ready promise is fulfilled with its AnimationPlayer');
>  
> +async_test(function(t) {
> +  var div = addDiv();

Pass in t.

@@ +102,5 @@
> +}, 'ready promise is rejected when an animation is cancelled by resetting'
> +   + ' the animation property');
> +
> +async_test(function(t) {
> +  var div = addDiv();

Pass in t.

Comment 138

[Matt Woodrow (:mattwoodrow)]

(In reply to Brian Birtles (:birtles) from comment #135)
> 
> This didn't seem to work for me. If I do that, sometimes we would not set
> mAnimationReadyTime at all and another assertion would fail. Can you explain
> what we should be doing here?

I would have expected that to work, we pass END_NO_COMPOSITE when drawing layers, but not we composite (which happens with the same layer manager).

It probably doesn't matter much though, updating mAnimationReadyTime again during composition won't break anything I don't think.

Comment 139

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

(In reply to Brian Birtles (:birtles) from comment #134)
> Created attachment 8538246 [details] [diff] [review]
> part 27 - Make DevTools actor wait on ready promise when playing
> 
> Hi Patrick, does this make any sense?

Hmm, this still doesn't work. Maybe passing the ready promise back like that is a bit naive?

Comment 140

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

Attachment: part 27 - Make DevTools actor wait on ready promise when playing

So that was pretty naive of me to think I could just pass a Promise across an
RDP boundary like that.

I think the way we ultimately want to solve this is by having the observer API
notify about changes in playState then we can wait on that.

For now, could we add a method waitUntilReady on the AnimationPlayerActor and
call that? Or should we just use do a requestAnimationFrame loop in
browser_animation_actors_04.js until the playState says running?

Currently I've just done the simplest thing imagineable which is to test for
*either* "pending" or "running". Even if we're pending, we will probably be
in the finished state after 1.5s, and if not, we can extend that timeout.

Comment 141

[Patrick Brosset <:pbro>]

Comment on attachment 8538341 [details] [diff] [review]
part 27 - Make DevTools actor wait on ready promise when playing

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

::: toolkit/devtools/server/actors/animation.js
@@ +171,5 @@
>     * Play the player.
>     */
>    play: method(function() {
>      this.player.play();
> +    return this.player.ready;

This makes sense to me. And protocol.js (the rdp protocol framework we use) accepts promises to be returned from protocol methods.

::: toolkit/devtools/server/tests/browser/browser_animation_actors_04.js
@@ +37,5 @@
>    cpow.classList.add("short-animation");
>  
>    let [player] = yield front.getAnimationPlayersForNode(node);
>  
> +  yield player.ready;

As discussed on IRC, |player| here isn't an AnimationPlayer object (as per the waapi spec), but a devtools AnimationPlayerFront object, and |ready| just doesn't exist on this object.

I'll apply the patches in this bug that haven't landed yet and will try and provide a patch for this. I think this will require an event to be sent by the actor, and a promise to be exposed by the front, so some detailed knowledge of how the devtools protocol works is required.

@@ +39,5 @@
>    let [player] = yield front.getAnimationPlayersForNode(node);
>  
> +  yield player.ready;
> +  ok(player.initialState.playState == "running" ||
> +     player.initialState.playState == "pending",

If we manage to wait until the player's ready promise resolve, can the playState be "pending" here still?

Comment 142

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #141)
> @@ +39,5 @@
> >    let [player] = yield front.getAnimationPlayersForNode(node);
> >  
> > +  yield player.ready;
> > +  ok(player.initialState.playState == "running" ||
> > +     player.initialState.playState == "pending",
> 
> If we manage to wait until the player's ready promise resolve, can the
> playState be "pending" here still?

No, it shouldn't be (I guess it's technically possible if some other task occurred in between resolving the promise and calling its resolve method but that seems unlikely).

Comment 143

[Patrick Brosset <:pbro>]

Comment on attachment 8538341 [details] [diff] [review]
part 27 - Make DevTools actor wait on ready promise when playing

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

::: toolkit/devtools/server/tests/browser/browser_animation_actors_04.js
@@ +39,5 @@
>    let [player] = yield front.getAnimationPlayersForNode(node);
>  
> +  yield player.ready;
> +  ok(player.initialState.playState == "running" ||
> +     player.initialState.playState == "pending",

As discussed on IRC, no sense blocking this patch on some devtools test failing, especially that this test is for a new animation devtools actor that isn't used yet and that depends on the platform API to be ready!

So I'm fine with adding a check for pending here, as long as you add a quick comment in the test saying why it's here. Also, you should remove line 41.

Comment 144

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/323f4574fd31
https://hg.mozilla.org/mozilla-central/rev/95eeca2c7e32
https://hg.mozilla.org/mozilla-central/rev/df1375d0e714
https://hg.mozilla.org/mozilla-central/rev/7441537d3f5b
https://hg.mozilla.org/mozilla-central/rev/f47c180eb1c3
https://hg.mozilla.org/mozilla-central/rev/19407030d97a
https://hg.mozilla.org/mozilla-central/rev/1e4ce2a592b8

Comment 145

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

I was *so* close to landing this but browser/components/places/tests/browser/browser_555547.js is failing intermittently.

Looks like a flaky test that mostly gets lucky (but not always[1]) but now that animations take longer to start, is less lucky.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=705682a40b27

[1] https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/899016

Comment 146

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

Oh, and this assertion is failing again:

  ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle()'

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-705682a40b27/try-win32-debug/try_win7-ix-debug_test-mochitest-5-bm112-tests1-windows-build2155.txt.gz

Comment 147

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

(In reply to Cameron McCormack (:heycam) from comment #137)
> Might be worth adding some tests that animations are cancelled if you set
> display:none on the element (assuming that's what's meant to happen).

It is, per spec, but we don't implement that yet. That is bug 822092 and bug 984850.

Comment 148

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

Attachment: Test for display:none

Here's a patch for testing that the ready promise gets cancelled when we set display:none. Unfortunately I don't think we have a good way of marking individual testharness.js tests as failing when they are outside the web-platform-tests area so I'm just going to leave this here for now.

Comment 149

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

Parts 8-21:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7da8aa9ec74
https://hg.mozilla.org/integration/mozilla-inbound/rev/f77e3d03e7e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/7df2255ce1e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b9b9370d2e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f40d9ed7a3df
https://hg.mozilla.org/integration/mozilla-inbound/rev/0743f344f853
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff976d369ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/da5d7b9bff8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6710df16f1e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d4ffa8d49ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/38cb27932c18
https://hg.mozilla.org/integration/mozilla-inbound/rev/41f0f3899b96
https://hg.mozilla.org/integration/mozilla-inbound/rev/73316b8d04a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7aaf7025e59

Try run of the same:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=15db2c988946

Comment 150

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7da8aa9ec74
https://hg.mozilla.org/mozilla-central/rev/f77e3d03e7e3
https://hg.mozilla.org/mozilla-central/rev/7df2255ce1e2
https://hg.mozilla.org/mozilla-central/rev/4b9b9370d2e1
https://hg.mozilla.org/mozilla-central/rev/f40d9ed7a3df
https://hg.mozilla.org/mozilla-central/rev/0743f344f853
https://hg.mozilla.org/mozilla-central/rev/9ff976d369ff
https://hg.mozilla.org/mozilla-central/rev/da5d7b9bff8f
https://hg.mozilla.org/mozilla-central/rev/6710df16f1e8
https://hg.mozilla.org/mozilla-central/rev/7d4ffa8d49ca
https://hg.mozilla.org/mozilla-central/rev/38cb27932c18
https://hg.mozilla.org/mozilla-central/rev/41f0f3899b96
https://hg.mozilla.org/mozilla-central/rev/73316b8d04a0
https://hg.mozilla.org/mozilla-central/rev/c7aaf7025e59
https://hg.mozilla.org/mozilla-central/rev/6ec55060ca19

Comment 151

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

(In reply to Brian Birtles (:birtles) from comment #146)
> Oh, and this assertion is failing again:
> 
>   ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange &
> nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() ||
> aFrame->StyleDisplay()->HasTransformStyle()'
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.
> com-705682a40b27/try-win32-debug/try_win7-ix-debug_test-mochitest-5-bm112-
> tests1-windows-build2155.txt.gz

I can trigger this assertion simply by opening the hamburger menu in an opt build a few times.

It seems what happens is this:

1. With these patches, we end up calling AnimationPlayer::PostUpdate at the end of painting. This marks the document as needing an animation restyle and also updates the animation generation so we will know to update layers.
2. On the next refresh driver tick we flush transitions. This happens in nsTransitionManager::WillRefresh.
3. Whilst flushing transitions we find a transition that has now finished. We mark if as finished but we keep it around until the next style change for reasons documented in nsTransitionManager.cpp
4. We also queue a transitionend event.
5. At the end of nsTransitionManager::FlushTransitions we dispatch the events.
6. The event dispatch triggers a style change. (I haven't dug down deep enough to see exactly where this gets triggered but I think it's somewhere inside EventTargetChainItem::HandleEventTargetChain.) This, incidentally, causes FlushTransitions to be called recursively.
7. We compute style changes and in ElementRestyle::Restyle we call AddLayerChangesForAnimation()
8. There we notice that we have a transform layer and its animation generation lags behind the frame's animation generation.
9. We check that the frame does actually have transform style set, which it does, so we add an UpdateTransformLayer change hint.
10. Then, back in ElementRestyle::Restyle, we go on to call RestyleSelf which actually applies the end-of-transition animation value removing the transform style and updating the frame.
11. At the end of ComputeAndProcessStyleChange we call ProcessRestyledFrame and, in turn, ApplyRenderingChangeToTree.
12. In ApplyRenderingChangeToTree we notice we have an UpdateTransformLayer hint but the frame isn't transformed and doesn't have transform style. And we assert.

I plan to change when we call PostUpdate in bug 1112480 but this situation could still happen if script called Play/Pause at such a time.

It seems like we should be adding the layer update hints after restyling self. I'm trying a patch that does that now:

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

Comment 152

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

Attachment: part 28 - Call AddLayerChangesForAnimation after updating style

The background for this patch is in comment 151

Comment 153

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

Attachment: part 29 - Disable failing browser_555547.js on Windows opt

As discussed on IRC, I'm skipping browser_555547.js on Windows opt for now.
I filed bug 1115276 to follow-up on this.

Also, I'm skipping test_deferred_start.html for Mulet too since it is
intermittent too (but I hadn't noticed before because I just got lucky).

If I don't hear from you I might just go ahead and set r=me since we already
discussed the first change on IRC. For the second one, there are a bunch of
animation tests that fail on Mulet which should all be investigated together.

Comment 154

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

Attachment: part 30 - Cancel transitions when we have a change to a non-animatable style

While skimming the source to track down the assertion in comment 151 I noticed
one place where we were failing to cancel transitions. I wrote a test to confirm
that this is indeed a bug then fixed it.

Comment 155

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

Comment on attachment 8541103 [details] [diff] [review]
part 28 - Call AddLayerChangesForAnimation after updating style

Adding heycam as reviewer just in case you feel like it!

Comment 156

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8541103 [details] [diff] [review]
part 28 - Call AddLayerChangesForAnimation after updating style

>+    // Some changes to animations don't affect the computed style and yet still
>+    // require the layer to be updated. For example, pausing an animation via
>+    // the Web Animations API won't affect an element's style but still
>+    // requires us to pull the animation off the layer.
>+    //
>+    // This needs to happen *after* calling RestyleSelf since
>+    // AddLayerChangesForAnimation checks if mFrame has transform style or
>+    // not.
>+    //
>+    // We don't need to do this for continuations, however, since they share
>+    // the same layer.

I'm not convinced you've explained why this is needed.  Other codepaths should handle the case of whether the frame has transform style having changed; this codepath is only for the case of the computed style not changing, and thus it shouldn't matter whether it's before or after RestyleSelf.

That said, I don't particularly mind it moving, so I'm ok with it if you need it for something but can't explain why.  But it would be better to have a correct comment if you can write one.

>+    if (f == mFrame) {
>+      AddLayerChangesForAnimation();
>+    }

Instead of having this inside the loop with an f == mFrame test, why not just put it after the loop?

r=dbaron with that

Comment 157

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

(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #156)
> Comment on attachment 8541103 [details] [diff] [review]
> part 28 - Call AddLayerChangesForAnimation after updating style
> 
> >+    // Some changes to animations don't affect the computed style and yet still
> >+    // require the layer to be updated. For example, pausing an animation via
> >+    // the Web Animations API won't affect an element's style but still
> >+    // requires us to pull the animation off the layer.
> >+    //
> >+    // This needs to happen *after* calling RestyleSelf since
> >+    // AddLayerChangesForAnimation checks if mFrame has transform style or
> >+    // not.
> >+    //
> >+    // We don't need to do this for continuations, however, since they share
> >+    // the same layer.
> 
> I'm not convinced you've explained why this is needed.  Other codepaths
> should handle the case of whether the frame has transform style having
> changed; this codepath is only for the case of the computed style not
> changing, and thus it shouldn't matter whether it's before or after
> RestyleSelf.

Thanks for the review!

I've stepped through with a debugger and for the case where the assertion fails I have verified that before RestyleSelf we have a transform style on the frame and after RestyleSelf we have no transform style. Perhaps this shouldn't be happening, but it does seem to be.

It's probably worth mentioning that this is happening on the final sample of a transition that has now been marked as "finished". We are also doing a nested call to FlushTransitions. I'm not sure if some invariants are being broken because of either of these factors.

> That said, I don't particularly mind it moving, so I'm ok with it if you
> need it for something but can't explain why.  But it would be better to have
> a correct comment if you can write one.

I'll update that comment to add some uncertainty that we're don't normally expect this to happen.

> >+    if (f == mFrame) {
> >+      AddLayerChangesForAnimation();
> >+    }
> 
> Instead of having this inside the loop with an f == mFrame test, why not
> just put it after the loop?

Will do.

Thanks David!

Comment 158

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

Parts 22 to 29:
https://hg.mozilla.org/integration/mozilla-inbound/rev/477f46897b0b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c345954bce5
https://hg.mozilla.org/integration/mozilla-inbound/rev/642e400b22a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bdf7c2279fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/79cac8c71159
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe757e478d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf834051cbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ab2ff9b9f94

I've run these through try several times including this run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08896f032b07

I did another run with the review feedback from above incorporated just to check it didn't create any new failures and so far it looks good so I decided to push while the try is quiet:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9a15fa4d25e

There's still a good chance this will cause trouble somewhere. I'll watch the tree as I can but I'll also upload a roll-up patch of parts 22 to 29 in case that helps with backing out, should that be needed.

This set of patches enables the new behavior: starting animations after their first frame has rendered. Part 30 can land later.

Comment 159

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

Attachment: Roll-up patch of parts 22-29

Comment 160

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/477f46897b0b
https://hg.mozilla.org/mozilla-central/rev/6c345954bce5
https://hg.mozilla.org/mozilla-central/rev/642e400b22a9
https://hg.mozilla.org/mozilla-central/rev/9bdf7c2279fa
https://hg.mozilla.org/mozilla-central/rev/79cac8c71159
https://hg.mozilla.org/mozilla-central/rev/cfe757e478d5
https://hg.mozilla.org/mozilla-central/rev/eaf834051cbd
https://hg.mozilla.org/mozilla-central/rev/3ab2ff9b9f94

Comment 161

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

Oh, that's embarrassing. I was so afraid of this bug causing problems that I started preparing a backout patch before I pushed. However, I accidentally applied the backout patch to my working directory before pushing! So all last night while I was watching m-i with bated breath I was watching a no-op push!

Here is a follow-up push that reverts the mistaken backout that got caught up in the push of part 29 from before:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa2358e8919

Comment 162

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

And backed out again because css-animations/test_animation-pausing.html fails intermittently on 10.8 opt.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3470cdf1cfa8

Comment 163

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

(In reply to Brian Birtles (:birtles) from comment #162)
> And backed out again because css-animations/test_animation-pausing.html
> fails intermittently on 10.8 opt.

I'm really not sure why this is failing. I can't reproduce it locally and its going to take me a while to debug over tryserver. On the other hand, the latest patch for bug 1112480 seems to fix this (see: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3b637702308) so I may just finish up that patch early in January and push the rest of this bug and that bug together and hope it all sticks before the next uplift.

Comment 164

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

I think it would be reasonable to land this and just disable test_animation-pausing.html temporarily since it's only testing functionality that is not shipping yet (the Web Animations API) and it's known to be fixed by bug 1112480.

I tried pushing this over the new year but it seemed like this was causing a number of intermittent tests to fail more frequently. Going over these this morning, however, I'm not sure that's the case:

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

I'll do another try run and if everything looks good, go ahead and push this.

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

Comment 165

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

(In reply to Brian Birtles (:birtles) from comment #164)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=599229a2a984

All looks good. If only the trees weren't closed all day :)

Comment 166

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

Trying again (fingers crossed):
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a4a22732aed
https://hg.mozilla.org/integration/mozilla-inbound/rev/37343347ab6a
https://hg.mozilla.org/integration/mozilla-inbound/rev/413db6659fef
https://hg.mozilla.org/integration/mozilla-inbound/rev/48523f909ffb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f50d69bc63f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d6a57b25ad0
https://hg.mozilla.org/integration/mozilla-inbound/rev/650223edc2b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7ac60fb354

Comment 167

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

This caused a pretty significant regression in the numbers for Tab Animation Test and Customization Animation Test. I'm not sure if this is an actual regression in terms of perceived performance, however. I've filed bug 1118034 for now to work out what's going on but I may need to back this out again if it's a real performance issue.

Comment 168

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

https://hg.mozilla.org/mozilla-central/rev/6a4a22732aed
https://hg.mozilla.org/mozilla-central/rev/37343347ab6a
https://hg.mozilla.org/mozilla-central/rev/413db6659fef
https://hg.mozilla.org/mozilla-central/rev/48523f909ffb
https://hg.mozilla.org/mozilla-central/rev/0f50d69bc63f
https://hg.mozilla.org/mozilla-central/rev/6d6a57b25ad0
https://hg.mozilla.org/mozilla-central/rev/650223edc2b4
https://hg.mozilla.org/mozilla-central/rev/7d7ac60fb354

Comment 169

[Chris Lord [:cwiiis]]

(In reply to Brian Birtles (:birtles) from comment #167)
> This caused a pretty significant regression in the numbers for Tab Animation
> Test and Customization Animation Test. I'm not sure if this is an actual
> regression in terms of perceived performance, however. I've filed bug
> 1118034 for now to work out what's going on but I may need to back this out
> again if it's a real performance issue.

We should definitely err on the side of leaving it in - I'd put money on it not being a real regression (likely the animation takes longer because it actually plays all the frames, or something along these lines), but we don't measure the very noticeable win that this patch is by allowing animations to actually play instead of skipping the start of them.

Comment 170

[Cameron McCormack (:heycam)]

Comment on attachment 8541108 [details] [diff] [review]
part 30 - Cancel transitions when we have a change to a non-animatable style

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

Stealing; looks good.

Comment 171

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

Part 30:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2c5a67125d6

Comment 172

[Chris Lord [:cwiiis]]

A thought: How easy would it be to put this behaviour behind a pref?

Rationale: This is something we desperately, desperately need on mobile - we need it on desktop too, but the problem is much less pronounced there. If synthetic performance numbers were going to drive us to back this out, could we instead put it behind a pref so that we can leave it enabled on b2g and Android? (much like we use tiles, progressive rendering, OMTC and APZC on mobile (and have been for a long time))

To be clear, I think this should be on, everywhere, regardless of what synthetic performance numbers say (unless those numbers are like 10x worse and are reflected in real-world situations) - but being pragmatic, if we had to turn this off temporarily on desktop, we should find a way to leave it on for mobile, where the difference it makes is truly night and day.

Comment 173

[Ryan VanderMeulen [:RyanVM]]

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

Comment 174

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

(In reply to Chris Lord [:cwiiis] from comment #172)
> A thought: How easy would it be to put this behaviour behind a pref?

It would be possible. We'd basically just short-circuit the step where we stick the animation in the pending player map and start it straight away instead.

The main complication is that we have tests that now expect this delayed behavior so we'd have to do a lot of conditional checks on those. It would also be a trap for people using the Web Animations API since on some platforms animations would start immediately and on others they would start asynchronously but as long as this is only a short-term measure that would probably be fine.

> Rationale: This is something we desperately, desperately need on mobile - we
> need it on desktop too, but the problem is much less pronounced there. If
> synthetic performance numbers were going to drive us to back this out, could
> we instead put it behind a pref so that we can leave it enabled on b2g and
> Android? (much like we use tiles, progressive rendering, OMTC and APZC on
> mobile (and have been for a long time))

In bug 1118034 we already resolved that the talos regression was expected and acceptable. This should be in the most recent nightly now so we'll see what feedback arises from that. If problems arise then I think your suggestion of temporarily putting this behind a pref is a good one.