1456389 - Investigate why transform (or opacity) animation on visibility:hidden element is not throttled by offscreen throttling machinery on WebRender

Status: NEW

(Core :: DOM: Animation, defect, P3)

Comment 1

[Hiroyuki Ikezoe (:hiro)]

OK, the machinery for animations in visibility:hidden element works as expected.  But it seems that we create a display item for the visibility:hidden element which has opacity or transform animations, thus we try to create a WebRender command for the display item, then the animation is sent to the compositor.  (Note that we force to do the initial paint for whatever animations). 

My best guess is that the display item is for creating a stacking context and on non-WebRender we don't create the dedicated layer in the case where the visibility:hidden element has no visible descendants, whereas on WebRender we don't have such optimization yet?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

> and on non-WebRender we don't create the dedicated layer in the case where
> the visibility:hidden element has no visible descendants

This guess for non-WebRender case wasn't correct.  We do create the layer for such case, but soon after the layer seems to be destroyed?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Yeah, I think you're basically right - we don't have any special handling for visibility:hidden elements so they get treated the same as visible elements. That is, they will create a stacking context with an animation, and on the compositor side we will sample the animation on every composite and trigger a new composite as long as the animation is still going. At no point do we check for visibility. WR internally will not draw the element but that doesn't feed back into the animation sampling code in WebRenderBridgeParent.

This might actually be the cause of bug 1452165.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I verified that we're continuously compositing at 60fps when there are CSS animations that are visibility:hidden. We should fix this.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

So with opacity animations that are visibility:hidden, for example, the FLB code at [1] is what prevents the layer from getting generated from the nsDisplayOpacity item. The animations (which get applied on the layer at [2]) are also not sent to the compositor as far as I can tell.

I wrote a small patch to do the equivalent visibility check and early-exit at the start of nsDisplayOpacity::CreateWebRenderCommands and that seems to work for the opacity animations I tested. Still need to look at transform animations and what the equivalent thing is there. I suspect it will be different because of [3] and other similar checks.

[1] https://searchfox.org/mozilla-central/rev/bf81d741ff5dd11bb364ef21306da599032fd479/layout/painting/FrameLayerBuilder.cpp#4460
[2] https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/layout/painting/nsDisplayList.cpp#6278
[3] https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/layout/painting/nsDisplayList.cpp#8399

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Don't build WR display items for opacity items that are hidden

Thoughts on this? I basically extracted it the relevant codepath in FrameLayerBuilder (see previous comment). I had a few questions:
1) Are there other cases we should handle here (specifically for opacity)?
2) Should this be put in a place that's more generic and applies to other display items as well?
3) With non-WR, it looks like some kinds of animated transforms (e.g. rotation) don't use OMTA and just always send over a display list but others (e.g. translation) do use OMTA. Is that correct?
4) With non-WR, it looks like even visibility:hidden animated transformed elements will get layers in the layers tree (which is different from opacity items). Is this correct, and if so, is it because the transform animation can result in the element becoming visible? In these cases it looks like the compositor-side layer tree invalidation is what prevents us from recompositing but we do all the other work involved per-frame.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: An example

The particular test case in file_restyles.html will be fixed by bug 1471174.  But will there are some cases we don't yet optimize.  Here is an example.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> But will there are some cases we don't yet optimize.  Here is an example.

*But still*

Comment 9

[Matt Woodrow (:mattwoodrow)]

Sorry for taking so long to get to this.

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> Thoughts on this? I basically extracted it the relevant codepath in
> FrameLayerBuilder (see previous comment). I had a few questions:
> 1) Are there other cases we should handle here (specifically for opacity)?
> 2) Should this be put in a place that's more generic and applies to other
> display items as well?

I think we should, won't transforms have the same issue?

Doing less work for all invisible items (regardless of whether they have async animations) also seems worthwhile.


> 3) With non-WR, it looks like some kinds of animated transforms (e.g.
> rotation) don't use OMTA and just always send over a display list but others
> (e.g. translation) do use OMTA. Is that correct?

That's doesn't sound expected to me at all. I think we're skipping OMTA for preserve-3d still, but that's coming soon! (thanks Hiro)

> 4) With non-WR, it looks like even visibility:hidden animated transformed
> elements will get layers in the layers tree (which is different from opacity
> items). Is this correct, and if so, is it because the transform animation
> can result in the element becoming visible? In these cases it looks like the
> compositor-side layer tree invalidation is what prevents us from
> recompositing but we do all the other work involved per-frame.

This also seems like a bug. We do want not-currently-visible transforms to be created for the reasons you stated, but visibility:hidden can't change on the compositor side, so we can skip it properly.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> > 1) Are there other cases we should handle here (specifically for opacity)?
> > 2) Should this be put in a place that's more generic and applies to other
> > display items as well?
> 
> I think we should, won't transforms have the same issue?
> 
> Doing less work for all invisible items (regardless of whether they have
> async animations) also seems worthwhile.

Agreed. I'm just trying to figure out where the best place is to put this code so that it affects all the right codepaths. It seems error-prone to copy it into each display item manually. If the code is correct for all display items then maybe we can do it in WebRenderCommandBuilder instead.

> > 3) With non-WR, it looks like some kinds of animated transforms (e.g.
> > rotation) don't use OMTA and just always send over a display list but others
> > (e.g. translation) do use OMTA. Is that correct?
> 
> That's doesn't sound expected to me at all. I think we're skipping OMTA for
> preserve-3d still, but that's coming soon! (thanks Hiro)

Ok, I filed bug 1471578 with the testcase where I was seeing the issue. It turned out to be more subtle than I thought, so I might have just been running into an edge case.

> This also seems like a bug. We do want not-currently-visible transforms to
> be created for the reasons you stated, but visibility:hidden can't change on
> the compositor side, so we can skip it properly.

Where is the gecko code that handles this? I thought it was [1] but ShouldBuildLayerEvenIfInvisible will return true for nsDisplayTransform items that have animations. Or am I missing something here?

[1] https://searchfox.org/mozilla-central/rev/bf81d741ff5dd11bb364ef21306da599032fd479/layout/painting/FrameLayerBuilder.cpp#4460

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8985176 [details] [diff] [review]
Don't build WR display items for opacity items that are hidden

Clearing feedback flag for now. We'll eventually want some sort of patch similar to this but I need to look into more how the layers code handles visibility:hidden animations.

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Here's a profile: https://share.firefox.dev/3fdZLPw

Hiro, this seems like still an issue, but is it only an issue when a descendant of the animated element becomes and then un-becomes visible like in your test-case?

If it's an issue for the more usual case (hiding the animating element or an ancestor of it) we should definitely fix it.

If not, it might not be super-prioritary (though still nice to fix ofc).