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)

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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+
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.
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.
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).
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.
> 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
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.
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
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
Hmm, maybe I should just drop the tests altogether.
Flags: needinfo?(bbirtles)
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
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
I've just split the tests off for now and landed the code changes as-is.
QA Contact: bbirtles
https://hg.mozilla.org/mozilla-central/rev/1bcc72890955
Status: ASSIGNED → RESOLVED
Closed: 6 years 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.

Attachment

General

Created:
Updated:
Size: