Move animation update to follow paint phase for SPv2.

Move animation update to follow paint phase for SPv2.

BUG=692310
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2693363002
Cr-Commit-Position: refs/heads/master@{#451399}
Committed: https://chromium.googlesource.com/chromium/src/+/3d2b854663e325f0618ff313a4c58ff20e959891

[wkorman, 2017-02-15 18:50:45]

Description was changed from

==========
Move Document::updateAnimations to follow paint phase for SPv2.

BUG=692310
==========

to

==========
Move Document::updateAnimations to follow paint phase for SPv2.

BUG=692310
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[wkorman, 2017-02-15 18:52:33]

wkorman@chromium.org changed reviewers:
+ pdr@chromium.org

[wkorman, 2017-02-15 18:52:34]

Running try job.

[pdr., 2017-02-15 18:54:21]

On 2017/02/15 at 18:52:34, wkorman wrote:
> Running try job.

LGTM if tests pass.

Can you also update the lifecycle assert in updateAnimations to check for >=
paintclean for spv2?

[wkorman, 2017-02-15 22:03:28]

Description was changed from

==========
Move Document::updateAnimations to follow paint phase for SPv2.

BUG=692310
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Move animation update to follow paint phase for SPv2.

BUG=692310
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[wkorman, 2017-02-15 22:05:33]

On 2017/02/15 at 18:54:21, pdr wrote:
> Can you also update the lifecycle assert in updateAnimations to check for >=
paintclean for spv2?

Done.

[wkorman, 2017-02-15 22:05:38]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-02-15 22:05:38]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2693363002/#ps20001
(title: "Update lifecycle phase assert.")

[commit-bot: I haz the power, 2017-02-15 22:06:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 22:37:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 22:37:30]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[wkorman, 2017-02-16 00:29:53]

On 2017/02/15 at 18:54:21, pdr wrote:
> Can you also update the lifecycle assert in updateAnimations to check for >=
paintclean for spv2?

This assert is failing for hit-tests, see for example:

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

I'm preparing to investigate but if you have thought let's discuss.

[pdr., 2017-02-16 01:05:40]

On 2017/02/16 at 00:29:53, wkorman wrote:
> On 2017/02/15 at 18:54:21, pdr wrote:
> > Can you also update the lifecycle assert in updateAnimations to check for >=
paintclean for spv2?
> 
> This assert is failing for hit-tests, see for example:
> 
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> 
> I'm preparing to investigate but if you have thought let's discuss.

Oh darn, I see what you mean. Is it important that we tick the animations before
doing those hit tests? I suspect we don't need to (but am not sure)... if we
don't need to, just move the updateAnimations call into the "targetState ==
DocumentLifecycle::PaintClean" conditional?

[wkorman, 2017-02-16 01:15:29]

On 2017/02/16 at 01:05:40, pdr wrote:
> Oh darn, I see what you mean. Is it important that we tick the animations
before doing those hit tests? I suspect we don't need to (but am not sure)... if
we don't need to, just move the updateAnimations call into the "targetState ==
DocumentLifecycle::PaintClean" conditional?

Sounds good, moved, chrishtr@ also proposed what you said. Seems kosher.

[wkorman, 2017-02-16 01:15:48]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-02-16 01:15:48]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2693363002/#ps40001
(title: "Move updateAnimations to happen within PaintClean target state block.")

[commit-bot: I haz the power, 2017-02-16 01:16:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 02:26:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 02:26:33]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[wkorman, 2017-02-16 04:18:20]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2017-02-16 04:19:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 05:51:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 05:51:52]

Exceeded global retry quota

[pdr., 2017-02-16 17:45:47]

I haven't looked too closely but it looks like this is failing an svg image
animation case. I wonder if we need to run more of the lifecycle in
SVGImage::serviceAnimations with spv2?

[wkorman, 2017-02-16 19:58:14]

On 2017/02/16 at 17:45:47, pdr wrote:
> I haven't looked too closely but it looks like this is failing an svg image
animation case. I wonder if we need to run more of the lifecycle in
SVGImage::serviceAnimations with spv2?

Interesting. Yes, see this comment there. Do you have more background on the
importance of the cache coherence mentioned? Are we in fact violating things
in some manner that would require add'l work to address for SVG? Since the
normal fix would be, for SPv2, to update lifecycle up to and including
paint, but the comment reads as if that will make SVG subsystem produce
incorrect output in certain cases.

  // Do *not* update the paint phase. It's critical to paint only when
  // actually generating painted output, not only for performance reasons,
  // but to preserve correct coherence of the cache of the output with
  // the needsRepaint bits of the PaintLayers in the image.
  toLocalFrame(m_page->mainFrame())
      ->view()
      ->updateAllLifecyclePhasesExceptPaint();

