1208646 - 3D CSS demo rendered extremely slowly

Status: NEW

(Core :: Graphics: WebRender, defect)

Description

[Clemens Eisserer]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20150925030206

Steps to reproduce:

Opened the 3D CSS demo at: http://www.keithclark.co.uk/labs/css-fps/nojs/


Actual results:

With firefox the animation is very slow and stutters a lot, whereas it is completely smooth when using Chrome


Expected results:

The animation should be as smooth as when using Chrome

Comment 1

[Loic]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7641104770a80015e63597b58cb312fefcbd9ab4&tochange=621ab19e86db28c38bbbf9119fbf6831ea344c54

Suspected bug: bug 1097464

Comment 2

[Alex]

You can make it run normally if you open the Inspector and hover the mouse over the <div> with class camera. The animation is running, it's just not repainting.

Comment 3

[Thinker Li [:sinker]]

I have tried this the demo.  It is painted normally with the patch on bug 1206418.  But, I am not sure what you talk stutter.  Could you make a video?

Comment 4

[Clemens Eisserer]

https://youtu.be/42Azv9num7I

Comment 5

[Loic]

Same behavior on my machine with FF44, I'll wait for the bug 1206418 to test if it's fixed or not.

Comment 6

[Toshihiro Yamada]

The problem still happens for me after a patch of bug 1206418 applied.

Comment 7

[Toshihiro Yamada]

I noticed that animation plays normally unless I move a mouse.

Comment 8

[Kate Glazko]

Tracking in 44 because regression.

Comment 9

[Thinker Li [:sinker]]

Clemens, I had add a patch for bug 1210784.  It seems also fix the bug here.  Could you check it?

Comment 10

[Toshihiro Yamada]

I tried the patch and the animation seems to be improved apparently (no more stops).
But by moving mouse, animation becomes sluggish (Is it probably an other problem?).

Comment 11

[Thinker Li [:sinker]]

Hmm.. Interest!  I would follow up sluggish problem.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Sinker, is the sluggish problem to be tracked via a separate bug? Is there any more work waiting to happen on this one? Or can I mark it as fixed for FF44? Please let me know. Thanks!

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Bug 1097464 was backed out from Fx44, which should be available on the beta channel in the next day or so. Fx45+ remain affected, however.

Might be interesting to know what animation perf looks like now on Fx44 in light of the backout, since that would probably shed some light on whether or not there's still an issue that needs fixing here or not, given the recent comments.

Comment 14

[Clemens Eisserer]

On Linux the css-demo still stutters heavily (WebContent process consuming 100%), there are z-ordering issues and artifacts all over the place - whereas it is completly smooth and artifact-free with chrome.

Comment 15

[Thinker Li [:sinker]]

For the z-ordering problem, it is because we stop doing z-ordering for too much layers.
Now, we try to do z-ordering for the number less than 100 layers, but we got more (250+) layers now.
I have try to change the maximum number of z-ordering to 300 (MAX_SORTABLE_LAYERS in LayerSorter.cpp), and it works.

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Thinker Li [:sinker] from comment #15)
> For the z-ordering problem, it is because we stop doing z-ordering for too
> much layers.
> Now, we try to do z-ordering for the number less than 100 layers, but we got
> more (250+) layers now.
> I have try to change the maximum number of z-ordering to 300
> (MAX_SORTABLE_LAYERS in LayerSorter.cpp), and it works.

Thanks Thinker! Do you plan to fix this in Beta44? Or will this be coming in 45?

Comment 17

[Clemens Eisserer]

> Do you plan to fix this in Beta44?

I am not convinced this is a proper fix at all - another website might use 450 layers and will be broken again.
I wonder what the reason for introducing such a constant limit anyway, as other browsers seem able to cope with it.

Comment 18

[Thinker Li [:sinker]]

I am not sure if this is good way to fix the bug by removing the limit.  The problem is that our current sorting algorithm is N square.  Maybe we should fix the algorithm, but I don't look into details yet.  I would look into it later.

Comment 19

[Thinker Li [:sinker]]

Attachment: Improve LayerSorter NumEdgesTo(), O(n^2) performance

Comment 20

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #19)
> Created attachment 8701993 [details] [diff] [review]
> Improve LayerSorter NumEdgesTo(), O(n^2) performance

 - Change value of MAX_SORTABLE_LAYERS to 1024.
 - Improve NumEdgesTo()
   - CPU usage of ContainerPrepare() is dropping to 33% from 56+%.

Potential improvements.
 - In some n^2 loops, transforms are computed redundantly.

Comment 21

[Thinker Li [:sinker]]

Attachment: Improve LayerSorter NumEdgesTo(), O(n^2) performance, v2

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

Comment 22

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #21)
> Created attachment 8702008 [details] [diff] [review]
> Improve LayerSorter NumEdgesTo(), O(n^2) performance, v2
> 
> --
> Attachment #8701993 [details] [diff] [diff] - Attachment is obsolete: true

 - Avoid redundant computation in CompareDepth() by moving out visible rect transformation.
 - CPU usage of ContainerPrepare() have been dropped to  24.4%.

Next CPU killer in LayerSorter is DirectedGraph::AddEdge().

Comment 23

[Thinker Li [:sinker]]

Clemens, could you try this patch?

Comment 24

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #22)
> Next CPU killer in LayerSorter is DirectedGraph::AddEdge().
                                    ^^^^^^^^^^^^^^^^^^^^^^^^ RecoverZDepth() actually.

Comment 25

[Clemens Eisserer]

Thinker Li: Sorry, I don't have a prepared machine ready to build gecko.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Thinker, I tried pushing this patch to Try for Clemens to try out but hit Werror bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=14998392&repo=try

That said, I was able to try out the patch locally on Win10 and found the site to still be very stuttery with artifacting.

Comment 27

[Thinker Li [:sinker]]

After studying the problem of artifacts, I found one problem.  Z-order sorting fails for planes that some corners of it falls behind the viewer with perspective view.  For example, there is a plane standing upright to the viewer.  Part of the plane may go behind the viewer, it would make w-component negative.  It means transformed position of corners are with a negative value of the w component. 

I think we have to file another bug for this problem.

Comment 28

[Thinker Li [:sinker]]

Attachment: 3d-css.html

This is the test case for the z-order sorting problem.  A artifact would occur at right wall while the viewer goes back and forth.  It occurs while coroners fall behind the viewer, the |v| of translateZ(v) for DIV#camera goes up to 300px or more.

Comment 29

[Thinker Li [:sinker]]

Attachment: Part 2: Avoid z-ordering for planes laying side by side

Comment 30

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #29)
> Created attachment 8705016 [details] [diff] [review]
> Part 2: Avoid z-ordering for planes laying side by side

We try to build a partial ordered list for layers, but sometime for computation error, we build out a circular ordering graph, it is the root cause of most of artifacts.  This problem have been there even before firefox 4.3.  This patch heals most of artifacts in the demo mentioned at comment 1.  With this patch, it is even more accurate than firefox 4.3.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Definitely an improvement, but still pretty janky and occasional artifacting on my system.

Comment 32

[Thinker Li [:sinker]]

I think this is a long running bug.  I would like to check in patches we already have, and have more following up patches.

Following is my understanding of this bug for now.
 - Most artifacts are caused by circular z-ordering dependence.
 - The material on walls is trembling when the viewer being close to the wall.
 - Computational errors are major reason for both cases.
   - The correctness of sin, cos, and PI maybe the root causes. (need more study)

Plane intersection algorithm is also helpfull to eliminate the problem here by reducing the number of z-ordering dependence.

For the performance issues, I had improve to an extent.  we need more study/profiling.

Robert,
would you review the patches here?  do you have other plans or opinions?

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need to implement plane splitting properly. See bug 689498. Bug 1175311 also tracks an ordering bug.

I think we should really focus on doing that right rather than making a wrong algorithm faster.

Comment 34

[Thinker Li [:sinker]]

Ryan, see comment 33.  It means this bug should depend on bug 689498.  If you think so too, I will stop here, un-assign myself, and mark the dependency.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

