1316764 - Graphic glitch persists after animation before repaint

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Xidorn Quan [:xidorn] UTC+11]

This bug is for the pre-existing graphics issue which is revealed in bug 1316236 that, a very small perspective value leads to some graphic glitch persists after animation finishes.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: testcase

This is a further reduced testcase from that in bug 1316236 which changes the perspective value from 0 to 1px so that rendering between Firefox and other browsers is comparable.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

I cannot reproduce this issue on macOS but I can on Windows, so setting the platform to Windows.

Comment 3

[Alice0775 White]

Attachment: testcase4

Comment 4

[Alice0775 White]

Attachment: screenshot on Nightly52.0a1 HWA on

There are 2 regression window:

#1 Ghost shadow remains when HWA on (but not often):
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2a61df4eaa2d&tochange=24ba8274ed60

Regressed by : Bug 1107297

#2 Ghost shadow remains when HWA on and off (often):
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=868407a1bd36a284484d2ce4d6ed0bac584a8536&tochange=c9a8a051dd30220ae7a575547de2888e794d67aa

Regressed by : Bug 1311196

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Yes, this is likely bug 1229283.  That bug was fixed by this change set <https://hg.mozilla.org/mozilla-central/rev/00799db70975> once, but returned because of this change set <https://hg.mozilla.org/mozilla-central/rev/c9a8a051dd30>.

We have to figure out another reliable way to pull animations back from the compositor.

Comment 6

[Bas Schouten (:bas.schouten)]

Brian probably knows more about this than I do, as far as I can tell there's no actual bug in the compositor, just in the area we tell the compositor has changed.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Two solutions I can think of

1) Invoke SchedulePaint() for the target frame when we destroy an effect in effect set.
2) Prolong an effect in effect set until the last composition for the effect is finished on the compositor.

1) is hack but easy, 2) is a proper way (I think) but seems to be difficult.

Comment 8

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

Hiro, it seems like you looked into this; what was the issue? Are we failing to trigger a paint that removes the animations from the compositor when the animation finishes? I thought we had code the triggered a layer restyle when an animation is newly-finished.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #8)
> Hiro, it seems like you looked into this; what was the issue? Are we failing
> to trigger a paint that removes the animations from the compositor when the
> animation finishes? I thought we had code the triggered a layer restyle when
> an animation is newly-finished.

Right, if the newly-finished animation is the last animation on an element, we destroy the EffectSet for the element before requesting restyle.  As a result, the restyle request is not processed.

Comment 10

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> Right, if the newly-finished animation is the last animation on an element,
> we destroy the EffectSet for the element before requesting restyle.  As a
> result, the restyle request is not processed.

So the layer restyle is triggered from Animation::Tick[1] (i.e. the timeline) and that posts a restyle for animations, but, in the case where transform values are equal before and after the restyle, we don't update layers. Normally we'd force that update to happen[2] by changing the animation generation but the check in [3] for an effect set fails so we don't do that either. Is that right?

[1] http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/dom/animation/Animation.cpp#603
[2] http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/layout/base/RestyleManager.cpp#1544
[3] http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/dom/animation/EffectCompositor.cpp#212

Comment 11

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

I wonder if we just change the > in [1] to != if this will work (and not generate unnecessary restyles).

There's already a comment saying it should be != and when there's no effect set the generation on the frame will be zero so it should compare false I expect.

[1] http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/layout/base/RestyleManager.cpp#1544

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Yes, from what I understand.