[pdr., 2017-02-16 20:08:58]

On 2017/02/16 at 19:58:14, wkorman wrote:
> On 2017/02/16 at 17:45:47, pdr wrote:
> > I haven't looked too closely but it looks like this is failing an svg image
animation case. I wonder if we need to run more of the lifecycle in
SVGImage::serviceAnimations with spv2?
> 
> Interesting. Yes, see this comment there. Do you have more background on the
> importance of the cache coherence mentioned? Are we in fact violating things
> in some manner that would require add'l work to address for SVG? Since the
> normal fix would be, for SPv2, to update lifecycle up to and including
> paint, but the comment reads as if that will make SVG subsystem produce
> incorrect output in certain cases.
> 
>   // Do *not* update the paint phase. It's critical to paint only when
>   // actually generating painted output, not only for performance reasons,
>   // but to preserve correct coherence of the cache of the output with
>   // the needsRepaint bits of the PaintLayers in the image.
>   toLocalFrame(m_page->mainFrame())
>       ->view()
>       ->updateAllLifecyclePhasesExceptPaint();

Oh good catch, yeah I think the comment is correct. A single SVGImage can be
used for two <img> tags of different sizes. When each <img> tag paints, it
re-lays out the SVGImage and paints. If we paint here, we might force an extra
paint that is immediately invalidated by another paint of a different size. The
animation ticks should ultimately lead to each <img> tag requesting its own
paint.

SVGImage doesn't have a compositor and shouldn't have compositor animations.
Could we use this fact to avoid the bug?

[wkorman, 2017-02-16 20:26:41]

On 2017/02/16 at 20:08:58, pdr wrote:
> On 2017/02/16 at 19:58:14, wkorman wrote:
> > On 2017/02/16 at 17:45:47, pdr wrote:
> > > I haven't looked too closely but it looks like this is failing an svg
image animation case. I wonder if we need to run more of the lifecycle in
SVGImage::serviceAnimations with spv2?
> > 
> > Interesting. Yes, see this comment there. Do you have more background on the
> > importance of the cache coherence mentioned? Are we in fact violating things
> > in some manner that would require add'l work to address for SVG? Since the
> > normal fix would be, for SPv2, to update lifecycle up to and including
> > paint, but the comment reads as if that will make SVG subsystem produce
> > incorrect output in certain cases.
> > 
> >   // Do *not* update the paint phase. It's critical to paint only when
> >   // actually generating painted output, not only for performance reasons,
> >   // but to preserve correct coherence of the cache of the output with
> >   // the needsRepaint bits of the PaintLayers in the image.
> >   toLocalFrame(m_page->mainFrame())
> >       ->view()
> >       ->updateAllLifecyclePhasesExceptPaint();
> 
> Oh good catch, yeah I think the comment is correct. A single SVGImage can be
used for two <img> tags of different sizes. When each <img> tag paints, it
re-lays out the SVGImage and paints. If we paint here, we might force an extra
paint that is immediately invalidated by another paint of a different size. The
animation ticks should ultimately lead to each <img> tag requesting its own
paint.
> 
> SVGImage doesn't have a compositor and shouldn't have compositor animations.
Could we use this fact to avoid the bug?

Given that there are no compositor animations, we could probably safely add an
explicit call to DocumentAnimations::updateAnimations() (or add the relevant
internal part, which is document.timeline().scheduleNextService();) to
SVGImage::serviceAnimations().

I'm not sure whether this is compatible with whatever preconditions
SVGImage::serviceAnimations() has built around it, but it looks like it would be
worth trying. SVGImage::serviceAnimations() is just called by a timer
constructed by SVGImageChromeClient.

[pdr., 2017-02-16 20:30:09]

