Compositor animations with a negative playback rate cause flicker when they finish

RESOLVED FIXED in Firefox 64

Status

()

defect
P1
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

(Depends on 1 bug)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
See discussion in bug 1486278.
(Assignee)

Comment 1

8 months 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.
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)

Updated

8 months ago
Depends on: 1495647
(Assignee)

Comment 4

8 months 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

8 months 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

8 months 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 8

8 months 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 10

8 months 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

8 months 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)

Updated

8 months ago
Depends on: 1496313
(Assignee)

Comment 12

8 months 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

8 months 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
(Assignee)

Comment 15

8 months ago
Hmm, maybe I should just drop the tests altogether.
Flags: needinfo?(bbirtles)

Comment 16

8 months 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

8 months 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

8 months ago
I've just split the tests off for now and landed the code changes as-is.
QA Contact: bbirtles

Comment 19

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1bcc72890955
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
QA Contact: bbirtles
You need to log in before you can comment on or make changes to this bug.