Bug 1443320 - Wait for an additional frame if the vsync tick happened within 1ms and check the observed result with tolerance. r=birtles
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Wed, 13 Feb 2019 04:46:31 +0000
changeset 458858 fc540ce0e4298b98f8961ecbc4b58733e35df0e0
parent 458857 1af69c96ec5f4eafb409b844b72dc1740b9ab1a0
child 458859 dcabcaa944f5130b4ddb5bccc6076ec50d46f2ca
push id78061
push userhikezoe@mozilla.com
push dateWed, 13 Feb 2019 05:38:51 +0000
treeherderautoland@dcabcaa944f5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbirtles
bugs1443320
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1443320 - Wait for an additional frame if the vsync tick happened within 1ms and check the observed result with tolerance. r=birtles SMILTime is clamped to milliseconds in SMILAnimationController::GetParentTime[1]. Unfortunately our software based vsync timer is no accurate, it ticks sometimes within 1ms. In such frames, it's possible that the SMILTime is not advanced due to the clamping. It's hard to precisely predict when the clamping happens, so we wait for an additinal frame in the case where the frame is ticked within 1ms and allow redundant restyling count for the case. [1] https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/dom/smil/SMILAnimationController.cpp#103 Depends on D19568 Differential Revision: https://phabricator.services.mozilla.com/D19570
layout/style/test/test_restyles_in_smil_animation.html
--- a/layout/style/test/test_restyles_in_smil_animation.html
+++ b/layout/style/test/test_restyles_in_smil_animation.html
@@ -13,44 +13,76 @@
   <svg>
     <rect id="svg-rect" width="100%" height="100%" fill="lime"/>
   </svg>
 </div>
 
 <script>
 "use strict";
 
+// Waits for |frameCount| requestAnimationFrame callbacks.
+// Returns the number of frame actually waited.
 function waitForAnimationFrames(frameCount) {
   return new Promise(function(resolve, reject) {
+    let previousTime = document.timeline.currentTime;
+    let framesWaited = 0;
     function handleFrame() {
-      if (--frameCount <= 0) {
-        resolve();
-      } else {
-        window.requestAnimationFrame(handleFrame); // wait another frame
+      // SMIL uses a time resolution of 1ms but our software-based vsync timer
+      // sometimes produces ticks with an interval of less than 1ms. In such
+      // cases we will skip restyling for SMIL animations since the SMIL time
+      // will not change.
+      //
+      // In the following we attempt to detect such situations and wait an
+      // additional frame when we detect it. However, the detection is not
+      // completely accurate since it uses the timeline time which is based on
+      // the navigation start time whereas the SMIL start time is based on the
+      // refresh driver time.
+      //
+      // To account for this inaccuracy the Promise returned by this method
+      // resolves with the number of frames waited with the additional frames.
+      // This can be used by the call site to add a suitable tolerance to the
+      // number of restylings it expects to happen. For example, if a call site
+      // is anticipating each animation frame to cause restyling, then the
+      // number of restylings, x, it should expect is frameCount <= x <=
+      // framesWaited.
+      const difference = document.timeline.currentTime - previousTime;
+      framesWaited++;
+      if (difference >= 1.0 && --frameCount <= 0) {
+        resolve(framesWaited);
+        return;
       }
+
+      previousTime = document.timeline.currentTime;
+      window.requestAnimationFrame(handleFrame); // wait another frame
     }
     window.requestAnimationFrame(handleFrame);
   });
 }
 
+// Returns an object consisting of observed styling count and the number of
+// frames actually waited because we detected a possibly overflapping SMIL
+// time.
 function observeStyling(frameCount) {
   var Ci = SpecialPowers.Ci;
   var docShell = SpecialPowers.wrap(window).docShell;
 
   docShell.recordProfileTimelineMarkers = true;
   docShell.popProfileTimelineMarkers();
 
   return new Promise(function(resolve) {
-    return waitForAnimationFrames(frameCount).then(function() {
+    return waitForAnimationFrames(frameCount).then(framesWaited => {
       var markers = docShell.popProfileTimelineMarkers();
       docShell.recordProfileTimelineMarkers = false;
       var stylingMarkers = markers.filter(function(marker, index) {
         return marker.name == 'Styles' && marker.isAnimationOnly;
       });
-      resolve(stylingMarkers);
+      resolve({
+        stylingCount: stylingMarkers.length,
+        framesWaited: framesWaited,
+      });
     });
   });
 }
 
 function ensureElementRemoval(aElement) {
   return new Promise(function(resolve) {
     aElement.remove();
     waitForAllPaintsFlushed(resolve);
@@ -74,39 +106,41 @@ add_task(async function smil_is_in_displ
   animate.setAttribute("attributeName", "fill");
   animate.setAttribute("values", "red;lime");
   animate.setAttribute("dur", "1s");
   animate.setAttribute("repeatCount", "indefinite");
   document.getElementById("svg-rect").appendChild(animate);
 
   await waitForAnimationFrames(2);
 
-  var displayMarkers = await observeStyling(5);
+  let result = await observeStyling(5);
   // FIXME: Bug 866411: SMIL animations sometimes skip restyles when the target
   // element is newly associated with an nsIFrame.
-  ok(displayMarkers.length == 5 || displayMarkers.length == 4,
-     `should restyle in most frames (got ${displayMarkers.length} restyles`
-     + ' over five frames, expected 4~5)');
+  ok(result.stylingCount >= 4 &&
+     result.stylingCount <= result.framesWaited,
+     `should restyle in most frames (got ${result.stylingCount} restyles ` +
+     `over ${result.framesWaited} frames, expected 4~${result.framesWaited})`);
 
   var div = document.getElementById("target-div");
 
   div.style.display = "none";
   getComputedStyle(div).display;
   await waitForPaintFlushed();
 
-  var displayNoneMarkers = await observeStyling(5);
-  is(displayNoneMarkers.length, 0, "should never restyle if display:none");
+  result = await observeStyling(5);
+  is(result.stylingCount, 0, "should never restyle if display:none");
 
   div.style.display = "";
   getComputedStyle(div).display;
   await waitForAnimationFrames(2);
 
-  var displayAgainMarkers = await observeStyling(5);
+  result = await observeStyling(5);
   // FIXME: Bug 866411: SMIL animations sometimes skip restyles when the target
   // element is newly associated with an nsIFrame.
-  ok(displayMarkers.length == 5 || displayMarkers.length == 4,
-     `should restyle again (got ${displayMarkers.length} restyles over`
-     + ' five frames, expected 4~5)');
+  ok(result.stylingCount >= 4 &&
+     result.stylingCount <= result.framesWaited,
+     `should restyle again (got ${result.stylingCount} restyles over ` +
+     `${result.framesWaited} frames, expected 4~${result.framesWaited})`);
 
   await ensureElementRemoval(animate);
 });
 </script>
 </body>