1018862 - Make tests for CSS Transitions (OMTA) confirm the animation is being performed on the compositor

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Brian Birtles (:birtles)]

In bug 964646 we discovered that the code for testing CSS Transitions on the compositor (OMTA) introduced in bug 788549 isn't actually run on any of the build machines. This is because getOMTAOrComputedStyle would fall back to using the computed style on the only platform that had OMTA enabled (B2G).

We need to make the CSS Transitions tests actually confirm they are running on the compositor when they are expected to be. We should then remove getOMTAOrComputedStyle.

We also need to increase test coverage of CSS transitions running on the compositor in general. For example, there are no tests that check that a transition with a delay is run correctly (and I know from ad-hoc testing that at least at one point they did not).

Comment 1

[Brian Birtles (:birtles)]

I've mostly converted the transitions tests but I was having trouble with one transition from "skew(45deg, 45deg)"--which is not invertible--to "none".

After some debugging it turns out:

* The transition gets created
* The updateTransformLayer hint gets set
* RestyleManager processes it and schedules an invalidating paint
* When we get to nsDisplayTransform::BuildLayer we call
    FrameLayerBuilder::BuildContainerLayerFor which calls
    ChooseScaleAndSetTransform
* The call to transform.IsSingular() returns false,
* FrameLayerBuilder::BuildContainerLayerFor returns nullptr,
* And nsDisplayTransform::BuildLayer bails out early since 'container' is null

I haven't been able to create a simplified test case that reproduces this problem in practice. Presumably we create the layer on the next sample where the transform is no longer non-invertible. But how do we update the transform in that case? Shouldn't any updates be throttled by that point?

Perhaps in CommonElementAnimationData::CanThrottleAnimation we detect that layer isn't there so we skip throttling then?

Comment 2

[Brian Birtles (:birtles)]

Attachment: part 1 - Factor out common async animation test methods

This patch moves some test utility methods from test_animations_omta.html to
animation_utils.js. It also renames addAsyncTest to addAsyncAnimTest and
likewise for a few other methods.

Comment 3

[Brian Birtles (:birtles)]

Attachment: part 2 - Make new_div no longer secretly flush styles

This patch removes the line from new_div that forced a style flush. This was
very confusing because:
* It behaved differently to new_div in test_animations.html so copying tests
  over was more complex (particularly when registering for events is involved).
* It meant after setting up initial style using new_div you could just call
  waitForPaints but if you updated style using elem.style you'd need to call
  waitForPaintsFlushed.

Comment 4

[Brian Birtles (:birtles)]

Attachment: part 3 - Fix the order of arguments to omta_is_approx

This patch simply re-arranges the order of arguments to omta_is_approx so that
the delta sits along side the values being compared.

This, I think, makes more sense and also is more consistent when converting
tests from test_animations.html to test_animations_omta.html since the
"RunningOn" parameter is consistently inserted in the second-to-last position
just before the description for both omta_is and omta_is_approx.

Comment 5

[Brian Birtles (:birtles)]

Attachment: part 4 - Move omta_is and friends to animation_utils.js

This patch simply moves code from test_animations_omta.html to
animation_utils.js so that we can use it for testing transitions as well.

Comment 6

[Brian Birtles (:birtles)]

Attachment: part 5 - Move paint listener promise wrappers to animation_utils.js

This patch moves some utility methods from test_animations_omta.html to
animation_utils.js so that they can be used for testing transitions as well.

Comment 7

[Brian Birtles (:birtles)]

Attachment: part 6 - Fix handling of 3d matrices in omta_is and co.

This patch fixes a bug in the handling of 3d matrices represented as an array of
numbers.

Comment 8

[Brian Birtles (:birtles)]

Attachment: part 7 - Convert test_transitions_per_property.html to use OMTA test methods

This patch takes the existing tests for transitions running on the compositor
and makes them re-use the same test utility methods as used for testing CSS
Animations that run on the compositor. This means these tests now also check
that the transition is in fact running on the compositor when it is expected to.

It seems the big_omta_round_error is no longer needed so I've removed that.

