1471221 - Flicker at the end of Google expando animation demo

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P3)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

URL: https://googlechromelabs.github.io/ui-element-samples/animated-clip/advanced/

At the end of the animation, the "Close" link flickers. It doesn't in Chrome.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Doesn't happen on WebRender.

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I doesn't seem to flicker much on Linux for me, but on a low end Windows machine it flickers really noticeable.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I managed to get the regression range down to the following:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=587daa4bdc4b40b7053f4ca3b74723ca747f3b52&tochange=4d3de12dcdc539f14fcb06539da39fa7176c8955

Trying to bisect it further, however, I get content process crashes on startup. I'll need to build locally to bisect further.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks to Hiro's help, I confirmed that this is regressed by advanced layers. If I set layers.mlgpu.dev-enabled to false, there is no flicker.

I'm seeing this on a Dell XPS 15 laptop and a Surface Pro (both of which use Intel HD graphics).

Kats, any ideas where to look?

Comment 5

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

I don't really know much about advanced layers, redirecting to Bas.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

This seems less like a performance issue and more of a general graphics glitch. qf-'ing for now, please renominate if we're mistaken!

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

Jamie, would you know how to investigate about this regression related to advanced layers? Thanks!

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Hi Ryan, would you happen to have any ideas about this regression from Advanced Layers?

Comment 9

[Jamie Nicol [:jnicol]]

Sorry, I forgot to reply. Bit I am looking in to this.

Comment 10

[Jamie Nicol [:jnicol]]

In Advanced Layers, we are very aggressive about computing the visible bounds for a container layer so that we only allocate the smallest necessary intermediate surface. See ContainerLayerMLGPU::OnPrepareToRender(). We AND together GetShadowVisibleRegion with ComputeIntermediateSurfaceBounds (which is the union of the children's visible bounds transformed in to the intermediate surface space).

The problem on this testcase, is that the contents of the menu have a compositor transform animation running. A child layer is asynchronously translated outwith the visible region of its parent, so it is clipped. When the transform finishes, the visible region of the parent is updated so it contains the child again. That's why we get a "flicker".

We could make the visible region calculation in FrameLayerBuilder take in to account the animation, but this would be less accurate as it would have to cover the entire duration of the animation rather than the value at the time of composition.

If ComputeIntermediateSurfaceBounds returns Some, then can we just use it directly instead of ANDing with mShadowVisibleRegion? Doesn't it calculate effectively the same thing as we did to set the visible region in FrameLayerBuilder, except it will take async transforms in to account?

Comment 11

[Matt Woodrow (:mattwoodrow)]

Right, so we have an issue where async transforms move a layer, but the ancestors of that layer don't get their shadow visible regions updated to account for the change.

For non-Advanced Layers we run LayerManagerComposite::PostProcessLayers, which walks the whole Layer tree and recomputes the shadow visible region for all layers from the leaves up.

Looking at the code, I think even ComputeIntermediateSurfaceBounds() could return the wrong area if there are two nested ContainerLayers with intermediate surfaces, and then an async transform within that.

The inner ContainerLayer would walk the children and get the new position for the async transform, but the outer ContainerLayer only descends down to layers that have an intermediate surface, so it'd grab the (wrong) shadow visible region from the inner.

If we stopped using mShadowVisibleRegion, and also made ComputeIntermediateSurfaceBounds/FindVisibleBounds descend through containers with an intermediate surface (maybe we could update their shadow visible region and flag it as being valid now?) then I think it would be ok.

Little bit worried about the perf cost of this though.

Comment 12

[Marcia Knous [:marcia]]

Hello Jamie - Do you have an update on this bug? Thanks.

Comment 13

[Jamie Nicol [:jnicol]]

Attachment: Bug 1471221 - Calculate intermediate surface bounds in advanced layers in pre-pass. r=mattwoodrow

Advanced layers was calculating incorrect visible regions for
container layers with intermediate surfaces, as it was not taking
async transform animations in to account. Rather than recursing the
depth of the tree for each layer to calculate this in
OnPrepareToRender, do a single pre-pass at the start of the frame.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9004809 [details]
Bug 1471221 - Calculate intermediate surface bounds in advanced layers in pre-pass. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc155fd5c1f0
Calculate intermediate surface bounds in advanced layers in pre-pass. r=mattwoodrow

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/fc155fd5c1f0