Check there is an animation on the compositor instead of all type of animations for flushing throttled animations

ASSIGNED
Assigned to

Status

()

enhancement
P3
normal
ASSIGNED
2 years ago
2 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox57 affected)

Details

Attachments

(2 attachments, 1 obsolete attachment)

I have no idea how bug 1388031 happens yet, but we can avoid flushing throttled animations in most cases to check HasThrottledStyleUpdates() instead of HasPendingStyleUpdates().  It will reduce the possibility of the crash.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf62d2383740529dcc8aa6ae75cbdafae0c1d12e
After some more thought, I think this would hardly reduce the crash possibility since it's almost impossible to have pending normal restyle requests between restyling process and event handlings.  But, I'd really like to change the HasPendingStyleUpdates call to HasThrottledStyleUpdates to avoid confusions.
I can see why this makes sense for the FlushThrottledStyles call site but for the GeckoRestyleManager::ProcessPendingRestyles() call site it seems like we really do want to flush all pending animation restyles, not just throttled ones, right?
(In reply to Brian Birtles (:birtles) from comment #3)
> I can see why this makes sense for the FlushThrottledStyles call site but
> for the GeckoRestyleManager::ProcessPendingRestyles() call site it seems
> like we really do want to flush all pending animation restyles, not just
> throttled ones, right?

Yes, right.
In that case we should make separate methods -- one which is clearly for flushing throttled styles and one which is for flushing pending animation styles.
Comment on attachment 8895680 [details]
Bug 1388692 - When doing hit testing, only flush animations if there are throttled compositor animations.

https://reviewboard.mozilla.org/r/166956/#review172962

::: layout/base/GeckoRestyleManager.h:245
(Diff revision 1)
> +  void UpdateOnlyThrottledAnimationStyles()
> +  {
> +    UpdateOnlyAnimationStyles();
> +  }

This isn't right. The function says it only updates throttled animation styles but it actually runs all pending animation restyles.
Comment on attachment 8895680 [details]
Bug 1388692 - When doing hit testing, only flush animations if there are throttled compositor animations.

https://reviewboard.mozilla.org/r/166956/#review172962

> This isn't right. The function says it only updates throttled animation styles but it actually runs all pending animation restyles.

OK. I will add a FIXME comment here in this patch, and implement it in the last patch. Is that OK with you?
Let me look into this properly on Monday. I presume the purpose of this bug is to stop doing the animation-only restyle when there are no throttled restyles.
Priority: -- → P3
Comment on attachment 8895680 [details]
Bug 1388692 - When doing hit testing, only flush animations if there are throttled compositor animations.

https://reviewboard.mozilla.org/r/166956/#review173192

::: layout/base/GeckoRestyleManager.h:245
(Diff revision 2)
> +  void UpdateOnlyThrottledAnimationStyles()
> +  {
> +    // FIXME: For throttled animation flush, we don't need to flush all
> +    // pending animations and SMIL animations.
> +    UpdateOnlyAnimationStyles();
> +  }

It seems that even with part 3 this doesn't do what the name suggests because:

a) Even with part 3 it updates pending animation styles (provided there is at least one throttled restyle)
b) Even with part 3 it doesn't update throttled SMIL animation restyles.

With regards to (b), I think that's ok because we only use this for updating positions for hit testing and the throttling we do for SMIL is for content in display:none subtrees.

What about calling this:

  UpdateAnimationStylesForHitTesting()

?

We should then add a comment for this method to RestyleManager.h (I think we do this in part 3) and update the comment for the above UpdateOnlyAnimationStyles since it's no longer accurate.