The test that begins with "skew(45deg, 45deg)" currently fails so it is skipped
here. This is addressed in the next patch in the series.

Comment 9

[Brian Birtles (:birtles)]

Attachment: part 8 - Add special handling for non-invertible transforms

This patch adjusts the tests for transform transitions that run on the
compositor to handle transitions that begin with a non-invertible transform.
In this case the first sample at the start of the animation won't create
a layer because in nsDisplayTransform::BuildLayer
/ FrameLayerBuilder::BuildContainerLayerFor we'll skip creating the layer once
we notice the equivalent matrix is singular.

In this patch we detect that case and force an extra sample betwee 0 and 200s at
100s. This means the layer will be created at t=100s and be available for
querying at the next sample.

Similar issues could occur, for example, if the transforms at both t=0s
and t=100s are not invertible but currently that doesn't occur. We can add
handling for that if and when it becomes necessary.

This patch adds an internal getDeterminant method to animation_utils.js which is
pretty massive. The 3D part is actually not used but it here for completeness.
Ultimately I hope we can remove much of this code and use DOMMatrix once it
lands. That's why I haven't factored this out into some generic matrix handling
utility class.

Comment 10

[Brian Birtles (:birtles)]

Attachment: part 9 - Remove nsIDOMWindowUtils::GetOMTAOrComputedStyle

Now that getOMTAOrComputedStyle is no longer used by any tests it should be
removed. It is a footgun because it can lead the test author to think they
are testing compositor animations (OMTA) when in fact they are just testing
main thread animations.

This has bitten us before because while we were getting results from the
compositor on some platforms when run locally, on Tinderbox the only build
configurations that had OMTA turned on were falling back to returning the
computed style.

Comment 11

[Brian Birtles (:birtles)]

Attachment: part 1 - Factor out common async animation test methods

Restore accidentally deleted tests

Comment 12

[Brian Birtles (:birtles)]

Attachment: part 2 - Make new_div no longer secretly flush styles

Rebase

Comment 13

[Brian Birtles (:birtles)]

Attachment: part 4 - Move omta_is and friends to animation_utils.js

Fix shortcuts for omta_is and co. since they were completely wrong.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8434707 [details] [diff] [review]
part 1 - Factor out common async animation test methods

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

With one exception, I think these review comments all are about pre-existing [moved-in-this-patch] code. Feel free to address them in a separate patch, if you'd like to keep this patch focused on moving-without-much-rewriting.  (The one exception is for my second comment, about 'tests.length = 0' -- I don't think that exists in the original code.)

::: layout/style/test/animation_utils.js
@@ +197,5 @@
> +
> +  // Returns a promise when all tests have run
> +  window.runAllAsyncAnimTests = function(aOnAbort) {
> +    // The async test runner returns a Promise that is resolved when the
> +    // test is finished so we can chain them together

"The async test runner" is a bit vague/abstract here.

This comment is talking about "runAsyncAnimTest", right? If so, maybe just use that function's name directly in the comment here, or mention it in a parenthetical.

@@ +203,5 @@
> +        return sequence.then(function() {
> +          return runAsyncAnimTest(test, aOnAbort);
> +        });
> +      }, Promise.resolve() /* the start of the sequence */)
> +      .then(function() { tests.length = 0; });

I'm not clear on why we clear 'tests' at the end.

Won't we already be done with this array, by the time we execute this line?  Does something depend on it being emptied? If not, it seems like it'd be simpler to leave it alone. (But if we do need to empty it, add a comment to explain why.)

@@ +208,5 @@
> +  };
> +
> +  // Takes a generator function that represents a test case. Each point in the
> +  // test case that waits asynchronously for some result yields a Promise that
> +  // is resolved when the asychronous action has completed. By chaining these

"asychronous" there is missing an 'n'.  (s/sych/synch/)

@@ +216,5 @@
> +  // function has completed.
> +  //
> +  // This arrangement is based on add_task() which is currently only available
> +  // in mochitest-chrome (bug 872229). Once add_task is available in
> +  // mochitest-plain we can remove this function and use add_task instead.

