Bug 1495350 - Adjust fill mode to use on the compositor based on the playback rate; r=hiro
☠☠ backed out by 8050702c7716 ☠ ☠
authorBrian Birtles <birtles@gmail.com>
Thu, 04 Oct 2018 05:10:38 +0000
changeset 487944 113f6263774f62d1dbf707e415e7f6f7040bbc60
parent 487943 7591833a8d93ddbe66bfd123d76d9efab493b5d4
child 487945 c50a3b343ac847e4e112bbb8b4c2892167813b80
push id246
push userfmarier@mozilla.com
push dateSat, 13 Oct 2018 00:15:40 +0000
reviewershiro
bugs1495350
milestone64.0a1
Bug 1495350 - Adjust fill mode to use on the compositor based on the playback rate; r=hiro 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. Differential Revision: https://phabricator.services.mozilla.com/D7259
dom/animation/test/mozilla/test_style_after_finished_on_compositor.html
gfx/layers/AnimationHelper.cpp
--- a/dom/animation/test/mozilla/test_style_after_finished_on_compositor.html
+++ b/dom/animation/test/mozilla/test_style_after_finished_on_compositor.html
@@ -1,12 +1,12 @@
 <!doctype html>
 <head>
 <meta charset=utf-8>
-<title>Test for styles after finished on the compositor</title>
+<title>Test for styles after an animation has finished on the compositor</title>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="../testcommon.js"></script>
 <style>
 .compositor {
   /* Element needs geometry to be eligible for layerization */
   width: 100px;
   height: 100px;
@@ -105,17 +105,16 @@ promise_test(async t => {
   await waitForNextFrame();
 
   const transform = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform');
 
   assert_equals(transform, '', 'No transform animation runs on the compositor');
 }, 'Transform animation initially transform: none is removed from compositor ' +
    'when finished');
 
-
 promise_test(async t => {
   const div = addDiv(t, { 'class': 'compositor' });
   const anim = div.animate([ { offset: 0,   transform: 'translateX(100px)' },
                              { offset: 0.5, transform: 'none' },
                              { offset: 0.9, transform: 'none' },
                              { offset: 1,   transform: 'translateX(100px)' } ],
                            { delay: 10, duration: 100 });
 
@@ -129,10 +128,192 @@ promise_test(async t => {
   await waitForNextFrame();
 
   const transform = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'transform');
 
   assert_equals(transform, '', 'No transform animation runs on the compositor');
 }, 'Transform animation is removed from compositor even when it only visits ' +
    'exactly the point where the transform: none value was set');
 
+/*
+ *
+ * Tests for synthesized fill modes used when an animation on the compositor
+ * has finished and is waiting to be removed by the main thread.
+ *
+ */
+
+promise_test(async t => {
+  // Below if painting takes too long we'll skip the test so in order to give
+  // the test a better chance of running, wait until the system settles down.
+  await waitForIdle();
+
+  const div = addDiv(t, { class: 'compositor' });
+  const anim = div.animate({ opacity: [1, 0] }, 100);
+
+  const timeBeforeStart = window.performance.now();
+  await waitForPaints();
+
+  // If it took over 50ms to paint the first frame of the animation, we can't
+  // reliably test it. This situation can happen if, for example, GC runs while
+  // waiting for the paint.
+  if (window.performance.now() - timeBeforeStart >= 50) {
+    console.log('Skipping test because it took too long to paint the ' +
+                'first frame');
+    return;
+  }
+
+  let opacity = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity');
+  assert_not_equals(
+    opacity,
+    '',
+    'The animation style is applied on the compositor'
+  );
+
+  // Generate artificial busyness on the main thread for 200ms.
+  const waitStart = window.performance.now();
+  while (window.performance.now() - waitStart < 200) {}
+
+  // By now the animation on the compositor should have finished but should
+  // stay at the end of the animation because the main thread hasn't had
+  // a chance to remove the animation from the compositor yet.
+  opacity = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity');
+  assert_equals(
+    opacity,
+    '0',
+    'The opacity style from the end of the animation is still applied on ' +
+    'the compositor'
+  );
+}, 'Opacity animation when playing forwards uses fill: forwards while ' +
+   'waiting for the main thread to catch up');
+
+promise_test(async t => {
+  await waitForIdle();
+
+  const div = addDiv(t, { class: 'compositor' });
+  const anim = div.animate({ opacity: [0, 1] }, 100);
+  anim.playbackRate = -1;
+  anim.play();
+
+  const timeBeforeStart = window.performance.now();
+  await waitForPaints();
+
+  // If it took over 50ms to paint the first frame of the animation, we can't
+  // reliably test it. This situation can happen if, for example, GC runs while
+  // waiting for the paint.
+  if (window.performance.now() - timeBeforeStart >= 50) {
+    console.log('Skipping test because it took too long to paint the ' +
+                'first frame');
+    return;
+  }
+
+  // Bug 1496313: For animations playing backwards, in some cases the animation
+  // will not have arrived at the compositor by the time we go to check it.
+  // For now we just skip that if it happens since it only happens occasionally.
+  if (SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity') === '') {
+    console.log('Skipping test because the  took too long to paint the ' +
+                'first frame');
+    return;
+  }
+
+  // Generate artificial busyness on the main thread for 200ms.
+  const waitStart = window.performance.now();
+  while (window.performance.now() - waitStart < 200) {}
+
+  // By now the animation on the compositor should have finished but should
+  // stay at the start of the animation because the main thread hasn't had
+  // a chance to remove the animation from the compositor yet.
+  const opacity = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity');
+  assert_equals(
+    opacity,
+    '0',
+    'The opacity style from the start of the animation is still ' +
+    'applied on the compositor'
+  );
+}, 'Opacity animation when playing backwards uses fill: backwards while ' +
+   'waiting for the main thread to catch up');
+
+/*
+ * The following tests are as above but for a different initial fill mode
+ * hence comments have been removed.
+ */
+
+promise_test(async t => {
+  await waitForIdle();
+
+  const div = addDiv(t, { class: 'compositor' });
+  const anim = div.animate(
+    { opacity: [1, 0] },
+    { duration: 100, fill: 'backwards' }
+  );
+
+  const timeBeforeStart = window.performance.now();
+  await waitForPaints();
+
+  if (window.performance.now() - timeBeforeStart >= 50) {
+    console.log('Skipping test because it took too long to paint the ' +
+                'first frame');
+    return;
+  }
+
+  let opacity = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity');
+  assert_not_equals(
+    opacity,
+    '',
+    'The animation style is applied on the compositor'
+  );
+
+  const waitStart = window.performance.now();
+  while (window.performance.now() - waitStart < 200) {}
+
+  opacity = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity');
+  assert_equals(
+    opacity,
+    '0',
+    'The opacity style from the end of the animation is still ' +
+      'applied on the compositor'
+  );
+}, 'Opacity animation playing forwards with fill: backwards, ' +
+   'uses fill: both while waiting for the main thread to catch up');
+
+promise_test(async t => {
+  await waitForIdle();
+
+  const div = addDiv(t, { class: 'compositor' });
+  const anim = div.animate(
+    { opacity: [0, 1] },
+    { duration: 100, fill: 'forwards' }
+  );
+  anim.playbackRate = -1;
+  anim.play();
+
+  const timeBeforeStart = window.performance.now();
+  await waitForPaints();
+
+  if (window.performance.now() - timeBeforeStart >= 50) {
+    console.log('Skipping test because it took too long to paint the ' +
+                'first frame');
+    return;
+  }
+
+  // Bug 1496313: For animations playing backwards, in some cases the animation
+  // will not have arrived at the compositor by the time we go to check it.
+  // For now we just skip that if it happens since it only happens occasionally.
+  if (SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity') === '') {
+    console.log('Skipping test because the  took too long to paint the ' +
+                'first frame');
+    return;
+  }
+
+  const waitStart = window.performance.now();
+  while (window.performance.now() - waitStart < 200) {}
+
+  const opacity = SpecialPowers.DOMWindowUtils.getOMTAStyle(div, 'opacity');
+  assert_equals(
+    opacity,
+    '0',
+    'The opacity style from the start of the animation is still ' +
+    'applied on the compositor'
+  );
+}, 'Opacity animation playing backwards with fill: forwards, ' +
+   'uses fill: both while waiting for the main thread to catch up');
+
 </script>
 </body>
--- a/gfx/layers/AnimationHelper.cpp
+++ b/gfx/layers/AnimationHelper.cpp
@@ -549,25 +549,36 @@ ToAnimationValue(const Animatable& aAnim
 void
 AnimationHelper::SetAnimations(
   AnimationArray& aAnimations,
   InfallibleTArray<AnimData>& aAnimData,
   RefPtr<RawServoAnimationValue>& aBaseAnimationStyle)
 {
   for (uint32_t i = 0; i < aAnimations.Length(); i++) {
     Animation& animation = aAnimations[i];
-    // Adjust fill mode to fill forwards so that if the main thread is delayed
-    // in clearing this animation we don't introduce flicker by jumping back to
-    // the old underlying value
+    // Adjust fill mode so that if the main thread is delayed in clearing
+    // this animation we don't introduce flicker by jumping back to the old
+    // underlying value.
     switch (static_cast<dom::FillMode>(animation.fillMode())) {
       case dom::FillMode::None:
-        animation.fillMode() = static_cast<uint8_t>(dom::FillMode::Forwards);
+        if (animation.playbackRate() > 0) {
+          animation.fillMode() = static_cast<uint8_t>(dom::FillMode::Forwards);
+        } else if (animation.playbackRate() < 0) {
+          animation.fillMode() = static_cast<uint8_t>(dom::FillMode::Backwards);
+        }
         break;
       case dom::FillMode::Backwards:
-        animation.fillMode() = static_cast<uint8_t>(dom::FillMode::Both);
+        if (animation.playbackRate() > 0) {
+          animation.fillMode() = static_cast<uint8_t>(dom::FillMode::Both);
+        }
+        break;
+      case dom::FillMode::Forwards:
+        if (animation.playbackRate() < 0) {
+          animation.fillMode() = static_cast<uint8_t>(dom::FillMode::Both);
+        }
         break;
       default:
         break;
     }
 
     if (animation.baseStyle().type() != Animatable::Tnull_t) {
       aBaseAnimationStyle = ToAnimationValue(animation.baseStyle());
     }