Closed Bug 1100357 Opened 10 years ago Closed 4 years ago

Async transform animations are disabled for large layers

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Project Flags:
Performance Impact low

People

(Reporter: cwiiis, Unassigned)

References

(Blocks 3 open bugs)

Details

(Keywords: perf, perf:responsiveness, Whiteboard: [layout:backlog])

Attachments

(2 files)

Don't always disable layer pre-rendering for large transformed frames
4.68 KB, patch
Details | Diff | Splinter Review
tangentially related testcase
575 bytes, text/html
Details 
We currently have code that disables OMTA for layers that are larger than the viewport. I assume the rationale is to stop possible pre-rendering of absolutely huge layers.

This seems reasonable (though I'd question the viewport being the size limitation), but doesn't make sense if that layer would be active regardless of the animation.

This also doesn't make sense for colour layers, whose size beyond a certain point doesn't really make a difference.

This is affecting animations in the vertical home-screen when large groups are moved - during this transition, the background is an active colour layer with no children, but OMTA will be disabled if it's larger than the screen (which is a reasonable common occurrence), and cause the animation to look janky.
Something like this, I guess. I may be totally off, but this fixes the issue for me and still works in the situations that I think it should.

Things that stand out to me:
- Could we do with a define for layer types that will always be active?
- Should the for-loop that checks for all-active-children be factored out into a method on nsDisplayTransform?
- Is it worth getting the transform from the frame for transformed frames to check if it's 2D, in the absence of a transform display item? (and if it is and you know off the top of your head, what's the most efficient way to do that?)
Attachment #8523891 - Flags: review?(tnikkel)
One thing I was concerned about with this patch is that it would pre-render the very large <body> in this testcase just because it has a transform and an active transformed child. But the patch doesn't do that, so everything's fine.
Comment on attachment 8523891 [details] [diff] [review]
Don't always disable layer pre-rendering for large transformed frames

Makes sense, but I'm going to defer to Matt, he knows the workings of this code better.

Also, I think you want to use RequiredLayerStateForChildren.
Attachment #8523891 - Flags: review?(tnikkel) → review?(matt.woodrow)
Comment on attachment 8523891 [details] [diff] [review]
Don't always disable layer pre-rendering for large transformed frames

Review of attachment 8523891 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we can do this.

Having layers active, and making them pre-rendered is not the same thing. Rendering areas that are offscreen has the potential to be very expensive, which is why we currently limit to to the size of the viewport.

This also wont change the caller at [1], so we won't use the correct region for building display lists.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1956
Attachment #8523891 - Flags: review?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Comment on attachment 8523891 [details] [diff] [review]
> Don't always disable layer pre-rendering for large transformed frames
> 
> Review of attachment 8523891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think we can do this.
> 
> Having layers active, and making them pre-rendered is not the same thing.
> Rendering areas that are offscreen has the potential to be very expensive,
> which is why we currently limit to to the size of the viewport.

I understand that pre-rendering and active layers aren't the same thing, but if all the child items of a pre-rendered item are active, the pre-rendered layer would be empty, wouldn't it? So whether it decides to pre-render or not is irrelevant? (except in this case, the async animation data is stored on the container layer)

> This also wont change the caller at [1], so we won't use the correct region
> for building display lists.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1956

I saw this, but wondered if it would actually matter(?) If it does, any ideas on working around it? Can a transform item be the root of a stacking context?

