Bug 1300653 - Update VideoPuppeteer to store state snapshots to prevent racing. r=maja_zf draft
authorBryce Van Dyk <bvandyk@mozilla.com>
Wed, 21 Sep 2016 14:08:34 +1200
changeset 415814 a25a9a45c8dced9439360b9664b1d768100ed2be
parent 414763 f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc
child 415815 99aac08fd86d41e7fa3df9b00604dd583ca27bf8
push id29978
push userbvandyk@mozilla.com
push dateWed, 21 Sep 2016 04:11:30 +0000
reviewersmaja_zf
bugs1300653
milestone51.0a1
Bug 1300653 - Update VideoPuppeteer to store state snapshots to prevent racing. r=maja_zf The tests that use VideoPuppeteer often expect the state queried by the puppeteer to be consistent if done closely in the code. However, this has not been the case, as there can be significant lags in the data returning from marionette. This means that if one line queries the current time of the underlying video, and the very next line queries the same thing, there can be significantly different results. This causes issues with tests making multiple sequential checks on the underlying video, and the state changing between checks. On test fails it means that the information logged my be inconsistent with the state that resulted in the test failing, as time passes between the fail check and the logging. This patch attempts to address this by having the VideoPuppeteer store a snapshot of state and examining that instead. This snap shot should be internally consistent. I've removed a large number of public members from the class, and moved a couple of the testing functions into the class. The thinking here is that the new logic complicates the internal state of the class, and I want to keep the interface slim to hide that complexity. *** Review feedback: Log interval, expected duration, stall wait time, and timeout times in VideoPuppeteer string. *** Review feedback: make video var script a class var instead of a staticmethod. *** Review feedback: move _fetch_state_script to be a property on VideoPuppeteer. *** Review feedback: simplify calling of _create_video_state_info with a dict. Fix played docstring. *** Review feedback: simplify _create_video_state_info using kwargs. MozReview-Commit-ID: 6mN56bFMge0
dom/media/test/external/external_media_harness/testcase.py
dom/media/test/external/external_media_tests/media_utils/video_puppeteer.py
dom/media/test/external/external_media_tests/media_utils/youtube_puppeteer.py
dom/media/test/external/external_media_tests/playback/youtube/test_basic_playback.py
--- a/dom/media/test/external/external_media_harness/testcase.py
+++ b/dom/media/test/external/external_media_harness/testcase.py
@@ -9,20 +9,18 @@ import time
 from marionette import BrowserMobProxyTestCaseMixin, MarionetteTestCase, Marionette
 from marionette_driver import Wait
 from marionette_driver.errors import TimeoutException
 from marionette.marionette_test import SkipTest
 
 from firefox_puppeteer.testcases import BaseFirefoxTestCase
 from external_media_tests.utils import (timestamp_now, verbose_until)
 from external_media_tests.media_utils.video_puppeteer import (
-    playback_done,
-    playback_started,
     VideoException,
-    VideoPuppeteer as VP
+    VideoPuppeteer
 )
 
 
 class MediaTestCase(BaseFirefoxTestCase, MarionetteTestCase):
 
     """
     Necessary methods for MSE playback
 
@@ -49,54 +47,54 @@ class MediaTestCase(BaseFirefoxTestCase,
             os.makedirs(screenshot_dir)
         with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
             img_data = self.marionette.screenshot()
         with open(path, 'wb') as f:
             f.write(img_data.decode('base64'))
         self.marionette.log('Screenshot saved in {}'
                             .format(os.path.abspath(path)))
 
-    def log_video_debug_lines(self):
+    def log_video_debug_lines(self, video):
         """
         Log the debugging information that Firefox provides for video elements.
         """
         with self.marionette.using_context(Marionette.CONTEXT_CHROME):
-            debug_lines = self.marionette.execute_script(VP._debug_script)
+            debug_lines = video.get_debug_lines()
             if debug_lines:
                 self.marionette.log('\n'.join(debug_lines))
 
     def run_playback(self, video):
         """
         Play the video all of the way through, or for the requested duration,
         whichever comes first. Raises if the video stalls for too long.
 
         :param video: VideoPuppeteer instance to play.
         """
         with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
             self.logger.info(video.test_url)
             try:
                 verbose_until(Wait(video, interval=video.interval,
                                    timeout=video.expected_duration * 1.3 +
                                    video.stall_wait_time),
-                              video, playback_done)
+                              video, VideoPuppeteer.playback_done)
             except VideoException as e:
                 raise self.failureException(e)
 
     def check_playback_starts(self, video):
         """
         Check to see if a given video will start. Raises if the video does not
         start.
 
         :param video: VideoPuppeteer instance to play.
         """
         with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
             self.logger.info(video.test_url)
             try:
                 verbose_until(Wait(video, timeout=video.timeout),
-                              video, playback_started)
+                              video, VideoPuppeteer.playback_started)
             except TimeoutException as e:
                 raise self.failureException(e)
 
     def skipTest(self, reason):
         """
         Skip this test.
 
         Skip with marionette.marionette_test import SkipTest so that it
@@ -130,17 +128,17 @@ class NetworkBandwidthTestCase(MediaTest
 
     def run_videos(self, timeout=60):
         """
         Run each of the videos in the video list. Raises if something goes
         wrong in playback.
         """
         with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
             for url in self.video_urls:
-                video = VP(self.marionette, url, stall_wait_time=60,
+                video = VideoPuppeteer(self.marionette, url, stall_wait_time=60,
                            set_duration=60, timeout=timeout)
                 self.run_playback(video)
 
 
 class VideoPlaybackTestsMixin(object):
 
     """
     Test MSE playback in HTML5 video element.
@@ -155,31 +153,31 @@ class VideoPlaybackTestsMixin(object):
     def test_playback_starts(self):
         """
         Test to make sure that playback of the video element starts for each
         video.
         """
         with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
             for url in self.video_urls:
                 try:
-                    video = VP(self.marionette, url, timeout=60)
+                    video = VideoPuppeteer(self.marionette, url, timeout=60)
                     # Second playback_started check in case video._start_time
                     # is not 0
                     self.check_playback_starts(video)
                     video.pause()
                 except TimeoutException as e:
                     raise self.failureException(e)
 
     def test_video_playback_partial(self):
         """
         Test to make sure that playback of 60 seconds works for each video.
         """
         with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
             for url in self.video_urls:
-                video = VP(self.marionette, url,
+                video = VideoPuppeteer(self.marionette, url,
                            stall_wait_time=10,
                            set_duration=60)
                 self.run_playback(video)
 
 
 class NetworkBandwidthTestsMixin(object):
 
     """
--- 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
 
 
@@ -33,17 +33,17 @@ for (var i=0; i < tabbrowser.browsers.le
 }"""
 
 
 class VideoPuppeteer(object):
     """
     Wrapper to control and introspect HTML5 video elements.
 
     A note about properties like current_time and duration:
-    These describe whatever stream is playing when the property is called.
+    These describe whatever stream is playing when the state is checked.
     It is possible that many different streams are dynamically spliced
     together, so the video stream that is currently playing might be the main
     video or it might be something else, like an ad, for example.
 
     :param marionette: The marionette instance this runs in.
     :param url: the URL of the page containing the video element.
     :param video_selector: the selector of the element that we want to watch.
      This is set by default to 'video', which is what most sites use, but
@@ -51,29 +51,63 @@ class VideoPuppeteer(object):
     :param interval: The polling interval that is used to check progress.
     :param set_duration: When set to >0, the polling and checking will stop at
      the number of seconds specified. Otherwise, this will stop at the end
      of the video.
     :param stall_wait_time: The amount of time to wait to see if a stall has
      cleared. If 0, do not check for stalls.
     :param timeout: The amount of time to wait until the video starts.
     """
+
+    _video_var_script = (
+        'var video = arguments[0];'
+        'var currentTime = video.wrappedJSObject.currentTime;'
+        'var duration = video.wrappedJSObject.duration;'
+        'var played = video.wrappedJSObject.played;'
+        'var timeRanges = [];'
+        'for (var i = 0; i < played.length; i++) {'
+        'timeRanges.push([played.start(i), played.end(i)]);'
+        '}'
+        'var totalFrames = '
+        'video.getVideoPlaybackQuality()["totalVideoFrames"];'
+        'var droppedFrames = '
+        'video.getVideoPlaybackQuality()["droppedVideoFrames"];'
+        'var corruptedFrames = '
+        'video.getVideoPlaybackQuality()["corruptedVideoFrames"];'
+    )
+    """
+    A string containing JS that assigns video state to variables.
+    The purpose of this string script is to be appended to by this and
+    any inheriting classes to return these and other variables. In the case
+    of an inheriting class the script can be added to in order to fetch
+    further relevant variables -- keep in mind we only want one script
+    execution to prevent races, so it wouldn't do to have child classes
+    run this script then their own, as there is potential for lag in
+    between.
+
+    This script assigns a subset of the vars used later by the
+    `_video_state_named_tuple` function. Please see that function's
+    documentation for further information on these variables.
+    """
+
     def __init__(self, marionette, url, video_selector='video', interval=1,
                  set_duration=0, stall_wait_time=0, timeout=60,
                  autostart=True):
         self.marionette = marionette
         self.test_url = url
         self.interval = interval
         self.stall_wait_time = stall_wait_time
         self.timeout = timeout
         self._set_duration = set_duration
         self.video = None
         self.expected_duration = 0
         self._first_seen_time = 0
         self._first_seen_wall_time = 0
+        self._fetch_state_script_string = None
+        self._last_seen_video_state = None
         wait = Wait(self.marionette, timeout=self.timeout)
         with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
             self.marionette.navigate(self.test_url)
             self.marionette.execute_script("""
                 log('URL: {0}');""".format(self.test_url))
             verbose_until(wait, self,
                           expected.element_present(By.TAG_NAME, 'video'))
             videos_found = self.marionette.find_elements(By.CSS_SELECTOR,
@@ -88,284 +122,305 @@ class VideoPuppeteer(object):
                 return
             self.video = videos_found[0]
             self.marionette.execute_script("log('video element obtained');")
             if autostart:
                 self.start()
 
     def start(self):
         # To get an accurate expected_duration, playback must have started
+        self._refresh_state()
         wait = Wait(self, timeout=self.timeout)
-        verbose_until(wait, self, playback_started,
+        verbose_until(wait, self, VideoPuppeteer.playback_started,
                       "Check if video has played some range")
-        self._first_seen_time = self.current_time
+        self._first_seen_time = self._last_seen_video_state.current_time
         self._first_seen_wall_time = clock()
-        self.update_expected_duration()
-
-    def update_expected_duration(self):
-        """
-        Update the duration of the target video at self.test_url (in seconds).
-
-        expected_duration represents the following: how long do we expect
-        playback to last before we consider the video to be 'complete'?
-        If we only want to play the first n seconds of the video,
-        expected_duration is set to n.
-        """
-        # self.duration is the duration of whatever is playing right now.
-        # In this case, we assume the video element always shows the same
-        # stream throughout playback (i.e. the are no ads spliced into the main
-        # video, for example), so self.duration is the duration of the main
-        # video.
-        video_duration = self.duration
-        # Do our best to figure out where the video started playing
-        played_ranges = self.played
-        if played_ranges.length > 0:
-            # If we have a range we should only have on continuous range
-            assert played_ranges.length == 1
-            start_position = played_ranges.start(0)
-        else:
-            # If we don't have a range we should have a current time
-            start_position = self._first_seen_time
-        # In case video starts at t > 0, adjust target time partial playback
-        remaining_video = video_duration - start_position
-        if 0 < self._set_duration < remaining_video:
-            self.expected_duration = self._set_duration
-        else:
-            self.expected_duration = remaining_video
+        self._update_expected_duration()
 
     def get_debug_lines(self):
         """
         Get Firefox internal debugging for the video element.
 
         :return: A text string that has Firefox-internal debugging information.
         """
         with self.marionette.using_context('chrome'):
             debug_lines = self.marionette.execute_script(debug_script)
         return debug_lines
 
     def play(self):
         """
         Tell the video element to Play.
         """
-        self.execute_video_script('arguments[0].wrappedJSObject.play();')
+        self._execute_video_script('arguments[0].wrappedJSObject.play();')
 
     def pause(self):
         """
         Tell the video element to Pause.
         """
-        self.execute_video_script('arguments[0].wrappedJSObject.pause();')
+        self._execute_video_script('arguments[0].wrappedJSObject.pause();')
 
-    @property
-    def duration(self):
+    def playback_started(self):
+        """
+        Determine if video has started
+
+        :param self: The VideoPuppeteer instance that we are interested in.
+
+        :return: True if is playing; False otherwise
         """
-        :return: Duration in seconds of whatever stream is playing right now.
+        self._refresh_state()
+        try:
+            played_ranges = self._last_seen_video_state.played
+            return (
+                played_ranges.length > 0 and
+                played_ranges.start(0) < played_ranges.end(0) and
+                played_ranges.end(0) > 0.0
+            )
+        except Exception as e:
+            print ('Got exception {}'.format(e))
+            return False
+
+    def playback_done(self):
         """
-        return self.execute_video_script('return arguments[0].'
-                                         'wrappedJSObject.duration;') or 0
+        If we are near the end and there is still a video element, then
+        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 self: The VideoPuppeteer instance that we are interested in.
 
-    @property
-    def current_time(self):
-        """
-        :return: Current time of whatever stream is playing right now.
+        :return: True if we are close enough to the end of playback; False
+            otherwise.
         """
-        # Note that self.current_time could temporarily refer to a
-        # spliced-in ad.
+        self._refresh_state()
+
+        if self._last_seen_video_state.remaining_time < self.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 (self.stall_wait_time and
+                self._last_seen_video_state.lag > self.stall_wait_time):
+            raise VideoException('Video {} stalled.\n{}'
+                                 .format(self._last_seen_video_state.video_url,
+                                         self))
 
-        return self.execute_video_script(
-            'return arguments[0].wrappedJSObject.currentTime;') or 0
+        # We are cruising, so we are not done.
+        return False
+
+    def _update_expected_duration(self):
+        """
+        Update the duration of the target video at self.test_url (in seconds).
+        This is based on the last seen state, so the state should be,
+        refreshed at least once before this is called.
+
+        expected_duration represents the following: how long do we expect
+        playback to last before we consider the video to be 'complete'?
+        If we only want to play the first n seconds of the video,
+        expected_duration is set to n.
+        """
 
-    @property
-    def remaining_time(self):
-        """
-        :return: How much time is remaining given the duration of the video
-            and the duration that has been set.
+        # self._last_seen_video_state.duration is the duration of whatever was
+        # playing when the state was checked. In this case, we assume the video
+        # element always shows the same stream throughout playback (i.e. the
+        # are no ads spliced into the main video, for example), so
+        # self._last_seen_video_state.duration is the duration of the main
+        # video.
+        video_duration = self._last_seen_video_state.duration
+        # Do our best to figure out where the video started playing
+        played_ranges = self._last_seen_video_state.played
+        if played_ranges.length > 0:
+            # If we have a range we should only have on continuous range
+            assert played_ranges.length == 1
+            start_position = played_ranges.start(0)
+        else:
+            # If we don't have a range we should have a current time
+            start_position = self._first_seen_time
+        # In case video starts at t > 0, adjust target time partial playback
+        remaining_video = video_duration - start_position
+        if 0 < self._set_duration < remaining_video:
+            self.expected_duration = self._set_duration
+        else:
+            self.expected_duration = remaining_video
+
+    @staticmethod
+    def _video_state_named_tuple():
         """
-        played_ranges = self.played
-        # Playback should be in one range (as tests do not currently seek).
-        assert played_ranges.length == 1
-        played_duration = self.played.end(0) - self.played.start(0)
-        return self.expected_duration - played_duration
+        Create a named tuple class that can be used to store state snapshots
+        of the wrapped element. The fields in the tuple should be used as
+        follows:
+
+        current_time: The current time of the wrapped element.
+        duration: the duration of the wrapped element.
+        played: the played ranges of the wrapped element. In its raw form this
+        is as a list where the first element is the length and the second
+        element is a list of 2 item lists, where each two items are a played
+        range. Once assigned to the tuple this data should be wrapped in the
+        TimeRanges class.
+        lag: the difference in real world time and wrapped element time.
+        Calculated as real world time passed - element time passed.
+        totalFrames: number of total frames for the wrapped element
+        droppedFrames: number of dropped frames for the wrapped element.
+        corruptedFrames: number of corrupted frames for the wrapped.
+        video_src: the src attribute of the wrapped element.
+        video_url: the url attribute of the wrapped element.
 
-    @property
-    def played(self):
+        :return: A 'video_state_info' named tuple class.
         """
-        :return: A TimeRanges objected containing the played time ranges.
+        return namedtuple('video_state_info',
+                          ['current_time',
+                           'duration',
+                           'remaining_time',
+                           'played',
+                           'lag',
+                           'total_frames',
+                           'dropped_frames',
+                           'corrupted_frames',
+                           'video_src',
+                           'video_url'])
+
+    def _create_video_state_info(self, **video_state_info_kwargs):
+        """
+        Create an instance of the video_state_info named tuple. This function
+        expects a dictionary populated with the following keys: current_time,
+        duration, raw_time_ranges, total_frames, dropped_frames, and
+        corrupted_frames.
+
+        Aside from raw_time_ranges, see `_video_state_named_tuple` for more
+        information on the above keys and values. For raw_time_ranges a
+        list is expected that can be consumed to make a TimeRanges object.
+
+        :return: A named tuple 'video_state_info' derived from arguments and
+        state information from the puppeteer.
         """
-        raw_time_ranges = self.execute_video_script(
-            '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 [played.length, timeRanges];')
-        return TimeRanges(raw_time_ranges[0], raw_time_ranges[1])
+        raw_time_ranges = video_state_info_kwargs['raw_time_ranges']
+        # Remove raw ranges from dict as it is not used in the final named
+        # tuple and will provide an unexpected kwarg if kept.
+        del video_state_info_kwargs['raw_time_ranges']
+        # Create played ranges
+        video_state_info_kwargs['played'] = (
+            TimeRanges(raw_time_ranges[0], raw_time_ranges[1]))
+        # Calculate elapsed times
+        elapsed_current_time = (video_state_info_kwargs['current_time'] -
+                                self._first_seen_time)
+        elapsed_wall_time = clock() - self._first_seen_wall_time
+        # Calculate lag
+        video_state_info_kwargs['lag'] = (
+            elapsed_wall_time - elapsed_current_time)
+        # Calculate remaining time
+        if video_state_info_kwargs['played'].length > 0:
+            played_duration = (video_state_info_kwargs['played'].end(0) -
+                               video_state_info_kwargs['played'].start(0))
+            video_state_info_kwargs['remaining_time'] = (
+                self.expected_duration - played_duration)
+        else:
+            # No playback has happened yet, remaining time is duration
+            video_state_info_kwargs['remaining_time'] = self.expected_duration
+        # Fetch non time critical source information
+        video_state_info_kwargs['video_src'] = self.video.get_attribute('src')
+        video_state_info_kwargs['video_url'] = self.video.get_attribute('url')
+        # Create video state snapshot
+        state_info = self._video_state_named_tuple()
+        return state_info(**video_state_info_kwargs)
 
     @property
-    def video_src(self):
-        """
-        :return: The url of the actual video file, as opposed to the url
-            of the page with the video element.
-        """
-        with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
-            return self.video.get_attribute('src')
+    def _fetch_state_script(self):
+        if not self._fetch_state_script_string:
+            self._fetch_state_script_string = (
+                self._video_var_script +
+                'return ['
+                'currentTime,'
+                'duration,'
+                '[played.length, timeRanges],'
+                'totalFrames,'
+                'droppedFrames,'
+                'corruptedFrames];')
+        return self._fetch_state_script_string
 
-    @property
-    def total_frames(self):
-        """
-        :return: Number of video frames created and dropped since the creation
-            of this video element.
-        """
-        return self.execute_video_script("""
-            return arguments[0].getVideoPlaybackQuality()["totalVideoFrames"];
-            """)
-
-    @property
-    def dropped_frames(self):
-        """
-        :return: Number of video frames created and dropped since the creation
-            of this video element.
-        """
-        return self.execute_video_script("""return
-            arguments[0].getVideoPlaybackQuality()["droppedVideoFrames"];
-            """) or 0
-
-    @property
-    def corrupted_frames(self):
+    def _refresh_state(self):
         """
-        :return: Number of video frames corrupted since the creation of this
-            video element. A corrupted frame may be created or dropped.
-        """
-        return self.execute_video_script("""return
-            arguments[0].getVideoPlaybackQuality()["corruptedVideoFrames"];
-            """) or 0
+        Refresh the snapshot of the underlying video state. We do this all
+        in one so that the state doesn't change in between queries.
 
-    @property
-    def video_url(self):
-        """
-        :return: The URL of the video that this element is playing.
-        """
-        return self.execute_video_script('return arguments[0].baseURI;')
-
-    @property
-    def lag(self):
+        We also store information that can be derived from the snapshotted
+        information, such as lag. This is stored in the last seen state to
+        stress that it's based on the snapshot.
         """
-        :return: The difference in time between where the video is currently
-            playing and where it should be playing given the time we started
-            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
+        keys = ['current_time', 'duration', 'raw_time_ranges', 'total_frames',
+                'dropped_frames', 'corrupted_frames']
+        values = self._execute_video_script(self._fetch_state_script)
+        self._last_seen_video_state = (
+            self._create_video_state_info(**dict(zip(keys, values))))
 
-    def measure_progress(self):
-        initial = self.current_time
+    def _measure_progress(self):
+        self._refresh_state()
+        initial = self._last_seen_video_state.current_time
         sleep(1)
-        return self.current_time - initial
+        self._refresh_state()
+        return self._last_seen_video_state.current_time - initial
 
-    def execute_video_script(self, script):
+    def _execute_video_script(self, script):
         """
         Execute JS script in content context with access  to video element.
 
         :param script: script to be executed
         :return: value returned by script
         """
         with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
             return self.marionette.execute_script(script,
                                                   script_args=[self.video])
 
     def __str__(self):
         messages = ['{} - test url: {}: '
                     .format(type(self).__name__, self.test_url)]
-        messages += '{'
-        if self.video:
-            messages += [
-                '\t(video)',
-                '\tcurrent_time: {},'.format(self.current_time),
-                '\tduration: {},'.format(self.duration),
-                '\texpected_duration: {},'.format(self.expected_duration),
-                '\tplayed: {}'.format(self.played),
-                '\tinterval: {}'.format(self.interval),
-                '\tlag: {},'.format(self.lag),
-                '\turl: {}'.format(self.video_url),
-                '\tsrc: {}'.format(self.video_src),
-                '\tframes total: {}'.format(self.total_frames),
-                '\t - dropped: {}'.format(self.dropped_frames),
-                '\t - corrupted: {}'.format(self.corrupted_frames)
-            ]
-        else:
+        if not self.video:
             messages += ['\tvideo: None']
-        messages.append('}')
+            return '\n'.join(messages)
+        if not self._last_seen_video_state:
+            messages += ['\tvideo: No last seen state']
+            return '\n'.join(messages)
+        # Have video and state info
+        messages += [
+            '{',
+            '\t(video)'
+        ]
+        messages += ['\tinterval: {}'.format(self.interval)]
+        messages += ['\texpected duration: {}'.format(self.expected_duration)]
+        messages += ['\tstall wait time: {}'.format(self.stall_wait_time)]
+        messages += ['\ttimeout: {}'.format(self.timeout)]
+        # Print each field on its own line
+        for field in self._last_seen_video_state._fields:
+            messages += [('\t{}: {}'
+                          .format(field, getattr(self._last_seen_video_state,
+                                                 field)))]
+        messages += '}'
         return '\n'.join(messages)
 
 
 class VideoException(Exception):
     """
     Exception class to use for video-specific error processing.
     """
     pass
 
 
 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)
         )
 
     def start(self, index):
         return self.ranges[index][0]
 
     def end(self, index):
         return self.ranges[index][1]
