Hiroyuki Ikezoe (:hiro) Reporter
	Description • 9 years ago

Attaching test case has an animation whose duration is 1ms. When reloading the test case, the animation rarely stops.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 1 • 9 years ago

This issue seems not happen on opt builds.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 2 • 9 years ago

Happened on opt builds. Seems not happen if the files on the web.

A reliable steps to reproduce this issue: 
1) save the file on local disk.
2) load the file
3) move mouse cursor outside the content (e.g. on the tab bar)
4) Ctrl+r

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 3 • 9 years ago

I can reproduce this issue with longer duration, e.g. 1s, if I don't move mouse at all until animation finished.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 4 • 9 years ago

Ugly but I have no other idea for now.

The reason of this issue is that when animations are normally finished we don't update layer at all.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 5 • 9 years ago

I guess bug 1227851 and bug 1204336 are caused by the same reason.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 6 • 9 years ago

This patch includes a reftest for this issue and another reftest which checks the last paint when animation.finish() is called. The latter test works without any fixes on current trunk.

	Brian Birtles (:birtles)
	Comment 7 • 9 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> I guess bug 1227851 and bug 1204336 are caused by the same reason.

Excellent!

I wonder if this is a regression.

My guess is that test_animations_omta.html doesn't pick this up because it might flush style accidentally (we might even be deliberately flushing style to work around this!).

test_animations_omta_start.html is really careful not to flush style unnecessarily because we used to have similar bugs when starting animations (i.e. if you move the mouse the animation starts, but otherwise it sometimes doesn't).

Animation::CanThrottle detects when we are *newly* finished by using the mFinishedAtLastComposeStyle flag so at very least we should use that and only request a layer update when we are *newly* finished.

Anyway, it would be good to know if this is a regression or not (we should probably check the last year, making sure to turn on OMTA for builds where it was off by default).

	Brian Birtles (:birtles)
	Comment 8 • 9 years ago

Interestingly, bug 1228229 should fix this but doesn't seem to.

Specifically, in that bug, we'll detect when we remove the animation from the effect set and trigger a layer update.

	Brian Birtles (:birtles)
	Comment 9 • 9 years ago

Comment on attachment 8694456 [details] [diff] [review]
Reftests that the last paint for compositor animations is surely processed

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

I'd like to understand what the actual problem is here so I can understand if this is a good test for it or not.

Why do we sometimes succeed in taking animations off layers but not in other cases?

::: layout/reftests/css-animations/last-paint-on-finished.html
@@ +25,5 @@
> +}
> +
> +function RemoveReftestWait() {
> +  document.documentElement.classList.remove("reftest-wait");
> +}

No need for all the extra functions (or lines over 80 chars), I think it's more idiomatic to just write:

document.getElementById("target").addEventListener("animationend",
  function() {
    setTimeout(function() {
      document.documentElement.classList.remove("reftest-wait");
    }, 0);
  });

But why the setTimeout? I presume you're waiting until after the next paint finishes. Does the test not reliably fail with requestAnimationFrame? Is the problem that we don't end up ticking the refresh driver?

::: layout/reftests/css-animations/last-paint-when-finish-called.html
@@ +20,5 @@
> +<script>
> +
> +var animation = document.getElementById("target").getAnimations()[0];
> +animation.finish();
> +document.documentElement.classList.remove("reftest-wait");

Does this pass or not? Calling finish() should update layers but I think we still need to wait for the next paint to happen?

::: layout/reftests/css-animations/reftest.list
@@ +4,5 @@
>  fails != print-no-animations.html print-no-animations-notref.html # reftest harness doesn't actually make pres context non-dynamic for reftest-print tests
>  == animate-opacity.html animate-opacity-ref.html
>  == animate-preserves3d.html animate-preserves3d-ref.html
> +== last-paint-on-finished.html last-paint-ref.html
> +pref(dom.animations-api.core.enabled,true) == last-paint-when-finish-called.html last-paint-ref.html

Do these both fail? I would expect the one that calls finish() to pass (at least after we wait for the next paint).

If you're planning to land this ahead of the patch that fixes it we should mark them as failing.

	Brian Birtles (:birtles)
	Comment 10 • 9 years ago