On 2017/02/16 at 20:26:41, wkorman wrote:
> On 2017/02/16 at 20:08:58, pdr wrote:
> > On 2017/02/16 at 19:58:14, wkorman wrote:
> > > On 2017/02/16 at 17:45:47, pdr wrote:
> > > > I haven't looked too closely but it looks like this is failing an svg
image animation case. I wonder if we need to run more of the lifecycle in
SVGImage::serviceAnimations with spv2?
> > > 
> > > Interesting. Yes, see this comment there. Do you have more background on
the
> > > importance of the cache coherence mentioned? Are we in fact violating
things
> > > in some manner that would require add'l work to address for SVG? Since the
> > > normal fix would be, for SPv2, to update lifecycle up to and including
> > > paint, but the comment reads as if that will make SVG subsystem produce
> > > incorrect output in certain cases.
> > > 
> > >   // Do *not* update the paint phase. It's critical to paint only when
> > >   // actually generating painted output, not only for performance reasons,
> > >   // but to preserve correct coherence of the cache of the output with
> > >   // the needsRepaint bits of the PaintLayers in the image.
> > >   toLocalFrame(m_page->mainFrame())
> > >       ->view()
> > >       ->updateAllLifecyclePhasesExceptPaint();
> > 
> > Oh good catch, yeah I think the comment is correct. A single SVGImage can be
used for two <img> tags of different sizes. When each <img> tag paints, it
re-lays out the SVGImage and paints. If we paint here, we might force an extra
paint that is immediately invalidated by another paint of a different size. The
animation ticks should ultimately lead to each <img> tag requesting its own
paint.
> > 
> > SVGImage doesn't have a compositor and shouldn't have compositor animations.
Could we use this fact to avoid the bug?
> 
> Given that there are no compositor animations, we could probably safely add an
explicit call to DocumentAnimations::updateAnimations() (or add the relevant
internal part, which is document.timeline().scheduleNextService();) to
SVGImage::serviceAnimations().
> 
> I'm not sure whether this is compatible with whatever preconditions
SVGImage::serviceAnimations() has built around it, but it looks like it would be
worth trying. SVGImage::serviceAnimations() is just called by a timer
constructed by SVGImageChromeClient.

+1, I think this is a good idea.

[wkorman, 2017-02-16 20:34:23]

On 2017/02/16 at 20:30:09, pdr wrote:
> +1, I think this is a good idea.

Trying it now. I wonder though whether we run the risk of scheduling multiple
frames or animation wake events. Is this guarded against in any manner, I am not
sure. Essentially wondering whether we might run updateAnimations() involved
logic twice in a single frame, once via path in FrameView and once via what we
add here.

[wkorman, 2017-02-16 22:19:07]

On 2017/02/16 at 20:34:23, wkorman wrote:
> On 2017/02/16 at 20:30:09, pdr wrote:
> > +1, I think this is a good idea.
> 
> Trying it now. I wonder though whether we run the risk of scheduling multiple
frames or animation wake events. Is this guarded against in any manner, I am not
sure. Essentially wondering whether we might run updateAnimations() involved
logic twice in a single frame, once via path in FrameView and once via what we
add here.

Somehow the SVG animation is stuck in playState = PENDING in SPv2 whenever
SVGImage::serviceAnimations is called, vs. RUNNING in SPv1.

Thus AnimationTimeline::scheduleNextService doesn't actually schedule an
animation frame servicing, because Animation::timeToEffectChange sees false for
hasStartTime() because the animation is not yet RUNNING.

Further LOG(ERROR) and cgdb debugging required.

[wkorman, 2017-02-16 23:14:26]

On 2017/02/16 at 22:19:07, wkorman wrote:
> On 2017/02/16 at 20:34:23, wkorman wrote:
> > On 2017/02/16 at 20:30:09, pdr wrote:
> > > +1, I think this is a good idea.
> > 
> > Trying it now. I wonder though whether we run the risk of scheduling
multiple frames or animation wake events. Is this guarded against in any manner,
I am not sure. Essentially wondering whether we might run updateAnimations()
involved logic twice in a single frame, once via path in FrameView and once via
what we add here.
> 
> Somehow the SVG animation is stuck in playState = PENDING in SPv2 whenever
SVGImage::serviceAnimations is called, vs. RUNNING in SPv1.
> 
> Thus AnimationTimeline::scheduleNextService doesn't actually schedule an
animation frame servicing, because Animation::timeToEffectChange sees false for
hasStartTime() because the animation is not yet RUNNING.
> 
> Further LOG(ERROR) and cgdb debugging required.

It turns out that document.compositorPendingAnimations().update() is responsible
for transitioning animations to RUNNING even when they're not composited
animations. That was surprising based on the name, CompositorPendingAnimations,
eh.

In any case, I'm preparing an update for this change that will allow passing a
param to updateAnimations() that we can use to alter the lifecycle DCHECK so
that we allow not-post-paint for the SVG case.

[wkorman, 2017-02-16 23:42:08]

PTAL

https://codereview.chromium.org/2693363002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/DocumentAnimations.cpp (right):