-
-
-def playback_started(video):
-    """
-    Determine if video has started
-
-    :param video: The VideoPuppeteer instance that we are interested in.
-
-    :return: True if is playing; False otherwise
-    """
-    try:
-        played_ranges = video.played
-        return (
-            played_ranges.length > 0 and
-            played_ranges.start(0) < played_ranges.end(0) and
-            played_ranges.end(0) > 0.0
-        )
-    except Exception as e:
-        print ('Got exception {}'.format(e))
-        return False
-
-
-def playback_done(video):
-    """
-    If we are near the end and there is still a video element, then
-    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:
-        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):
-        raise VideoException('Video {} stalled.\n{}'
-                             .format(video.video_url, video))
-
-    # We are cruising, so we are not done.
-    return False
--- a/dom/media/test/external/external_media_tests/media_utils/youtube_puppeteer.py
+++ b/dom/media/test/external/external_media_tests/media_utils/youtube_puppeteer.py
@@ -56,20 +56,20 @@ class YouTubePuppeteer(VideoPuppeteer):
             self.marionette.execute_script("log('.html5-video-player "
                                            "element obtained');")
         # When an ad is playing, self.player_duration indicates the duration
         # of the spliced-in ad stream, not the duration of the main video, so
         # we attempt to skip the ad first.
         for attempt in range(5):
             sleep(1)
             self.process_ad()
-            if (self.ad_inactive and self.duration and not
+            if (self.ad_inactive and self._last_seen_video_state.duration and not
                     self.player_buffering):
                 break
-        self.update_expected_duration()
+        self._update_expected_duration()
 
     def player_play(self):
         """
         Play via YouTube API.
         """
         self.execute_yt_script('arguments[1].wrappedJSObject.playVideo();')
 
     def player_pause(self):
@@ -383,19 +383,20 @@ class YouTubePuppeteer(VideoPuppeteer):
     def search_ad_duration(self):
         """
 
         :return: ad duration in seconds, if currently displayed in player
         """
         if not (self.ad_playing or self.player_measure_progress() == 0):
             return None
         # If the ad is not Flash...
-        if (self.ad_playing and self.video_src.startswith('mediasource') and
-                self.duration):
-            return self.duration
+        if (self.ad_playing and
+                self._last_seen_video_state.video_src.startswith('mediasource') and
+                self._last_seen_video_state.duration):
+            return self._last_seen_video_state.duration
         selector = '.html5-media-player .videoAdUiAttribution'
         wait = Wait(self.marionette, timeout=5)
         try:
             with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
                 wait.until(expected.element_present(By.CSS_SELECTOR,
                                                     selector))
                 countdown = self.marionette.find_element(By.CSS_SELECTOR,
                                                          selector)
@@ -418,17 +419,17 @@ class YouTubePuppeteer(VideoPuppeteer):
          excludes ad breaks. Note that the player might just be busy with
          buffering due to a slow network.
         """
 
         # `current_time` stands still while ad is playing
         def condition():
             # no ad is playing and current_time stands still
             return (not self.ad_playing and
-                    self.measure_progress() < 0.1 and
+                    self._measure_progress() < 0.1 and
                     self.player_measure_progress() < 0.1 and
                     (self.player_playing or self.player_buffering))
 
         if condition():
             sleep(2)
             if self.player_buffering:
                 sleep(5)
             return condition()
