Last Comment Bug 1105509 - OMTA-able animations not throttled for offscreen elements
: OMTA-able animations not throttled for offscreen elements
Status: NEW
[Power:P2][platform]
: perf, power
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
P2 normal with 2 votes (vote)
: ---
Assigned To: Hiroyuki Ikezoe (:hiro)
:
: Jet Villegas (:jet)
Mentors:
: 1104726 (view as bug list)
Depends on: 1237454 1166500 1216030
Blocks:
  Show dependency treegraph
 
Reported: 2014-11-26 14:15 PST by Markus Stange [:mstange]
Modified: 2016-01-06 15:58 PST (History)
26 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (850 bytes, text/html)
2014-11-26 14:15 PST, Markus Stange [:mstange]
no flags Details
Incomplete fix (1.33 KB, patch)
2015-07-08 15:29 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Splinter Review
An attempt to solve the problem in EnsureStyleRuleFor() (3.34 KB, patch)
2015-07-13 16:52 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Splinter Review
Possible fix (3.40 KB, patch)
2015-07-13 23:14 PDT, Hiroyuki Ikezoe (:hiro)
dbaron: feedback-
Details | Diff | Splinter Review
Throttle offscreen animations (4.17 KB, patch)
2015-08-05 21:46 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Splinter Review

Description User image Markus Stange [:mstange] 2014-11-26 14:15:38 PST
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 User image Markus Stange [:mstange] 2014-11-26 14:15:59 PST
Created attachment 8529294 [details]
testcase
Comment 2 User image Sotaro Ikeda [:sotaro] 2014-11-27 10:49:16 PST
*** Bug 1104726 has been marked as a duplicate of this bug. ***
Comment 3 User image David Baron :dbaron: ⌚️UTC-8 2014-12-08 11:23:33 PST
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 User image Milan Sreckovic [:milan] 2015-02-11 00:58:30 PST
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 User image Sotaro Ikeda [:sotaro] 2015-02-11 01:06:19 PST
I do not know current gaia hit this. Anyway, even if it happen, gaia side could do workaround.
Comment 6 User image Sotaro Ikeda [:sotaro] 2015-02-11 01:07:24 PST
(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 User image No-Jun Park [:njpark] 2015-02-19 12:58:07 PST
[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 User image David Baron :dbaron: ⌚️UTC-8 2015-05-19 19:03:38 PDT
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 User image Hiroyuki Ikezoe (:hiro) 2015-07-08 15:29:30 PDT
Created attachment 8631302 [details] [diff] [review]
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 User image Hiroyuki Ikezoe (:hiro) 2015-07-13 16:52:04 PDT
Created attachment 8633210 [details] [diff] [review]
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 User image Hiroyuki Ikezoe (:hiro) 2015-07-13 23:14:58 PDT
Created attachment 8633316 [details] [diff] [review]
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 User image Hiroyuki Ikezoe (:hiro) 2015-07-13 23:17:33 PDT
: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 User image Brian Birtles (:birtles) 2015-07-13 23:37:59 PDT
(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 User image Hiroyuki Ikezoe (:hiro) 2015-07-13 23:48:06 PDT
(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 User image Hiroyuki Ikezoe (:hiro) 2015-07-14 00:32:17 PDT
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 User image David Baron :dbaron: ⌚️UTC-8 2015-07-14 01:27:39 PDT
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 User image Hiroyuki Ikezoe (:hiro) 2015-08-05 21:46:13 PDT
Created attachment 8644135 [details] [diff] [review]
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 User image Hiroyuki Ikezoe (:hiro) 2015-08-05 21:47:38 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ab18c38b73
Still running.
Comment 19 User image David Baron :dbaron: ⌚️UTC-8 2015-08-05 21:53:53 PDT
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 User image Michael:mtreese 2015-08-12 11:09:37 PDT
2.5+ under the assumption that this bug would substantially degrade performance on a device.
Comment 21 User image Mahendra Potharaju [:mahe] 2015-10-22 10:42:15 PDT
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 User image Milan Sreckovic [:milan] 2015-10-22 10:58:21 PDT
I'd revisit this being a blocker at this point, perhaps deciding that bug 1103207 is a better candidate to pick up?
Comment 23 User image Hiroyuki Ikezoe (:hiro) 2015-10-22 12:58:38 PDT
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 User image Mahendra Potharaju [:mahe] 2015-10-22 15:17:19 PDT
Thank you Hiro. 

Josh - Please look at comment 23 and let respective TV dev know. Hiro's comment could help.
Comment 25 User image Josh Cheng [:josh] 2015-10-26 07:55:14 PDT
Hi SC, Hi ShianYow,
Could you help to check comment 23 from Hiroyuki and see whether we need the work around?
Thanks,
Comment 26 User image Shian-Yow Wu [:swu] 2015-10-27 00:16:07 PDT
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 User image Andrew Overholt [:overholt] 2015-10-29 10:08:46 PDT
(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 User image Mahendra Potharaju [:mahe] 2015-10-29 10:25:13 PDT
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 User image Andrew Overholt [:overholt] 2015-11-04 08:39:15 PST
Seems like we should remove the 2.5? here and ensure we get bug 1103207 into 2.5 (it's already 2.5+).

Note You need to log in before you can comment on or make changes to this bug.