I'd like to know why we sometimes succeed in pulling animations off the compositor but not in other cases.

Given that this bug manifests more frequently in debug builds, with resources loaded from local files, and when the browser is under load (bug 1227851 comment 2), my guess is the bug has to do with the number of refresh driver ticks we get per animation.

For example, perhaps it only occurs if our first tick occurs after the animation has finished etc.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 11 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> > I guess bug 1227851 and bug 1204336 are caused by the same reason.
> 
> Excellent!
> 
> I wonder if this is a regression.

Last night I did mozregression partway (end of this April), had a conclusion that this is caused by https://hg.mozilla.org/mozilla-central/rev/76671894fe90. So I guress this issue has been there in the beginning of the async animations.

> My guess is that test_animations_omta.html doesn't pick this up because it
> might flush style accidentally (we might even be deliberately flushing style
> to work around this!).

Yeah, some of tests in test_animations_omta.html call GetPropertyValue which leads style updating. (See bug 1197620#c6). So those tests were not useful in this case.

> test_animations_omta_start.html is really careful not to flush style
> unnecessarily because we used to have similar bugs when starting animations
> (i.e. if you move the mouse the animation starts, but otherwise it sometimes
> doesn't).

If test_animations_omta_start.html has a test case something like this issue, the test might catch this issue. But I did write a reftest because I do want to avoid the test mode of refresh driver/

> Animation::CanThrottle detects when we are *newly* finished by using the
> mFinishedAtLastComposeStyle flag so at very least we should use that and
> only request a layer update when we are *newly* finished.

Thanks for the suggestion. I will do it if we take the approach here.

> Anyway, it would be good to know if this is a regression or not (we should
> probably check the last year, making sure to turn on OMTA for builds where
> it was off by default).

I will try mozregression again.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 12 • 9 years ago

I did mozregression with enabling async-animations.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e78a6795d2

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 13 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #10)
> I'd like to know why we sometimes succeed in pulling animations off the
> compositor but not in other cases.
> 
> Given that this bug manifests more frequently in debug builds, with
> resources loaded from local files, and when the browser is under load (bug
> 1227851 comment 2), my guess is the bug has to do with the number of refresh
> driver ticks we get per animation.

Comment#2 was not correct. As far as I can tell this issue will not happen on non-E10S. I did use a nightly opt build with non-E10S settings in comment#2. On E10S, I just confirmed now that this issue happens on opt builds with the file on the web.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 14 • 9 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> (In reply to Brian Birtles (:birtles) from comment #10)
> > I'd like to know why we sometimes succeed in pulling animations off the
> > compositor but not in other cases.
> > 
> > Given that this bug manifests more frequently in debug builds, with
> > resources loaded from local files, and when the browser is under load (bug
> > 1227851 comment 2), my guess is the bug has to do with the number of refresh
> > driver ticks we get per animation.
> 
> Comment#2 was not correct. As far as I can tell this issue will not happen
> on non-E10S.

I meant *will not happen easily*.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 15 • 9 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> (In reply to Brian Birtles (:birtles) from comment #10)
> > I'd like to know why we sometimes succeed in pulling animations off the
> > compositor but not in other cases.
> > 
> > Given that this bug manifests more frequently in debug builds, with
> > resources loaded from local files, and when the browser is under load (bug
> > 1227851 comment 2), my guess is the bug has to do with the number of refresh
> > driver ticks we get per animation.
> 
> Comment#2 was not correct. As far as I can tell this issue will not happen
> on non-E10S. I did use a nightly opt build with non-E10S settings in
> comment#2. On E10S, I just confirmed now that this issue happens on opt
> builds with the file on the web.

OK. I can now understand why that test case does not fail on non-E10S with the file on the web.
The reason is that there was a loading animation icon in a tab. The animation icon led a paint for finished opacity animation because on non-E10S chrome's refresh driver kicks paint process for child content.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 16 • 9 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)

> OK. I can now understand why that test case does not fail on non-E10S with
> the file on the web.
> The reason is that there was a loading animation icon in a tab. The
> animation icon led a paint for finished opacity animation because on
> non-E10S chrome's refresh driver kicks paint process for child content.

That means reftest for this issue is more reliable than mochitest.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 17 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #9)
> I'd like to understand what the actual problem is here so I can understand
> if this is a good test for it or not.
> 
> Why do we sometimes succeed in taking animations off layers but not in other
> cases?

I did answer to the question in comment#15. The loading icon in a tab causes flush painting.

> ::: layout/reftests/css-animations/last-paint-on-finished.html
> @@ +25,5 @@
> > +}
> > +
> > +function RemoveReftestWait() {
> > +  document.documentElement.classList.remove("reftest-wait");
> > +}
> 
> No need for all the extra functions (or lines over 80 chars), I think it's
> more idiomatic to just write:
> 
> document.getElementById("target").addEventListener("animationend",
>   function() {
>     setTimeout(function() {
>       document.documentElement.classList.remove("reftest-wait");
>     }, 0);
>   });
> 
> But why the setTimeout? I presume you're waiting until after the next paint
> finishes. Does the test not reliably fail with requestAnimationFrame? Is the
> problem that we don't end up ticking the refresh driver?

That was a copy-and-pasted code. Actually the setTimeout is unnecessary.
I removed it.

> ::: layout/reftests/css-animations/last-paint-when-finish-called.html
> @@ +20,5 @@
> > +<script>
> > +
> > +var animation = document.getElementById("target").getAnimations()[0];
> > +animation.finish();
> > +document.documentElement.classList.remove("reftest-wait");
> 
> Does this pass or not? Calling finish() should update layers but I think we
> still need to wait for the next paint to happen?

This test passes. You are right. This test case is for just in case. In future someone (I!) might cause this kind of regressions. 

> ::: layout/reftests/css-animations/reftest.list
> @@ +4,5 @@
> >  fails != print-no-animations.html print-no-animations-notref.html # reftest harness doesn't actually make pres context non-dynamic for reftest-print tests
> >  == animate-opacity.html animate-opacity-ref.html
> >  == animate-preserves3d.html animate-preserves3d-ref.html
> > +== last-paint-on-finished.html last-paint-ref.html
> > +pref(dom.animations-api.core.enabled,true) == last-paint-when-finish-called.html last-paint-ref.html
> 
> Do these both fail? I would expect the one that calls finish() to pass (at
> least after we wait for the next paint).
> 
> If you're planning to land this ahead of the patch that fixes it we should
> mark them as failing.

I don't want to land this test without fix.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 18 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #8)
> Interestingly, bug 1228229 should fix this but doesn't seem to.
> 
> Specifically, in that bug, we'll detect when we remove the animation from
> the effect set and trigger a layer update.

I did try patches for bug 1228229 and am reading them now.
The reason why bug 1228229 does not help this issue is that EffectSet is already empty when triggering a layer update.

	Brian Birtles (:birtles)
	Comment 19 • 9 years ago

Comment on attachment 8694604 [details] [diff] [review]
Reftests that the last paint for compositor animations is surely processed v2

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

r=birtles with comments addressed

::: layout/reftests/css-animations/last-paint-on-finished.html
@@ +1,2 @@
> +<!DOCTYPE HTML>
> +<html class="reftest-no-flush reftest-no-sync-layers reftest-wait">

The docs for reftest-no-sync-layers say we should wait for a MozAfterPaint before ending:

  https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.txt

@@ +1,3 @@
> +<!DOCTYPE HTML>
> +<html class="reftest-no-flush reftest-no-sync-layers reftest-wait">
> +<title>The last paint is surely processed for compositor animations</title>

"Test the last frame of compositor animations is painted"

::: layout/reftests/css-animations/last-paint-ref.html
@@ +1,3 @@
> +<!DOCTYPE HTML>
> +<html>
> +<title>In visibility hidden color animation on pseudo element</title>

This title doesn't seem correct.

::: layout/reftests/css-animations/last-paint-when-finish-called.html
@@ +1,3 @@
> +<!DOCTYPE HTML>
> +<html class="reftest-no-flush reftest-no-sync-layers reftest-wait">
> +<title>The last paint is surely processed for compositor animations when animation.finish() is called</title>

"Test the last frame of compositor animations is painted when animation.finish() is called"

@@ +9,5 @@
> +
> +#target {
> +  background-color: blue;
> +  opacity: 0;
> +  animation: opacity 100s forwards;

I don't understand why the setup for this test is different to last-paint-on-finished.html?

In that test we use a backwards fill mode. Can't we use the same setup here?

@@ +20,5 @@
> +<script>
> +
> +var animation = document.getElementById("target").getAnimations()[0];
> +animation.finish();
> +document.documentElement.classList.remove("reftest-wait");

Again, I think we're supposed to wait for the next MozAfterPaint before ending since we use reftest-no-sync-layers?

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 20 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #19)
> Comment on attachment 8694604 [details] [diff] [review]
> Reftests that the last paint for compositor animations is surely processed v2
> 
> Review of attachment 8694604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=birtles with comments addressed
> 
> ::: layout/reftests/css-animations/last-paint-on-finished.html
> @@ +1,2 @@
> > +<!DOCTYPE HTML>
> > +<html class="reftest-no-flush reftest-no-sync-layers reftest-wait">
> 
> The docs for reftest-no-sync-layers say we should wait for a MozAfterPaint
> before ending:
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/README.
> txt

Thanks! I did not read the document carefully.  I thought reftest-no-sync-layers avoids calling updateLayerTree (this leads a paint) in SynchronizeForSnapshot, but it is not a problem because SynchronizeForSnapshot is not called at all in this case.  reftest-no-flush is sufficient for this case.
https://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js#943

I will remove reftest-no-sync-layers.

> @@ +1,3 @@
> > +<!DOCTYPE HTML>
> > +<html class="reftest-no-flush reftest-no-sync-layers reftest-wait">
> > +<title>The last paint is surely processed for compositor animations</title>
> 
> "Test the last frame of compositor animations is painted"

Thanks! Thanks! I will revise it.

> ::: layout/reftests/css-animations/last-paint-when-finish-called.html
> @@ +1,3 @@
> > +<!DOCTYPE HTML>
> > +<html class="reftest-no-flush reftest-no-sync-layers reftest-wait">
> > +<title>The last paint is surely processed for compositor animations when animation.finish() is called</title>
> 
> "Test the last frame of compositor animations is painted when
> animation.finish() is called"
> 
> @@ +9,5 @@
> > +
> > +#target {
> > +  background-color: blue;
> > +  opacity: 0;
> > +  animation: opacity 100s forwards;
> 
> I don't understand why the setup for this test is different to
> last-paint-on-finished.html?
> 
> In that test we use a backwards fill mode. Can't we use the same setup here?

I thought if we use backwards here, we should wait some frames to avoid mis-checking the difference between the initial composed frame and the last one.
But, yes we can use backwards with negative delay.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 22 • 9 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> (In reply to Brian Birtles (:birtles) from comment #8)
> > Interestingly, bug 1228229 should fix this but doesn't seem to.
> > 
> > Specifically, in that bug, we'll detect when we remove the animation from
> > the effect set and trigger a layer update.
> 
> I did try patches for bug 1228229 and am reading them now.
> The reason why bug 1228229 does not help this issue is that EffectSet is
> already empty when triggering a layer update.

There are two ways to trigger a layer update when we remove the animation.

a) Add a flag named mLayerNeedsUpdated to EffectSet and set the flag true in EffectSet::RemoveEffect just as mCascadeNeedsUpdate, then call RequestRestyle if the cascading results are changed or mLayerNeedsUpdated is true in UpdateCascadeResults (or MaybeUpdateCascadeRestuls).

b) Call explicitly RequestRestyle in KeyframeEffectReadOnly::UpdateTargetRegistration at https://dxr.mozilla.org/mozilla-central/rev/eec9a0bfd929a68237f0ef799a9d8f28c4749296/dom/animation/KeyframeEffect.cpp#553

I'd take b) here because calling RequestRestyle not for *cascading results* in UpdateCascadeResults is somewhat misleading to me.

Brian, any thoughts?

	Brian Birtles (:birtles)
	Comment 23 • 9 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> > (In reply to Brian Birtles (:birtles) from comment #8)
> > > Interestingly, bug 1228229 should fix this but doesn't seem to.
> > > 
> > > Specifically, in that bug, we'll detect when we remove the animation from
> > > the effect set and trigger a layer update.
> > 
> > I did try patches for bug 1228229 and am reading them now.
> > The reason why bug 1228229 does not help this issue is that EffectSet is
> > already empty when triggering a layer update.
> 
> There are two ways to trigger a layer update when we remove the animation.
> 
> a) Add a flag named mLayerNeedsUpdated to EffectSet and set the flag true in
> EffectSet::RemoveEffect just as mCascadeNeedsUpdate, then call
> RequestRestyle if the cascading results are changed or mLayerNeedsUpdated is
> true in UpdateCascadeResults (or MaybeUpdateCascadeRestuls).
> 
> b) Call explicitly RequestRestyle in
> KeyframeEffectReadOnly::UpdateTargetRegistration at
> https://dxr.mozilla.org/mozilla-central/rev/
> eec9a0bfd929a68237f0ef799a9d8f28c4749296/dom/animation/KeyframeEffect.cpp#553
> 
> I'd take b) here because calling RequestRestyle not for *cascading results*
> in UpdateCascadeResults is somewhat misleading to me.
> 
> Brian, any thoughts?