--- a/dom/media/test/external/external_media_tests/playback/youtube/test_basic_playback.py
+++ b/dom/media/test/external/external_media_tests/playback/youtube/test_basic_playback.py
@@ -6,32 +6,33 @@ from marionette import Marionette
 from marionette_driver import Wait
 from marionette_driver.errors import TimeoutException
 
 from external_media_tests.utils import verbose_until
 from external_media_harness.testcase import MediaTestCase
 from external_media_tests.media_utils.video_puppeteer import VideoException
 from external_media_tests.media_utils.youtube_puppeteer import (
     YouTubePuppeteer,
-    playback_done,
-    wait_for_almost_done
+    wait_for_almost_done,
+    playback_done
 )
 
 
 class TestBasicYouTubePlayback(MediaTestCase):
     def test_mse_is_enabled_by_default(self):
         with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
             youtube = YouTubePuppeteer(self.marionette, self.video_urls[0],
                                        timeout=60)
             wait = Wait(youtube,
                         timeout=min(300, youtube.expected_duration * 1.3),
                         interval=1)
             try:
                 verbose_until(wait, youtube,
-                              lambda y: y.video_src.startswith('blob'),
+                              lambda y: y._last_seen_video_state.
+                              video_src.startswith('blob'),
                               "Failed to find 'blob' in video src url.")
             except TimeoutException as e:
                 raise self.failureException(e)
 
     def test_video_playing_in_one_tab(self):
         with self.marionette.using_context(Marionette.CONTEXT_CONTENT):
             for url in self.video_urls:
                 self.logger.info(url)