(This implies that there are plans to make add_task available in mochitest-plain -- is that actually the case? If so, is there a bug for this, which we can link to here? I couldn't find one in a quick bugzilla search.)

@@ +254,5 @@
> +  }
> +
> +  window.addAsyncAnimTest = function(generator) {
> +    tests.push(generator);
> +  };

Maybe move these last 3 lines (for addAsyncAnimTest) to right after where you declare "var tests"?  In its current spot, at the end of the file, this code gets kind of gets visually mixed in with runAsyncAnimTest()'s impl.

(And since it's a tiny one-off that just appends to tests, it seems like it makes more sense to have it live right next to tests.)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8434718 [details] [diff] [review]
part 2 - Make new_div no longer secretly flush styles

r=me

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8434693 [details] [diff] [review]
part 3 - Fix the order of arguments to omta_is_approx

r=me

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8436680 [details] [diff] [review]
part 4 - Move omta_is and friends to animation_utils.js

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

::: layout/style/test/test_animations_omta.html
@@ +170,5 @@
> +    var args = Array.slice(arguments);
> +    if (!(args[0] instanceof Element)) {
> +      args.unshift(gDiv);
> +    }
> +    return origFn.apply(undefined, args);

Nit: In this "Shortcut omta_is and friends" code, I think we should be passing "window" as the first ("this") arg to the apply() call, instead of "undefined", right?

(These methods probably don't currently care about their 'this' pointer, so it doesn't much matter. But if someone ever tweaks them such that they do start caring, then I think the "undefined" in this apply() call would break things in a sneaky-and-mysterious way.)

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8434695 [details] [diff] [review]
part 5 - Move paint listener promise wrappers to animation_utils.js

r=me

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8434696 [details] [diff] [review]
part 6 - Fix handling of 3d matrices in omta_is and co.

Yikes! Do you know why this 3D matrix slice() issue hasn't caused problems?  (Were we just not using it for actual 4x4 matrices yet?)

r=me, anyway

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8434698 [details] [diff] [review]
part 7 - Convert test_transitions_per_property.html to use OMTA test methods

r=me

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 8434700 [details] [diff] [review]
part 8 - Add special handling for non-invertible transforms

(In reply to Brian Birtles (:birtles) from comment #9)
> This patch adds an internal getDeterminant method to animation_utils.js
> which is
> pretty massive. The 3D part is actually not used but it here for
> completeness.

Hmmm... Give that the 3D part is not used, I worry a bit that we have no way of knowing whether it's correct or not (or of testing that it remains correct, assuming that it is correct now).

I wonder if it might be better to leave that unused code commented-out, with a helpful ok(false, "...") message adjacent to it, telling people to uncomment it if they ever need it?  (And if anyone does need it, they'll fail that "ok(false" and see your helpful message.)

Or instead of commenting it out, you could even copypaste it into a bug comment here, and reference the bug comment's URL from the ok(false) message.

> Ultimately I hope we can remove much of this code and use DOMMatrix

(This is part of the reason I'm wondering about this.)

If you'd like to keep it, though, I'm also OK with that.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8434702 [details] [diff] [review]
part 9 - Remove nsIDOMWindowUtils::GetOMTAOrComputedStyle

r=me

Comment 23

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Comment on attachment 8434707 [details] [diff] [review]
> part 1 - Factor out common async animation test methods
> 
> Review of attachment 8434707 [details] [diff] [review]:
> ...
> @@ +203,5 @@
> > +        return sequence.then(function() {
> > +          return runAsyncAnimTest(test, aOnAbort);
> > +        });
> > +      }, Promise.resolve() /* the start of the sequence */)
> > +      .then(function() { tests.length = 0; });
> 
> I'm not clear on why we clear 'tests' at the end.
> 
> Won't we already be done with this array, by the time we execute this line? 
> Does something depend on it being emptied? If not, it seems like it'd be
> simpler to leave it alone. (But if we do need to empty it, add a comment to
> explain why.)

The idea is that you use it like so:

  addAsyncAnimTest(function *() { ... }); // A
  addAsyncAnimTest(function *() { ... }); // B
  addAsyncAnimTest(function *() { ... }); // C
  runAllAsyncAnimTests() // Runs A, B, C
  .then(function() {
    addAsyncAnimTest(function *() { ... }); // D
    addAsyncAnimTest(function *() { ... }); // E
    runAllAsyncAnimTests(); // Runs D, E
  });

So I thought it makes sense to clear the internal list of tests after running them. What do you think?

I guess I could just drop it and then add a "clear-list" type function later if it becomes necessary. Or alternatively name it "queueAsyncAnimTest".

> @@ +216,5 @@
> > +  // function has completed.
> > +  //
> > +  // This arrangement is based on add_task() which is currently only available
> > +  // in mochitest-chrome (bug 872229). Once add_task is available in
> > +  // mochitest-plain we can remove this function and use add_task instead.
> 
> (This implies that there are plans to make add_task available in
> mochitest-plain -- is that actually the case? If so, is there a bug for
> this, which we can link to here? I couldn't find one in a quick bugzilla
> search.)

There are no concrete plans that I'm aware of but it seems likely to happen eventually. I've changed the comment to say "If add_task becomes available...".

Comment 24

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Comment on attachment 8434696 [details] [diff] [review]
> part 6 - Fix handling of 3d matrices in omta_is and co.
> 
> Yikes! Do you know why this 3D matrix slice() issue hasn't caused problems? 
> (Were we just not using it for actual 4x4 matrices yet?)

That's right. We'd never encountered 3D matrices in this code path before.

Comment 25

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #21)
> Or instead of commenting it out, you could even copypaste it into a bug
> comment here, and reference the bug comment's URL from the ok(false) message.

Possible code for getting the determinant of a 3D matrix:

    return   matrix[0][3] * matrix[1][2] * matrix[2][1] * matrix[3][0]
           - matrix[0][2] * matrix[1][3] * matrix[2][1] * matrix[3][0]
           - matrix[0][3] * matrix[1][1] * matrix[2][2] * matrix[3][0]
           + matrix[0][1] * matrix[1][3] * matrix[2][2] * matrix[3][0]
           + matrix[0][2] * matrix[1][1] * matrix[2][3] * matrix[3][0]
           - matrix[0][1] * matrix[1][2] * matrix[2][3] * matrix[3][0]
           - matrix[0][3] * matrix[1][2] * matrix[2][0] * matrix[3][1]
           + matrix[0][2] * matrix[1][3] * matrix[2][0] * matrix[3][1]
           + matrix[0][3] * matrix[1][0] * matrix[2][2] * matrix[3][1]
           - matrix[0][0] * matrix[1][3] * matrix[2][2] * matrix[3][1]
           - matrix[0][2] * matrix[1][0] * matrix[2][3] * matrix[3][1]
           + matrix[0][0] * matrix[1][2] * matrix[2][3] * matrix[3][1]
           + matrix[0][3] * matrix[1][1] * matrix[2][0] * matrix[3][2]
           - matrix[0][1] * matrix[1][3] * matrix[2][0] * matrix[3][2]
           - matrix[0][3] * matrix[1][0] * matrix[2][1] * matrix[3][2]
           + matrix[0][0] * matrix[1][3] * matrix[2][1] * matrix[3][2]
           + matrix[0][1] * matrix[1][0] * matrix[2][3] * matrix[3][2]
           - matrix[0][0] * matrix[1][1] * matrix[2][3] * matrix[3][2]
           - matrix[0][2] * matrix[1][1] * matrix[2][0] * matrix[3][3]
           + matrix[0][1] * matrix[1][2] * matrix[2][0] * matrix[3][3]
           + matrix[0][2] * matrix[1][0] * matrix[2][1] * matrix[3][3]
           - matrix[0][0] * matrix[1][2] * matrix[2][1] * matrix[3][3]
           - matrix[0][1] * matrix[1][0] * matrix[2][2] * matrix[3][3]
           + matrix[0][0] * matrix[1][1] * matrix[2][2] * matrix[3][3];

Comment 26

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #23)
> The idea is that you use it like so:
> 
>   addAsyncAnimTest(function *() { ... }); // A
>   addAsyncAnimTest(function *() { ... }); // B
>   addAsyncAnimTest(function *() { ... }); // C
>   runAllAsyncAnimTests() // Runs A, B, C
>   .then(function() {
>     addAsyncAnimTest(function *() { ... }); // D
>     addAsyncAnimTest(function *() { ... }); // E
>     runAllAsyncAnimTests(); // Runs D, E
>   });
> 
> So I thought it makes sense to clear the internal list of tests after
> running them. What do you think?

Hmm.... I worry a bit that this sort of hypothetical control flow -- with multiple chained runAllAsyncAnimTests calls -- could get a bit hard to follow.  What's the benefit of that kind of structure, as compared to just making all of our addAsyncAnimTest() calls up-front? (I think that's what we do with your patches right now, right?)

(I can imagine that one use-case would be if there were some state that we need to set up before starting tests D and E, in your example... but it's probably better for any such state to be built-up/torn-down by those test functions themselves, individually).

Anyway, if you think the array-clearing is useful, I don't care enough to push back too hard on it. :) Maybe just add a comment explaining it (e.g. "Clear tests in case more addAsyncAnimTest / runAllAsyncAnimTests calls are coming.)

Comment 27

[Brian Birtles (:birtles)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f3384e94822
https://hg.mozilla.org/integration/mozilla-inbound/rev/4900384bce9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c0a44e761ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/49775bc6825e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f6fd048a317
https://hg.mozilla.org/integration/mozilla-inbound/rev/99f24b59c782
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7898e89dab
https://hg.mozilla.org/integration/mozilla-inbound/rev/cea6a1920ce7
https://hg.mozilla.org/integration/mozilla-inbound/rev/441a1c280cd8

(In reply to Daniel Holbert [:dholbert] from comment #26)
> (In reply to Brian Birtles (:birtles) from comment #23)
> > The idea is that you use it like so:
> > 
> >   addAsyncAnimTest(function *() { ... }); // A
> >   addAsyncAnimTest(function *() { ... }); // B
> >   addAsyncAnimTest(function *() { ... }); // C
> >   runAllAsyncAnimTests() // Runs A, B, C
> >   .then(function() {
> >     addAsyncAnimTest(function *() { ... }); // D
> >     addAsyncAnimTest(function *() { ... }); // E
> >     runAllAsyncAnimTests(); // Runs D, E
> >   });
> > 
> > So I thought it makes sense to clear the internal list of tests after
> > running them. What do you think?
> 
> Hmm.... I worry a bit that this sort of hypothetical control flow -- with
> multiple chained runAllAsyncAnimTests calls -- could get a bit hard to
> follow.

I've removed this list-clearing behaviour for now since it's not needed.


(In reply to Brian Birtles (:birtles) from comment #25)
> (In reply to Daniel Holbert [:dholbert] from comment #21)
> > Or instead of commenting it out, you could even copypaste it into a bug
> > comment here, and reference the bug comment's URL from the ok(false) message.
> 
> Possible code for getting the determinant of a 3D matrix:
> 
>     return   matrix[0][3] * matrix[1][2] * matrix[2][1] * matrix[3][0]
> ...

It turns out this was needed because a couple of the test cases do produce 3D matrices so I've left it in.

Comment 28

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #0)
> We also need to increase test coverage of CSS transitions running on the
> compositor in general. For example, there are no tests that check that a
> transition with a delay is run correctly (and I know from ad-hoc testing
> that at least at one point they did not).

I decided to look into this in a separate bug and make this bug just about making the OMTA transitions tests confirm the animations are running on the compositor.

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f3384e94822
https://hg.mozilla.org/mozilla-central/rev/4900384bce9f
https://hg.mozilla.org/mozilla-central/rev/1c0a44e761ec
https://hg.mozilla.org/mozilla-central/rev/49775bc6825e
https://hg.mozilla.org/mozilla-central/rev/0f6fd048a317
https://hg.mozilla.org/mozilla-central/rev/99f24b59c782
https://hg.mozilla.org/mozilla-central/rev/1d7898e89dab
https://hg.mozilla.org/mozilla-central/rev/cea6a1920ce7
https://hg.mozilla.org/mozilla-central/rev/441a1c280cd8