Provide a way for changes to AnimationPlayer / Animation objects to update style

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(19 attachments, 9 obsolete attachments)

12.48 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.09 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.91 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
4.25 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.32 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.82 KB, patch
dbaron
: review+
bzbarsky
: feedback+
Details | Diff | Splinter Review
2.84 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.10 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.35 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.30 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.91 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.60 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.08 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.32 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.99 KB, patch
dbaron
: feedback+
Details | Diff | Splinter Review
1.88 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.42 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
11.18 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
9.40 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
For a lot of the work going on for script-driven playback control of animations (bug 1070744) and all future work surrounding dynamic modification of animations from script we need a way to trigger style updates.

Scenarios:

A. Change to AnimationPlayer.currentTime
- Needs to trigger a style update without queuing events
   (Events should be queued when we actually do the style flush so we coalesce events. See bug 1072037 comment 3.)
- Needs to ensure style gets updated even if we are paused
- Needs to ensure we start watching the refresh driver again if we had previously reached the end
*** Needs to be able to do all this even if we don't have source content / target element since we still need to dispatch events
--> Suggests an animation player needs to know its manager or its collection since a collection has a pointer to its manager

B. Change to AnimationPlayer pause state (e.g. by calling play())
- Requirements are roughly the same as A.

C. Change to Animation.timing.duration
- Requirements are roughly the same as A except that the context is the Animation object.
--> Suggests we need a way to go from Animation to either its player / collection / manager
    We can probably get to the manager through the target element but we won't always have one and we still need to trigger events when we don't have a target element.
    However, currently the mNeedsRefreshes flag--which needs to be updated--is stored on the collection, so it would be better to tell the collection and have it tell its manager.
    Furthermore, if an animation is *not* attached to a player, then it has no effect (its inherited time is null), so there's no need to update. So, alternatively, we could just look up the animation's player, tell it, and let it tell its collection.

D. Change to Animation.effect.setKeyframes
- Needs to trigger a style update (events shouldn't change)
- Unlike C, however, we don't need to trigger any sampling here.
- Also, this is happening inside the effect, so either we need a pointer from the effect back to its Animation, or perhaps some out-of-band "do full animation update" mechanism.

E. Change to Animation.target
- This is going to cause the player to be moved from one collection to another which suggests an Animation needs to know its player
(When/if we support timing groups this whole architecture will need to change but that's a long way off.)

F. Finally starting/pausing an animation after async waiting for setup to complete
- Needs to trigger a style update and probably *should* cause events to be queued immediately. (i.e. it shouldn't be possible to perform an action that causes the event to be coalesced)


Approach 1: Discrete messaging

We could just add pointers between the objects so that it is possible to go

  AnimationEffect -> Animation -> Player -> Collection -> Manager

Then, animations tell their players things like:
* UpdatedTiming
* UpdatedEffect
* UpdatedTarget

This could be a straight-forward callback, e.g. Notify(message).

Then players tell their collection:
* NotifyPlayerUpdated(<dispatch events or not>)

When collections get this, they:
* Set mNeedsRefresh to true (otherwise EnsureStyleRuleFor is basically a no-op)
* Likewise set mStyleRuleRefreshTime to null
* Call EnsureStyleRuleFor
  Pass the latest refresh driver time as the current time
  Set aFlags to EnsureStyleRule_IsNotThrottled
* Call the player telling it to sample
  (See below. Basically we should change the behavior of EnsureStyleRule to delegate more to the player. Players would take a reference to an nsRefPtr<AnimValuesStyleRule> and create / add to it by, in turn, passing it to their animation which pass it to their effect.)
* Optionally, tell the player to queue events
* Tell their manager to CheckNeedsRefresh


Approach 2. Some sort of global thing...

Is there some global object we can get a hold of and use to get to our animation manager then just call ForceAnimationUpdate which basically resamples the world ignoring stuff like mNeedsRefresh, mStyleRuleRefreshTime etc.


Other approaches?


Somewhat orthogonal to this is the fact that we really need to rework these animation objects for a few reasons:

* The current break down of animation manager / collection / player / animation has weak encapsulation and it's getting harder to understand and manage.

* In the near future we want to support script-generated animations even to the point where the list of players returned by elem.getAnimationPlayers() may have script-generated animations and CSS Animations intermingling (CSS Transitions will always appear at the start of the list I believe).

  This suggests pushing CSS-specific behavior further down the chain (e.g. into CSS-specific subclasses of AnimationPlayer) and making the manager objects more generic (perhaps even combining nsAnimationManager and nsTransitionManager and farming off the creation / updating of players to helper objects called by a common manager?)

I think the first parts of this bug should probably be refactoring these objects for better encapsulation. The work to merge nsAnimationManager and nsTransitionManager further can happen later.
(Assignee)

Updated

4 years ago
Blocks: 1073379
(Assignee)

Updated

4 years ago
Blocks: 927349
(Assignee)

Updated

4 years ago
No longer blocks: 1070745
Depends on: 1070745
(Assignee)

Updated

4 years ago
Depends on: 1078122
(Assignee)

Comment 1

4 years ago
Created attachment 8502985 [details] [diff] [review]
part 1 - Move CheckNeedsRefreshes to CommonAnimationManager

In order to add AnimationPlayerCollection::NotifyPlayerUpdated, collections
need a way of updating their managers to inform them that their mNeedsRefreshes
flag has changed and hence the manager may need to resume observing the refresh
driver.

Currently, only nsAnimationManager makes use of mNeedsRefreshes and provides
a CheckNeedsRefresh method. In order to allow AnimationPlayerCollection to
operate independently of the type of manager it is attached to (and because
there's a lot of similar code here that we eventually want to move to a common
manager anyway), this patch moves CheckNeedsRefreshes and associated
machinery to CommonAnimationManager.
Attachment #8502985 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Assignee: nobody → birtles
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Created attachment 8502986 [details] [diff] [review]
part 2 - Call CheckNeedsRefresh from within EnsureStyleRuleFor

Now that CheckNeedsRefresh is a member of the base class,
CommonAnimationManager, we no longer need to rely on callers of
AnimationPlayerCollection::EnsureStyleRuleFor to remember to call this method
but can do it automatically.
Attachment #8502986 - Flags: review?(dbaron)
(Assignee)

Comment 3

4 years ago
Created attachment 8502987 [details] [diff] [review]
part 3 - Add mInStyleRecalc member

When an AnimationPlayer is updated, it will inform its AnimationPlayerCollection
and which will then inform the manager. When it reaches the manager we need to
distinguish why the change came about.

For example, a call to AnimationPlayer::Play() or
AnimationPlayer::SetCurrentTime() may have come as part of creating/updating the
animation due to style processing. Alternatively, it might come about as
a result of a call from script.

For calls that come from script we should mark the document as needing a style
flush. However, for calls that result from style processing the extra style
flush is not needed.

We could have different entry points for these (we already have, for example,
PlayFromStyle and PlayFromJS) and then pass different flags to the manager but
it's clumsy to pass these flags from player -> collection -> manager (and, in
future, from animation -> player as well). Rather, this patch simply adds a bool
to CommonAnimationManager, mInStyleRecalc, that we set while we are making
changes to animation objects that should not cause us to mark the document as
needing a style flush.

REVIEW: Is there a better way of detecting this? We used to have
IsProcessingRestyles() but that's no longer available.
Attachment #8502987 - Flags: review?(dbaron)
(Assignee)

Comment 4

4 years ago
Created attachment 8502988 [details] [diff] [review]
part 4 - Add CommonAnimationManager::CollectionUpdated

Adds a method to the animation manager base class to handle changes to one of
its associated collections.
Attachment #8502988 - Flags: review?(dbaron)
(Assignee)

Comment 5

4 years ago
Created attachment 8502989 [details] [diff] [review]
part 5 - Add AnimationPlayerCollection::PlayerUpdated
Attachment #8502989 - Flags: review?(dbaron)
(Assignee)

Comment 6

4 years ago
Created attachment 8502990 [details] [diff] [review]
part 6 - Add CSSTransitionPlayer

In order to be able to find the collection a player belongs to from its source
content, we first need to be able to determine which manager--the animation
manager or transition manager--to look up.

We eventually plan to push transition event dispatch down to a CSS
transitions-specific subclass of AnimationPlayer, so this seems like a suitable
point to introduce this class.

Using this subclass we can define a virtual GetManager method that will
return the appropriate animation/transition manager for the player.
Attachment #8502990 - Flags: review?(dbaron)
(Assignee)

Comment 7

4 years ago
Created attachment 8502991 [details] [diff] [review]
part 7 - Move style flushing to CSSAnimationPlayer and CSSTransitionPlayer

Previously AnimationPlayer::Play() and AnimationPlayer::PlayState() would flush
styles as part of their operation. This, however, is only needed when the player
corresponds to a CSS Animation or CSS Transition. Now that we have concrete
subclasses for each of these cases we can move style flushing to the subclasses
and remove it from the base class (which is expected to be shared with
animations that are not dependent on style).
Attachment #8502991 - Flags: review?(dbaron)
(Assignee)

Comment 8

4 years ago
Created attachment 8502992 [details] [diff] [review]
part 8 - Add protected AnimationPlayer::GetDocument()

In order for AnimationPlayer objects to be able to notify their
collection/manager, the can either store an extra pointer member, or they can
navigate to the collection as follows:

  player->source(animation)->target(element)->document
    ->presShell->presContext->manager->collection

This patch adds a getter for the first part of this journey up to the document.
Attachment #8502992 - Flags: review?(dbaron)
(Assignee)

Comment 9

4 years ago
Created attachment 8502993 [details] [diff] [review]
part 9 - Add protected AnimationPlayer::GetPresContext()

This patch adds a further getter to find the pres context associated with an
animation player's target element, if any.
Attachment #8502993 - Flags: review?(dbaron)
(Assignee)

Comment 10

4 years ago
Created attachment 8502994 [details] [diff] [review]
part 10 - Add AnimationPlayer::GetAnimationManager()

This patch introduces an abstract method to AnimationPlayer to fetch the manager
object associated with the player. This method is implemented separate by
CSSAnimationPlayer and CSSTransitionPlayer to return the nsAnimationManager or
nsTransitionManager accordingly.
Attachment #8502994 - Flags: review?(dbaron)
(Assignee)

Comment 11

4 years ago
Created attachment 8502995 [details] [diff] [review]
part 11 - Move GetAnimationPlayers to base CommonAnimationManager class

nsAnimationManager provides GetAnimationPlayers while nsTransitionManager
provides GetElementTransitions. Both perform the same function, namely, fetching
(and optionally creating if it does not exist) the AnimationPlayerCollection for
the specified element/pseudo. Furthermore, both take the same arguments.

This patch aligns the method names and makes this a virtual method on the base
class CommonAnimationManager so that it can be used generically from a pointer
to a CommonAnimationManager.
Attachment #8502995 - Flags: review?(dbaron)
(Assignee)

Comment 12

4 years ago
Created attachment 8502996 [details] [diff] [review]
part 12 - Add AnimationPlayer::GetCollection()

This patch adds a method to animation players that looks up the
AnimationPlayerCollection to which the player belongs.
Attachment #8502996 - Flags: review?(dbaron)
(Assignee)

Comment 13

4 years ago
Created attachment 8502997 [details] [diff] [review]
part 13 - Add AnimationPlayer::PostUpdate

Adds a method for notifying the collection of changes to one of its players.
Attachment #8502997 - Flags: review?(dbaron)
(Assignee)

Comment 14

4 years ago
Created attachment 8502999 [details] [diff] [review]
part 14 - Add a method to post restyle for layer changes

Adds a method that looks for possible changes to layers and posts the relevant
style update so that animations can be added to the next layer transaction.
Attachment #8502999 - Flags: review?(dbaron)
(Assignee)

Comment 15

4 years ago
Created attachment 8503000 [details] [diff] [review]
part 15 - Switch AnimationPlayer to using less aggressive update mechanism
Attachment #8503000 - Flags: review?(dbaron)
Comment on attachment 8502985 [details] [diff] [review]
part 1 - Move CheckNeedsRefreshes to CommonAnimationManager

I suspect this might cause us to leave a finished transition in the transition manager's list longer than needed.  Did you check that completed transitions still get removed from the transition manager's list one refresh cycle after completing?  If you did and that still happens reliably, then r=dbaron.
Attachment #8502985 - Flags: review?(dbaron) → review+
Attachment #8502986 - Flags: review?(dbaron) → review+
Comment on attachment 8502988 [details] [diff] [review]
part 4 - Add CommonAnimationManager::CollectionUpdated

>+void
>+CommonAnimationManager::NotifyCollectionUpdated()
>+{
>+  CheckNeedsRefresh();
>+  if (!mInStyleRecalc) {
>+    mPresContext->Document()->SetNeedStyleFlush();
>+  }
>+}

It's not clear to me what makes this sufficient. 

It won't cause this check in PresShell::FlushPendingNotifications:
      if (aFlush.mFlushAnimations &&
          !mPresContext->StyleUpdateForAllAnimationsIsUpToDate()) {
to be true, and it also won't put any pending restyles in the queue.

Maybe I'm missing something, though, but it seems like there's a risk that the changes here won't go into effect until the next refresh cycle, which seems wrong.  (Please re-request review if I am.)

>+  // Notify this manager that one of its collections of animation players,
>+  // has been updated.
>+  void NotifyCollectionUpdated();

no comma
Attachment #8502988 - Flags: review?(dbaron) → review-
Comment on attachment 8502987 [details] [diff] [review]
part 3 - Add mInStyleRecalc member

I don't see why this is needed.  How do we call into NotifyCollectionUpdated when mInStyleRecalc is true?

(And, if we do, would it make more sense to pass that explicitly rather than maintain this state?)

Feel free to re-request with an answer.
Attachment #8502987 - Flags: review?(dbaron) → review-
Comment on attachment 8502989 [details] [diff] [review]
part 5 - Add AnimationPlayerCollection::PlayerUpdated

>+  // On the next sample, force us to update the style rule

Seems like this should be the next flush rather than the next refresh driver tick, no?  (Per my comment on patch 4.)

Otherwise this seems fine.
Attachment #8502989 - Flags: review?(dbaron) → review+
Comment on attachment 8502990 [details] [diff] [review]
part 6 - Add CSSTransitionPlayer

Seems like the patch this goes on top of hasn't landed yet (CSSAnimationPlayer isn't in the tree), so this is a little confusing.  but r=dbaron
Attachment #8502990 - Flags: review?(dbaron) → review+
Attachment #8502991 - Flags: review?(dbaron) → review+
Comment on attachment 8502992 [details] [diff] [review]
part 8 - Add protected AnimationPlayer::GetDocument()

r=dbaron, although I wonder if we want it to be called GetDocument or GetComposedDoc.  Do we, in general, want to expose that naming distinction as widely as possible, or just cover it up in places where we always want one in particular?  feedback? to bz if he has an opinion
Attachment #8502992 - Flags: review?(dbaron)
Attachment #8502992 - Flags: review+
Attachment #8502992 - Flags: feedback?(bzbarsky)
Attachment #8502993 - Flags: review?(dbaron) → review+
Comment on attachment 8502994 [details] [diff] [review]
part 10 - Add AnimationPlayer::GetAnimationManager()

I wonder if you want the base class method to be pure virtual or returning null by default.  Your comments about wanting AnimationPlayer for other types of animations make me suspect perhaps the latter, but I'm not sure.  Either way is fine with me for now.
Attachment #8502994 - Flags: review?(dbaron) → review+
Attachment #8502995 - Flags: review?(dbaron) → review+
Attachment #8502996 - Flags: review?(dbaron) → review+
Attachment #8502997 - Flags: review?(dbaron) → review+
Comment on attachment 8502999 [details] [diff] [review]
part 14 - Add a method to post restyle for layer changes

Could you explain why this is needed?  I don't see why it should be -- shouldn't style change handling produce these hints when style actually changes?
Attachment #8502999 - Flags: review?(dbaron) → review-
Comment on attachment 8503000 [details] [diff] [review]
part 15 - Switch AnimationPlayer to using less aggressive update mechanism

I think I missed some of these callers when I was commenting on patch 3, though I'm actually still not sure if I want to revisit that comment.  I wonder why you think it's preferable to remove the UpdateFlags parameter here and add the mInStyleRecalc boolean in patch 3.  Aren't they basically equivalent?  (I guess you'd need to pass the UpdateFlags a little further down through PostUpdate and some of the functions it calls (that you're adding in other patches), rather than skipping PostUpdate entirely?)  Being explicit about passing the data through seems to me like it might be preferable.

Otherwise this seems fine, although I suppose it might change a bit.
Attachment #8503000 - Flags: review?(dbaron) → review+
(Assignee)

Comment 25

4 years ago
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #16)
> Comment on attachment 8502985 [details] [diff] [review]
> part 1 - Move CheckNeedsRefreshes to CommonAnimationManager
> 
> I suspect this might cause us to leave a finished transition in the
> transition manager's list longer than needed.  Did you check that completed
> transitions still get removed from the transition manager's list one refresh
> cycle after completing?  If you did and that still happens reliably, then
> r=dbaron.

Yes, I've added some trace statements and confirmed that we're marking the transition as finished and then, on the following refresh driver tick, removing it from the list.
Comment on attachment 8502992 [details] [diff] [review]
part 8 - Add protected AnimationPlayer::GetDocument()

I think using GetDocument() here is fine.  Though GetRenderedDocument() might be even better, to make it clear what we're really getting...
Attachment #8502992 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 27

4 years ago
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #17)
> Comment on attachment 8502988 [details] [diff] [review]
> part 4 - Add CommonAnimationManager::CollectionUpdated
> 
> >+void
> >+CommonAnimationManager::NotifyCollectionUpdated()
> >+{
> >+  CheckNeedsRefresh();
> >+  if (!mInStyleRecalc) {
> >+    mPresContext->Document()->SetNeedStyleFlush();
> >+  }
> >+}
> 
> It's not clear to me what makes this sufficient. 
> 
> It won't cause this check in PresShell::FlushPendingNotifications:
>       if (aFlush.mFlushAnimations &&
>           !mPresContext->StyleUpdateForAllAnimationsIsUpToDate()) {
> to be true, and it also won't put any pending restyles in the queue.
> 
> Maybe I'm missing something, though, but it seems like there's a risk that
> the changes here won't go into effect until the next refresh cycle, which
> seems wrong.  (Please re-request review if I am.)

I think that's quite possible. I simply haven't been able to create a test to prove it because of the two methods that rely on NotifyCollectionUpdated so far--play() and pause()--neither produce any immediate change to the style rule. Their result is only noticeable on the next tick.

That will soon change however (e.g. when make AnimationPlayer.currentTime writeable or when we implement the auto-rewinding behavior of play()).

I'll update this as follows:

  void
  CommonAnimationManager::NotifyCollectionUpdated(AnimationPlayerCollection &aCollection)
  {
    CheckNeedsRefresh();
    mPresContext->ClearLastStyleUpdateForAllAnimations();
    if (!mInStyleRecalc) {
      aCollection.PostRestyleForAnimation(mPresContext);
    }
  }

Where nsPresContext::ClearLastStyleUpdateForAllAnimations is a new method that resets mLastStyleUpdateForAllAnimations to null.

I've tried this and it seems to at least bring forward the update.

Previously (i.e. with the current set of patches) the flow looked something like (based on a simple test case I prepared with some trace statements):

  pause()
    Marking document needing style flush
  * Tick
    Flushing styles
      Running ComposeStyle (as of bug 1078122 this is where we calculate the animation style)
      CheckAnimationRule
  * Tick (no observers)
  * Tick (no observers)
  PresShell::ScheduleViewManagerFlush
  * Tick
    ViewManager::ProcessingPendingUpdates
      < Paint >

But with the above changes it looks like:

  pause()
    Posting restyle for animation
  * Tick
    Flushing styles
      Running ComposeStyle
      CheckAnimationRule
  * Tick
    PresShell::ScheduleViewManagerFlush
    PresShell::ScheduleViewManagerFlush
    ViewManager::ProcessingPendingUpdates
      < Paint >

In either case we're not recalculating the animation style until the next tick but presumably if something else attempted to flush animation styles before the next tick we'd do the correct thing in the latter case.

Does the change I proposed above better match what you had in mind?
(Assignee)

Updated

4 years ago
Flags: needinfo?(dbaron)
(Assignee)

Comment 28

4 years ago
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #18)
> Comment on attachment 8502987 [details] [diff] [review]
> part 3 - Add mInStyleRecalc member
> 
> I don't see why this is needed.  How do we call into NotifyCollectionUpdated
> when mInStyleRecalc is true?

In CheckAnimationRule we create a new set of AnimationPlayer objects (via BuildAnimations) and then compare them with the current set and try to match them up.

If the newly created object is paused but the old object is not, we call AnimationPlayer::PauseFromStyle() on the old object which calls Animation::Pause() internally after setting some internal state. Pause() then calls up to the collection to tell it to clear any necessary state so that animation gets updated. (This mostly matters when we get a call to Play() from script and need to make sure we start observing the refresh driver again etc.)

Likewise if the old object is paused and the new object is not, we call PlayFromStyle().

> (And, if we do, would it make more sense to pass that explicitly rather than
> maintain this state?)

Looking at your comment 14 about part 15 I think you're right and keeping the explicit flags is probably the better way to go. I'll drop this patch from the series and simply tidy up the flags in the last patch.
(Assignee)

Comment 29

4 years ago
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #20)
> Comment on attachment 8502990 [details] [diff] [review]
> part 6 - Add CSSTransitionPlayer
> 
> Seems like the patch this goes on top of hasn't landed yet
> (CSSAnimationPlayer isn't in the tree), so this is a little confusing.  but
> r=dbaron

That's bug 1078122 which currently awaiting review.
(Assignee)

Comment 30

4 years ago
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #23)
> Comment on attachment 8502999 [details] [diff] [review]
> part 14 - Add a method to post restyle for layer changes
> 
> Could you explain why this is needed?  I don't see why it should be --
> shouldn't style change handling produce these hints when style actually
> changes?

The reason the explicit layer updates are needed is we need to sync the animations even if there is no change in the visual result. For example, in the case of a call to 'pause()' we need to ensure the animations get taken off the compositor. The style change handling won't detect any change since there's no change to style.

There is a test for this in part 9 of bug 1070745 which fails if we don't add these hints since the animations on the layer won't be updated until the next time we schedule a paint.
(Assignee)

Updated

4 years ago
Attachment #8502999 - Flags: review- → review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8502987 - Attachment is obsolete: true
(Assignee)

Comment 31

4 years ago
Created attachment 8505314 [details] [diff] [review]
part 3a - Allow TimeStamp == and != operators to work with null timestamps
Attachment #8505314 - Flags: review?(nfroyd)
(Assignee)

Comment 32

4 years ago
Created attachment 8505319 [details] [diff] [review]
part 3b - Add nsPresContext::ClearLastStyleUpdateForAllAnimations

This is the additional method mentioned in comment 27
Attachment #8505319 - Flags: review?(dbaron)
(Assignee)

Comment 33

4 years ago
Created attachment 8505321 [details] [diff] [review]
part 4 - Add CommonAnimationManager::CollectionUpdated

Applying changes outlined in comment 27. This method could possibly also have a debug-only block that asserts that the passed-in collection is, in fact, one this manager's collections. I'll wait to see if you agree with this general approach, though, before adding that.
Attachment #8505321 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8502988 - Attachment is obsolete: true
Attachment #8505314 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 34

4 years ago
(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #22)
> Comment on attachment 8502994 [details] [diff] [review]
> part 10 - Add AnimationPlayer::GetAnimationManager()
> 
> I wonder if you want the base class method to be pure virtual or returning
> null by default.  Your comments about wanting AnimationPlayer for other
> types of animations make me suspect perhaps the latter, but I'm not sure. 
> Either way is fine with me for now.

Yes, we'll want to be able to instantiate base class AnimationPlayer objects in the future (for script-generated animations) which means we'll need to fill in this method at some point. I'm not sure how that will work yet: will we have a separate manager for script-generated animations or will we succeed in merging the other managers? I'm not sure, but in any case, we'll have to update this method at that point so I think pure virtual is ok for now.
(Assignee)

Comment 35

4 years ago
Created attachment 8505904 [details] [diff] [review]
part 15 - Switch AnimationPlayer to using less aggressive update mechanism

David, I've updated this to keep the update flags in and wondered if you wanted
to check it over. I suppose it would might make sense to rename the enum value
eUpdateStyle to something like eDoUpdate or eUpdateStyleAndLayers?

In effect, PostUpdate is now doing everything that would otherwise happen if we
made the change as a result of processing style changes.
Attachment #8505904 - Flags: feedback?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8503000 - Attachment is obsolete: true
(Assignee)

Comment 36

4 years ago
(I'll fix the noise surrounding aFlags/aUpdateFlags before pushing.)
(In reply to Brian Birtles (:birtles) from comment #27)
> Does the change I proposed above better match what you had in mind?

yes.

(In reply to Brian Birtles (:birtles) from comment #34)
> Yes, we'll want to be able to instantiate base class AnimationPlayer objects
> in the future (for script-generated animations) which means we'll need to
> fill in this method at some point. I'm not sure how that will work yet: will
> we have a separate manager for script-generated animations or will we
> succeed in merging the other managers? I'm not sure, but in any case, we'll
> have to update this method at that point so I think pure virtual is ok for
> now.

ok, sounds good.
Flags: needinfo?(dbaron)
Attachment #8505319 - Flags: review?(dbaron) → review+
Attachment #8505321 - Flags: review?(dbaron) → review+
(In reply to Brian Birtles (:birtles) from comment #30)
> The reason the explicit layer updates are needed is we need to sync the
> animations even if there is no change in the visual result. For example, in
> the case of a call to 'pause()' we need to ensure the animations get taken
> off the compositor. The style change handling won't detect any change since
> there's no change to style.
> 
> There is a test for this in part 9 of bug 1070745 which fails if we don't
> add these hints since the animations on the layer won't be updated until the
> next time we schedule a paint.

I'm still not convinced this isn't covering up something else.

Two things that I think do need to happen (in the order given), and that I think may be missing, are:

 (1) bumping RestyleManager::mAnimationGeneration when something changes animations through the Web Animations API (this is definitely missing).  The concept of mAnimationGeneration is based on the idea that what animations are running can only change when there's a style changed.  (It's just a counter that increments whenever animations might have changed, so that we can compare the animation generation on the AnimationPlayerCollection to the one on the layer.  I keep meaning to fix the mess that the layer only has one for both animations and transitions...)

 (2) Ensuring AnimationPlayerCollection::UpdateAnimationGeneration is called (this probably is not missing, although there's a chance it could be).

If those things both happen, then I would think AnimationPlayerCollection::CanThrottleAnimation should handle syncing the animation data to the layer as needed.
Flags: needinfo?(birtles)
Comment on attachment 8505904 [details] [diff] [review]
part 15 - Switch AnimationPlayer to using less aggressive update mechanism


(In reply to Brian Birtles (:birtles) from comment #36)
> (I'll fix the noise surrounding aFlags/aUpdateFlags before pushing.)

I think this is making things more consistent about whether the parameter is called aFlags or aUpdateFlags, actually.  In the old code it looks like there were cases where .h and .cpp were inconsistent (though I'm skimming quickly and might be wrong).  (Though maybe aUpdateFlags is better?  Not really sure.)
Attachment #8505904 - Flags: feedback?(dbaron) → feedback+
(Assignee)

Comment 40

4 years ago
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #38)
> I'm still not convinced this isn't covering up something else.
> 
> Two things that I think do need to happen (in the order given), and that I
> think may be missing, are:
> 
>  (1) bumping RestyleManager::mAnimationGeneration when something changes
> animations through the Web Animations API (this is definitely missing).  The
> concept of mAnimationGeneration is based on the idea that what animations
> are running can only change when there's a style changed.  (It's just a
> counter that increments whenever animations might have changed, so that we
> can compare the animation generation on the AnimationPlayerCollection to the
> one on the layer.  I keep meaning to fix the mess that the layer only has
> one for both animations and transitions...)
>  (2) Ensuring AnimationPlayerCollection::UpdateAnimationGeneration is called
> (this probably is not missing, although there's a chance it could be).
>
> If those things both happen, then I would think
> AnimationPlayerCollection::CanThrottleAnimation should handle syncing the
> animation data to the layer as needed.

Indeed, neither of those things are happening currently. I've updated the patches to add this but this alone isn't enough to trigger a layer transaction. CanThrottleAnimation simply causes us to not throttle the sample.

(In the particular test case I'm looking at that's not needed anyway since we get called from here with aFlags == Cannot_Throttle: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=3a53ee57c736#4216)

From there, however, nothing triggers the layer transaction. We post another animation restyle since the style rules differ in identity but when we go to compare them here, they're the same:

  http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp?rev=46b36a35fddd#2756

Likewise, all the other tests in that function for, e.g. 'mAnimationDurationCount != aOther.mAnimationDurationCount' detect no difference and we end up returning nsChangeHint(0).

I haven't tracked all the other changes from there on but I can see that we don't end up calling ApplyRenderingChangeToTree, presumably because RestyleManager only sees an empty change.

I'll check exactly what's happening but I wonder what you expect to trigger the changes to the layer?
Flags: needinfo?(birtles) → needinfo?(dbaron)
(Assignee)

Comment 41

4 years ago
(In reply to Brian Birtles (:birtles) from comment #40)
> I'll check exactly what's happening but I wonder what you expect to trigger
> the changes to the layer?

Just confirming that ElementRestyler ends up adding nothing to its mChangeList member in this case.

As a result RestyleManager::ComputeAndProcessStyleChange gets back an empty change list (via RestyleManager::ComputeStyleChangeFor).

Then in RestyleManager::ProcessRestyledFrames we return early because the list is empty. ProcessRestyleFrames is where we'd normally call ApplyRenderingChangeToTree and add the animations to the layer.
(Assignee)

Comment 42

4 years ago
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #39)
> Comment on attachment 8505904 [details] [diff] [review]
> part 15 - Switch AnimationPlayer to using less aggressive update mechanism
> 
> 
> (In reply to Brian Birtles (:birtles) from comment #36)
> > (I'll fix the noise surrounding aFlags/aUpdateFlags before pushing.)
> 
> I think this is making things more consistent about whether the parameter is
> called aFlags or aUpdateFlags, actually.  In the old code it looks like
> there were cases where .h and .cpp were inconsistent (though I'm skimming
> quickly and might be wrong).  (Though maybe aUpdateFlags is better?  Not
> really sure.)

Yes, I've made this aUpdateFlags everywhere now.
(Assignee)

Comment 43

4 years ago
Created attachment 8506664 [details] [diff] [review]
part 14b - Update animation generation when changing animations via the API
Attachment #8506664 - Flags: review?(dbaron)
(Assignee)

Comment 44

4 years ago
Created attachment 8506668 [details] [diff] [review]
part 14c - Check for a null style refresh time in AnimationPlayerCollection::CanThrottleTransformChanges

We often set mStyleRuleRefreshTime to null to ensure styles get updated.
However, CanThrottleTransformChanges doesn't check for this case and blindly
does subtraction using this value.

Until now we've got away with this but now that we set mStyleRuleRefreshTime to
null when making changes via the API this case crops up in different
circumstances and we can trip over it.

This patch simply adds a null check before using mStyleRuleRefreshTime in
CanThrottleTransformChanges. All other cases where we operate on
mStyleRuleRefreshTime check for null.
Attachment #8506668 - Flags: review?(dbaron)
(In reply to Brian Birtles (:birtles) from comment #40)
> Indeed, neither of those things are happening currently. I've updated the
> patches to add this but this alone isn't enough to trigger a layer
> transaction. CanThrottleAnimation simply causes us to not throttle the
> sample.

Which I suppose we will repeatedly do every tick until there's an actual style change that causes us to send style data to the layer?

Doesn't this mean that we could have the same problem with OMTA, e.g., if there's a dynamic change in animation-delay (making it longer).  We wouldn't have an actual change in the value of the property on the main thread until the new delay is done, but the layer would still have the old delay.  (Or do we not send animations to the layer during the delay phase, only once it starts running?)

> I'll check exactly what's happening but I wonder what you expect to trigger
> the changes to the layer?

I guess I had in my head that I'd fixed this in bug 828173, but it looks like I didn't.

I think we probably should have something, though, that triggers updates to the layer if the animation generation on the layer is behind.
Flags: needinfo?(dbaron)
(Assignee)

Comment 46

4 years ago
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #45)
> Doesn't this mean that we could have the same problem with OMTA, e.g., if
> there's a dynamic change in animation-delay (making it longer).  We wouldn't
> have an actual change in the value of the property on the main thread until
> the new delay is done, but the layer would still have the old delay.  (Or do
> we not send animations to the layer during the delay phase, only once it
> starts running?)

I haven't checked but this line *might* mean we're ok for the case of changing animation-delay:

  http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp?rev=46b36a35fddd#2839

We set a neutral change hint although we undo that later:

  http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp?rev=e0f6f0f43e78#820
(Assignee)

Comment 47

4 years ago
Created attachment 8508401 [details] [diff] [review]
WIP: Trigger restyle when animation generation is out of date:

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from comment #45)
> I think we probably should have something, though, that triggers updates to
> the layer if the animation generation on the layer is behind.

Here is a rough attempt at doing that. The tests pass with this but I suspect it's not hooking in entirely the right place (probably belongs in RestyleSelf?).

If this approach seems suitable then I'll replace part 14 with this and put up for review.
Attachment #8508401 - Flags: feedback?(dbaron)
Comment on attachment 8508401 [details] [diff] [review]
WIP: Trigger restyle when animation generation is out of date:

If we're layerizing something that has continuations, presumably each
continuation will have its own layer.  So I'd actually suggest putting
the call in the loop over continuations that calls RestyleSelf, since
eventually we want to stop calling RestyleSelf for each continuation,
but we'll still want this.

(Or do the continuations share a layer?  Maybe they do.  Worth checking.
Either way, I think I prefer having the code in Restyle rather than
RestyleSelf, because I want to change when RestyleSelf is called so it
doesn't happen for each continuation.)

>+    if (layer && mAnimationGeneration > layer->GetAnimationGeneration()) {

It would seem better to just use
mPresContext->RestyleManager()->GetAnimationGeneration() rather than
adding the new member variable.

(In reply to Brian Birtles (:birtles) from comment #46)
> I haven't checked but this line *might* mean we're ok for the case of
> changing animation-delay:

That's not relevant.

I think what saves us for animation-delay is that
AddAnimationsForProperty in nsDisplayList.cpp checks
AnimationPlayer::IsRunning, which means we don't put animations on the
compositor during their delay phase, and we keep ticking them on the
main thread the entire time (which isn't ideal).
Attachment #8508401 - Flags: feedback?(dbaron) → feedback+
Also, is there layer updating code elsewhere that we can remove as a result of adding this code?
(Assignee)

Updated

4 years ago
Blocks: 1085769
(Assignee)

Updated

4 years ago
Attachment #8502999 - Attachment is obsolete: true
Attachment #8502999 - Flags: review?(dbaron)
Comment on attachment 8502985 [details] [diff] [review]
part 1 - Move CheckNeedsRefreshes to CommonAnimationManager

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

::: layout/style/AnimationCommon.cpp
@@ +76,5 @@
> +                                               aCollection)
> +{
> +  if (!mIsObservingRefreshDriver) {
> +    NS_ASSERTION(
> +      static_cast<AnimationPlayerCollection*>(aCollection)->mNeedsRefreshes,

You don't need this cast.
(Assignee)

Comment 51

4 years ago
(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) (away/busy Oct. 24-31) from comment #48)
> Comment on attachment 8508401 [details] [diff] [review]
> WIP: Trigger restyle when animation generation is out of date:
> 
> If we're layerizing something that has continuations, presumably each
> continuation will have its own layer.  So I'd actually suggest putting
> the call in the loop over continuations that calls RestyleSelf, since
> eventually we want to stop calling RestyleSelf for each continuation,
> but we'll still want this.
> 
> (Or do the continuations share a layer?  Maybe they do.  Worth checking.
> Either way, I think I prefer having the code in Restyle rather than
> RestyleSelf, because I want to change when RestyleSelf is called so it
> doesn't happen for each continuation.)

The continuations share a layer. I've confirmed this in layout debugger and with layer borders turned on.

Also, when we interact with layers in CommonAnimationManager we get the layer corresponding to the style frame (as returned by nsLayoutUtils, i.e. the primary frame except for tables).

> >+    if (layer && mAnimationGeneration > layer->GetAnimationGeneration()) {
> 
> It would seem better to just use
> mPresContext->RestyleManager()->GetAnimationGeneration() rather than
> adding the new member variable.

Fixed.

> (In reply to Brian Birtles (:birtles) from comment #46)
> > I haven't checked but this line *might* mean we're ok for the case of
> > changing animation-delay:
> 
> That's not relevant.
> 
> I think what saves us for animation-delay is that
> AddAnimationsForProperty in nsDisplayList.cpp checks
> AnimationPlayer::IsRunning, which means we don't put animations on the
> compositor during their delay phase and we keep ticking them on the
> main thread the entire time (which isn't ideal).

Yes, I agree that's not ideal. However, that only covers the case where we change the delay while we're in the delay, not if we're already animating.

I'll check what happens then once I finish building but for now these patches don't make things any worse.

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) (away/busy Oct. 24-31) from comment #49)
> Also, is there layer updating code elsewhere that we can remove as a result
> of adding this code?

Nothing comes to mind yet. I think the only place we're directly interacting with layers in the animation manager code is to check their animation generation and tell the ActiveLayerTracker that the layer is animated. I'll keep an eye out for other places however.

(Obviously the code I previously added for updating layers is no longer needed so I've made that patch obsolete.)
(Assignee)

Comment 52

4 years ago
Created attachment 8510023 [details] [diff] [review]
part 14a - Update animation generation when changing animations via the API

Moving part 14b to 14a
Attachment #8510023 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8506664 - Attachment is obsolete: true
Attachment #8506664 - Flags: review?(dbaron)
(Assignee)

Comment 53

4 years ago
Created attachment 8510025 [details] [diff] [review]
part 14b - Make ElementRestyler detect changes to the animation generation
Attachment #8510025 - Flags: review?(dbaron)
(Assignee)

Comment 54

4 years ago
(In reply to David Zbarsky (:dzbarsky) from comment #50)
> Comment on attachment 8502985 [details] [diff] [review]
> part 1 - Move CheckNeedsRefreshes to CommonAnimationManager
> 
> Review of attachment 8502985 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/AnimationCommon.cpp
> @@ +76,5 @@
> > +                                               aCollection)
> > +{
> > +  if (!mIsObservingRefreshDriver) {
> > +    NS_ASSERTION(
> > +      static_cast<AnimationPlayerCollection*>(aCollection)->mNeedsRefreshes,
> 
> You don't need this cast.

Thanks, I've fixed this locally.
Attachment #8510023 - Flags: review?(dbaron) → review+
Attachment #8506668 - Flags: review?(dbaron) → review+
Comment on attachment 8510025 [details] [diff] [review]
part 14b - Make ElementRestyler detect changes to the animation generation

>+    Layer* layer =
>+      FrameLayerBuilder::GetDedicatedLayer(mFrame, sLayerTypes[i]);
>+    if (layer &&
>+        mPresContext->RestyleManager()->GetAnimationGeneration()
>+          > layer->GetAnimationGeneration()) {
>+      NS_UpdateHint(hint, sHints[i]);

Sorry, I noticed a rather serious mistake here.  You don't want to compare the RestyleManager's animation generation with the layer's; you want to compare the AnimationPlayerCollection's with the layer.

Remember, the generation number propagates through 3 places:
 - the RestyleManager, where it increments every time something might change animation data somewhere
 - the AnimationPlayerCollection, which pulls the new number from the restylemanager every time *it* changes
 - the Layer, which gets the number from the AnimationPlayerCollection every time it is updated from the AnimationPlayerCollection

(And, yes, there are two AnimationPlayerCollections, one for animations and one for transitions; I've been meaning to finish and land https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/42046d24d464/separate-generation-counters for a while, but haven't.)

So you really want to compare to the animation generation on both the transitions and animations for the element.


I also wonder if it makes sense to reuse some of the code from AnimationPlayerCollection::CanThrottleAnimation?


(Sorry for not realizing yesterday that this was an update of the WIP patch and thus the whole thing was ready for review.)
Attachment #8510025 - Flags: review?(dbaron) → review-
(Assignee)

Comment 56

4 years ago
(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) (away/busy Oct. 24-31) from comment #55)
> Comment on attachment 8510025 [details] [diff] [review]
> part 14b - Make ElementRestyler detect changes to the animation generation
> 
> >+    Layer* layer =
> >+      FrameLayerBuilder::GetDedicatedLayer(mFrame, sLayerTypes[i]);
> >+    if (layer &&
> >+        mPresContext->RestyleManager()->GetAnimationGeneration()
> >+          > layer->GetAnimationGeneration()) {
> >+      NS_UpdateHint(hint, sHints[i]);
> 
> Sorry, I noticed a rather serious mistake here.  You don't want to compare
> the RestyleManager's animation generation with the layer's; you want to
> compare the AnimationPlayerCollection's with the layer.

But we make sure both get updated in part 14a.

> Remember, the generation number propagates through 3 places:
>  - the RestyleManager, where it increments every time something might change
> animation data somewhere
>  - the AnimationPlayerCollection, which pulls the new number from the
> restylemanager every time *it* changes
>  - the Layer, which gets the number from the AnimationPlayerCollection every
> time it is updated from the AnimationPlayerCollection

Right, and part 14a updates the RestyleManager and makes the current collection pull the new number from it.

> So you really want to compare to the animation generation on both the
> transitions and animations for the element.

I don't really understand how this helps. I don't think we want to update *only* the animation generation on one of the collections and *not* the restyle manager right? Otherwise on the next restyle we'll fail to detect that the collection is now out-of-step with the layer (since we'll update the restyle manager's animation generation so it that now equals that of the updated collection).

So I think we need to update the restyle manager at least. And so long as it is always up-to-date it seems suitable for testing if the layer is up-to-date?

> I also wonder if it makes sense to reuse some of the code from
> AnimationPlayerCollection::CanThrottleAnimation?

Probably. Perhaps we can tackle that in bug 1084220 when we rework that code there? (I'd like to get this landed soonish since it's blocking a bunch of other work.)
Flags: needinfo?(dbaron)
(In reply to Brian Birtles (:birtles) from comment #56)
> (In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions)
> (away/busy Oct. 24-31) from comment #55)
> > Remember, the generation number propagates through 3 places:
> >  - the RestyleManager, where it increments every time something might change
> > animation data somewhere
> >  - the AnimationPlayerCollection, which pulls the new number from the
> > restylemanager every time *it* changes
> >  - the Layer, which gets the number from the AnimationPlayerCollection every
> > time it is updated from the AnimationPlayerCollection
> 
> Right, and part 14a updates the RestyleManager and makes the current
> collection pull the new number from it.

But it makes only the collection that changed pull the new number -- not *all* the collections.

Which will mean that your new code will make us resend every animation to the layer when we recompute style for its element, whether or not there's any new information, as long as we've ticked the RestyleManager's generation.  And it will keep doing it every time, because we won't update the AnimationPlayerCollection's generation (and thus the layer's generation) unless the AnimationPlayerCollection actually gets updated in some way.
Flags: needinfo?(dbaron)
(Assignee)

Comment 58

4 years ago
(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) (away/busy Oct. 24-31) from comment #57)
> (In reply to Brian Birtles (:birtles) from comment #56)
> > (In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions)
> > (away/busy Oct. 24-31) from comment #55)
> > > Remember, the generation number propagates through 3 places:
> > >  - the RestyleManager, where it increments every time something might change
> > > animation data somewhere
> > >  - the AnimationPlayerCollection, which pulls the new number from the
> > > restylemanager every time *it* changes
> > >  - the Layer, which gets the number from the AnimationPlayerCollection every
> > > time it is updated from the AnimationPlayerCollection
> > 
> > Right, and part 14a updates the RestyleManager and makes the current
> > collection pull the new number from it.
> 
> But it makes only the collection that changed pull the new number -- not
> *all* the collections.
> 
> Which will mean that your new code will make us resend every animation to
> the layer when we recompute style for its element, whether or not there's
> any new information, as long as we've ticked the RestyleManager's
> generation.  And it will keep doing it every time, because we won't update
> the AnimationPlayerCollection's generation (and thus the layer's generation)
> unless the AnimationPlayerCollection actually gets updated in some way.

But the comparison is a > comparison and we update the layer's generation the first time we send it. So it would seem, at first, that we won't keep sending to the layer in the case where one collection has an older animation generation number.

The problem is that if we have both animations and transitions, then the generation set on the *animations* will always win since in nsDisplayList::AddAnimationsAndTransitionsToLayer we clobber the generation set from transitions with the one set on animations.

So if we have both animations and transitions and we update the *transitions* collection, the layer will end up getting the old *animations* generations and then we could end up updating the layer repeatedly.

We could possibly fix this in nsDisplayList by just taking the maximum of the two animation generations and setting that on the layer (or even just getting the number from the RestyleManager).

That way even if one of the collections had an animation generation that lagged behind the number on the restyle manager, they wouldn't continually force us to resend to the layer since the > comparison would fail.

What do you think?
Flags: needinfo?(dbaron)
(In reply to Brian Birtles (:birtles) from comment #58)
> But the comparison is a > comparison and we update the layer's generation
> the first time we send it. So it would seem, at first, that we won't keep
> sending to the layer in the case where one collection has an older animation
> generation number.
> 
> The problem is that if we have both animations and transitions, then the
> generation set on the *animations* will always win since in
> nsDisplayList::AddAnimationsAndTransitionsToLayer we clobber the generation
> set from transitions with the one set on animations.

Using std::max() there (and making sure we do > comparisons) would help this problem, except I don't want to use > comparisons (I'd much rather use ==) because the generation counter is designed to allow wraparound.  In the case of wraparound, there's a very low chance of failure, but you don't have an "everything breaks" situation.

> So if we have both animations and transitions and we update the
> *transitions* collection, the layer will end up getting the old *animations*
> generations and then we could end up updating the layer repeatedly.
> 
> We could possibly fix this in nsDisplayList by just taking the maximum of
> the two animation generations and setting that on the layer (or even just
> getting the number from the RestyleManager).

These both require settling on > comparisons, which I don't want (see above).  I'd rather just store 2 numbers on the layer, and settle on == or != comparisons.
Flags: needinfo?(dbaron)
(And note that this patch series also has the potential to make the generation counter increment much faster than it used to.)
(Assignee)

Comment 61

4 years ago
(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) (away/busy Oct. 24-31) from comment #60)
> (And note that this patch series also has the potential to make the
> generation counter increment much faster than it used to.)

Hmm, we only do this out-of-band update to the generation count when someone calls play() / pause() from script. I guess in the future, though, we'll end up calling this for many other kinds of changes.

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) (away/busy Oct. 24-31) from comment #59)
> Using std::max() there (and making sure we do > comparisons) would help this
> problem, except I don't want to use > comparisons (I'd much rather use ==)
> because the generation counter is designed to allow wraparound.  In the case
> of wraparound, there's a very low chance of failure, but you don't have an
> "everything breaks" situation.

Right. We currently do > comparisons everywhere. I wonder if adding separate counters and doing != comparisons is really in scope for this bug. Especially since there's a lot of logic that needs to be checked for such a change.

I guess I'll have a go at doing that sometime next week or the week after.
For this bug, could you just check the generation on the layer against the animations and transitions AnimationPlayerCollections?
(Assignee)

Comment 63

4 years ago
Created attachment 8516408 [details] [diff] [review]
part 14b - Make ElementRestyler detect changes to the animation generation

For some kinds of changes we need to update the layer tree even though there is
no change to style. For example, if an animation is paused via the Web
Animations API, we need to remove the animation from the layer even though the
style will not change.

This patch detects such changes by making ElementRestyler check for an
out-of-date animation generation on layers. This is complicated by the fact that
we currently maintain *two* animation generation numbers for nsAnimationManager
and nsTransitionManager respectively, but we only have *one* animation
generation number on each layer. This is a known issue (bug 847286).

As a result, until bug 847286 is fixed, we need to be careful to compare against
the greater of the two numbers.

This patch also adjusts nsDisplayListBuilder to address the case where we have:
  i.  *both* animations and transitions for a given layer and,
  ii. the transitions are updated but the animations are not.
Prior to this patch we would end up clobbering the animation generation set by
the updated transitions with that of the animations. This patch makes us set the
greater of the two values.
Attachment #8516408 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8510025 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8508401 - Attachment is obsolete: true
(Assignee)

Comment 64

4 years ago
Created attachment 8516424 [details] [diff] [review]
part 16 - Factor out animation-layer related information to a common database

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #55)
> Comment on attachment 8510025 [details] [diff] [review]
> part 14b - Make ElementRestyler detect changes to the animation generation
...
> I also wonder if it makes sense to reuse some of the code from
> AnimationPlayerCollection::CanThrottleAnimation?

Here's an attempt at unifying some of the code that deals with the list of
layer-animatable properties. I'm not sure about the naming, or if the way I've
arranged the static array is suitable.
Attachment #8516424 - Flags: review?(dbaron)
(Assignee)

Comment 65

4 years ago
Comment on attachment 8516424 [details] [diff] [review]
part 16 - Factor out animation-layer related information to a common database

The static array in this patch leaks. Cancelling review for now.
Attachment #8516424 - Flags: review?(dbaron)
(Assignee)

Comment 66

4 years ago
Comment on attachment 8516408 [details] [diff] [review]
part 14b - Make ElementRestyler detect changes to the animation generation

Cancelling review for now. This triggers an assertion on B2G emulator here:

  http://hg.mozilla.org/mozilla-central/file/9c51ab5be6bb/layout/base/RestyleManager.cpp#l311

There are two cases where this occurs. One is where we have a transform animation and then remove the transform property from the keyframes rule. In this case the animation is out of date but somewhere we've already updated the frame to remove the transform (I haven't tracked down just where yet).

I'm not sure if it's related but I'm concerned that just blindly adding changes to the change list whenever we detect a discrepancy in the animation generations will lead to redundant work since we'll possibly already have the same change in the list based on comparing changes to style.

If that's the case, then the original approach this patch took of only updating the layer when an out-of-band change was made to animations seems preferable. I need to investigate further.
Attachment #8516408 - Flags: review?(dbaron)
Blocks: 1081007
(Assignee)

Comment 67

4 years ago
Created attachment 8517972 [details] [diff] [review]
part 14b - Make ElementRestyler detect changes to the animation generation
Attachment #8517972 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8516408 - Attachment is obsolete: true
(Assignee)

Comment 68

4 years ago
Created attachment 8517973 [details] [diff] [review]
part 16 - Factor out animation-layer related information to a common database
Attachment #8517973 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8516424 - Attachment is obsolete: true
Attachment #8517972 - Flags: review?(dbaron) → review+
Comment on attachment 8517973 [details] [diff] [review]
part 16 - Factor out animation-layer related information to a common database

I really don't like constructing an nsAutoTArray for each caller.  You should just expose the const array.

In particular, if you declare the array as:
  static const LayerAnimationRecord sLayerAnimationInfo[kLayerRecords];
then I believe ArrayLength() (preferred) or MOZ_ARRAY_LENGTH() should work.

So r=dbaron with that (i.e., getting rid of LayerAnimationInfo / GetLayerAnimationInfo and just exposing the array).
Attachment #8517973 - Flags: review?(dbaron) → review+
(Assignee)

Comment 71

4 years ago
Thanks for the reviews!

I've made the suggested changes but I haven't landed this yet since yesterday when I was working on bug 927349 (which builds on top of this) I noticed some failed assertions when trying to get an OMTA test to pass. The assertions are possibly related to these patches (relating to the UpdateTransformLayer hint being set when there is no transform) so I want to make sure these patches are ok before landing.

A try run looks good though:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2d3ab1a6a2d7
(Assignee)

Comment 73

4 years ago
(In reply to Brian Birtles (:birtles) from comment #71)
> I've made the suggested changes but I haven't landed this yet since
> yesterday when I was working on bug 927349 (which builds on top of this) I
> noticed some failed assertions when trying to get an OMTA test to pass. The
> assertions are possibly related to these patches (relating to the
> UpdateTransformLayer hint being set when there is no transform) so I want to
> make sure these patches are ok before landing.

I confirmed that the failed assertions were due to the problems in the patches for bug 927349 hence I decided this was safe to land.

Also, I ran the patches through try again since I had to a bit of rebasing of these patches and wanted to check everything was still working as expected.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a1c5869b036f
You need to log in before you can comment on or make changes to this bug.