Fix the double loadstart event and fix it good

CHANGELOG.md

@@ -135,6 +135,7 @@ CHANGELOG
 * @heff made tech related functions private in the player ([view](https://github.com/videojs/video.js/pull/2590))
 * @heff removed the loadedalldata event ([view](https://github.com/videojs/video.js/pull/2591))
 * @dmlap switched to using raynos/xhr for requests ([view](https://github.com/videojs/video.js/pull/2594))
+* @heff Fixed double loadstart and ready events ([view](https://github.com/videojs/video.js/pull/2605))
 
 --------------------
 

src/js/player.js

@@ -495,10 +495,6 @@ class Player extends Component {
     // Turn off API access because we're loading a new tech that might load asynchronously
     this.isReady_ = false;
 
-    var techReady = Fn.bind(this, function() {
-      this.triggerReady();
-    });
-
     // Grab tech-specific options from player options and add source and parent element to use.
     var techOptions = assign({
       'nativeControlsForTouch': this.options_.nativeControlsForTouch,
@@ -532,11 +528,12 @@ class Player extends Component {
     let techComponent = Component.getComponent(techName);
     this.tech_ = new techComponent(techOptions);
 
-    textTrackConverter.jsonToTextTracks(this.textTracksJson_ || [], this.tech_);
+    // player.triggerReady is always async, so don't need this to be async
+    this.tech_.ready(Fn.bind(this, this.handleTechReady_), true);
 
-    this.on(this.tech_, 'ready', this.handleTechReady_);
+    textTrackConverter.jsonToTextTracks(this.textTracksJson_ || [], this.tech_);
 
-    // Listen to every HTML5 events and trigger them back on the player for the plugins
+    // Listen to all HTML5-defined events and trigger them on the player
     this.on(this.tech_, 'loadstart', this.handleTechLoadStart_);
     this.on(this.tech_, 'waiting', this.handleTechWaiting_);
     this.on(this.tech_, 'canplay', this.handleTechCanPlay_);
@@ -581,9 +578,6 @@ class Player extends Component {
       this.tag.player = null;
       this.tag = null;
     }
-
-    // player.triggerReady is always async, so don't need this to be async
-    this.tech_.ready(techReady, true);
   }
 
   /**
@@ -605,7 +599,22 @@ class Player extends Component {
   }
 
   /**
-   * Add playback technology listeners
+   * Set up click and touch listeners for the playback element
+   *
+   * On desktops, a click on the video itself will toggle playback,
+   * on a mobile device a click on the video toggles controls.
+   * (toggling controls is done by toggling the user state between active and
+   * inactive)
+   * A tap can signal that a user has become active, or has become inactive
+   * e.g. a quick tap on an iPhone movie should reveal the controls. Another
+   * quick tap should hide them again (signaling the user is in an inactive
+   * viewing state)
+   * In addition to this, we still want the user to be considered inactive after
+   * a few seconds of inactivity.
+   * Note: the only part of iOS interaction we can't mimic with this setup
+   * is a touch and hold on the video element counting as activity in order to
+   * keep the controls showing, but that shouldn't be an issue. A touch and hold
+   * on any controls will still keep the user active
    *
    * @private
    * @method addTechControlsListeners_

src/js/tech/html5.js

@@ -36,6 +36,8 @@ class Html5 extends Tech {
     // anyway so the error gets fired.
     if (source && (this.el_.currentSrc !== source.src || (options.tag && options.tag.initNetworkState_ === 3))) {
       this.setSource(source);
+    } else {
+      this.handleLateInit_(this.el_);
     }
 
     if (this.el_.hasChildNodes()) {
@@ -167,6 +169,87 @@ class Html5 extends Tech {
     // jenniisawesome = true;
   }
 
+  // If we're loading the playback object after it has started loading
+  // or playing the video (often with autoplay on) then the loadstart event
+  // has already fired and we need to fire it manually because many things
+  // rely on it.
+  handleLateInit_(el) {
+    if (el.networkState === 0 || el.networkState === 3) {
+      // The video element hasn't started loading the source yet
+      // or didn't find a source
+      return;
+    }
+
+    if (el.readyState === 0) {
+      // NetworkState is set synchronously BUT loadstart is fired at the
+      // end of the current stack, usually before setInterval(fn, 0).
+      // So at this point we know loadstart may have already fired or is
+      // about to fire, and either way the player hasn't seen it yet.
+      // We don't want to fire loadstart prematurely here and cause a
+      // double loadstart so we'll wait and see if it happens between now
+      // and the next loop, and fire it if not.
+      // HOWEVER, we also want to make sure it fires before loadedmetadata
+      // which could also happen between now and the next loop, so we'll
+      // watch for that also.
+      let loadstartFired = false;
+      let setLoadstartFired = function() {
+        loadstartFired = true;
+      };
+      this.on('loadstart', setLoadstartFired);
+
+      let triggerLoadstart = function() {
+        // We did miss the original loadstart. Make sure the player
+        // sees loadstart before loadedmetadata
+        if (!loadstartFired) {
+          this.trigger('loadstart');
+        }
+      };
+      this.on('loadedmetadata', triggerLoadstart);
+
+      this.ready(function(){
+        this.off('loadstart', setLoadstartFired);
+        this.off('loadedmetadata', triggerLoadstart);
+
+        if (!loadstartFired) {
+          // We did miss the original native loadstart. Fire it now.
+          this.trigger('loadstart');
+        }
+      });
+
+      return;
+    }
+
+    // From here on we know that loadstart already fired and we missed it.
+    // The other readyState events aren't as much of a problem if we double
+    // them, so not going to go to as much trouble as loadstart to prevent
+    // that unless we find reason to.
+    let eventsToTrigger = ['loadstart'];
+
+    // loadedmetadata: newly equal to HAVE_METADATA (1) or greater
+    eventsToTrigger.push('loadedmetadata');
+
+    // loadeddata: newly increased to HAVE_CURRENT_DATA (2) or greater
+    if (el.readyState >= 2) {
+      eventsToTrigger.push('loadeddata');
+    }
+
+    // canplay: newly increased to HAVE_FUTURE_DATA (3) or greater
+    if (el.readyState >= 3) {
+      eventsToTrigger.push('canplay');
+    }
+
+    // canplaythrough: newly equal to HAVE_ENOUGH_DATA (4)
+    if (el.readyState >= 4) {
+      eventsToTrigger.push('canplaythrough');
+    }
+
+    // We still need to give the player time to add event listeners
+    this.ready(function(){
+      eventsToTrigger.forEach(function(type){
+        this.trigger(type);
+      }, this);
+    });
+  }
 
   proxyNativeTextTracks_() {
     let tt = this.el().textTracks;
@@ -390,14 +473,18 @@ class Html5 extends Tech {
    * @deprecated
    * @method setSrc
    */
-  setSrc(src) { this.el_.src = src; }
+  setSrc(src) {
+    this.el_.src = src;
+  }
 
   /**
    * Load media into player
    *
    * @method load
    */
-  load(){ this.el_.load(); }
+  load(){
+    this.el_.load();
+  }
 
   /**
    * Get current source

src/js/tech/tech.js

@@ -53,8 +53,6 @@ class Tech extends Component {
       this.manualTimeUpdatesOn();
     }
 
-    this.initControlsListeners();
-
     if (options.nativeCaptions === false || options.nativeTextTracks === false) {
       this.featuresNativeTextTracks = false;
     }
@@ -69,39 +67,6 @@ class Tech extends Component {
     this.emitTapEvents();
   }
 
-  /**
-   * Set up click and touch listeners for the playback element
-   * On desktops, a click on the video itself will toggle playback,
-   * on a mobile device a click on the video toggles controls.
-   * (toggling controls is done by toggling the user state between active and
-   * inactive)
-   * A tap can signal that a user has become active, or has become inactive
-   * e.g. a quick tap on an iPhone movie should reveal the controls. Another
-   * quick tap should hide them again (signaling the user is in an inactive
-   * viewing state)
-   * In addition to this, we still want the user to be considered inactive after
-   * a few seconds of inactivity.
-   * Note: the only part of iOS interaction we can't mimic with this setup
-   * is a touch and hold on the video element counting as activity in order to
-   * keep the controls showing, but that shouldn't be an issue. A touch and hold on
-   * any controls will still keep the user active
-   *
-   * @method initControlsListeners
-   */
-  initControlsListeners() {
-    // if we're loading the playback object after it has started loading or playing the
-    // video (often with autoplay on) then the loadstart event has already fired and we
-    // need to fire it manually because many things rely on it.
-    // Long term we might consider how we would do this for other events like 'canplay'
-    // that may also have fired.
-    this.ready(function(){
-      if (this.networkState && this.networkState() > 0) {
-        this.trigger('loadstart');
-      }
-    // Allow the tech ready event to handle synchronisity
-    }, true);
-  }
-
   /* Fallbacks for unsupported event types
   ================================================================================ */
   // Manually trigger progress events based on changes to the buffered amount

test/unit/tech/html5.test.js

@@ -228,3 +228,52 @@ if (Html5.supportsNativeTextTracks()) {
     equal(adds[2][0], rems[2][0], 'removetrack event handler removed');
   });
 }
+
+test('should fire makeup events when a video tag is initialized late', function(){
+  let lateInit = Html5.prototype.handleLateInit_;
+  let triggeredEvents = [];
+  let mockHtml5 = {
+    readyListeners: [],
+    ready(listener){
+      this.readyListeners.push(listener);
+    },
+    triggerReady(){
+      this.readyListeners.forEach(function(listener){
+        listener.call(this);
+      }, this);
+    },
+    trigger(type){
+      triggeredEvents.push(type);
+    },
+    on: function(){},
+    off: function(){}
+  };
+
+  function resetMock() {
+    triggeredEvents = {};
+    mockHtml5.readyListeners = [];
+  }
+
+  function testStates(statesObject, expectedEvents) {
+    lateInit.call(mockHtml5, statesObject);
+    mockHtml5.triggerReady();
+    deepEqual(triggeredEvents, expectedEvents, `wrong events triggered for networkState:${statesObject.networkState} and readyState:${statesObject.readyState || 'no readyState'}`);
+
+    // reset mock
+    triggeredEvents = [];
+    mockHtml5.readyListeners = [];
+  }
+
+  // Network States
+  testStates({ networkState: 0, readyState: 0 }, []);
+  testStates({ networkState: 1, readyState: 0 }, ['loadstart']);
+  testStates({ networkState: 2, readyState: 0 }, ['loadstart']);
+  testStates({ networkState: 3, readyState: 0 }, []);
+
+  // Ready States
+  testStates({ networkState: 1, readyState: 0 }, ['loadstart']);
+  testStates({ networkState: 1, readyState: 1 }, ['loadstart', 'loadedmetadata']);
+  testStates({ networkState: 1, readyState: 2 }, ['loadstart', 'loadedmetadata', 'loadeddata']);
+  testStates({ networkState: 1, readyState: 3 }, ['loadstart', 'loadedmetadata', 'loadeddata', 'canplay']);
+  testStates({ networkState: 1, readyState: 4 }, ['loadstart', 'loadedmetadata', 'loadeddata', 'canplay', 'canplaythrough']);
+});