Composited Animations: Introduce pixel-ref layout tests.

Composited Animations: Introduce pixel-ref layout tests.

We get the animation to effectively start paused at the time fraction we care about.

In order to test a number of time fractions with a single pixel result, we're creating a new
element for each sample so we can check them simultaneously.

BUG=509482

[loyso (OOO), 2015-08-05 07:08:52]

loyso@chromium.org changed reviewers:
+ ajuma@chromium.org, alancutter@chromium.org, dstockwell@chromium.org,
vollick@chromium.org

[loyso (OOO), 2015-08-05 07:09:27]

Naming and grouping suggestions are welcome!

[Ian Vollick, 2015-08-05 13:58:46]

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
File LayoutTests/animations/resources/composited-animation-test.js (right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:107: var duration
= 1000 * 1.0 / (kBezierEpsilon * 200.0);
nit: isn't this just 5.0/kBezierEpsilon?

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:109: // duration =
5000;
What's with this commented-out line? Leftover from debugging?

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:117: delay:
-duration * stamp.at,
Ah, so these tests work by getting the animation to effectively start paused at
the time fraction you care about?

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:128:
assertRunningThread() {
nit: assertAnimationsRunningOnCompositorThread, or if that's too verbose, at
least assertRunningOnThread.

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
File LayoutTests/animations/resources/composited-animations-data/rotate-scale.js
(right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animations-data/rotate-scale.js:34:
marker: false, // TODO(loyso): This mustn't affect cc/blink consistency.
What do you mean by this TODO?

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animations-data/rotate-scale.js:35:
stamps: [
Just to make sure I'm understanding how your test works, in order to test a
number of time fractions with a single pixel result, you're creating a new
element for each stamp so you can check them simultaneously?

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/virtual/threade...
File
LayoutTests/virtual/threaded/animations/composited-animations-rotate-scale.html
(right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/virtual/threade...
LayoutTests/virtual/threaded/animations/composited-animations-rotate-scale.html:7:
var test = new CompositedAnimationTest(false);
boolean parameter is super-confusing, especially since true == disabled. Can you
please find some way to make this named (even if you just set a local variable
to the boolean value)? Also, if the compositor animations are disabled via
InternalSettings (this is my preference), you could leave the settings stuff to
the wrapping html files, and just construct a CompositedAnimationTest with no
parameters.

[dstockwell, 2015-08-06 00:23:20]

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/VirtualTestSuites
File LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/VirtualTestSuit...
LayoutTests/VirtualTestSuites:37: "--disable-composited-antialiasing"]
Add a comment about why this flag is needed.

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
File LayoutTests/animations/resources/composited-animation-test.js (right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:65: if
(test.data.vertical == null || !test.data.vertical)
These seem fairly arbitrary inputs to provide to the test configuraiton, can you
just use CSS?

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:99:
internals.forceCompositingUpdate(document);
// why

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:124:
internals.pauseAnimations(0);
Doesn't this get called before we start the compositor animations?

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
File LayoutTests/animations/resources/composited-animations-data/rotate-scale.js
(right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animations-data/rotate-scale.js:9:
stamps: [
Why isn't this just an array of numbers?

`stamps` doesn't seem like the best name, perhaps `times` or `samples`?

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/virtual/threade...
File
LayoutTests/virtual/threaded/animations/composited-animations-rotate-scale.html
(right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/virtual/threade...
LayoutTests/virtual/threaded/animations/composited-animations-rotate-scale.html:7:
var test = new CompositedAnimationTest(false);
On 2015/08/05 at 13:58:46, vollick wrote:
> boolean parameter is super-confusing, especially since true == disabled. Can
you please find some way to make this named (even if you just set a local
variable to the boolean value)? Also, if the compositor animations are disabled
via InternalSettings (this is my preference), you could leave the settings stuff
to the wrapping html files, and just construct a CompositedAnimationTest with no
parameters.

+1 on the boolean param

We also seem to always call these three functions, you could just define:
runComposited(data) and runNonComposited(data)

[loyso (OOO), 2015-08-06 06:44:18]

I'm answering fundamental questions only for now since they can affect the
design.

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
File LayoutTests/animations/resources/composited-animation-test.js (right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:117: delay:
-duration * stamp.at,
On 2015/08/05 13:58:46, vollick wrote:
> Ah, so these tests work by getting the animation to effectively start paused
at
> the time fraction you care about?
Yes, exactly. This is special "paused mode" where you can freeze time on
per-animation basis. Relevant fix: https://codereview.chromium.org/1255883006/
This is the most simple and inexpensive way to test composited animations for
now. If you have other ideas, it's time to propose them. :) They are welcome.

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:124:
internals.pauseAnimations(0);
On 2015/08/06 00:23:19, dstockwell wrote:
> Doesn't this get called before we start the compositor animations?
Internals::pauseAnimations calls updateAllLifecyclePhases, which causes
CompositorPendingAnimations::update + Animation::preCommit with a creation of
main-thread cc::Animation objects. Impl-thread cc::Animation objects get
run_state_ PAUSED and pause_time_ during next commit.

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
File LayoutTests/animations/resources/composited-animations-data/rotate-scale.js
(right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animations-data/rotate-scale.js:35:
stamps: [
On 2015/08/05 13:58:46, vollick wrote:
> Just to make sure I'm understanding how your test works, in order to test a
> number of time fractions with a single pixel result, you're creating a new
> element for each stamp so you can check them simultaneously?
Yes, that's correct. See the comments in composited-animation-test.js

[loyso (OOO), 2015-08-06 07:03:33]

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/virtual/threade...
File
LayoutTests/virtual/threaded/animations/composited-animations-rotate-scale.html
(right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/virtual/threade...
LayoutTests/virtual/threaded/animations/composited-animations-rotate-scale.html:7:
var test = new CompositedAnimationTest(false);
On 2015/08/05 13:58:46, vollick wrote:
> boolean parameter is super-confusing, especially since true == disabled. Can
you
> please find some way to make this named (even if you just set a local variable
> to the boolean value)? Also, if the compositor animations are disabled via
> InternalSettings (this is my preference), you could leave the settings stuff
to
> the wrapping html files, and just construct a CompositedAnimationTest with no
> parameters.
Sure, I'll make it named. CompositedAnimationTest not only passes it to
Internals.disableThreadedAnimation but also checks, whether produced animations
are run on compositor in fact. See that assertRunningThread method.

[loyso (OOO), 2015-09-14 06:19:08]

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/VirtualTestSuites
File LayoutTests/VirtualTestSuites (right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/VirtualTestSuit...
LayoutTests/VirtualTestSuites:37: "--disable-composited-antialiasing"]
On 2015/08/06 00:23:19, dstockwell wrote:
> Add a comment about why this flag is needed.
1) We already have layer-edge anti-aliasing disabled for android.
2) How can I add comment to a strict JSON file? We don't have a notaion like
"_comment":

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
File LayoutTests/animations/resources/composited-animation-test.js (right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:65: if
(test.data.vertical == null || !test.data.vertical)
On 2015/08/06 00:23:19, dstockwell wrote:
> These seem fairly arbitrary inputs to provide to the test configuraiton, can
you
> just use CSS?
Yeah. WIP, I'll add it in a special Patch Set.

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:99:
internals.forceCompositingUpdate(document);
On 2015/08/06 00:23:19, dstockwell wrote:
> // why

Done.

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:107: var duration
= 1000 * 1.0 / (kBezierEpsilon * 200.0);
On 2015/08/05 13:58:46, vollick wrote:
> nit: isn't this just 5.0/kBezierEpsilon?
This is a reverse for the mentioned function:

// Calculates the accuracy for evaluating a timing function for an animation
with the specified duration.
inline double accuracyForDuration(double duration)
{
    return 1.0 / (200.0 * duration);
}

... and 1000 is a conversion from seconds to ms.

I could set a crbug to make this beavior consistent in blink and cc.

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:109: // duration =
5000;
On 2015/08/05 13:58:46, vollick wrote:
> What's with this commented-out line? Leftover from debugging?
Yep. If you don't adjust the duration then it becomes inconsistent so the test
fails.

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:128:
assertRunningThread() {
On 2015/08/05 13:58:46, vollick wrote:
> nit: assertAnimationsRunningOnCompositorThread, or if that's too verbose, at
> least assertRunningOnThread.

Done.

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
File LayoutTests/animations/resources/composited-animations-data/rotate-scale.js
(right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animations-data/rotate-scale.js:9:
stamps: [
On 2015/08/06 00:23:19, dstockwell wrote:
> Why isn't this just an array of numbers?
I want to keep it extensible so we can add extra properties to individual
samples.

> `stamps` doesn't seem like the best name, perhaps `times` or `samples`?
`samples` is great, thanks!

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animations-data/rotate-scale.js:34:
marker: false, // TODO(loyso): This mustn't affect cc/blink consistency.
On 2015/08/05 13:58:46, vollick wrote:
> What do you mean by this TODO?

Done.

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/virtual/threade...
File
LayoutTests/virtual/threaded/animations/composited-animations-rotate-scale.html
(right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/virtual/threade...
LayoutTests/virtual/threaded/animations/composited-animations-rotate-scale.html:7:
var test = new CompositedAnimationTest(false);
On 2015/08/05 13:58:46, vollick wrote:
> boolean parameter is super-confusing, especially since true == disabled. Can
you
> please find some way to make this named (even if you just set a local variable
> to the boolean value)? Also, if the compositor animations are disabled via
> InternalSettings (this is my preference), you could leave the settings stuff
to
> the wrapping html files, and just construct a CompositedAnimationTest with no
> parameters.

Done.

[loyso (OOO), 2015-09-15 07:50:52]

PTAL, everyone!

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
File LayoutTests/animations/resources/composited-animation-test.js (right):

https://codereview.chromium.org/1270303002/diff/1/LayoutTests/animations/reso...
LayoutTests/animations/resources/composited-animation-test.js:65: if
(test.data.vertical == null || !test.data.vertical)
On 2015/08/06 00:23:19, dstockwell wrote:
> These seem fairly arbitrary inputs to provide to the test configuraiton, can
you
> just use CSS?
Done. Note that I want to keep tests grouped so all test data resides in js file
and .html and -expected.html files are only a sub. So I can't use <template>
html stuff for markers/div contents 'as-is'.

I want to keep it simple, so each test case contains a style declaration. It
suits our needs.

[loyso (OOO), 2015-09-15 07:51:59]

On 2015/09/15 07:50:52, loyso wrote:
> Done. Note that I want to keep tests grouped so all test data resides in js
file
> and .html and -expected.html files are only a sub. So I can't use <template>
> html stuff for markers/div contents 'as-is'.
* .html and -expected.html files are only a stub.

[loyso (OOO), 2015-09-22 00:14:58]

PTAL!

[loyso (OOO), 2015-09-24 03:24:31]

Migrated to this chromium CL:
https://codereview.chromium.org/1360233004/

[Ian Vollick, 2015-09-30 21:29:54]

On 2015/09/24 03:24:31, loyso wrote:
> Migrated to this chromium CL:
> https://codereview.chromium.org/1360233004/

I'm so sorry for slow reply. Thank you for the updates. This lgtm % dstockwell.