1282312 - invalidation problems with combination of -moz-element() and 3D transforms

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase

From this example:
https://twitter.com/anatudor/status/746501526168936448
http://codepen.io/thebabydino/pen/JKEyBv/

we appear to have problems with invalidation given combinations of 3d transforms and -moz-element().

The lower half of the testcase should be spinning just like the top half (in reflection), but instead it only updates occasionally.

Comment 1

[Timothy Nikkel (:tnikkel)]

Regression window for the blue one on the left:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=895f66c4eada&tochange=85f561c755f6

Regression window for the red one on the right:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f7c89de3ab43&tochange=3f0587ce1774

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://twitter.com/anatudor/status/747483867754471424
http://codepen.io/thebabydino/pen/QENNEY
http://s.codepen.io/boomerang/6e11fcdecb56dcf090c8519f116c7afb1467065165290/index.html
points out the same problem with Video.

Comment 3

[Matt Woodrow (:mattwoodrow)]

There's a few bugs here, I have patches for it, just need to write some tests.

Our way of handling invalidation with -moz-element is pretty fragile, it would be much better if we could hook into DLBI and get normal invalidation. I filed bug 1284053 for this.

Comment 4

[Matt Woodrow (:mattwoodrow)]

I'm having issues writing a test for this. We seem to only miss the invalidation for intermediate steps of an animation, once we hit the end then we invalidate correctly.

We can't easily have a reftest that captures the intermediate state of an animation.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Make sure we invalidate rendering observers when triggering paints

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Disable async animations when we have rendering observers

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Reftest (that doesn't work)

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 8768946 [details] [diff] [review]
Disable async animations when we have rendering observers

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

::: dom/animation/test/chrome/test_running_on_compositor.html
@@ +180,5 @@
> +    assert_equals(animation.isRunningOnCompositor, false,
> +       'Animation reports that it is NOT running on the compositor'
> +       + ' when referenced by -moz-element');
> +  });
> +}, 'isRunningOnCompositor is false while in the delay phase');

This comment needs to be updated to say something about referencing with -moz-element.

Really, though, we should add this test to test_animation_performance_warning.html (along with a test that checks that the warning is cleared when the condition is resolved). test_animation_performance_warning.html can be a little bit hard to follow, however, so perhaps Hiro can help with that.

Comment 9

[Brian Birtles (:birtles)]

Hiro, would you be able to help Matt write a test for test_animation_performance_warning.html for this new warning? (If we do that, perhaps we don't need the additional test in test_running_on_compositor.html from the second patch in this series.)

Comment 10

[Hiroyuki Ikezoe (:hiro)]

We can write a similar test just like this[1].

[1] http://hg.mozilla.org/mozilla-central/file/94cce4e79310/dom/animation/test/chrome/test_animation_performance_warning.html#l645

But, in this case, we need to add/remove id attribute instead of adding/removing style.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Async animations test

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8769570 [details] [diff] [review]
Async animations test

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

This basically looks good to me.  But in this case I'd prefer to create a separate promise_test and create the div element for -moz-element in the test so that we can avoid any side effects of the new div element.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8768945 [details] [diff] [review]
Make sure we invalidate rendering observers when triggering paints

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

Sorry for the delay. I actually had this mostly reviewed last week but forgot to finish my comment and submit it...

::: layout/generic/nsFrame.cpp
@@ +5716,5 @@
>  void
>  nsIFrame::SchedulePaint(PaintType aType)
>  {
> +  nsSVGEffects::InvalidateDirectRenderingObservers(this);
> +  if (!nsLayoutUtils::IsPopup(this)) {

Instead of checking IsPopup on every iteration, you could get the display root frame first, and then just walk up until you encounter it. For the common case that we're not in a popup, doing so would be faster because nsLayoutUtils::GetDisplayRootFrame has a fast path to return the root frame. The in-popup case might be slightly slower but that shouldn't matter. (When I started writing this comment last week I knew why it would be slower, but I've forgotten the reason in the meantime.)

@@ +5743,5 @@
>  
>    Layer* layer = FrameLayerBuilder::GetDedicatedLayer(this, aDisplayItemKey);
>  
> +  nsSVGEffects::InvalidateDirectRenderingObservers(this);
> +  if (!nsLayoutUtils::IsPopup(this)) {

This loop looks the same as the one above. Can you share?

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Make sure we invalidate rendering observers when triggering paints

Updated patch, carrying r=mstange.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Async animations test

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8775872 [details] [diff] [review]
Async animations test

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

Thanks for fixing the test!

::: dom/animation/test/chrome/test_animation_performance_warning.html
@@ +684,5 @@
>  
> +  gPerformanceWarningTestsId.forEach(function(subtest) {
> +    promise_test(function(t) {
> +      if (subtest.createelement) {
> +        var referenced = addDiv(t, { style: subtest.createelement });

Is this 'var referenced' really needed?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24fc0e07876b
Part 1: Invalidate direct rendering observers for a frame when we schedule a paint due to changes. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe1b11f09d3
Part 2:  Disable async animations when we have rendering observers. r=bbirtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/09874746f3e0
Part 3: Add test for disabling of async animations with rendering observers. r=hiro

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/24fc0e07876b
https://hg.mozilla.org/mozilla-central/rev/dbe1b11f09d3
https://hg.mozilla.org/mozilla-central/rev/09874746f3e0