1225654 - GTA CSS demo renders incorrectly and slowly

Status: RESOLVED WORKSFORME

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Demo:
http://codepen.io/Beclamide/full/bNyxVz/

I'm getting 25 FPS vs. what is probably 60 FPS on chrome. There's major correctness issue.

This is probably the same issue as the CSS FPS demo but I can't find the bug ATM.

Comment 1

[Benoit Girard (:BenWa)]

Not sure if you're still looking at 3D CSS issues :kip?

Comment 2

[u279076]

I can reproduce the general performance and correctness issues but where do I see the FPS?

Comment 3

[:kip (Kearwood Gilbert)]

The rendering correctness issues appear to be a polygon sorting issue.  It could simply be that the maximum number of layers we sort has been hit, or could be something deeper.  I'll take a closer look.

Comment 4

[Thinker Li [:sinker]]

I have tried the patch on bug .  The correctness is fixed, but it is still slow.  I did profiling, and found that the bottleneck is at the content process to build displaylist, paint, and forward.  This example have a div on the top of the scene, it changes the transform of the scene regularly.  So, Gecko spends most of time on |PaintFrame()|.

https://cleopatra.io/#report=952c42f54084ab3e2552684fb724a4d97ceb3cd3&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A34577,%22end%22%3A36610%7D%5D&selection=0,10136,10137,350,351,10138,10139,349,350,351,352,353,354,355,356,357,358,359,360,361,11054,11061,11062,11063,419,420,421,422,497,498,499,500,501,502,526,527,1090,1091,1092,1093,1094,1095,1096,599,600,601,1097,1098

Comment 5

[Thinker Li [:sinker]]

Attachment: fast-path-js-transform-animation.diff

Comment 6

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #5)
> Created attachment 8749620 [details] [diff] [review]
> fast-path-js-transform-animation.diff

This patch forward layers to the compositor after update transforms, without reflow and paint.
So, no display list has been built, and layer tree has not been recreated.  It get 28fps on my laptop for the example here with this patch and https://bugzilla.mozilla.org/attachment.cgi?id=8744134 on bug 1208646.

Comment 7

[Thinker Li [:sinker]]

kip, how do you think about this approach?

Comment 8

[:kip (Kearwood Gilbert)]