Assuming this is just totally the wrong way to go about it, could you suggest a right way to go about it? The current behaviour is definitely quite limiting.
Flags: needinfo?(matt.woodrow)
(In reply to Chris Lord [:cwiiis] from comment #6)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> > Comment on attachment 8523891 [details] [diff] [review]
> > Don't always disable layer pre-rendering for large transformed frames
> > 
> > Review of attachment 8523891 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I don't think we can do this.
> > 
> > Having layers active, and making them pre-rendered is not the same thing.
> > Rendering areas that are offscreen has the potential to be very expensive,
> > which is why we currently limit to to the size of the viewport.
> 
> I understand that pre-rendering and active layers aren't the same thing, but
> if all the child items of a pre-rendered item are active, the pre-rendered
> layer would be empty, wouldn't it? So whether it decides to pre-render or
> not is irrelevant? (except in this case, the async animation data is stored
> on the container layer)

I'm not sure what you mean by this. If the only child is an active opacity layer, which then has a child painted layer with content that covers 100k x 100k pixels, then this will treat that entire region as 'visible' and paint it (though scaled down like crazy so that it fits within a texture). There's some pretty bad edge cases here.

> 
> > This also wont change the caller at [1], so we won't use the correct region
> > for building display lists.
> > 
> > [1]
> > http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#1956
> 
> I saw this, but wondered if it would actually matter(?) If it does, any
> ideas on working around it? Can a transform item be the root of a stacking
> context?
> 
> Assuming this is just totally the wrong way to go about it, could you
> suggest a right way to go about it? The current behaviour is definitely
> quite limiting.

I think we could solve the case where we have only a ColorLayer child without solving the general problem.

The main problem with the general solution (I've tried this before) is that we don't really have a 'max texture size' for tiling, since we can basically always add more tiles. It's very hard to tell if we should render the content at full res regardless of how many tiles it takes, or if we should impose some arbitrary limit and scale content down to fit within in. The b2g mail app has an active transform around a huge area, and they aren't ok with mail content being scaled down (unsurprisingly). I haven't figured out sane heuristics that solve all use cases.
Flags: needinfo?(matt.woodrow)
See Also: → 1104081
Blocking app-grouping on bug 1104081 instead.
No longer blocks: app-grouping
I think this would be a nice one to fix - I'm commonly seeing weird animations in FxOS where async animation is disabled in a situation I'm pretty certain it doesn't actually make any savings in.

n?jet to see if there's any resources to have a look at this. Possibly not in the near future, but it'd be good to make sure it doesn't get lost.
Flags: needinfo?(bugs)
Hiro might be interested after he's finished work on the Frame Timing API (late Q3~Q4).
Flags: needinfo?(bugs)
Thinker, were you going to look at this?
Flags: needinfo?(tlee)
We really need to fix this. It's making it hard to create smooth cartoons where sprites are larger than the viewport (e.g. background assets). Regarding the pathological edge cases, I wonder if our will-change budgeting can be exploited here. Perhaps we could even use will-change as a signal for when we should try applying this.
Using will-change sounds reasonable to me.  According to the usage, this should apply only for transform animations.  Is there any use cases for opacity?
Summary: Async animations should not be disabled for large layers that are otherwise active anyway → Async transform animations should not be disabled for large layers that are otherwise active anyway
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Using will-change sounds reasonable to me.  According to the usage, this
> should apply only for transform animations.  Is there any use cases for
> opacity?

Oops, sorry we don't have the restriction for opacity animations.
Spoke with dbaron about this briefly. One approach might be to tile large layers so we only need to render part of them at any given time. Kats / Botond might be interested in this?

Test case: https://mozanime.github.io/taketori/scene-take/

This is an example of something we'd like to make smooth (and which looks poor at full-screen on high-res screen or lower-end devices). The full animation includes some other examples: https://mozanime.github.io/taketori/
I won't have time in the forseeable future to look into this; I'll be busy with quantum render work.
(In reply to Brian Birtles (:birtles, high review load, away most of 27 Oct-4 Nov) from comment #15)
> Spoke with dbaron about this briefly. One approach might be to tile large
> layers so we only need to render part of them at any given time.

Making the layer tiled is orthogonal to the actual problem here, I think. We can limit the prerendered region of a layer without tiling. The question is: What do we want the limit to be, and what should happen if OMTA changes the transform such that not-prerendered regions of the layer come into view? Do we want to "checkerboard" until the main thread catches up? Do we want to pause the animation?

Do we know what other browsers do if the animated element is too big to prerender completely?
(In reply to Markus Stange [:mstange] from comment #17)
> (In reply to Brian Birtles (:birtles, high review load, away most of 27
> Oct-4 Nov) from comment #15)
> > Spoke with dbaron about this briefly. One approach might be to tile large
> > layers so we only need to render part of them at any given time.
> 
> Making the layer tiled is orthogonal to the actual problem here, I think. We
> can limit the prerendered region of a layer without tiling. The question is:
> What do we want the limit to be, and what should happen if OMTA changes the
> transform such that not-prerendered regions of the layer come into view? Do
> we want to "checkerboard" until the main thread catches up? Do we want to
> pause the animation?

Ok, that sounds fine, I guess changing the prerendered region in chunks is a kind of tiling.

I'm not sure what the exact limit would be but we can use the content in comment 15 to try different options. Furthermore, we could limit the change to layers that have async animations applied to them.

With regards to the behavior if we don't prerender enough, I think a kind of compositor jank would be the safest option, i.e. pause all compositor animations and then jump to catch up when the prerendered content is available.

Does that make sense?
That makes sense, yes. But we should also have a look at how other browsers handle this.
This bug is very similar to bug 1271983.  (In reply to Markus Stange [:mstange] from comment #17)
> can limit the prerendered region of a layer without tiling. The question is:
> What do we want the limit to be, and what should happen if OMTA changes the
> transform such that not-prerendered regions of the layer come into view? Do

Usually, we can know it during building animations.  It is possible that each single picture doesn't, or very little, overlay with adjacent frames for a translate with a big distance.  For this situation, disabling async animation seems reasonable.

Tiling wins limiting pre-rendered region in some cases.  For examples, when the view port move along a path crossing a layer, tiling use less memory by pre-rendering tiles along the path.
Flags: needinfo?(tlee)
Blocks: 1316658
I'm going to have a look at this because it's blocking our ability to write APZ-friendly parallax effects with scroll-driven animations. I understand it's also causing performance issues for regular (time-driven) animations, such as the one in bug 1316658.
Assignee: nobody → botond
Depends on: 1321412
Depends on: 1324591
Hi Botond, where are we up to with this bug? This might be worth doing in the 57 timeframe.
Flags: needinfo?(botond)
I can try to finish this off after landing the initial version of ScrollTimeline (bug 1321428).

On a related note, Ehsan and I were talking about this feature, and he was wondering if we have any metrics about how often users encounter animations that would benefit from this change. Do you know if we have any such data?
Flags: needinfo?(botond) → needinfo?(bbirtles)
No, we don't have that data. We could probably record it -- we set some state on animations whenever we encounter this but it's not hooked up to telemtry.
Flags: needinfo?(bbirtles)
It would be super useful to collect this telemetry.  I'd like to know if this is a commonly encountered issue in the wild.
Filed bug 1349808 for getting that data.
See Also: → 1349808
Whiteboard: [qf:investigate]
Depends on: 1361915
(In reply to Brian Birtles (:birtles, away May 3~4) from comment #26)
> Filed bug 1349808 for getting that data.

When we attempted to analyze this telemetry, it came to light that it wasn't sufficient. I filed bug 1361915 to collect additional telemetry; see that bug for details.
I had a look at the telemetry added in bug 1349808 and bug 1361915. 

Over the last 10 days, we tried to run ~167 million transform animations on the compositor. Of these, ~6 million could not be run on the compositor because the animated content was too large (the scenario that this bug would address by partial pre-rendering). That's approximately 3% of all transform animations.

(It's worth noting that the 167 million figure includes small animations like CSS spinners and subtle button transitions, where the user probably doesn't even notice or care about jank resulting from not running the animation on the compositor. Of the animations operating on larger elements, where the user is more likely to care, the fraction of affected ones will be even higher than 3%.)

Brian and I think this is a significant fraction of animations that would be improved by this bug. I am therefore re-nominating this bug for Quantum Flow triage.
Whiteboard: [qf:investigate] → [qf]
(In reply to Botond Ballo [:botond] from comment #28)
> Over the last 10 days, we tried to run ~167 million transform animations on
> the compositor.

Correction: that should be ~197 million. Hence the 3% figure (6/197).

The data was taken from here [1] and here [2].

[1] https://mzl.la/2qiaEUV
[2] https://mzl.la/2qiEtEC
Whiteboard: [qf] → [qf:p3]
Keywords: perf
Blocks: 1427203
No longer blocks: 1427203
I spoke with Botond about this briefly in December.

The main work required is bug 1324591. However, there may be some other bugs to sort out in the existing partial pre-rendering implementation. For example, after setting layout.animation.prerender.partial and loading https://mozanime.github.io/taketori/ I see a lot of glitching in the initial bamboo scene.
I'd like to get back to working on this, but I'm busy with other work at the moment, so I'm unassigning myself.

Please see comment 31 for what still needs to be done here. If someone would like to pick up one or both of those tasks, I'd be happy to provide guidance.
Assignee: botond → nobody
Blocks: 1504270
Blocks: 1508522
Blocks: 1213513
Blocks: 1537714
Blocks: 1452017
Summary: Async transform animations should not be disabled for large layers that are otherwise active anyway → Async transform animations are disabled for large layers
Whiteboard: [qf:p3] → [qf:p3:responsiveness]
No longer blocks: 1537714
Blocks: 1197529
Priority: -- → P3
Whiteboard: [qf:p3:responsiveness] → [qf:p3:responsiveness][layout:backlog]
Depends on: 1633979
Blocks: 1649051
Depends on: 1656418
No longer blocks: 1316658

Closing since with enabling partial pre-render (bug 1656418) transform animations on large elements can be run on the compositor now.

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Performance Impact: --- → P3
Keywords: perf:responsiveness
Whiteboard: [qf:p3:responsiveness][layout:backlog] → [layout:backlog]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: