Closed Bug 1316764 Opened 4 years ago Closed 4 years ago

Graphic glitch persists after animation before repaint

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: xidorn, Assigned: birtles)

References

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(6 files)

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.
Attached file 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.
I cannot reproduce this issue on macOS but I can on Windows, so setting the platform to Windows.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Priority: -- → P3
Whiteboard: [gfx-noted]
Attached file testcase4
Blocks: 1311196
Flags: needinfo?(hiikezoe)
Keywords: regression
Blocks: 1107297
Flags: needinfo?(bas)
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.
Flags: needinfo?(hiikezoe)
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.
Flags: needinfo?(bas) → needinfo?(bbirtles)
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.
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.
Flags: needinfo?(bbirtles) → needinfo?(hiikezoe)
(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.
Flags: needinfo?(hiikezoe)
(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
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
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.
(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.
(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
Hey RyanVM, why do you think this does not affect 50 and 51? Looks like the original bug 1316236 affects 51 ~53
Flags: needinfo?(ryanvm)
Sorry, I got fixated on the regression from bug 1311196 part.
Brian, do we have a conclusion as to how to proceed?
Flags: needinfo?(bbirtles)
Yes, I think the proposal in comment 11 is the correct approach here. Moving to DOM: Animation so we can track it there.
Component: Graphics → DOM: Animation
Flags: needinfo?(bbirtles)
Brian, is there a simple patch coming that we could uplift?
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
Flags: needinfo?(bbirtles)
OS: Windows 10 → All
Hardware: x86_64 → All
See Also: → 1325235
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
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.
Attached file 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.
Attachment #8820944 - Flags: review?(hiikezoe)
(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 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!
Attachment #8820944 - Flags: review?(hiikezoe) → review+
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/073d993c46f9
Update animations on layers whenever the frame generation is not equal; r=hiro
https://hg.mozilla.org/mozilla-central/rev/073d993c46f9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Confirmed the leftover artifact after animation is finished is fixed on nightly FF53 latest build.
Blocks: 1316236
Updating the tracking flags since this bug has been narrowed to cover just the regression from bug 1311196 which landed in Firefox 52.
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.
Attachment #8820944 - Flags: approval-mozilla-aurora?
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
Attachment #8820944 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.