https://codereview.chromium.org/2693363002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/DocumentAnimations.cpp:71:
DCHECK(document.lifecycle().state() >= requiredLifecycleState);
Passing the state with which we DCHECK feels weird. The whole point of DCHECK is
more for the implementation methods such as this one to enforce their
preconditions. Having the callsite responsible for knowing the precondition and
passing it along is strange, see for example in the PaintLayerCompositor edit in
this change where we pass CompositingClean immediately after we've advanced
lifecycle to CompositingClean.

But the obvious alternatives were to either copy this method's logic into
SVGImage which I didn't like, or do something like passing a param (bool, enum,
or what have you) to specialize DCHECK further than just by REF::spv2 here. The
latter seemed like a messier version of what I've actually done here.

Open to thoughts.

https://codereview.chromium.org/2693363002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/2693363002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:537: // TODO(wkorman):
I'm not actually sure whether this is kosher. Is it possible
This patch now fixes the SVG image test that was failing, but see my TODO
comment here.

To gain confidence and remove this TODO I believe we'd need to know that all
code paths that involve adding an animation are assured to have moved document
lifecycle past paint before this method in SVGImage is called.

I will look into that, your feedback as add'l checkpoint helpful.

[wkorman, 2017-02-16 23:44:39]

The CQ bit was checked by wkorman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 23:45:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 02:12:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 02:12:04]

Dry run: This issue passed the CQ dry run.

[wkorman, 2017-02-17 19:35:14]

https://codereview.chromium.org/2693363002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/DocumentAnimations.cpp (right):

https://codereview.chromium.org/2693363002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/DocumentAnimations.cpp:71:
DCHECK(document.lifecycle().state() >= requiredLifecycleState);
On 2017/02/16 at 23:42:08, wkorman wrote:
> Passing the state with which we DCHECK feels weird. The whole point of DCHECK
is more for the implementation methods such as this one to enforce their
preconditions. Having the callsite responsible for knowing the precondition and
passing it along is strange, see for example in the PaintLayerCompositor edit in
this change where we pass CompositingClean immediately after we've advanced
lifecycle to CompositingClean.
> 
> But the obvious alternatives were to either copy this method's logic into
SVGImage which I didn't like, or do something like passing a param (bool, enum,
or what have you) to specialize DCHECK further than just by REF::spv2 here. The
latter seemed like a messier version of what I've actually done here.
> 
> Open to thoughts.

After sleeping on this I think it's ok.

https://codereview.chromium.org/2693363002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right):

https://codereview.chromium.org/2693363002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:537: // TODO(wkorman):
I'm not actually sure whether this is kosher. Is it possible
On 2017/02/16 at 23:42:08, wkorman wrote:
> This patch now fixes the SVG image test that was failing, but see my TODO
comment here.
> 
> To gain confidence and remove this TODO I believe we'd need to know that all
code paths that involve adding an animation are assured to have moved document
lifecycle past paint before this method in SVGImage is called.
> 
> I will look into that, your feedback as add'l checkpoint helpful.

I looked through things and I think this is ok too.

Lingering concern is that if we have multiple SVGImages I believe they each have
their own timer, and so depending on when they fire we could call
updateAnimations() multiple times. But I think that will just cause performance
overhead, which hopefully we'd see in perf benchmarks or UMA or otherwise
realize if it were problematic, and we could find a way to aggregate or
otherwise resolve then.

[wkorman, 2017-02-17 19:36:13]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2017-02-17 19:36:14]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2693363002/#ps80001
(title: "Update comment.")

[commit-bot: I haz the power, 2017-02-17 19:36:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 20:38:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 20:38:33]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[wkorman, 2017-02-17 21:00:20]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2017-02-17 21:01:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 23:07:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-17 23:07:03]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[wkorman, 2017-02-17 23:17:49]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2017-02-17 23:18:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 23:39:48]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487373469639970,
"parent_rev": "fb1a92c477c4dc2f72fdbeaa28b09b632cd928da", "commit_rev":
"3d2b854663e325f0618ff313a4c58ff20e959891"}

[commit-bot: I haz the power, 2017-02-17 23:40:54]

Description was changed from

==========
Move animation update to follow paint phase for SPv2.

BUG=692310
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Move animation update to follow paint phase for SPv2.

BUG=692310
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2693363002
Cr-Commit-Position: refs/heads/master@{#451399}
Committed:
https://chromium.googlesource.com/chromium/src/+/3d2b854663e325f0618ff313a4c5...
==========

[commit-bot: I haz the power, 2017-02-17 23:40:55]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3d2b854663e325f0618ff313a4c5...