(In reply to Thinker Li [:sinker] from comment #7)
> kip, how do you think about this approach?

This looks good to me.

Comment 9

[Thinker Li [:sinker]]

Attachment: Change layer transforms without reflow & repaint, v2

--
Attachment #8749620 [details] [diff] - Attachment is obsolete: true

Comment 10

[Thinker Li [:sinker]]

Attachment: Remove redundant computation in ComputePreserve3DChildrenOverflow()

Comment 11

[Thinker Li [:sinker]]

Now, bottleneck seems to be at compositor.  It uses up to 92+% of CPU time while the main thread of content process uses only 49%.

Comment 12

[Thinker Li [:sinker]]

https://cleopatra.io/#report=ff11083486eafb480b00ab89980b0a1b5effd4ac&selection=0,2265,239,83,2466,2467

Comment 13

[Thinker Li [:sinker]]

Attachment: Stop doing LayerTreeInvalidation

Comment 14

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #13)
> Created attachment 8750708 [details] [diff] [review]
> Stop doing LayerTreeInvalidation

Another improvement for 10%.
Since we draw layers with GPU, culling here seems to be useless.  I guess we can stop LayerTreeInvaidation for certain situation.  For instance, GPU is enabled, has no presentation API, is on desktop (not for mobile?), and number of layer is up to some threshold.

Comment 15

[Thinker Li [:sinker]]

28fps ~ 50fps, 32fps in average.  Compositor is the bottleneck.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8750644 [details] [diff] [review]
Change layer transforms without reflow & repaint, v2

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

We already have  nsIFrame::TryUpdateTransformOnly which does this for simple transform changes. We should be able to extend that to handle the case you're hitting.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8750708 [details] [diff] [review]
Stop doing LayerTreeInvalidation

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

LayerManagerD3D11 and BasicCompositor restrict composition to the dirty rect we compute, and it's shown real performance improvements.

We also need to know the dirty region (or just changed layers?) for Hardware composer 2d, but I'm not sure how much we care about that any more.

There's also ContainerLayer intermediate surface caches that need to be invalidated.

We could skip a lot of work for CompositorD3D9 and CompositorOGL (non-b2g) cases easily, by only looking for ContainerLayers to invalidate.

We could also detect sufficiently complex layer trees, and just abandon the precise calculations in that case.

Comment 18

[Matt Woodrow (:mattwoodrow)]

(In reply to Thinker Li [:sinker] from comment #14)
> (In reply to Thinker Li [:sinker] from comment #13)
> > Created attachment 8750708 [details] [diff] [review]
> > Stop doing LayerTreeInvalidation
> 
> Another improvement for 10%.
> Since we draw layers with GPU, culling here seems to be useless.  I guess we
> can stop LayerTreeInvaidation for certain situation.  For instance, GPU is
> enabled, has no presentation API, is on desktop (not for mobile?), and
> number of layer is up to some threshold.

Sorry, I didn't read this properly before making comment 17.

You've basically covered everything I said, so yes, looks good!

Comment 19

[Thinker Li [:sinker]]

Attachment: Fix TryUpdateTransformOnly() for preserve-3d

Comment 20

[Thinker Li [:sinker]]

Comment on attachment 8753326 [details] [diff] [review]
Fix TryUpdateTransformOnly() for preserve-3d

Matt, is this what you want?

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8753326 [details] [diff] [review]
Fix TryUpdateTransformOnly() for preserve-3d

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

Looking good, just worried about the scale factors still.

::: layout/generic/nsFrame.cpp
@@ +5633,5 @@
>    // (We can often do better than this, for example for scale-down
>    // changes.)
> +  //
> +  // Since preserve-3d is handling at compositor, we don't take care
> +  // scale-factors for these layers here anymore.

It looks like ChooseScaleAndSetTransform doesn't check Extend3DContext(), so it seems possible that scale factors from this transform will be passed down to descendants.

Changing the transform could change the scale factors, which would result in us re-rasterizing the children when taking the normal code path and not when taking this one.

I think it's preferable to maintain behaviour when taking this optimization.

One option is to factor out the code for computing scale factors into a separate function (hard, since it takes ancestors transforms into account) and then confirm that they match for the between the old and new transform.

The other is to disable scale factor for Extend3DContext layers, and then we no longer have the problem. We may instead get low quality content when scaling up though, which isn't great. I think we'd still want a helper function that both ChooseScaleAndSetTransform and this code both use, so that's obvious that the two callers are in sync.

Comment 22

[Thinker Li [:sinker]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> The other is to disable scale factor for Extend3DContext layers, and then we
> no longer have the problem. We may instead get low quality content when
> scaling up though, which isn't great.

It is inevitable to re-rasterize the children to keep a certain quality when a container layer scales up it's children.  We can get scale factors from mInherited(X|Y)Scale and mPre(X|Y)Scale of layers.    So, we can compute new base transform from mInherited(X|Y)Scale.  And mPre(X|Y)Scale would be applied by Layer::GetTransform() on the fly.

Comment 23

[Thinker Li [:sinker]]

Attachment: Fix TryUpdateTransformOnly() for preserve-3d, v2

--
Attachment #8753326 [details] [diff] - Attachment is obsolete: true

Comment 24

[Thinker Li [:sinker]]

Attachment: Fix TryUpdateTransformOnly() for preserve-3d, v3

--
Attachment #8753685 [details] [diff] - Attachment is obsolete: true

Comment 25

[Thinker Li [:sinker]]

Comment on attachment 8753686 [details] [diff] [review]
Fix TryUpdateTransformOnly() for preserve-3d, v3

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

::: layout/generic/nsFrame.cpp
@@ +5633,5 @@
> +  if (container && Extend3DContext()) {
> +    // Since preserve-3d is handling at compositor, we just apply
> +    // scale-factors for these layers here anymore.
> +    transform.PostScale(container->GetInheritedXScale(),
> +                        container->GetInheritedYScale());

Here is what I had mentioned at comment 22.