I'm inclined to trust yours and roc's judgement here. Looking at older releases, it looks like we're basically trading one poor experience on this site for another :\

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking as this is a regression.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

The change that caused this regression was backed out from Firefox 45. A current Developer Edition build should now work correctly, or you can wait until 45 goes to Beta some time next week.

https://hg.mozilla.org/releases/mozilla-aurora/rev/64ec448f156d

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

Reading back, it seems we may not want to track this for 46 and instead focus on other bugs as mentioned in comment 33.

Comment 39

[Brad Lassey [:blassey] (use needinfo?)]

Jet, can you help sort this out? Maybe get movement on bug bug 689498 or bug 1175311?

Since this is still a regression in 46, we should look at backing out again. Flipping the tracking flag back to +

Comment 40

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

^

Comment 41

[Jet Villegas (inactive)]

We decided to keep bug 1097464 despite this regression because we never rendered the test URL correctly. That is, we want to get correct, then fast. I'll bump bug 1175311 up the priority queue to fix our correctness here.

Comment 42

[Thinker Li [:sinker]]

After a thought, I think I can do more performance improvement in https://bugzilla.mozilla.org/attachment.cgi?id=8702008 .  I guess I can do it in two weeks.  Then, we can land the performance improvement patch.  How do you think?

Comment 43

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8702008 [details] [diff] [review]
Improve LayerSorter NumEdgesTo(), O(n^2) performance, v2

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

Doing this seems reasonable if we want something to uplift.

This isn't an alternative to bug 689498 though, we still need to find someone to work on that.

Comment 44

