Closed
Bug 1495350
Opened 6 years ago
Closed 6 years ago
Compositor animations with a negative playback rate cause flicker when they finish
Categories
(Core :: DOM: Animation, defect, P1)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: birtles, Assigned: birtles)
References
(Depends on 1 open bug)
Details
Attachments
(1 file)
See discussion in bug 1486278.
Assignee | ||
Comment 1•6 years ago
|
||
When a compositor animation finishes that doesn't apply a fill, rather than jumping back to the underlying value immediately we should apply a fill mode until the main thread has a chance to remove the animation from the compositor. This ensures that any main thread effects that are intended to synchronize with the end of the animation have a chance to run before we show the underlying style and helps to avoid flicker in such cases. Currently we apply this synthesized fill mode to animations when they run forwards (i.e. positive playback rate), but not backwards. This patch makes us apply the same handling when running in reverse.
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=437a5c4f3afda1bd5bcccffc5fbed3ed4dcd3e5e
Comment 3•6 years ago
|
||
Comment on attachment 9013204 [details] Bug 1495350 - Adjust fill mode to use on the compositor based on the playback rate; r=hiro Hiroyuki Ikezoe (:hiro) has approved the revision.
Attachment #9013204 -
Flags: review+
Assignee | ||
Comment 4•6 years ago
|
||
I found one reason for failing tests is bug 1495647 which appears to be an easy fix. However, there still seems to be at least one other bug in there.
Assignee | ||
Comment 5•6 years ago
|
||
After looking at little further, the difference between updatePlaybackRate and setting the playbackRate in this case is that we will not have a resolved progress value in the former case (because the updated playback rate has not yet been resolved). I'm not actually sure if we should or not. I suspect this is behaving as expected but need to think about it further.
Assignee | ||
Comment 6•6 years ago
|
||
Oh, and I suspect that even if this behavior is right, we probably should still send the animation to the compositor straight away anyway, rather than waiting for the next tick (although it is an edge case so maybe it's not so important).
Assignee | ||
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aa205fa224253439538ddd45ef86580edde7403 (I expect there will still be failures here though.)
Assignee | ||
Comment 8•6 years ago
|
||
For the test failures: * Bug 1495647 will mean that we don't incorrectly consider an animation about to play backwards as finished but it won't actually help here. * That's because when we use updatePlaybackRate() the current progress will still be 'null' while we're waiting to start. That, I believe, is the correct behavior since we don't want the computed timing to reflect the playback rate before it is applied. * As a result we won't initially send the animation to the compositor because in nsIFrame::HasOpacityInternal we'll discover there is no effect set on the element and will return false. * There won't be an effect set because we don't consider the animation relevant yet so we don't register it with the effect set. * We don't consider it relevant, because it's not current, because it's in the after phase, because we haven't updated the playback rate yet. * We need to be careful about what we put in the effect set because it forms the basis of what we return from getAnimations() so changing that will have Web-observable effects (once we ship getAnimations()). * So I think the difference in behavior between updatePlaybackRate() and setting playbackRate directly here is acceptable. If we were to change it, it would be simply an optimization, and I don't think it's necessary yet anyway. * Even when setting playbackRate directly, however, there can be test failures where the compositor animation returns ''. I'm inclined to suspect this happens because the animation hasn't arrived at the compositor yet. I believe this is related to bug 1415065. In particular, these tests are calling: await waitForAnimationReadyToRestyle(anim); which is: async function waitForAnimationReadyToRestyle(aAnimation) { await aAnimation.ready; // If |aAnimation| begins at the current timeline time, we will not process // restyling in the initial frame because of aligning with the refresh driver, // the animation frame in which the ready promise is resolved happens to // coincide perfectly with the start time of the animation. In this case no // restyling is needed in the frame so we have to wait one more frame. if (animationStartsRightNow(aAnimation)) { await waitForNextFrame(); } } which calls: function animationStartsRightNow(aAnimation) { return aAnimation.startTime === aAnimation.timeline.currentTime && aAnimation.currentTime === 0; } which I believe, will not work for this test case since aAnimation.currentTime is 100. As a result, in some cases I suspect we end up fetching the value from the compositor before we've had a chance to do a restyle that would cause us to send the animation. I'm going to switch back to using waitForPaints and see if it helps.
Assignee | ||
Comment 9•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c3fe725c7de9f0e12df14d925cf3d60dcaeaa0b
Assignee | ||
Comment 10•6 years ago
|
||
> I'm going to switch back to using waitForPaints and see if it helps. It still fails in the same way: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c3fe725c7de9f0e12df14d925cf3d60dcaeaa0b&selectedJob=203025029
Assignee | ||
Comment 11•6 years ago
|
||
At this point I'm inclined to make these tests just bail if the animation hasn't reached the compositor yet. It only happens occasionally and I can't reproduce it locally. I'm not sure if it's worth a couple of days of debugging on try.
Assignee | ||
Comment 12•6 years ago
|
||
I've made these tests bail if the animation is not on the compositor yet and filed bug 1496313 to investigate why. https://treeherder.mozilla.org/#/jobs?repo=try&revision=346b8be8c276f0666052c0693c6445b54994515f
Comment 13•6 years ago
|
||
Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/113f6263774f Adjust fill mode to use on the compositor based on the playback rate; r=hiro
Comment 14•6 years ago
|
||
Backed out changeset 113f6263774f (Bug 1495350) for TV failures that turned to tier1 failures in dom/animation/test/mozilla/test_style_after_finished_on_compositor.html https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&classifiedState=unclassified&selectedJob=203319214 https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&classifiedState=unclassified&selectedJob=203327726 https://treeherder.mozilla.org/logviewer.html#?job_id=203327726&repo=autoland&lineNumber=3621
Flags: needinfo?(bbirtles)
Assignee | ||
Comment 15•6 years ago
|
||
Hmm, maybe I should just drop the tests altogether.
Flags: needinfo?(bbirtles)
Comment 16•6 years ago
|
||
Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8050702c7716 Backed out changeset 113f6263774f for TV failures that turned to tier1 failures in dom/animation/test/mozilla/test_style_after_finished_on_compositor.html
Comment 17•6 years ago
|
||
Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1bcc72890955 Adjust fill mode to use on the compositor based on the playback rate; r=hiro
Assignee | ||
Comment 18•6 years ago
|
||
I've just split the tests off for now and landed the code changes as-is.
QA Contact: bbirtles
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1bcc72890955
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•6 years ago
|
QA Contact: bbirtles
You need to log in
before you can comment on or make changes to this bug.
Description
•