1105509 - OMTA-able animations not throttled for offscreen elements

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Markus Stange [:mstange]]

STR:
 1. Turn on the FPS display and OMTA in a Firefox Desktop build.
 2. Load the testcase.
 3. Observe how the FPS display shows a high composition frame rate (~60) and a very low content thread refresh rate (2 to 3).
 4. Click the button that says "Send offscreen" or scroll down far enough that the blue box is no longer drawn.

Expected results:
The content thread refresh rate should stay at 2-3fps.

Actual results:
The content thread refresh rate goes up to ~60fps.

This happens because AnimationPlayerCollection::CanThrottleAnimation returns false because FrameLayerBuilder::GetDedicatedLayer doesn't find a layer for the element.

It's very unfortunate that invisible animations cause a higher CPU load than visible ones.

Comment 1

[Markus Stange [:mstange]]

Attachment: testcase

Comment 3

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

I wonder if we want to use FrameLayerBuilder::IterateRetainedDataFor to test for retained data, and throttle any paint-only animations that don't have any?  (Or do we want a more generic mechanism... one that could also be used by SchedulePaint to bail out?)  If we do that, we'd need to figure out at what point we'd need to force unthrottling.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Sotaro, I can't tell here if there are additional things that are 2.2 blocking for this bug, or if fixing bug 1072616 took care of the immediate reason to make it 2.2+?  Doesn't sound small though...

Comment 5

[Sotaro Ikeda [:sotaro]]

I do not know current gaia hit this. Anyway, even if it happen, gaia side could do workaround.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Milan Sreckovic [:milan] from comment #4)
> Sotaro, I can't tell here if there are additional things that are 2.2
> blocking for this bug, or if fixing bug 1072616 took care of the immediate
> reason to make it 2.2+?  Doesn't sound small though...

By the way, gaia side's implementation seems to be changed since bug 1072616 fix.

Comment 7

[No-Jun Park [:njpark]]

[Blocking Requested - why for this release]:
Per comment 5 and comment 6, (and talking to milan about it) it would be reasonable to postpone this to 3.0 since the task would require sizable effort.

Comment 8

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

I think we should really just solve this more generally, by doing it for all animations whose effects are paint-only, whether OMTA-able or not.  That's bug 1166500.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Incomplete fix

This patch fixes a problem in AnimationPlayerCollection::CanThrottleAnimation but there is another problem in AnimationCollection::EnsureStyleRuleFor.
mAnimations[animIdx]->CanThrottle() returns false because mIsRunningOnCompositor is false at that time.  (mIsRunningOnCompositor is set false in nsAnimationManager::CheckAnimationRule)

I think Animation::CanThrottle() should return true if the animation is offscreen (and invisible too).

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: An attempt to solve the problem in EnsureStyleRuleFor()

This patch does not fix as the previous one. The animation does not throttle.
But if a comment above calling CanThrottle() in EnsureStyleRuleFor() is correct, we should check mFinishedAtLastComposeStyle instead of calling CanThrottle().

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Possible fix

I think mFinishedAtLastComposeStyle condition in the previous attempt is inverted. mFinishedAtLastComposeStyle is true if the animation state is |finished| so we can not throttle the animation if mFinishedAtLastComposeStyle is true.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

:birtles, I think the mFinishedAtLastComposeStyle in Animation::CanThrottle() is also inverted.
What do you think?

https://hg.mozilla.org/mozilla-central/file/e7e69cc8c07b/dom/animation/Animation.cpp#l478

Unfortunately attachment 8633316 [details] [diff] [review] causes a lot of failures in layout/style/test/test_animations_omta.html. I will investigate failure reasons.

Comment 13

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> I think mFinishedAtLastComposeStyle condition in the previous attempt is
> inverted. mFinishedAtLastComposeStyle is true if the animation state is
> |finished| so we can not throttle the animation if
> mFinishedAtLastComposeStyle is true.

I'm not exactly sure what you're asking but, in any case, I think you can just change Animation::CanThrottle directly without introducing a FinishedAtLastComposeStyle() method. CanThrottle() is only called by EnsureStyleRuleFor().

Which condition in CanThrottle() is causing it to return false? Is it because we don't generate a layer for the offscreen content, so we never do the layer transaction that sets mIsRunningOnCompositor to true?

If that's the case, maybe we need another flag on Animation for "is invisible" and then in CanThrottle we can return true if mIsInvisible is set?

(Unfortunately mIsRunningOnCompositor is not very accurate: bug 1151694)

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #13)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> > I think mFinishedAtLastComposeStyle condition in the previous attempt is
> > inverted. mFinishedAtLastComposeStyle is true if the animation state is
> > |finished| so we can not throttle the animation if
> > mFinishedAtLastComposeStyle is true.
> 
> I'm not exactly sure what you're asking but, in any case, I think you can
> just change Animation::CanThrottle directly without introducing a
> FinishedAtLastComposeStyle() method. CanThrottle() is only called by
> EnsureStyleRuleFor().