[Jet Villegas (inactive)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #43)
> Comment on attachment 8702008 [details] [diff] [review]
> Improve LayerSorter NumEdgesTo(), O(n^2) performance, v2
> 
> Review of attachment 8702008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Doing this seems reasonable if we want something to uplift.
> 
> This isn't an alternative to bug 689498 though, we still need to find
> someone to work on that.

Right! My thinking is that we'll fix correctness via bug 689498 and bug 1175311, but will still need to fix performance for layer sorting.

Comment 45

[Kan-Ru Chen [:kanru] (UTC+9)]

Given comment 41 should we drop blocking 46?

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

We are two weeks away from shipping 46, and many regressions from moving preserve-3d into the compositor have been fixed. I don't think we are well set up to back it out.  Do we have examples of pages other than this demo failing badly?  

Matt, do you think we are ready to ship this, even though bug 68498 and bug 1175311 probably won't be fixed by the time 46 is released?  I think we are in decent shape despite this performance regression.

Comment 47

[Matt Woodrow (:mattwoodrow)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #46)
> We are two weeks away from shipping 46, and many regressions from moving
> preserve-3d into the compositor have been fixed. I don't think we are well
> set up to back it out.  Do we have examples of pages other than this demo
> failing badly?

I agree, backing out at this point would be painful (and error prone), and unless we know of critical regressions then I don't think it's a good idea.
  
> 
> Matt, do you think we are ready to ship this, even though bug 68498 and bug
> 1175311 probably won't be fixed by the time 46 is released?  I think we are
> in decent shape despite this performance regression.

Yes, I think we should ship preserve-3d in the compositor regardless.

Getting this bug fixed as well would be nice (it's a very isolated fix to an algorithmic issue, and shouldn't have any effect outside of performance), but probably not something we need to block on.

Thinker, do you want to get this patch ready for review? We should be aiming to get it into 47 at the very least, and ideally be in a position to consider it for 46.

Comment 48

[:kip (Kearwood Gilbert)]

Needinfo'ing myself to add comment on relationship between plane splitting and better sorting algorithm for greater correctness.

Comment 49

[Thinker Li [:sinker]]

I am working on an improvement of sorting algorithm that the complexity would be down to N.log(N) for most cases, but O(N^2) for worst case.  I think we can review current patch, and a follow up patch for my current work.

Comment 50

[Thinker Li [:sinker]]

Attachment: Improve LayerSorter NumEdgesTo(), O(n^2) performance, v3

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

Comment 51

[Thinker Li [:sinker]]

Attachment: Improve the layer sorter to get big theta nLog(n)

Comment 52

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks Matt, that is helpful!  I think wontfixing for 46 is best at this point. We could still take a patch for 47 though.

Comment 53

[:kip (Kearwood Gilbert)]

(In reply to :kip (Kearwood Gilbert) from comment #48)
> Needinfo'ing myself to add comment on relationship between plane splitting
> and better sorting algorithm for greater correctness.

I have sent an invite by email to :sinker and :mattwoodrow to meet up and talk about better polygon sorting algorithms that will affect bugs such as these.

Clearing my needinfo for now.  Please also see Bug 1175311 for details.

Comment 54

[Thinker Li [:sinker]]

Attachment: layersorter-wip.diff

boost the example to 19fps from 9fps.

Comment 55

[Thinker Li [:sinker]]

This patch I did following changes:
 - sorting function
   - select a layer with largest area size and not marked as sorted.
   - compare other layers with the selected layer.
   - put layers into three groups, A: layers covered by the selected layer, B: layers covering the selected layer, and C: no overlay with the selected layer.
   - recursively sort group C, and mark them as sorted.
   - recursively sort group A + C.
   - recursively sort group B + C.
   - unmark layers.
 - remove the layers that are contained by the selected layer from the list for further sorting.
 - clip the layer at the edges of |RectDouble::MaxIntRect()|, and remove layers that fall beyond the singular where w-component is negative.
 - put layers into groups according their positions in 2D space of user's eyes.  So, two layers in different groups are not compared. |spaceSplit2D()|
 - shrink the size of the layer a little bit, (0.05 pixel at each side) to reduce false intersection detection caused by floating error.
 - Change |RecoverZDepth()| to get correct Z-value.

Comment 56

[Thinker Li [:sinker]]

Attachment: layersorter-wip.diff

This patch is a little bit different with the previous one.  It always picks the layer with biggest area size to classify other layers into 3 groups.

2D (x and y-axis) splitting is removed.  The code is far more simple and fast.  21+fps in average.  Very close to 24fps, the most fast frame rate in all probability got by disable z-ordering sorting.

Comment 57

[Thinker Li [:sinker]]

Attachment: layersorter-wip.diff

Consider the case on bug 1225654, it has 1000+ layers.  Improving some hot spots accord to the result of profiling.  The frame rate goes up to 23fps for the example on this bug, so I think there has no much worth to improve of LayerSorter that can improve dramatically comparing to the rest parts of the compositor.

I will look over other parts of the compositor, to find out things worth to improve.  Following are things taking up large part of time of the compositor from profiling result.
 - IPC takes up 37% of time to handle PLayerTransactionParent::RecvUpdate(). (Bug 1225654 with 1000+ layers)
 - AutoLockCompositableHost() called by ImageHost takes up to 9.6% to upload a texture.
 - LayerPropertiesBase::ComputeDifferences() takes up 7.7% of time. (1000+ layers)
 - CompositorOGL::BindAndDrawQuad() takes up 3.3% called for ColorLayerComposite::RenderLayer(). (Bug 1225654 with 1000+ layers, most of layers are color layer.)



Profile: https://cleopatra.io/#report=5430cf205ae0aade3c66a282375304a5e5bd4a79&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A142417,%22end%22%3A145594%7D,%7B%22type%22%3A%22FocusedFrameSampleFilter%22,%22name%22%3A%22base%3A%3AMessagePumpDefault%3A%3ARun(base%3A%3AMessagePump%3A%3ADelegate*)%20%2Fhome%2Fthinker%2Fprogm%2Fmozilla-central%2Fipc%2Fchromium%2Fsrc%2Fbase%2Fmessage_pump_default.cc%3A34%22,%22focusedSymbol%22%3A6803%7D%5D

Comment 58

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] off before Apr 11 from comment #57)
>  - IPC takes up 37% of time to handle PLayerTransactionParent::RecvUpdate().
> (Bug 1225654 with 1000+ layers)
17% of 37% is doing de-serialization of IPC message.
20% is for running LayerTransactionParent::RecvUpdate().

Comment 59

[Thinker Li [:sinker]]

I just found animation with preserve3d are handled at content process.  I think we can move it to the compositor to avoid costly restyling and reflowing at content process.

Comment 60

[Thinker Li [:sinker]]

Attachment: Move transform animations with preserve-3d to the compositor

Comment 61

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] off before Apr 11 from comment #60)
> Created attachment 8744599 [details] [diff] [review]
> Move transform animations with preserve-3d to the compositor

This patch push frame rate to 56~59fps for the example on this bug.

Comment 62

[Jet Villegas (inactive)]

(In reply to Thinker Li [:sinker] from comment #61)
> (In reply to Thinker Li [:sinker] off before Apr 11 from comment #60)
> > Created attachment 8744599 [details] [diff] [review]
> > Move transform animations with preserve-3d to the compositor
> 
> This patch push frame rate to 56~59fps for the example on this bug.

Nice! How does bug 779598 look with this patch?

Comment 63

[Liz Henry (:lizzard) (relman/hg->git project)]

Thinker, are you still working on this? Wontfix for 47/48, since it seems like work in progress. 
Earlier comments mentioned fixing correctness first in bug 1175311 and bug 689498.

Comment 64

[Thinker Li [:sinker]]

Yes! I am still working on this bug.  I work on this bug and bug 1225654 at the same time.
I think some of patches can be reviewed before correctness bugs, like OMTA patch, but I would do it on 49.

Comment 65

[Thinker Li [:sinker]]

Attachment: Move transform animations with preserve-3d to the compositor, v2

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

Comment 66

[Thinker Li [:sinker]]

Comment on attachment 8758117 [details] [diff] [review]
Move transform animations with preserve-3d to the compositor, v2

Hi Brian, would you review this change?

Comment 67

[Brian Birtles (:birtles)]

Comment on attachment 8758117 [details] [diff] [review]
Move transform animations with preserve-3d to the compositor, v2

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

I think hiro would be a better reviewer for this. Also, we may need to remove some performance warning enum values / string values if they're no longer being used.

Comment 68

[Hiroyuki Ikezoe (:hiro)]

In my understandings, we don't send transform animations with backface-visibility:hidden to the compositor until bug 1186204 is fixed.  Has that bug been already fixed by other bugs?
Does the test case attached in bug 1186061 comment 3 surely work now?

Also, have all of bugs which blocks running transform animations with preserved-3d been on the compositor fixed?

(In reply to Brian Birtles (:birtles) from comment #67)
> Comment on attachment 8758117 [details] [diff] [review]
> Move transform animations with preserve-3d to the compositor, v2
> 
> Review of attachment 8758117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think hiro would be a better reviewer for this. Also, we may need to
> remove some performance warning enum values / string values if they're no
> longer being used.

We should also drop test cases in test_animation_performance_warning.html.

Comment 69

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8758117 [details] [diff] [review]
Move transform animations with preserve-3d to the compositor, v2

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

::: layout/base/nsDisplayList.cpp
@@ +594,5 @@
>  
> +  // This line is here for depending on the value of
> +  // nsIFrame::RefusedAsyncAnimationProperty().
> +  nsTArray<RefPtr<dom::Animation>> compositorAnimations =
> +    EffectCompositor::GetAnimationsForCompositor(aFrame, aProperty);

This change looks reasonable to me.  Do you have any test cases which need this change?  Could you please make this change as a separate patch *with* the test cases.

Comment 70

[Thinker Li [:sinker]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #68)
> In my understandings, we don't send transform animations with
> backface-visibility:hidden to the compositor until bug 1186204 is fixed. 
> Has that bug been already fixed by other bugs?
> Does the test case attached in bug 1186061 comment 3 surely work now?
> 
> Also, have all of bugs which blocks running transform animations with
> preserved-3d been on the compositor fixed?

Bug 1097464 should fix all these bug.  And, I will add a new testcase for that.

Comment 71

[Thinker Li [:sinker]]

Attachment: Fix backface hidden feature at compositor

Comment 72

[Thinker Li [:sinker]]

(In reply to Thinker Li [:sinker] from comment #71)
> Created attachment 8759009 [details] [diff] [review]
> Fix backface hidden feature at compositor

I tried the test case in bug 1186061 comment 3, and found bug 1097464 has fixed the problem for perserve3d cases with OMTA, but not for flatten elements.  This patch will fix it.

Comment 73

[Thinker Li [:sinker]]

I have found the testcase layout/reftests/transform-3d/animate-backface-hidden.html can catch the problem that attachment 8759009 [details] [diff] [review] trying to fix.  I will add preserve3d cases to this reftest.

Comment 74

[Thinker Li [:sinker]]

Attachment: Move transform animations with preserve-3d to the compositor, v3

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

Comment 75

[Thinker Li [:sinker]]

Comment on attachment 8759009 [details] [diff] [review]
Fix backface hidden feature at compositor

Compositor should check shadow-transform instead of base transform for backface hidden since animation at compositor works out by changing shadow transforms.

Comment 76

[Thinker Li [:sinker]]

Comment on attachment 8759137 [details] [diff] [review]
Move transform animations with preserve-3d to the compositor, v3

Change testcases, and remove the unnecessary change mentioned at comment 69.

I realize that the change mentioned at comment 69 would cause unnecessary calls to SchedulePaint().

Comment 77

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8759009 [details] [diff] [review]
Fix backface hidden feature at compositor

I am sorry this is out of my knowledge.  Matt?

Comment 78

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8759137 [details] [diff] [review]
Move transform animations with preserve-3d to the compositor, v3

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

Yay! 
You can also drop corresponding strings in dom/locales/en-US/chrome/layout/layout_errors.properties.

r=me changes in dom/animation.

I think changes in layout/ code should be reviewed by Matt.

Comment 79

[Thinker Li [:sinker]]

Attachment: Improve LayerSorter NumEdgesTo(), O(n^2) performance, v4

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

Comment 80

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8759137 [details] [diff] [review]
Move transform animations with preserve-3d to the compositor, v3

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

::: dom/animation/test/chrome/test_animation_performance_warning.html
@@ -237,5 @@
>      }
>    },
>  ];
>  
> -var gPerformanceWarningTests = [

Do we want to add new tests that are the reverse of these ones? Confirming that these examples *are* running on the compositor?

::: layout/base/nsDisplayList.cpp
@@ +5853,5 @@
>  
>      return false;
>    }
>  
> +  if (aFrame->Extend3DContext() || aFrame->IsPreserve3DLeaf()) {

I don't think we should do this, we intentionally restrict prerendering based on size to avoid massive memory usage and paint times for large elements.

Did you need this for this test case? Can we instead look at adjusting the size logic below to be sufficient for this page without just allowing prerendering of arbitrary sized content?

It would be preferable to do this as a separate patch too.

Comment 81

[Hiroyuki Ikezoe (:hiro)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #80)
> Comment on attachment 8759137 [details] [diff] [review]
> Move transform animations with preserve-3d to the compositor, v3
> 
> Review of attachment 8759137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/test/chrome/test_animation_performance_warning.html
> @@ -237,5 @@
> >      }
> >    },
> >  ];
> >  
> > -var gPerformanceWarningTests = [
> 
> Do we want to add new tests that are the reverse of these ones? Confirming
> that these examples *are* running on the compositor?

Yeah, good idea!  We have such tests in dom/animation/test/chrome/test_running_on_compositor.html.

Comment 82

[Thinker Li [:sinker]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #80)
> Comment on attachment 8759137 [details] [diff] [review]
> Move transform animations with preserve-3d to the compositor, v3
> 
> Review of attachment 8759137 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: layout/base/nsDisplayList.cpp
> @@ +5853,5 @@
> >  
> >      return false;
> >    }
> >  
> > +  if (aFrame->Extend3DContext() || aFrame->IsPreserve3DLeaf()) {
> 
> I don't think we should do this, we intentionally restrict prerendering
> based on size to avoid massive memory usage and paint times for large
> elements.
> 
> Did you need this for this test case? Can we instead look at adjusting the
> size logic below to be sufficient for this page without just allowing
> prerendering of arbitrary sized content?
> 
> It would be preferable to do this as a separate patch too.

This change is required for this test case.  The memory usage is not only the size of single layer, but also number of layers.  And, desktops and phones have different needs on memory usage.  We might should make this change conditional compiled.

Comment 83

[Thinker Li [:sinker]]

Attachment: Move transform animations with preserve-3d to the compositor, v4

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

Comment 84

[Thinker Li [:sinker]]

Attachment: Change size restriction for prerendering preserve-3d layers

Comment 85

[Thinker Li [:sinker]]

Attachment: Move transform animations with preserve-3d to the compositor, v5

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

Comment 86

[Thinker Li [:sinker]]

Comment on attachment 8760203 [details] [diff] [review]
Move transform animations with preserve-3d to the compositor, v5

Add change test_running_on_compositor.html to cover transformation on perserve-3d elements.  Move out the change about restriction on layer size.

Comment 87

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8760203 [details] [diff] [review]
Move transform animations with preserve-3d to the compositor, v5

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

r=me in dom/animation.

You can drop CompositorAnimationWarningTransformBackfaceVisibilityHidden and CompositorAnimationWarningTransformPreserve3D in dom/locales/en-US/chrome/layout/layout_errors.properties.

::: dom/animation/test/chrome/test_running_on_compositor.html
@@ +455,5 @@
> +    assert_equals(animation.isRunningOnCompositor, omtaEnabled,
> +      'Animation reports that it is running on the compositor');
> +  });
> +}, 'transformation animation is running on compositor' +
> +   ' when it is in a preserve-3d rendering context');