I was going to handle this in bug 1230040. Specifically, this code which triggers a layer-type restyle when the animation first finishes:

https://bugzilla.mozilla.org/attachment.cgi?id=8695113&action=diff#a/dom/animation/Animation.cpp_sec3

Are there other cases we need to consider?

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 24 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #23)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> > (In reply to Hiroyuki Ikezoe (:hiro) from comment #18)
> > > (In reply to Brian Birtles (:birtles) from comment #8)
> > > > Interestingly, bug 1228229 should fix this but doesn't seem to.
> > > > 
> > > > Specifically, in that bug, we'll detect when we remove the animation from
> > > > the effect set and trigger a layer update.
> > > 
> > > I did try patches for bug 1228229 and am reading them now.
> > > The reason why bug 1228229 does not help this issue is that EffectSet is
> > > already empty when triggering a layer update.
> > 
> > There are two ways to trigger a layer update when we remove the animation.
> > 
> > a) Add a flag named mLayerNeedsUpdated to EffectSet and set the flag true in
> > EffectSet::RemoveEffect just as mCascadeNeedsUpdate, then call
> > RequestRestyle if the cascading results are changed or mLayerNeedsUpdated is
> > true in UpdateCascadeResults (or MaybeUpdateCascadeRestuls).
> > 
> > b) Call explicitly RequestRestyle in
> > KeyframeEffectReadOnly::UpdateTargetRegistration at
> > https://dxr.mozilla.org/mozilla-central/rev/
> > eec9a0bfd929a68237f0ef799a9d8f28c4749296/dom/animation/KeyframeEffect.cpp#553
> > 
> > I'd take b) here because calling RequestRestyle not for *cascading results*
> > in UpdateCascadeResults is somewhat misleading to me.
> > 
> > Brian, any thoughts?
> 
> I was going to handle this in bug 1230040. Specifically, this code which
> triggers a layer-type restyle when the animation first finishes:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=8695113&action=diff#a/dom/
> animation/Animation.cpp_sec3
> 
> Are there other cases we need to consider?

Nothing I can think of. That will fix this issue!

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 25 • 8 years ago

This issue is still there, but only opacity animations.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 26 • 8 years ago

This bug has been fixed part 3 patch for bug 1223658 because now we pass fill mode to the compositor.  Before the part 3 patch, we passed 'fill:both' to the compositor, as a result, the last opacity value persisted.

Now we need a test case for this bug, unfortunately, attachment 8695212 [details] [diff] [review] fails intermittently, we need to figure out to avoid the failure.

	Hiroyuki Ikezoe (:hiro) Reporter
	Comment 27 • 8 years ago

As per bug 1311196 comment 18, I am going to re-use this bug for writing additional test case that checks transitions is pulled back from the compositor as well.