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 458841 fc540ce0e429
parent 458840 1af69c96ec5f
child 458842 dcabcaa944f5
push id35548
push useropoprus@mozilla.com
push dateWed, 13 Feb 2019 09:48:26 +0000
treeherdermozilla-central@93e37c529818 [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>