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.

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.

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...

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

(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.

[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.

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.

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).

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().

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.

: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.

(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)

(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.

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 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.

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.

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

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?)

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

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

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

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).

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

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

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.

(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?

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.

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

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