If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add tests for async CSS Animations (OMTA)

RESOLVED FIXED in mozilla32

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla32
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(21 attachments, 22 obsolete attachments)

7.28 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
19.67 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
13.18 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
10.28 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
11.31 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.56 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.83 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.04 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.35 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
8.70 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.75 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
10.24 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.04 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.18 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.96 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.23 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.42 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
4.82 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.12 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.29 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.77 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
Splitting this off from bug 788549 which I'll mark as specifically covering CSS Transitions only.
(Assignee)

Updated

4 years ago
Blocks: 846091, 788522
(Assignee)

Comment 1

4 years ago
Created attachment 8366479 [details] [diff] [review]
WIP part 1 - Add nsDOMWindowUtils.getOMTAStyle

nsDOMWindowUtils.getOMTAOrComputedStyle falls back to using getComputedStyle
when an OMTA style is not available. However, in order to be sure we are testing
OMTA this patch adds getOMTAStyle that returns an empty string if no OMTA style
is available.

This patch also includes some minor stylistic tweaks. The method signature for
getOMTAOrComputedStyle now takes an nsIDOMElement parameter rather than
nsIDOMNode in order to simplify error-checking. (When we support OMTA of
pseudo-elements we will have to adjust the method signature but for now we only
support elements.) Also, some lines have been wrapped, ErrorResult is
declared closer to where it is used, and the return value aResult is only
truncated when returning NS_OK.
(Assignee)

Comment 2

4 years ago
Created attachment 8366482 [details] [diff] [review]
WIP part 2 - Make LayerTransactionParent::RecvGetTransform convert to CSS pixels

LayerTransactionParent::RecvGetTransform takes care to reverse all the
transformations applied by AsyncCompositionManager::SampleValue to the CSS
values calculated so that it can return CSS values for testing. However, it
fails to revert the conversion from CSS pixels to device pixels applied to the
translation components of the transform by
nsStyleTransformMatrix::ProcessTranslatePart as called by
nsDisplayTransform::GetResultingTransformMatrix.

This patch converts the resulting transform's translation components from device
pixels back to CSS pixels. It also adds documentation for the other operations
in LayerTransactionParent::RecvGetTransform.
(Assignee)

Comment 3

4 years ago
Created attachment 8366484 [details] [diff] [review]
WIP part 3 - Rename PLayerTransaction.GetTransform to GetAnimationTransform

PLayerTransaction.GetTransform doesn't actually return the same kind of value
when the transform on the layer is not set by animation. This is because it uses
information stored with the animation to undo various transforms. We shouldn't
pretend to return something useful/similar when we don't have that information
available.

This patch renames GetTransform to GetAnimationTransform and makes it take an
extra parameter to indicate if the layer's transform is set by animation or not.
(Assignee)

Comment 4

4 years ago
Created attachment 8366485 [details] [diff] [review]
WIP part 4 - Add OMTA version of test_animations.html fill mode tests

This patch adds an additional mochitest for specifically targetting CSS
Animations that run on the compositor thread. The content of the test mimicks
test_animations.html but using properties whose animations are expected to run
on the compositor thread.

For platforms where OMTC is not enabled by default the test is skipped since
OMTC cannot be enabled at run-time. For platforms where OMTC is enabled by
default, OMTA is switched on as necessary.
(Assignee)

Comment 5

4 years ago
These tests are very much a work in progress. For my own notes, they can be run on Windows using:

export EXTRA_TEST_ARGS='--test-path=layout/style/test/test_animations_omta.html --setpref=layers.offmainthreadcomposition.enabled=true --setpref=layers.offmainthreadcomposition.async-animations=true --setpref=layers.offmainthreadcomposition.log-animations=true'

Then, from the obj dir:
  mozmake mochitest-plain

Currently the tests don't pass for me on Windows because delayed animations seem to get started very late on Windows. Running a manual test I see the same behavior so I don't think the tests themselves are to blame.

Also, currently we don't test values when the animation isn't actually running (e.g. when its filling). We probably should:
1. Add a LayerTransaction.GetTransform that just calls GetLocalTransform on the shadow layer
2. In DOMWindowUtils::GetOMTAStyle, if isAnimated is false, call GetTransform

The problem is GetTransform returns the transform with the translation from the reference frame incorporated. This is set in nsDisplayTransform::GetTransform. I'm not sure yet how to recover this reference frame so we can pull out that translation component.
(Assignee)

Updated

4 years ago
Depends on: 972199
(Assignee)

Comment 6

4 years ago
I'm having trouble making these tests deterministic. The problem is knowing how long to wait for values to appear on the compositor thread.

The problem is further complicated by the fact that I have the refresh driver under test control so it doesn't tick unless I tell it to.

For an animation that begins right away I can:
a) Call elem.clientTop to force style resolution which will trigger nsAnimationManager::BuildAnimations
b) Then I can wait for pending paints to complete by making the test into a chrome test and using paint_listener.js.