(It might be nice to rename UpdateOnlyAnimationStyles to just UpdateAnimationStyles for consistency, but I'll leave that up to you.)

::: layout/base/ServoRestyleManager.h:189
(Diff revision 2)
>    /**
>     * Performs a Servo animation-only traversal to compute style for all nodes
>     * with the animation-only dirty bit in the document.
>     *
>     * This processes just the traversal for animation-only restyles and skips the
>     * normal traversal for other restyles unrelated to animations.
>     * This is used to bring throttled animations up-to-date such as when we need
>     * to get correct position for transform animations that are throttled because
>     * they are running on the compositor.
>     *
>     * This will traverse all of the document's style roots (that is, its document
>     * element, and the roots of the document-level native anonymous content).
>     */

Should this comment or part of it move to RestyleManager.h at some point?
Attachment #8895680 - Flags: review?(bbirtles)
Comment on attachment 8895681 [details]
Bug 1388692 - Make GeckoRestyleManager::UpdateAnimationStyles() private.

https://reviewboard.mozilla.org/r/166958/#review173196
Attachment #8895681 - Flags: review?(bbirtles) → review+
Comment on attachment 8895558 [details]
Bug 1388692 - Check that there are throttled animations instead of all type of animations for event handling.

https://reviewboard.mozilla.org/r/166770/#review173190

::: layout/base/GeckoRestyleManager.cpp:685
(Diff revision 3)
> +  bool doCSS = PresContext()->EffectCompositor()->HasThrottledStyleUpdates();
> +
> +  if (!doCSS) {
> +    return;
> +  }

I don't think there's any value in using the same |doCSS| naming here. Let's just make this:

if (!PresContext()->EffectCompositor()->HasThrottledStyleUpdates()) {
  return;
}

and drop the `if (doCSS) {` check later in the function.

::: layout/base/ServoRestyleManager.cpp:1191
(Diff revision 3)
>  }
>  
>  void
>  ServoRestyleManager::UpdateOnlyThrottledAnimationStyles()
>  {
>    // Bug 1365855: We also need to implement this for SMIL.

Based on what we name this method, we might be able to drop this comment or simply say -- "Throttled SMIL samples never affect hit-testing since they only apply to display:none subtrees so we don't need to handle them here." Likewise for GeckoRestyleManager::UpdateAnimationStylesForHitTesting().
Attachment #8895558 - Flags: review?(bbirtles)
This would be easier to review if it was rolled up into one patch. I don't think splitting it up makes sense here since the changes are inter-related.
(In reply to Brian Birtles (:birtles) from comment #18)
> This would be easier to review if it was rolled up into one patch. I don't
> think splitting it up makes sense here since the changes are inter-related.

OK. I will fold the third patch into the first one leave the second one as it is.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #19)
> (In reply to Brian Birtles (:birtles) from comment #18)
> > This would be easier to review if it was rolled up into one patch. I don't
> > think splitting it up makes sense here since the changes are inter-related.
> 
> OK. I will fold the third patch into the first one leave the second one as
> it is.

Thanks!
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(In reply to Brian Birtles (:birtles) from comment #15)
> Comment on attachment 8895680 [details]
> Bug 1388692 - Rename RestyleManager::UpdateOnlyAnimationStyles() to
> UpdateOnlyThrottledAnimationStyles().
> 
> https://reviewboard.mozilla.org/r/166956/#review173192
> 
> ::: layout/base/GeckoRestyleManager.h:245
> (Diff revision 2)
> > +  void UpdateOnlyThrottledAnimationStyles()
> > +  {
> > +    // FIXME: For throttled animation flush, we don't need to flush all
> > +    // pending animations and SMIL animations.
> > +    UpdateOnlyAnimationStyles();
> > +  }
> 
> It seems that even with part 3 this doesn't do what the name suggests
> because:
> 
> a) Even with part 3 it updates pending animation styles (provided there is
> at least one throttled restyle)
> b) Even with part 3 it doesn't update throttled SMIL animation restyles.
> 
> With regards to (b), I think that's ok because we only use this for updating
> positions for hit testing and the throttling we do for SMIL is for content
> in display:none subtrees.
> 
> What about calling this:
> 
>   UpdateAnimationStylesForHitTesting()
> 
> ?
> 
> We should then add a comment for this method to RestyleManager.h (I think we
> do this in part 3) and update the comment for the above
> UpdateOnlyAnimationStyles since it's no longer accurate.
> 
> (It might be nice to rename UpdateOnlyAnimationStyles to just
> UpdateAnimationStyles for consistency, but I'll leave that up to you.)

OK, I will rename it too, but leave ServoTraversalFlags::FlushThrottledAnimations name.  I need to think about it more, what name will be fit there.

> ::: layout/base/ServoRestyleManager.h:189
> (Diff revision 2)
> >    /**
> >     * Performs a Servo animation-only traversal to compute style for all nodes
> >     * with the animation-only dirty bit in the document.
> >     *
> >     * This processes just the traversal for animation-only restyles and skips the
> >     * normal traversal for other restyles unrelated to animations.
> >     * This is used to bring throttled animations up-to-date such as when we need
> >     * to get correct position for transform animations that are throttled because
> >     * they are running on the compositor.
> >     *
> >     * This will traverse all of the document's style roots (that is, its document
> >     * element, and the roots of the document-level native anonymous content).
> >     */
> 
> Should this comment or part of it move to RestyleManager.h at some point?

After landing bug 1388031, the comment here isn't right. We do traverse normal dirty bit now. I will drop this comment.
Attachment #8895558 - Attachment is obsolete: true
Comment on attachment 8895680 [details]
Bug 1388692 - When doing hit testing, only flush animations if there are throttled compositor animations.

https://reviewboard.mozilla.org/r/166956/#review173214

::: layout/base/GeckoRestyleManager.h:220
(Diff revision 3)
>    // Update styles for animations that are running on the compositor and
>    // whose updating is suppressed on the main thread (to save
>    // unnecessary work), while leaving all other aspects of style
>    // out-of-date.
>    //
>    // Performs an animation-only style flush to make styles from
>    // throttled transitions up-to-date prior to processing an unrelated
>    // style change, so that any transitions triggered by that style
>    // change produce correct results.
>    //
>    // In more detail:  when we're able to run animations on the
>    // compositor, we sometimes "throttle" these animations by skipping
>    // updating style data on the main thread.  However, whenever we
>    // process a normal (non-animation) style change, any changes in
>    // computed style on elements that have transition-* properties set
>    // may need to trigger new transitions; this process requires knowing
>    // both the old and new values of the property.  To do this correctly,
>    // we need to have an up-to-date *old* value of the property on the
>    // primary frame.  So the purpose of the mini-flush is to update the
>    // style for all throttled transitions and animations to the current
>    // animation state without making any other updates, so that when we
>    // process the queued style updates we'll have correct old data to
>    // compare against.  When we do this, we don't bother touching frames
>    // other than primary frames.

I don't think this comment is quite accurate?

::: layout/base/GeckoRestyleManager.cpp:683
(Diff revision 3)
> +GeckoRestyleManager::UpdateAnimationStylesForHitTesting()
> +{
> +  nsTransitionManager* transitionManager = PresContext()->TransitionManager();
> +
> +  transitionManager->SetInAnimationOnlyStyleUpdate(true);

Isn't this supposed to check for HasThrottledStyleUpdates first?

::: layout/base/RestyleManager.h:195
(Diff revision 3)
>                                 nsIAtom* aAttribute,
>                                 int32_t aModType,
>                                 const nsAttrValue* aOldValue);
>    inline nsresult ReparentStyleContext(nsIFrame* aFrame);
>  
> -  inline void UpdateOnlyAnimationStyles();
> +  // The only one purpose of this function is to get correct elements' position

// The purpose of this function...

::: layout/base/RestyleManagerInlines.h:86
(Diff revision 3)
>  
>  void
> -RestyleManager::UpdateOnlyAnimationStyles()
> +RestyleManager::UpdateAnimationStylesForHitTesting()
>  {
> -  MOZ_STYLO_FORWARD(UpdateOnlyAnimationStyles, ());
> +  // Throttled SMIL samples never affect hit-testing since they only apply to
> +  // display:none subtrees so we don't need to handle them here

Nit: . at the end of the sentence.

::: layout/base/RestyleManagerInlines.h:89
(Diff revision 3)
> +  // compositor since opacity animations don't move at all (i.e. positioned at
> +  // the same place).

Nit: ...since opacity animations don't affect hit-testing.
(In reply to Brian Birtles (:birtles) from comment #24)
> Comment on attachment 8895680 [details]
> Bug 1388692 - Make animation flush for event handling more proper.
> 
> https://reviewboard.mozilla.org/r/166956/#review173214
> 
> ::: layout/base/GeckoRestyleManager.h:220
> (Diff revision 3)
> >    // Update styles for animations that are running on the compositor and
> >    // whose updating is suppressed on the main thread (to save
> >    // unnecessary work), while leaving all other aspects of style
> >    // out-of-date.
> >    //
> >    // Performs an animation-only style flush to make styles from
> >    // throttled transitions up-to-date prior to processing an unrelated
> >    // style change, so that any transitions triggered by that style
> >    // change produce correct results.
> >    //
> >    // In more detail:  when we're able to run animations on the
> >    // compositor, we sometimes "throttle" these animations by skipping
> >    // updating style data on the main thread.  However, whenever we
> >    // process a normal (non-animation) style change, any changes in
> >    // computed style on elements that have transition-* properties set
> >    // may need to trigger new transitions; this process requires knowing
> >    // both the old and new values of the property.  To do this correctly,
> >    // we need to have an up-to-date *old* value of the property on the
> >    // primary frame.  So the purpose of the mini-flush is to update the
> >    // style for all throttled transitions and animations to the current
> >    // animation state without making any other updates, so that when we
> >    // process the queued style updates we'll have correct old data to
> >    // compare against.  When we do this, we don't bother touching frames
> >    // other than primary frames.
> 
> I don't think this comment is quite accurate?

I did untouch this comment intentionally. Actually I removed once most of parts of them other than the second paragraph.  But after some thought, the third paragraph is not yet clear to me. It seems to me that the sentence about what we do as RequestRestyle(Layer) or some such, or just about animation-only restyle.  Anyway, I don't quite understand about this comment, so I couldn't rephrase it.
Ok, I'll try writing some suitable comments.
Comment on attachment 8895680 [details]
Bug 1388692 - When doing hit testing, only flush animations if there are throttled compositor animations.

https://reviewboard.mozilla.org/r/166956/#review173288

::: commit-message-a4a44:1
(Diff revision 3)
> +Bug 1388692 - Make animation flush for event handling more proper. r?birtles

When doing hit testing, only flush animations if there are throttled compositor animations

::: commit-message-a4a44:3
(Diff revision 3)
> +1) Rename UpdateOnlyAnimationStyles to UpdateAnimationStylesForHitTesting.
> +2) Trigger the process only if we have throttled animations.
> +   (We will refine this in bug 1353212)

This patch changes UpdateAnimationOnlyStyles to only flush animation styles if there are throttled animation styles that could affect hit-testing and renames the function to UpdateAnimationStylesForHitTesting at the same time.

For GeckoRestyleManager, the original UpdateAnimationOnlyStyles which flushes animation styles if there are any pending animation styles, is renamed to UpdateAnimationStyles for consistency.

::: layout/base/GeckoRestyleManager.h:220
(Diff revision 3)
>    // Update styles for animations that are running on the compositor and
>    // whose updating is suppressed on the main thread (to save
>    // unnecessary work), while leaving all other aspects of style
>    // out-of-date.
>    //
>    // Performs an animation-only style flush to make styles from
>    // throttled transitions up-to-date prior to processing an unrelated
>    // style change, so that any transitions triggered by that style
>    // change produce correct results.
>    //
>    // In more detail:  when we're able to run animations on the
>    // compositor, we sometimes "throttle" these animations by skipping
>    // updating style data on the main thread.  However, whenever we
>    // process a normal (non-animation) style change, any changes in
>    // computed style on elements that have transition-* properties set
>    // may need to trigger new transitions; this process requires knowing
>    // both the old and new values of the property.  To do this correctly,
>    // we need to have an up-to-date *old* value of the property on the
>    // primary frame.  So the purpose of the mini-flush is to update the
>    // style for all throttled transitions and animations to the current
>    // animation state without making any other updates, so that when we
>    // process the queued style updates we'll have correct old data to
>    // compare against.  When we do this, we don't bother touching frames
>    // other than primary frames.

How about something like this:

In GeckoRestyleManager.h

  // Perform any pending animation restyles.
  //
  // This is a superset of the work performed by
  // UpdateAnimationStylesForHitTesting since it includes not only any
  // animations whose restyles might have been suppressed on the main
  // thread because they are running on the compositor, but *any*
  // animations with pending restyles including SMIL animation restyles.
  //
  // In more detail:  when we're able to run animations on the
  // compositor, we sometimes "throttle" these animations by skipping
  // updating style data on the main thread.  However, whenever we
  // process a normal (non-animation) style change, any changes in
  // computed style on elements that have transition-* properties set
  // may need to trigger new transitions; this process requires knowing
  // both the old and new values of the property.  To do this correctly,
  // we need to have an up-to-date *old* value of the property on the
  // primary frame.  So the purpose of the mini-flush is to update the
  // style for all throttled transitions and animations to the current
  // animation state without making any other updates, so that when we
  // process the queued style updates we'll have correct old data to
  // compare against.  When we do this, we don't bother touching frames
  UpdateAnimationStyles();

In RestyleManager.h

  // Update styles for animations if there are animations that are running
  // on the compositor and whose updating is suppressed on the main thread
  // (to save unnecessary work). All other aspects of style will remain
  // out-of-date. This is needed to ensure that transform animations running
  // on the compositor are updated on the main thread giving elements their
  // up-to-date geometry before we attempt to perform hit-testing.
  //
  // Although SMIL animations may also be throttled, such animations do not
  // affect hit-testing since only animations on elements in display:none
  // subtrees are throttled for SMIL. As a result, this method does not
  // update throttled SMIL animations.
  inline void UpdateAnimationStylesForHitTesting();
Attachment #8895680 - Flags: review?(bbirtles)
Thanks for the long comment!

(In reply to Brian Birtles (:birtles) from comment #27)
> 
> In RestyleManager.h
> 
>   // Update styles for animations if there are animations that are running
>   // on the compositor and whose updating is suppressed on the main thread
>   // (to save unnecessary work). All other aspects of style will remain
>   // out-of-date.

I did drop this 'All other..' sentence since it's not true any more since bug 1388031.
Comment on attachment 8895680 [details]
Bug 1388692 - When doing hit testing, only flush animations if there are throttled compositor animations.

https://reviewboard.mozilla.org/r/166956/#review173310

::: layout/base/GeckoRestyleManager.cpp:685
(Diff revision 4)
>  }
>  
>  void
> +GeckoRestyleManager::UpdateAnimationStylesForHitTesting()
> +{
> +  nsTransitionManager* transitionManager = PresContext()->TransitionManager();

Perhaps we should assert that PresContext()->EffectCompositor()->HasThrottledStyleUpdates() is true here?

(It seems a little odd the RestyleManager does this for us since it normally just forwards methods on so it might be good to document by means of an assertion that we're expecting RestyleManager to check this.)

Likewise in ServoRestyleManager::UpdateAnimationStylesForHitTesting I suppose.
Attachment #8895680 - Flags: review?(bbirtles) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/27e08c76b767
When doing hit testing, only flush animations if there are throttled compositor animations. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4bc49a585e26
Make GeckoRestyleManager::UpdateAnimationStyles() private. r=birtles
Backed out for failing mochitest layout/style/test/test_animations_styles_on_event.html on Android:

https://hg.mozilla.org/integration/autoland/rev/8fba8ca208f29b73161d38eb06f91ae3325fb3fb
https://hg.mozilla.org/integration/autoland/rev/ba989c655eb9827885e62d14fc3923054dfb91a2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4bc49a585e26df37d713a19d6a100280001d206e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122971496&repo=autoland

[task 2017-08-14T11:51:23.933584Z] 11:51:23     INFO -  134 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_animations_styles_on_event.html | Test timed out.
[task 2017-08-14T11:51:23.933973Z] 11:51:23     INFO -      reportError@SimpleTest/TestRunner.js:121:7
[task 2017-08-14T11:51:23.934419Z] 11:51:23     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7

Please also check the other failures on the Android opt push if they are related. There is bustage from https://hg.mozilla.org/integration/autoland/rev/918f0fc919113df0ec2d1876efe8e609f7579a79 on that push.
Flags: needinfo?(hikezoe)
Ugh.  I had been believing we don't need to flush normal restyle in case of event handling, but.. test_animations_styles_on_event.html is a case that needs to be flushed even if it's normal restyle.  The case is

1) Create a transform animation
2) The transform animation is running on the compositor
3) Manipulate the animation with web animation api (e.g. setting currentTime, finish() or something)
4) As a result of 3), we override throttled restyle request with normal restyle request
5) Right after 3), move mouse

I am inclined to close this bug as invalid...

A problem for the test case is that it did not fail on all of platforms other than android, we should fix it somehow.
OK, what we should do is that we check whether there are animations on the compositor or not, instead of checking HasThrottledStyleUpdates().
Flags: needinfo?(hikezoe)
Summary: stylo: check throttled animation instead of all type of animations for flushing throttled animations → Check there is an animation on the compositor instead of all type of animations for flushing throttled animations
You need to log in before you can comment on or make changes to this bug.