You can also add a test case for backface-visibility:hidden?

Comment 88

[Thinker Li [:sinker]]

https://dxr.mozilla.org/mozilla-central/source/layout/reftests/transform-3d/animate-backface-hidden.html#20

Matt, do you know why it has a delay of -99.9s here?  It makes the blue rect flashing at the head of the animation, and it makes test wrong.

Comment 89

[Thinker Li [:sinker]]

I know the reason now.

Comment 90

[Thinker Li [:sinker]]

The test case is fail for the reftest-no-flush.  It would rely on bug 1248828 to support OMTA.

Comment 91

[Thinker Li [:sinker]]

Comment on attachment 8760150 [details] [diff] [review]
Change size restriction for prerendering preserve-3d layers

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

bug 1100357 is working on the problem that this patch want to take care of.

Comment 92

[Jet Villegas (inactive)]

The plane-splitting work in bug 689498 would change a bunch of this code around. Let's wait to get that landed and get fresh timing results.

Comment 93

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for beta but we could still take a patch for 51/50.

Comment 94

[Clemens Eisserer]

Tested Platform: Apple IMac, Radeon M285

Tested Browsers:
* Safari
* Chrome
* Firefox 58.0.2

The only browser which didn't correctly render the demo was Firefox:
https://www.youtube.com/watch?v=9TrSOfv-l0c

Comment 95

[niels.breuker]

Here is an observation I made in the latest Nightly with WebRender enabled:

The CSS demo is smooth but occasionally it starts to lag like crazy. Moving the cursor around immediately makes it smooth again.

Comment 96

[Gregory Pappas [:gregp]]

Attachment: software_webrender.mp4

Still slow in Nightly 94.0a1 (2021-09-08) :(

Some observations: Demo looks accurate with WR, no glitches or anything but the slowness is still present.

SWR is having more issues with this demo. It is much slower than WR and has some glitches at points.

Comment 97

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 98

[Tom25519]

I do a test in Firefox 102.0.1 and Edge 103.0.1264.71, website: http://www.keithclark.co.uk/labs/css-fps/nojs/, Edge is smoothly, Firefox is frozen.

Comment 100

[Gregory Pappas [:gregp]]

(Carried over from bug 1530185 comment 3, thanks Alice!!)

Comment 101

[Firefox User]

Software WebRender test on Firefox 111.0.1 shows that glitches still exist, performance is very low (~1-7 fps).

Comment 102

[Firefox User]

Should the component of this bug be WebRender now?