(In reply to Brian Birtles (:birtles) from comment #10)

> So the layer restyle is triggered from Animation::Tick[1] (i.e. the
> timeline) and that posts a restyle for animations, but, in the case where
> transform values are equal before and after the restyle, we don't update
> layers. Normally we'd force that update to happen[2] by changing the
> animation generation but the check in [3] for an effect set fails so we
> don't do that either. Is that right?
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 886d5186f0598ab2f8a9953eb5a4dab9750ef834/dom/animation/Animation.cpp#603
> [2]
> http://searchfox.org/mozilla-central/rev/
> 886d5186f0598ab2f8a9953eb5a4dab9750ef834/layout/base/RestyleManager.cpp#1544

Moreover, in [2] RestyleManager::GetAnimationGenerationForFrame() is useless in that case because it uses EffectSet::GetEffectSet() in restyling process.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #11)
> I wonder if we just change the > in [1] to != if this will work (and not
> generate unnecessary restyles).

I am unsure it will not cause the unnecessary restyles...  It seems to me that it will.

Comment 14

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> (In reply to Brian Birtles (:birtles) from comment #11)
> > I wonder if we just change the > in [1] to != if this will work (and not
> > generate unnecessary restyles).
> 
> I am unsure it will not cause the unnecessary restyles...  It seems to me
> that it will.

Why is that, the animation generation on the layer will be ultimately updated to the value on the effect set or zero if the effect set is missing[1] so this should only evaluate false at the point where we have yet to call AddAnimationsAndTransitionsToLayer. Perhaps we could have a situation where we drop all animations and hence never call AddAnimationsAndTransitionsToLayer? For that, perhaps we just need to force the animation generation on the layer to zero at that point?

[1] http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/layout/base/nsDisplayList.cpp#654

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

Hey RyanVM, why do you think this does not affect 50 and 51? Looks like the original bug 1316236 affects 51 ~53

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Sorry, I got fixated on the regression from bug 1311196 part.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Brian, do we have a conclusion as to how to proceed?

Comment 18

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

Yes, I think the proposal in comment 11 is the correct approach here. Moving to DOM: Animation so we can track it there.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Brian, is there a simple patch coming that we could uplift?

Comment 20

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

Attachment: Bug 1316764 - Update animations on layers whenever the frame generation is not equal;

This fixes a problem where we fail to update animations on layers when an
EffectSet is destroyed. In this case when we call
RestyleManager::GetAnimationGenerationForFrame in
ElementRestyler::AddLayerChangesForAnimation(), it will return zero but the
animation generation on the layer will set to some positive non-zero value.

If we compare the to generation numbers using < we will think the layer is
up-to-date. Using != fixes this. We only used < while we had independent
generation numbers of animations and transitions but that was fixed in bug
1229280.

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

Comment 21

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

Yes, I have a patch on try now for fixing the regression from bug 1311196.[1] It's very small but needs a test case before it's ready for review.

The regression from bug 1107297, however, is not fixed by this. That looks like something for Bas. I've filed bug 1325235 for that.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd614856dc9d4d0e0d9e6dfe5ab01aada12e66bc

Comment 22

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

Attachment: Attempt test case

I tried to write an automated test for this but it always passes even without the fix applied. There are at least three approaches I can think of:

* A mochitest like test_animations_omta.html that puts the refresh driver in test mode. This didn't work--I think because advancing the refresh driver triggered the missing paint.
* A mochitest like test_deferred_start.html that *doesn't* put the refresh driver in test mode. I had so much trouble in the past making that test not fail intermittently I'd rather avoid that approach again.
* A reftest. This doesn't seem to work due to bug 1248828 it seems.

I'm attaching the reftest for now but I'm inclined to give up on getting an automated test to work at this stage. We can revisit that after bug 1248828 is resolved.

Comment 23

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

Attachment: Simplified test case

Here is a standalone version of the test case.

Be careful not to move the mouse when using it. The animation does not have a fill mode so it should return to its original position on the left after a short time but ~50% of the time it does not until you move the mouse.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #22)
> Created attachment 8821031 [details] [diff] [review]
> Attempt test case
> 
> I tried to write an automated test for this but it always passes even
> without the fix applied. There are at least three approaches I can think of:
> 
> * A mochitest like test_animations_omta.html that puts the refresh driver in
> test mode. This didn't work--I think because advancing the refresh driver
> triggered the missing paint.
> * A mochitest like test_deferred_start.html that *doesn't* put the refresh
> driver in test mode. I had so much trouble in the past making that test not
> fail intermittently I'd rather avoid that approach again.
> * A reftest. This doesn't seem to work due to bug 1248828 it seems.
> 
> I'm attaching the reftest for now but I'm inclined to give up on getting an
> automated test to work at this stage. We can revisit that after bug 1248828
> is resolved.

Yeah, bug 1248828 is a big missing piece for testing animations on the compositor.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8820944 [details]
Bug 1316764 - Update animations on layers whenever the frame generation is not equal;

https://reviewboard.mozilla.org/r/100328/#review100930

The commit message is really easy to understand.  Thank you!

Comment 26

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

Comment on attachment 8820944 [details]
Bug 1316764 - Update animations on layers whenever the frame generation is not equal;

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/073d993c46f9
Update animations on layers whenever the frame generation is not equal; r=hiro

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/073d993c46f9

Comment 29

[Astley Chen (inactive)]

Confirmed the leftover artifact after animation is finished is fixed on nightly FF53 latest build.

Comment 30

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

Updating the tracking flags since this bug has been narrowed to cover just the regression from bug 1311196 which landed in Firefox 52.

Comment 31

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

Comment on attachment 8820944 [details]
Bug 1316764 - Update animations on layers whenever the frame generation is not equal;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1311196
[User impact if declined]: Occasional graphical artifacts left over after animations finish
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:
1. Load test case from attachment 8809735 [details]
2. Click the 'click' button being very careful not to move the mouse.
The long green box should return to its starting position (without moving the mouse). Repeat the test several times to be sure.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: It's possible that the fix generates additional restyles which would regress performance. However, there have been no reports or indications of this since it landed on trunk a few days ago.
[String changes made/needed]: None.

Comment 32

[Julien Cristau [:jcristau]]

Comment on attachment 8820944 [details]
Bug 1316764 - Update animations on layers whenever the frame generation is not equal;

fix regression from bug 1311196 in aurora52

Comment 33

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7e613c48e2a