Bug 1300653 - VideoPuppeteer.playback_done() now queries time data once to prevent racing. r?maja_zf draft
authorBryce Van Dyk <bvandyk@mozilla.com>
Tue, 06 Sep 2016 14:09:06 +1200
changeset 410024 52e625df1a92987e85603aee5b6c9705670550b9
parent 409759 2fdc46a694c29b9c395627311b410b95bc84c4a6
child 530481 cb49b28c1e322c1260ee3d52a8cba1173f2d1e40
push id28626
push userbvandyk@mozilla.com
push dateTue, 06 Sep 2016 02:16:47 +0000
reviewersmaja_zf
bugs1300653
milestone51.0a1
Bug 1300653 - VideoPuppeteer.playback_done() now queries time data once to prevent racing. r?maja_zf This attempts to address a race that can take place in playback_done(). Instead of querying the timing information piece wise, it is now requested all at once to prevent lags in between calls. MozReview-Commit-ID: 2uGGLVvIhuS
dom/media/test/external/external_media_tests/media_utils/video_puppeteer.py
--- a/dom/media/test/external/external_media_tests/media_utils/video_puppeteer.py
+++ b/dom/media/test/external/external_media_tests/media_utils/video_puppeteer.py
@@ -1,12 +1,12 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
-
+from collections import namedtuple
 from time import clock, sleep
 
 from marionette import Marionette
 from marionette_driver import By, expected, Wait
 
 from external_media_tests.utils import verbose_until
 
 
@@ -252,16 +252,61 @@ class VideoPuppeteer(object):
             the video.
         """
         # Note that self.current_time could temporarily refer to a
         # spliced-in ad
         elapsed_current_time = self.current_time - self._first_seen_time
         elapsed_wall_time = clock() - self._first_seen_wall_time
         return elapsed_wall_time - elapsed_current_time
 
+    def timing_info(self):
+        """
+        Fetches timing information in a single JS execution. This is done to
+        remove timing discrepancies between different JS requests and counter
+        races that are introduced by doing so.
+
+        :return: named tuple containing the current time, duration, lag,
+        played ranges, and remaining time keyed with 'current_time',
+        'duration', 'lag', 'played', and 'remaining_time', respectively
+        """
+        current_time, duration, raw_time_ranges, = self.execute_video_script(
+            'var currentTime = arguments[0].wrappedJSObject.currentTime;'
+            'var duration = arguments[0].wrappedJSObject.duration;'
+            'var played = arguments[0].wrappedJSObject.played;'
+            'var timeRanges = [];'
+            'for (var i = 0; i < played.length; i++) {'
+            'timeRanges.push([played.start(i), played.end(i)]);'
+            '}'
+            'return [currentTime, duration, [played.length, timeRanges]];'
+        )
+        # Calculate lag
+        elapsed_current_time = current_time - self._first_seen_time
+        elapsed_wall_time = clock() - self._first_seen_wall_time
+        lag = elapsed_wall_time - elapsed_current_time
+        # Create played ranges
+        played_ranges = TimeRanges(raw_time_ranges[0], raw_time_ranges[1])
+        # We only expect one range in played ranges as tests don't seek
+        assert played_ranges.length == 1
+        # Calculate remaining time
+        played_duration = self.played.end(0) - self.played.start(0)
+        remaining_time = self.expected_duration - played_duration
+        # Create named tuple for Timing Info
+        timing_info = namedtuple('timing_info',
+                                 ['current_time',
+                                  'duration',
+                                  'lag',
+                                  'played',
+                                  'remaining_time'])
+        # Construct and return named tuple
+        return timing_info(current_time,
+                           duration,
+                           lag,
+                           played_ranges,
+                           remaining_time)
+
     def measure_progress(self):
         initial = self.current_time
         sleep(1)
         return self.current_time - initial
 
     def execute_video_script(self, script):
         """
         Execute JS script in content context with access  to video element.
@@ -306,16 +351,20 @@ class VideoException(Exception):
 
 
 class TimeRanges:
     """
     Class to represent the TimeRanges data returned by played(). Exposes a
     similar interface to the JavaScript TimeRanges object.
     """
     def __init__(self, length, ranges):
+        # These should be the same,. Theoretically we don't need the length,
+        # but since this should be used to consume data coming back from
+        # JS exec, this is a valid sanity check.
+        assert length == len(ranges)
         self.length = length
         self.ranges = [(pair[0], pair[1]) for pair in ranges]
 
     def __repr__(self):
         return (
             'TimeRanges: length: {}, ranges: {}'
             .format(self.length, self.ranges)
         )
@@ -353,19 +402,24 @@ def playback_done(video):
     we are essentially done. If this happens to be last time we are polled
     before the video ends, we won't get another chance.
 
     :param video: The VideoPuppeteer instance that we are interested in.
 
     :return: True if we are close enough to the end of playback; False
         otherwise.
     """
-    if video.remaining_time < video.interval:
+    # Use timing_info to get info so there's less lag between first and second
+    # checks, minimise races.
+    timing_info = video.timing_info()
+    lag, remaining_time = timing_info.lag, timing_info.remaining_time
+
+    if remaining_time < video.interval:
         return True
 
     # Check to see if the video has stalled. Accumulate the amount of lag
     # since the video started, and if it is too high, then raise.
-    if video.stall_wait_time and (video.lag > video.stall_wait_time):
+    if video.stall_wait_time and (lag > video.stall_wait_time):
         raise VideoException('Video {} stalled.\n{}'
                              .format(video.video_url, video))
 
     # We are cruising, so we are not done.
     return False