Wow, thanks! I thought CanThrottle() is used in various places.

> Which condition in CanThrottle() is causing it to return false? Is it
> because we don't generate a layer for the offscreen content, so we never do
> the layer transaction that sets mIsRunningOnCompositor to true?

Right. And mFinishedAtLastComposeStyle condition (It will not be a problem if CanThrottle() returns true before checking mIsRunningOnCompositor in case of this bug though).

> If that's the case, maybe we need another flag on Animation for "is
> invisible" and then in CanThrottle we can return true if mIsInvisible is set?

Thanks, I will try to implement it.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Hmm, EnsureStyleRuleFor() with EnsureStyleRule_IsThrottled is only invoked when AnimationCollection::CanPerformOnCompositorThread() is true. So chechking mIsRunningCompositor in Animation::CanThrottle() is redundant for now and it is weird that the value is false in there...

https://hg.mozilla.org/mozilla-central/file/e7e69cc8c07b/layout/style/nsAnimationManager.cpp#l905

Comment 16

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

Comment on attachment 8633316 [details] [diff] [review]
Possible fix

I'm a little unsure about what you're trying to do here.  This patch looks like it completely disables throttling of animations since it removes the only caller of CanThrottle(), so we no longer have anything that throttles animations (suppresses style updates on the main thread) when they're running on the compositor.  Although I don't understand the logic of the change to CanThrottleAnimation() and how that relates to the other change.  I'm also not sure what feedback you want.

I think in the end we're going to want to take the approach in comment 8, since that accomplishes more in total.

I'm also not sure why the commit message references finished animations; finished animations should (once they've refreshed once since finishing) have mNeedsRefreshes false, so they shouldn't even be causing ticking of the refresh driver.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Throttle offscreen animations

This patch does the similar thing to bug 1145439.
I hope this patch gets closer to the right thing what we should do there.

As far as I run animation tests locally there is no regression.

Any feedback would be very appreciated.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ab18c38b73
Still running.

Comment 19

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

No, we want to throttle animations if the element is offscreen, not just if the entire page is undisplayed.  (When the entire page is undisplayed, we ought to be throttling the refresh driver down to a much lower frequency already, although maybe the test you have covers more conditions than the test we use for that?)

Comment 20

[Michael:mtreese]

2.5+ under the assumption that this bug would substantially degrade performance on a device.

Comment 21

[Mahendra Potharaju [:mahe]]

Hiro, 

Can you please share some progress on this bug? Its a blocker for TV and there hasn't been much updates here. 

Thanks

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

I'd revisit this being a blocker at this point, perhaps deciding that bug 1103207 is a better candidate to pick up?

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Reviewing process of patch set for a dependent bug (bug 1216030) has been started yesterday.

If the issue on TV product is caused by animations on visibility:hidden elements and the elements do not affect layout, I'd suggest to use display:none for the elements.  All of animations in display:none subtree stop (Fixed by bug 1197620).

Comment 24

[Mahendra Potharaju [:mahe]]

Thank you Hiro. 

Josh - Please look at comment 23 and let respective TV dev know. Hiro's comment could help.

Comment 25

[Josh Cheng [:josh]]

Hi SC, Hi ShianYow,
Could you help to check comment 23 from Hiroyuki and see whether we need the work around?
Thanks,

Comment 26

[Shian-Yow Wu [:swu]]

There is no known TV apps impacted by this bug, but not sure if any potential 2d gaming apps will hit such scenario.  If such case, the suggestion of comment 23 from Hiroyuki can help.

Comment 27

[Andrew Overholt [:overholt]]

(In reply to Milan Sreckovic [:milan] from comment #22)
> I'd revisit this being a blocker at this point, perhaps deciding that bug
> 1103207 is a better candidate to pick up?

Looks like bug 1103207 has yet to land but has an r+'d patch. Mahe told me we'd be picking up 2.5 fixes post-44 branch so maybe bug 1103207 can be uplifted to the b2g 2.5 branch?

Comment 28

[Mahendra Potharaju [:mahe]]

Thanks NI Andrew. 

Yes, we will need to land this on 2.5. 

Will set the 2.5+ flag on bug 1103207 to track it to land.

Comment 29

[Andrew Overholt [:overholt]]

Seems like we should remove the 2.5? here and ensure we get bug 1103207 into 2.5 (it's already 2.5+).

Comment 30

[Hiroyuki Ikezoe (:hiro)]

I am pretty sure the test case in comment 1 has been properly optimized now.