For animations that start with a delay, however, I'm not sure what to wait on. BuildAnimations creates the animations during the delay phase but won't add them to the layer until the end of the delay. Once we advance the refresh driver to that point there are no pending paints so I can't wait on that. I can just keep polling the compositor thread but that will probably lead to random failures. Should there be a scheduled paint? Is there something I can tickle to trigger a paint to be scheduled? Can I force a synchronous paint in GetOMTAStyle or when I advance the refresh driver?

I've been digging into this all week but I could do with some pointers.
Flags: needinfo?(ncameron)
(Assignee)

Comment 7

4 years ago
David, if you have any suggestions regarding comment 6 they'd be very welcome.
Flags: needinfo?(dzbarsky)
(In reply to Brian Birtles (:birtles) from comment #6)
Is there
> something I can tickle to trigger a paint to be scheduled? Can I force a
> synchronous paint in GetOMTAStyle or when I advance the refresh driver?
> 

Does calling Invalidate on the element's frame in GetOMTAStyle help? I think if you invalidate and then wait for a setTimeout(0) you should get a paint
Flags: needinfo?(dzbarsky)
(Assignee)

Comment 9

4 years ago
(In reply to David Zbarsky (:dzbarsky) from comment #8)
> Does calling Invalidate on the element's frame in GetOMTAStyle help? I think
> if you invalidate and then wait for a setTimeout(0) you should get a paint

Thanks David, that seems to do the trick.

It's a bit odd I guess--with this approach you have to call getOMTAStyle twice and spin the event loop in-between to be sure you're seeing the latest result. But I don't have any better ideas short of making getOMTAStyle return a promise.

I'll check tomorrow to see if this removes the need for paint_listener.js (previously just using SimpleTest.executeSoon wasn't enough to ensure the paint happened but with this explicit invalidation it may work).
(Assignee)

Updated

4 years ago
Depends on: 975261
(Assignee)

Comment 10

4 years ago
(In reply to Brian Birtles (:birtles) from comment #9)
> (In reply to David Zbarsky (:dzbarsky) from comment #8)
> > Does calling Invalidate on the element's frame in GetOMTAStyle help? I think
> > if you invalidate and then wait for a setTimeout(0) you should get a paint
> 
> Thanks David, that seems to do the trick.

This seems like a bug. It seems like we should be already be invalidating this elsewhere, i.e. I should only have to tick the refresh driver. I filed bug 975261 which shows that in some cases we're not correctly starting animations with a delay. This may be the same bug I'm seeing here.
Flags: needinfo?(ncameron)
(Assignee)

Comment 11

4 years ago
Created attachment 8379512 [details] [diff] [review]
part 1 - Add check that caller is chrome to GetOMTAOrComputedStyle

Every other exposed method in nsDOMWindowUtils except getViewPortInfo and
getViewId performs this check. This patch makes getOMTAOrComputedStyle check the
called is chrome as well.
(Assignee)

Comment 12

4 years ago
Created attachment 8379513 [details] [diff] [review]
part 2 - Add nsDOMWindowUtils.getOMTAStyle

nsDOMWindowUtils.getOMTAOrComputedStyle falls back to using getComputedStyle
when an OMTA style is not available. However, in order to be sure we are testing
OMTA this patch adds getOMTAStyle that returns an empty string if no OMTA style
is available.

This patch also includes some minor stylistic tweaks. The method signature for
getOMTAOrComputedStyle now takes an nsIDOMElement parameter rather than
nsIDOMNode in order to simplify error-checking. (When we support OMTA of
pseudo-elements we will have to adjust the method signature but for now we only
support elements.) Also, some lines have been wrapped, ErrorResult is
declared closer to where it is used, and the return value aResult is only
truncated when returning NS_OK.
(Assignee)

Updated

4 years ago
Attachment #8366479 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8379514 [details] [diff] [review]
part 3 - Make LayerTransactionParent::RecvGetTransform convert to CSS pixels
(Assignee)

Updated

4 years ago
Attachment #8366482 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Created attachment 8379516 [details] [diff] [review]
part 4 - Rename PLayerTransaction.GetTransform to GetAnimationTransform
(Assignee)

Updated

4 years ago
Attachment #8366484 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
Created attachment 8379517 [details] [diff] [review]
part 5 - Add OMTA version of test_animations.html fill mode tests (Chrome test)
(Assignee)

Updated

4 years ago
Attachment #8379517 - Attachment description: part 4 - Add OMTA version of test_animations.html fill mode tests (Chrome test) → part 5 - Add OMTA version of test_animations.html fill mode tests (Chrome test)
(Assignee)

Updated

4 years ago
Attachment #8366485 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
The tests in part 5 currently fail on Linux (try run here: https://tbpl.mozilla.org/?tree=Try&rev=dd4a8c97d9df) not sure why.

Also, the call to 'renderDocument' shouldn't be necessary. Hopefully bug 975261 will shed some light on that.
(Assignee)

Updated

4 years ago
Depends on: 979658
(Assignee)

Updated

4 years ago
Attachment #8379512 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8379513 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8379514 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8379516 - Attachment is obsolete: true
(Assignee)

Comment 17

4 years ago
Moved GetOMTAStyle refactoring to bug 979658
(Assignee)

Comment 18

4 years ago
Created attachment 8387255 [details] [diff] [review]
Add OMTA version of test_animations.html fill mode tests

This patch adds an additional mochitest for specifically targetting CSS
Animations that run on the compositor thread. The content of the test mimicks
test_animations.html but using properties whose animations are expected to run
on the compositor thread.

For platforms where OMTC is not enabled by default the test is skipped since
OMTC cannot be enabled at run-time. For platforms where OMTC is enabled by
default, OMTA is switched on as necessary.
(Assignee)

Updated

4 years ago
Attachment #8379517 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8387255 - Attachment description: Add OMTA version of test_animations.html fill mode tests (Chrome test) → Add OMTA version of test_animations.html fill mode tests
Blocks: 980770
(In reply to Brian Birtles (:birtles) from comment #18)
> For platforms where OMTC is not enabled by default the test is skipped since
> OMTC cannot be enabled at run-time. For platforms where OMTC is enabled by
> default, OMTA is switched on as necessary.

It seems like this is something we're going to want to do in quite a few places (I was thinking about similar things for tests I want to write as part of bug 960465), so perhaps the code needed to do this could be refactored into a separate JS file that multiple tests can use?

I'd imagine having an API something like this:

  test_if_omt_animations(function() {
    // tests to run if OMT animations are supported
  });

Then, test_if_omt_animations could, internally:
 * call pushPrefEnv to set up the appropriate prefs
 * do a brief internal test that getOMTAStyle works as expected
   - if that test fails, use either is() or todo() depending on 
     platform to report a single test failure (TEST-KNOWN-FAIL on 
     some platforms, TEST-UNEXPECTED-FAIL on platforms where we 
     expect OMTA)
   - if that test passes, call the callback function

This would let us have mochitests that:
 * run on tinderbox on platforms where we can expect OMTA to be 
   supported
 * can be run by developers on other platforms if they're using a 
   configuration with OMTA supported
(Assignee)

Comment 20

4 years ago
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #19)
> (In reply to Brian Birtles (:birtles) from comment #18)
> > For platforms where OMTC is not enabled by default the test is skipped since
> > OMTC cannot be enabled at run-time. For platforms where OMTC is enabled by
> > default, OMTA is switched on as necessary.
> 
> It seems like this is something we're going to want to do in quite a few
> places (I was thinking about similar things for tests I want to write as
> part of bug 960465), so perhaps the code needed to do this could be
> refactored into a separate JS file that multiple tests can use?

Sounds great.

> I'd imagine having an API something like this:
> 
>   test_if_omt_animations(function() {
>     // tests to run if OMT animations are supported
>   });
> 
> Then, test_if_omt_animations could, internally:
>  * call pushPrefEnv to set up the appropriate prefs
>  * do a brief internal test that getOMTAStyle works as expected
>    - if that test fails, use either is() or todo() depending on 
>      platform to report a single test failure (TEST-KNOWN-FAIL on 
>      some platforms, TEST-UNEXPECTED-FAIL on platforms where we 
>      expect OMTA)
>    - if that test passes, call the callback function

Yes, sounds good. I've noticed that after calling pushPrefEnv the first call to getOMTAStyle fails. I think subsequent calls occur after a waitForAllPaints so there may be some asynchronous overhead required for setting up OMTA. This method may need to spin the event loop before calling getOMTAStyle.

Also, I'm seeing test failures in bug 975261 on B2G ICS despite the fact that OMTA is enabled and OMTC is available. I'm not sure why yet but there may be further requirements to ensuring OMTA is running.
(Assignee)

Comment 21

4 years ago
Created attachment 8396171 [details] [diff] [review]
part 1 - Add common OMTA test runner to animation_utils.js

Common test runner as described in comment 19
Attachment #8396171 - Flags: review?(dbaron)
(Assignee)

Comment 22

4 years ago
Created attachment 8396173 [details] [diff] [review]
part 2 - Add OMTA version of test_animations.html fill mode tests
(Assignee)

Updated

4 years ago
Attachment #8387255 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
(In reply to Brian Birtles (:birtles) from comment #21)
> Created attachment 8396171 [details] [diff] [review]
> part 1 - Add common OMTA test runner to animation_utils.js
> 
> Common test runner as described in comment 19

On further thought, I think the automatic calling of SimpleTest.finish is probably not necessary and may confuse authors. It's probably better to require explicitly doing this, as in:

  SimpleTest.waitForExplicitFinish();
  runOMTATest(testFunc, SimpleTest.finish);
(Assignee)

Comment 24

4 years ago
Try run: https://tbpl.mozilla.org/?tree=Try&rev=7a71b92856a1
Depends on: 986367
(Assignee)

Comment 25

4 years ago
Created attachment 8396861 [details] [diff] [review]
part 1 - Add common OMTA test runner to animation_utils.js

Add the tweak mentioned in comment 23
Attachment #8396861 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8396171 - Attachment is obsolete: true
Attachment #8396171 - Flags: review?(dbaron)
(Assignee)

Comment 26

4 years ago
Created attachment 8396862 [details] [diff] [review]
part 2 - Add OMTA version of test_animations.html fill mode tests

Reworked to explicitly test values on compositor thread in all cases
Attachment #8396862 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8396173 - Attachment is obsolete: true
(Assignee)

Comment 27

4 years ago
Updated try run: https://tbpl.mozilla.org/?tree=Try&rev=4f37e604d4a0
(Assignee)

Comment 28

4 years ago
Created attachment 8396887 [details] [diff] [review]
part 2 - Add OMTA version of test_animations.html fill mode tests
Attachment #8396887 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8396862 - Attachment is obsolete: true
Attachment #8396862 - Flags: review?(dbaron)
(Assignee)

Comment 29

4 years ago
Try again: https://tbpl.mozilla.org/?tree=Try&rev=96cf516ff207
(Assignee)

Updated

4 years ago
Depends on: 988161
(Assignee)

Comment 30

4 years ago
Created attachment 8396957 [details] [diff] [review]
part 3 - Add OMTA version of test_animations.html animation list tests
(Assignee)

Comment 31

4 years ago
Created attachment 8397516 [details] [diff] [review]
part 3 - Add OMTA version of test_animations.html animation list tests

Tests for animation list handling on the compositor thread. Some checks are
currently marked todo_is because they depend on Bug 980769 in order to pass.
Attachment #8397516 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8396957 - Attachment is obsolete: true
(Assignee)

Comment 32

4 years ago
Created attachment 8397629 [details] [diff] [review]
part 3 - Refactor OMTA test methods to include opacity too

This patch takes the compareTransform utility methods and makes them more
generic so they can be used for testing opacity too (the other property
currently animated on the compositor thread).

The naming omta_is and omta_is_approx is intended to mirror is and is_approx in
test_animations.html. We don't include omta_todo_is since there are multiple
ways the test could fail (compositor thread value is not set, compositor thread
value doesn't match computed style etc.) and so defining a suitable todo method
is difficult (we'd have to run all tests and assert that at least one failed).
Attachment #8397629 - Flags: review?(dbaron)
(Assignee)

Comment 33

4 years ago
Created attachment 8397630 [details] [diff] [review]
part 4 - Add OMTA version of test_animations.html animation list tests

I thought I'd take the chance to rebase these patches on top of some fixes to the test framework
Attachment #8397630 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8397516 - Attachment is obsolete: true
Attachment #8397516 - Flags: review?(dbaron)
(Assignee)

Comment 34

4 years ago
Created attachment 8397632 [details] [diff] [review]
part 5 - Add OMTA version of test_animations.html keyframe tests
Attachment #8397632 - Flags: review?(dbaron)
(Assignee)

Comment 35

4 years ago
Sorry for the noise with all the patch updates. I won't go adding any more tests until I get review feedback for these first few patches in case there's something fundamentally amiss with the approach I've taken.
(Assignee)

Comment 36

4 years ago
Try run with current queue: https://tbpl.mozilla.org/?tree=Try&rev=dba30c824d14
(Assignee)

Updated

4 years ago
Attachment #8397630 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8397632 - Flags: review?(dbaron)
(Assignee)

Comment 37

4 years ago
Cancelling review for parts 4 and 5 due to test failures on B2G. Opacity appears to be handled differently to transforms. Will debug next week.
(Assignee)

Comment 38

4 years ago
Created attachment 8400294 [details] [diff] [review]
part 4 - Add OMTA version of test_animations.html animation list tests

Fix tests so they pass on B2G
Attachment #8400294 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8397630 - Attachment is obsolete: true
(Assignee)

Comment 39

4 years ago
Created attachment 8400295 [details] [diff] [review]
part 5 - Add OMTA version of test_animations.html keyframe tests
Attachment #8400295 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8397632 - Attachment is obsolete: true
(Assignee)

Comment 40

4 years ago
Parts 4 and 5 now pass on B2G: https://tbpl.mozilla.org/?tree=Try&rev=e2da2b2e05c8

For my own reference, the test failed on B2G because:
* they failed to flush styles after changing specified style when waiting for paints (although I'm still not exactly sure why this worked on other platforms with OMTC enabled)
* they positioned things outside the viewport on B2G (left: 300px is outside the viewport so we never generated display list items for those things)
Comment on attachment 8396861 [details] [diff] [review]
part 1 - Add common OMTA test runner to animation_utils.js

>+// This function also does an internal test to verify that OMTA is working at
>+// all so that if OMTA is not functioning correctly only a single failure is
>+// produced.

maybe add "when it is expected to function" after the "not functioning
correctly" (probably with some commas)

>+  var onSkip     = aOnSkip || function() {};
>+      utils      = SpecialPowers.DOMWindowUtils,
>+      expectOMTA = utils.layerManagerRemote &&
>+                   // ^ Off-main thread animation cannot be used if off-main
>+                   // thread composition (OMTC) is not available
>+                   SpecialPowers.getBoolPref(OMTAPrefKey);

Put var before each of the three, and change the , after the second
to a ;.  (Note you had a ; after the first, so you were introducing
globals!)

Also, I think you should require an onSkip function rather than
having the "|| function() {}" since every caller has to at least
call SimpleTest.finish.

I'm new to reading promises code, so if you want somebody to review
who isn't, you should ask -- but I think it's probably fine if you've
tested that it works as you expect.

r=dbaron
Attachment #8396861 - Flags: review?(dbaron) → review+
Comment on attachment 8396887 [details] [diff] [review]
part 2 - Add OMTA version of test_animations.html fill mode tests

I'm not reviewing these (patches 2-5) all that closely, but I think that's fine.  (Sorry, I should have decided to not review them all that closely a few days ago..)
Attachment #8396887 - Flags: review?(dbaron) → review+
Comment on attachment 8397629 [details] [diff] [review]
part 3 - Refactor OMTA test methods to include opacity too

>+    return convertArrayTo3dMatrix([1, 0, 0, 1, 0 ,0]);

fix the misplaced final comma


If you need a todo mechanism, I expect you'll want to pass some sort
of complex parameter to the function.  But if you don't need it yet,
don't bother inventing it yet.
Attachment #8397629 - Flags: review?(dbaron) → review+
Attachment #8400294 - Flags: review?(dbaron) → review+
Comment on attachment 8400295 [details] [diff] [review]
part 5 - Add OMTA version of test_animations.html keyframe tests

Have you gone back and tested any of the recently-fixed OMTA bugs to see if they're covered by these tests?  If so, it would be useful to comment both here and in the fixed bugs.  (And I think it probably would be useful to check, at least assuming the patches can be reverted or disabled in a straightforward way today, and assuming the patches fix things that should be covered by these tests, since it will validate that the tests are testing what is expected.)
Attachment #8400295 - Flags: review?(dbaron) → review+
Comment on attachment 8396861 [details] [diff] [review]
part 1 - Add common OMTA test runner to animation_utils.js

>+// Since this function relies on various asynchronous operations, the caller is
>+// responsible for calling SimpleTest.waitForExplicitFinish() before calling
>+// this and SimpleTest.finish() within aTestFunction and aOnSkip.
>+function runOMTATest(aTestFunction, aOnSkip) {
>+  const OMTAPrefKey = "layers.offmainthreadcomposition.async-animations";
>+  var onSkip     = aOnSkip || function() {};
>+      utils      = SpecialPowers.DOMWindowUtils,
>+      expectOMTA = utils.layerManagerRemote &&
>+                   // ^ Off-main thread animation cannot be used if off-main
>+                   // thread composition (OMTC) is not available
>+                   SpecialPowers.getBoolPref(OMTAPrefKey);

One other comment here -- it seems like if we have OMTC, we ought to be able to change the OMTA pref and run the tests.  Probably best to do that as a followup patch after landing these, though.
(Assignee)

Comment 46

4 years ago
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #44)
> Comment on attachment 8400295 [details] [diff] [review]
> part 5 - Add OMTA version of test_animations.html keyframe tests
> 
> Have you gone back and tested any of the recently-fixed OMTA bugs to see if
> they're covered by these tests?  If so, it would be useful to comment both
> here and in the fixed bugs.  (And I think it probably would be useful to
> check, at least assuming the patches can be reverted or disabled in a
> straightforward way today, and assuming the patches fix things that should
> be covered by these tests, since it will validate that the tests are testing
> what is expected.)

Yes, the issues in bug 975261 were all identified using these tests (as were the issues in bug 988161 which turned out to be a dupe of bug 980769). I decided to file those as a separate bug with separate reduced test cases since they arose largely as a product of the way the tests were structured such that if test_animations_omta.html were refactored in such a way that inadvertently introduced an additional style flush, the bug could be masked.

I have just now tested that reverting the changes in bug 975261 causes these tests to fail.

I'm fairly sure if we revert bug 828173 these tests will fail but I haven't yet tried reverting those patches. Will try next week.

Thanks for all the reviews!
(Assignee)

Comment 47

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc8ad1175324
https://hg.mozilla.org/integration/mozilla-inbound/rev/da72761ebf82
https://hg.mozilla.org/integration/mozilla-inbound/rev/33a00dec3dbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5f9279dbaf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bdb40bd3103
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/cc8ad1175324
https://hg.mozilla.org/mozilla-central/rev/da72761ebf82
https://hg.mozilla.org/mozilla-central/rev/33a00dec3dbd
https://hg.mozilla.org/mozilla-central/rev/f5f9279dbaf4
https://hg.mozilla.org/mozilla-central/rev/8bdb40bd3103
Flags: in-testsuite+
(Assignee)

Comment 49

4 years ago
(In reply to Brian Birtles (:birtles) from comment #46)
> I'm fairly sure if we revert bug 828173 these tests will fail but I haven't
> yet tried reverting those patches. Will try next week.

Verified that these tests fail when reverting bug 828173 and added a comment to that bug to that effect.
(Assignee)

Updated

4 years ago
Depends on: 998162
(Assignee)

Comment 50

4 years ago
Created attachment 8408835 [details] [diff] [review]
part 6 - Add OMTA tests for timing functions on keyframes

This patch converts the tests in test_animations.html under the heading,
"css3-animations:  3.1. Timing functions for keyframes" to an equivalent version
for testing animations that run on the compositor thread.
(Assignee)

Comment 51

4 years ago
Created attachment 8408837 [details] [diff] [review]
part 7 - Handle NaN values when comparing matrices

The test harness code for normalizing transform inputs to a standard form for
comparison fails to detect the case where the input is a string such as

 { tx: "20px" }

instead of:

 { tx: 20 }

When we go to compare matrix components we fail if:

  Math.abs(a.comp - b.comp) > tolerance

But if a.comp or b.comp is a string, we'll get NaN on the LHS and
"NaN > tolerance" will return false so we'll skip the failure handling and
continue onto the next component. That means if we have input { tx: "30px" } and
we get "20" as the x-translation component we'll pass the test.

This patch fixes this condition to check for isNaN.

We *could* also just drop a few .map(parseFloat) calls into
convertObjectTo3dMatrix and convertArrayTo3dMatrix to ensure "20px" becomes 20
but there may be situations where that masks bugs (since "20px" and "20em" turn
into the same thing) so for now this minimal fix should be enough.
(Assignee)

Comment 52

4 years ago
Created attachment 8408838 [details] [diff] [review]
part 8 - Add OMTA tests for animation-name

This patch adds a version of the tests in test_animations.html under the
heading, "css3-animations:  3.2. The 'animation-name' Property" that tests the
same features when animations are running on the compositor thread.
(Assignee)

Comment 53

4 years ago
Created attachment 8408839 [details] [diff] [review]
part 9 - Add OMTA tests for animation-iteration-count

This patch adds a version of tests in test_animations.html under the heading,
"css3-animation: 3.5 The 'animation-iteration-count' Property for animations
that run on the compositor thread.

These tests surface two issues:

a) Animations cut short by the iteration count are not removed immediately from
   the compositor (filed as bug 998162).
b) In some cases precision errors lead to discrepencies between the OMTA style
   and computed style (to be fixed in a subsequent patch).
(Assignee)

Comment 54

4 years ago
Created attachment 8408841 [details] [diff] [review]
part 10 - Fix floating point precision issues when comparing matrices

This patch addresses and issue where the OMTA style and computed style were not
comparing equal in one particular case.

In this case AddTransformTranslate in nsStyleAnimation would give us
a translate-y value of 94.331673 in both cases (i.e. when calculating the
animated value on the compositor thread or when fetching computed style).

For the OMTA case, hoever, after we apply additional layer transformations and
then reverse them (so we can query the CSS value) we'd end up with 94.331642,
a difference of 0.000031. The reversing procedure is only used for testing so
the actual error introduced here by the additional layer transformations is
probably less.

Unfortunately, when we pass 94.331642 this along to MatrixToCSSValue we get back
matrix(1, 0, 0, 1, 94.3316) since it only outputs 6 digits of precision.

On the other hand, on the computed style end we'd pass 94.331673 to
MatrixToCSSValue which gives us matrix(1, 0, 0, 1, 94.3317), so the error swells
from 0.000031 to 0.0001.

Then when we subtract 94.3316 from 94.3317 in Javascript we get
0.00010000000000331966 due to floating-point precision issues which compares
greater than the default tolerance of 0.0001.

This patch simply adjusts the default tolerance to 0.00011 to accommodate
these floating-point differences.
(Assignee)

Comment 55

4 years ago
Created attachment 8408842 [details] [diff] [review]
part 11 - Add RunningOn.TodoMainThread for marking animations that are known to run on the compositor when they should not

This patch also ensures that when we have an animation running on the compositor
when it should not that we still compare the values produced on the compositor
and on the main thread so that the visual result is correct even if the
performance characteristics are not.
(Assignee)

Comment 56

4 years ago
Try run for parts 6-11: https://tbpl.mozilla.org/?tree=Try&rev=5ee32e8e89af
(Assignee)

Comment 57

4 years ago
Comment on attachment 8408835 [details] [diff] [review]
part 6 - Add OMTA tests for timing functions on keyframes

Here is the next round of OMTA tests. There are still more to come. I'll probably add them in batches over the next few weeks.
Attachment #8408835 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8408837 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8408838 - Flags: review?(dbaron)
(Assignee)

Comment 58

4 years ago
Created attachment 8409542 [details] [diff] [review]
part 9 - Add OMTA tests for animation-iteration-count
Attachment #8409542 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8408839 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8408841 - Flags: review?(dbaron)
(Assignee)

Comment 59

4 years ago
Created attachment 8409543 [details] [diff] [review]
part 11 - Add RunningOn.TodoMainThread for marking animations that are known to run on the compositor when they should not
Attachment #8409543 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8408842 - Attachment is obsolete: true
(Assignee)

Comment 60

4 years ago
Follow-up try run: https://tbpl.mozilla.org/?tree=Try&rev=afcfe6e51ba6
(Assignee)

Comment 61

4 years ago
(In reply to Brian Birtles (:birtles) from comment #60)
> Follow-up try run: https://tbpl.mozilla.org/?tree=Try&rev=afcfe6e51ba6

Actually, it's still broken due to my incompetence with understanding B2G build scripts.
(Assignee)

Comment 62

4 years ago
Comment on attachment 8409542 [details] [diff] [review]
part 9 - Add OMTA tests for animation-iteration-count

Cancelling review for now while I fix this.
Attachment #8409542 - Flags: review?(dbaron)
(Assignee)

Comment 63

4 years ago
Comment on attachment 8409543 [details] [diff] [review]
part 11 - Add RunningOn.TodoMainThread for marking animations that are known to run on the compositor when they should not

This part too will change so cancelling review on this patch for now too.
Attachment #8409543 - Flags: review?(dbaron)
(Assignee)

Comment 64

4 years ago
Created attachment 8410753 [details] [diff] [review]
part 9 - Add OMTA tests for animation-iteration-count
Attachment #8410753 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8409542 - Attachment is obsolete: true
(Assignee)

Comment 65

4 years ago
Created attachment 8410754 [details] [diff] [review]
part 11 - Add OMTA tests for animation-direction
Attachment #8410754 - Flags: review?(dbaron)
(Assignee)

Comment 66

4 years ago
Created attachment 8410755 [details] [diff] [review]
part 12 - Add OMTA tests for animation-play-state
Attachment #8410755 - Flags: review?(dbaron)
(Assignee)

Comment 67

4 years ago
Created attachment 8410757 [details] [diff] [review]
part X - Add RunningOn.TodoMainThread for marking animations that are known to run on the compositor when they should not

This is no longer needed so I'm not putting it up for review, but just leaving it here in case we need this feature later.
(Assignee)

Updated

4 years ago
Attachment #8409543 - Attachment is obsolete: true
(Assignee)

Comment 68

4 years ago
Parts 6-12 look ok on try: https://tbpl.mozilla.org/?tree=Try&rev=b9031b54367c
(Assignee)

Updated

4 years ago
Depends on: 1000692
(Assignee)

Comment 69

3 years ago
Created attachment 8416255 [details] [diff] [review]
part 13 - Add OMTA tests for animation-delay
Attachment #8416255 - Flags: review?(dbaron)
(Assignee)

Comment 70

3 years ago
Created attachment 8416257 [details] [diff] [review]
part 14 - Add OMTA tests for multi-property animations
Attachment #8416257 - Flags: review?(dbaron)
(Assignee)

Comment 71

3 years ago
Created attachment 8416258 [details] [diff] [review]
part 15 - Add OMTA tests for sampling animations with same timestamp
Attachment #8416258 - Flags: review?(dbaron)
(Assignee)

Comment 72

3 years ago
Created attachment 8416259 [details] [diff] [review]
part 16 - Add OMTA tests for !important rules and animations
Attachment #8416259 - Flags: review?(dbaron)
(Assignee)

Comment 73

3 years ago
Created attachment 8416260 [details] [diff] [review]
part 17 - Add RunningOn.TodoMainThread for marking animations that are known to run on the compositor when they should not
Attachment #8416260 - Flags: review?(dbaron)
(Assignee)

Updated

3 years ago
Attachment #8410757 - Attachment is obsolete: true
(Assignee)

Comment 74

3 years ago
Created attachment 8416261 [details] [diff] [review]
part 18 - Add omta_todo_is for marking OMTA animations that are known to fail.
Attachment #8416261 - Flags: review?(dbaron)
(Assignee)

Comment 75

3 years ago
Created attachment 8416262 [details] [diff] [review]
part 19 - Add OMTA tests for restyling interaction
Attachment #8416262 - Flags: review?(dbaron)
(Assignee)

Comment 76

3 years ago
Created attachment 8416263 [details] [diff] [review]
part 20 - Add OMTA tests for cascading between keyframe rules
Attachment #8416263 - Flags: review?(dbaron)
(Assignee)

Comment 77

3 years ago
Created attachment 8416264 [details] [diff] [review]
part 21 - Add OMTA tests for animation list lengths and dynamic style rule changes
Attachment #8416264 - Flags: review?(dbaron)
(Assignee)

Comment 78

3 years ago
That should be all the patches now.

Currently running through try but it passes for me on Windows and B2G emulator
https://tbpl.mozilla.org/?tree=Try&rev=5b00e4459249

Some possible tweaks I could make:
* Add comments explaining when and why we use waitForPaints vs waitForPaintsFlushed
  (We could drop the trigger to flush styles in new_div and then just use waitForPaintsFlushed everyhwere)
* Change the order of parameters to omta_is_approx so that the tolerance comes before the "where is it animating" parameter. That would probably make more sense.

Comment 79

3 years ago
This is some exceptionally fine testing work. I have been following along with a child-like sense of wonder. Thanks a lot for all the effort here. Nice set of fixes for the corner cases you discovered, too. Thanks!
Attachment #8408835 - Flags: review?(dbaron) → review+
Attachment #8408837 - Flags: review?(dbaron) → review+
Attachment #8408838 - Flags: review?(dbaron) → review+
Attachment #8408841 - Flags: review?(dbaron) → review+
Attachment #8410753 - Flags: review?(dbaron) → review+
Attachment #8410754 - Flags: review?(dbaron) → review+
Attachment #8410755 - Flags: review?(dbaron) → review+
Attachment #8416255 - Flags: review?(dbaron) → review+
Attachment #8416257 - Flags: review?(dbaron) → review+
Attachment #8416258 - Flags: review?(dbaron) → review+
Comment on attachment 8416261 [details] [diff] [review]
part 18 - Add omta_todo_is for marking OMTA animations that are known to fail.

>+const ExpectComparisonTo = {
>+  Pass: 0,
>+  Fail: 1
>+};

Add a comment that many callers will pass undefined for this parameter.

And maybe make these 1 and 2 instead of 0 and 1, to be clearer that ! isn't safe.

r=dbaron with that
Attachment #8416261 - Flags: review?(dbaron) → review+
Comment on attachment 8416259 [details] [diff] [review]
part 16 - Add OMTA tests for !important rules and animations

Might have been nice to do 18 before 16, but ok as is (although you're welcome to reorder as well).
Attachment #8416259 - Flags: review?(dbaron) → review+
Comment on attachment 8416260 [details] [diff] [review]
part 17 - Add RunningOn.TodoMainThread for marking animations that are known to run on the compositor when they should not

And I guess that reordering would be moving 16 after both 17 and 18, then...
Attachment #8416260 - Flags: review?(dbaron) → review+
Comment on attachment 8416262 [details] [diff] [review]
part 19 - Add OMTA tests for restyling interaction

Not sure whether this test tests anything interesting when the animation is running on the compositor, but may as well keep it now that you have it.
Attachment #8416262 - Flags: review?(dbaron) → review+
Attachment #8416263 - Flags: review?(dbaron) → review+
Comment on attachment 8416264 [details] [diff] [review]
part 21 - Add OMTA tests for animation list lengths and dynamic style rule changes

sorry for the delay getting to these; they were easier than I thought to review
Attachment #8416264 - Flags: review?(dbaron) → review+
(Assignee)

Comment 85

3 years ago
Thanks for the reviews David and the encouragement Andreas!

I haven't landed these yet because I'm hitting an assertion in RestyleTracker.cpp on emulator debug builds:

  http://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleTracker.cpp#75

Try run: https://tbpl.mozilla.org/?tree=Try&rev=58e4e6e0d5e7
(Assignee)

Comment 86

3 years ago
(In reply to Brian Birtles (:birtles) from comment #85)
> I haven't landed these yet because I'm hitting an assertion in
> RestyleTracker.cpp on emulator debug builds:
>  
> http://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleTracker.
> cpp#75

Reduced and filed as bug 1012527
(Assignee)

Comment 87

3 years ago
After annotating the test for the assertions filed as bug 1012527, these tests look good on try:

  https://tbpl.mozilla.org/?tree=Try&rev=e93be63c852e

I'm going to carry forward the r+ to the updated test with the assertion annotations added.
(Assignee)

Comment 88

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2215666b9dbc
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8345c6d0f78
https://hg.mozilla.org/integration/mozilla-inbound/rev/20d451095cf9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e4c3d7de7d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/be00b4cb3997
https://hg.mozilla.org/integration/mozilla-inbound/rev/0606fc7897e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d34520be880
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f18555a6468
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8794c1163b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/67d88958c18c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2fc0c54c81
https://hg.mozilla.org/integration/mozilla-inbound/rev/f179bf862af5
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a19b4d3255b
https://hg.mozilla.org/integration/mozilla-inbound/rev/406c8dfdf0ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/90a9471d2ec7
https://hg.mozilla.org/integration/mozilla-inbound/rev/38bb31689d42
https://hg.mozilla.org/mozilla-central/rev/2215666b9dbc
https://hg.mozilla.org/mozilla-central/rev/a8345c6d0f78
https://hg.mozilla.org/mozilla-central/rev/20d451095cf9
https://hg.mozilla.org/mozilla-central/rev/2e4c3d7de7d6
https://hg.mozilla.org/mozilla-central/rev/be00b4cb3997
https://hg.mozilla.org/mozilla-central/rev/0606fc7897e6
https://hg.mozilla.org/mozilla-central/rev/9d34520be880
https://hg.mozilla.org/mozilla-central/rev/5f18555a6468
https://hg.mozilla.org/mozilla-central/rev/c8794c1163b6
https://hg.mozilla.org/mozilla-central/rev/67d88958c18c
https://hg.mozilla.org/mozilla-central/rev/ee2fc0c54c81
https://hg.mozilla.org/mozilla-central/rev/f179bf862af5
https://hg.mozilla.org/mozilla-central/rev/0a19b4d3255b
https://hg.mozilla.org/mozilla-central/rev/406c8dfdf0ab
https://hg.mozilla.org/mozilla-central/rev/90a9471d2ec7
https://hg.mozilla.org/mozilla-central/rev/38bb31689d42
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla32

Updated

3 years ago
Depends on: 1021766
Blocks: 1065141
You need to log in before you can comment on or make changes to this bug.