The default bug view has changed. See this FAQ.

OMTA-able animations not throttled for offscreen elements

NEW
Assigned to

Status

()

Core
Layout
P2
normal
2 years ago
a year ago

People

(Reporter: mstange, Assigned: hiro)

Tracking

(Depends on: 1 bug, {perf, power})

Trunk
perf, power
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Power:P2][platform])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8529294 [details]
testcase
Keywords: perf, power

Updated

2 years ago
Duplicate of this bug: 1104726

Updated

2 years ago
blocking-b2g: --- → 2.2?
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.

Updated

2 years ago
Flags: needinfo?(milan)
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...
Flags: needinfo?(milan) → needinfo?(sotaro.ikeda.g)
I do not know current gaia hit this. Anyway, even if it happen, gaia side could do workaround.
Flags: needinfo?(sotaro.ikeda.g)
(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.
blocking-b2g: 2.2? → 3.0?
Blocks: 980770
No longer blocks: 980770
Depends on: 1166500
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.
(Assignee)

Comment 9

2 years ago
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).
Assignee: nobody → hiikezoe
(Assignee)

Comment 10

2 years ago
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().
Attachment #8631302 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
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.
Attachment #8633210 - Attachment is obsolete: true
Attachment #8633316 - Flags: feedback?(dbaron)
(Assignee)

Comment 12

2 years ago
: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.
Flags: needinfo?(bbirtles)
(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)
Flags: needinfo?(bbirtles)
(Assignee)

Comment 14

2 years ago
(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.
(Assignee)

Comment 15

2 years ago
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.
Attachment #8633316 - Flags: feedback?(dbaron) → feedback-
(Assignee)

Comment 17

2 years ago
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.
Attachment #8633316 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
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?)

Comment 20

2 years ago
2.5+ under the assumption that this bug would substantially degrade performance on a device.
blocking-b2g: 2.5? → 2.5+
Whiteboard: [Power]
Priority: -- → P2
Whiteboard: [Power] → [Power:P2]

Updated

2 years ago
Blocks: 1214245

Updated

2 years ago
Whiteboard: [Power:P2] → [Power:P2][platform]
Hiro, 

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

Thanks
Flags: needinfo?(hiikezoe)
I'd revisit this being a blocker at this point, perhaps deciding that bug 1103207 is a better candidate to pick up?
blocking-b2g: 2.5+ → 2.5?
(Assignee)

Comment 23

a year ago
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).
Flags: needinfo?(hiikezoe)
Thank you Hiro. 

Josh - Please look at comment 23 and let respective TV dev know. Hiro's comment could help.
Flags: needinfo?(jocheng)
Hi SC, Hi ShianYow,
Could you help to check comment 23 from Hiroyuki and see whether we need the work around?
Thanks,
Flags: needinfo?(swu)
Flags: needinfo?(schien)
Flags: needinfo?(jocheng)
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.
Flags: needinfo?(swu)
Flags: needinfo?(schien)

Updated

a year ago
Depends on: 1216030
(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?
Flags: needinfo?(mpotharaju)
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.
Flags: needinfo?(mpotharaju)
Seems like we should remove the 2.5? here and ensure we get bug 1103207 into 2.5 (it's already 2.5+).
blocking-b2g: 2.5? → ---

Updated

a year ago
No longer blocks: 1214245
(Assignee)

Updated

a year ago
Depends on: 1237454
You need to log in before you can comment on or make changes to this bug.