1361234 - Start time for compositor animations should not include the delay

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement)

Description

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Test case

In investigating bug 1334583 I noticed that when we calculate the pending start time for an animation running on a layer we factor in the delay[1] even though we also pass the delay to the compositor.

This appears to be an oversight where in bug 1223658 we separated out the delay in the following changeset but failed to remove it from this calculation:

  https://hg.mozilla.org/mozilla-central/rev/b66b75c2d042101b954e6423438cc07955c2b9bd

In the attached test case you can often see the animation jump part way through when the layer is later updated. (You don't always see it, however, since sometimes I suppose we update the layer soon enough that we cover up the bug.)

[1] http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/gfx/layers/Layers.cpp#296

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(The change to the playbackRate in the test case is not strictly necessary--I was just trying to understand why we could incorporate the delay without also having to incorporate the playbackRate, then I realized we should not be incorporating the delay at all.)

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

And I realize now why none of our tests caught this: the code path through Layer::StartPendingAnimations is not called when the refresh driver is under test control :(

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Actually, we have a test specifically for this code path: test_deferred_start.html. However, it looks like it was refactored in bug 1168759 and now no longer covers this case. Furthermore, it currently doesn't even run at all since, as of [1] we now do:

  return waitForPaints(function() {
    ... test code goes here ...
  });

But the test code never runs because the argument to waitForPaints is ignored. This is supposed to be:

  return waitForPaints().then(function() {
    ...

[1] https://hg.mozilla.org/mozilla-central/rev/986b18fdfdba

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #2)
> And I realize now why none of our tests caught this: the code path through
> Layer::StartPendingAnimations is not called when the refresh driver is under
> test control :(

☹

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1361234 - Restore test_deferred_start.html to actually testing delay;

Once upon a time[1] a test was added to test_deferred_start.html to test the
code path where we establish the start time to pass to the compositor when
resolving a pending animation.

Later[2], however, we encountered intermittent failures on B2G so we made it
stop waiting on animation.ready and use waitForPaints instead. There were two
problems with this, however.

Firstly, waiting for paints often means that extra paints are processed such
that we end up updating the start time on the layer using a different code path
and masking any potential bugs in the code path under test.

Secondly, when we made this change we replaced:

  return animation.ready.then(function() { /* test code */ })

with:

  return waitForPaints(function() { /* test code */ })

And sadly that means that 'test code' never runs. Of course, what we meant to
write was:

  return waitForPaints().then(function() { /* test code */ })

As a result, when we later broke the code path under test no one noticed.

This patch restores the test so that it tests what it intends to (and currently
fails, at least most of the time).

[1] https://hg.mozilla.org/mozilla-central/rev/79cac8c71159
[2] https://hg.mozilla.org/mozilla-central/rev/986b18fdfdba
[3] https://hg.mozilla.org/mozilla-central/rev/b66b75c2d042101b954e6423438cc07955c2b9bd

Review commit: https://reviewboard.mozilla.org/r/135406/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135406/

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1361234 - Fix start time calculation for pending animations on layers;

In bug 1223658 we separated out the delay from the start time but we failed to
remove it from this calculation. As a result, when a pending animation begins it
will have the delay applied twice (once here, and once when it is sampled on the
compositor). This will happen until the layer is next updated.

This bug was not exposed by any existing tests since we don't use this code path
when the refresh driver is under test control. Furthermore, the one test that
was supposed to cover this was refactored in such a way that it stopped testing
this code path. That test is restored earlier in this patch series and enabled
in this patch.

Review commit: https://reviewboard.mozilla.org/r/135408/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135408/

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863617 [details]
Bug 1361234 - Restore test_deferred_start.html to actually testing delay;

https://reviewboard.mozilla.org/r/135406/#review138344

Thanks for fixing!

::: dom/animation/test/mozilla/file_deferred_start.html:34
(Diff revision 1)
> +function waitForIdle() {
> +  return new Promise(resolve => {
> +    requestIdleCallback(resolve);
> +  });
> +}

Now requestIdleCall back is enabled on all channels (bug 1314959). 
We can define this function in testcommon.js.

::: dom/animation/test/mozilla/file_deferred_start.html:108
(Diff revision 1)
> +                { duration: 200 * MS_PER_SEC,
> +                  delay: -100 * MS_PER_SEC });

Nice longer duration.

::: dom/animation/test/mozilla/file_deferred_start.html:129
(Diff revision 1)
>  
> -    // If the delay has been applied correctly we should be at least
> -    // half-way through the animation
> -    assert_true(matrixComponents[4] >= 50,
> -                'Animation is at least half-way through on the compositor'
> -                + ' (got translation of ' + matrixComponents[4] + ')');
> +    // If the delay has been applied we should be at least half-way through
> +    // the animation. However, if we applied it twice we will be at the
> +    // end of the animation already.
> +    const translateX = matrixComponents[4];
> +    assert_between_inclusive(translateX, 50, 75,

I am not convinced that we can get the value from the compositor within 50 sec on busy main-thread on slow platforms.  (IIRC I saw over 60 sec. on Android)  Would you mind doubling the duration?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863618 [details]
Bug 1361234 - Fix start time calculation for pending animations on layers;

https://reviewboard.mozilla.org/r/135408/#review138348

A terrible mistake.

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #7)
> ::: dom/animation/test/mozilla/file_deferred_start.html:129
> (Diff revision 1)
> >  
> > -    // If the delay has been applied correctly we should be at least
> > -    // half-way through the animation
> > -    assert_true(matrixComponents[4] >= 50,
> > -                'Animation is at least half-way through on the compositor'
> > -                + ' (got translation of ' + matrixComponents[4] + ')');
> > +    // If the delay has been applied we should be at least half-way through
> > +    // the animation. However, if we applied it twice we will be at the
> > +    // end of the animation already.
> > +    const translateX = matrixComponents[4];
> > +    assert_between_inclusive(translateX, 50, 75,
> 
> I am not convinced that we can get the value from the compositor within 50
> sec on busy main-thread on slow platforms.  (IIRC I saw over 60 sec. on
> Android)  Would you mind doubling the duration?

I think I might need to change the lower-bound of that check too. I just got the following failure on try for the test added in bug 1361260:

> dom/animation/test/mozilla/test_deferred_start.html | Starting an animation with a playbackRate starts from the correct point - Starting an animation with a playbackRate starts from the correct point: assert_between_inclusive: Animation is at least half-way but no more than 75% of the way through on the compositor expected a number greater than or equal to 50 and less than or equal to 75 but got 49.6996

I suspect we didn't encouter similar tests before because we used CSS animations with the default easing of 'ease' so that even if we were a little under the 50% mark, the easing would push us over. So I'll update this check to go from 45 to 75 (as well as doubling the duration).

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8863617 [details]
Bug 1361234 - Restore test_deferred_start.html to actually testing delay;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135406/diff/1-2/

Comment 11

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8863618 [details]
Bug 1361234 - Fix start time calculation for pending animations on layers;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135408/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4317e4c26505
Restore test_deferred_start.html to actually testing delay; r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/438c1291a77a
Fix start time calculation for pending animations on layers; r=hiro

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4317e4c26505
https://hg.mozilla.org/mozilla-central/rev/438c1291a77a