Add test

src/js/big-play-button.js

@@ -3,6 +3,7 @@
  */
 import Button from './button.js';
 import Component from './component.js';
+import {isPromise} from './utils/promise';
 
 /**
  * The initial play button that shows before the video has played. The hiding of the
@@ -58,10 +59,8 @@ class BigPlayButton extends Button {
 
     const playFocus = () => playToggle.focus();
 
-    if (playPromise && playPromise.then) {
-      const ignoreRejectedPlayPromise = () => {};
-
-      playPromise.then(playFocus, ignoreRejectedPlayPromise);
+    if (isPromise(playPromise)) {
+      playPromise.then(playFocus, () => {});
     } else {
       this.setTimeout(playFocus, 1);
     }

src/js/player.js

@@ -24,6 +24,7 @@ import MediaError from './media-error.js';
 import safeParseTuple from 'safe-json-parse/tuple';
 import {assign} from './utils/obj';
 import mergeOptions from './utils/merge-options.js';
+import {silencePromise} from './utils/promise';
 import textTrackConverter from './tracks/text-track-list-converter.js';
 import ModalDialog from './modal-dialog';
 import Tech from './tech/tech.js';
@@ -456,8 +457,6 @@ class Player extends Component {
 
     this.on('fullscreenchange', this.handleFullscreenChange_);
     this.on('stageclick', this.handleStageClick_);
-
-    this.changingSrc_ = false;
   }
 
   /**
@@ -1613,35 +1612,33 @@ class Player extends Component {
   }
 
   /**
-   * start media playback
+   * Begin playback.
    *
    * @return {Promise|undefined}
-   *         Returns a `Promise` if the browser returns one, for most browsers this will
-   *         return undefined.
+   *         Returns a `Promise` only if the browser returns one and the player
+   *         is ready to begin playback. For most browsers and non-ready
+   *         situations, this will return undefined.
    */
   play() {
-    if (this.changingSrc_) {
-      this.ready(function() {
-        const retval = this.techGet_('play');
 
-        // silence errors (unhandled promise from play)
-        if (retval !== undefined && typeof retval.then === 'function') {
-          retval.then(null, (e) => {});
-        }
+    // If the player is not ready, queue up a call to the tech's `play()`
+    // method for when it _is_ ready.
+    if (!this.isReady_) {
+      this.ready(() => {
+        silencePromise(this.techGet_('play'));
       });
 
-    // Only calls the tech's play if we already have a src loaded
-    } else if (this.isReady_ && (this.src() || this.currentSrc())) {
+    // If the player is ready and there is a source, call the tech's `play()`
+    // method.
+    } else if (this.src() || this.currentSrc()) {
       return this.techGet_('play');
-    } else {
-      this.ready(function() {
-        this.tech_.one('loadstart', function() {
-          const retval = this.play();
 
-          // silence errors (unhandled promise from play)
-          if (retval !== undefined && typeof retval.then === 'function') {
-            retval.then(null, (e) => {});
-          }
+    // Finally, if the player is ready, but we don't have a source, wait for
+    // one to be set.
+    } else {
+      this.ready(() => {
+        this.tech_.one('loadstart', () => {
+          silencePromise(this.techGet_('play'));
         });
       });
     }
@@ -2274,58 +2271,77 @@ class Player extends Component {
    *         URL. Otherwise, returns nothing/undefined.
    */
   src(source) {
-    // getter usage
-    if (typeof source === 'undefined') {
+
+    // Getter usage.
+    if (source === undefined) {
       return this.cache_.src;
     }
-    // filter out invalid sources and turn our source into
-    // an array of source objects
+
+    // Queues "error 4" to be triggered on the player on the next tick. This
+    // is done on a delay to give users a chance to listen for "error" events.
+    const queueError = () => {
+      this.setTimeout(() => {
+        this.error({
+          code: 4,
+          message: this.localize(this.options_.notSupportedMessage)
+        });
+      }, 1);
+    };
+
+    // Filter out invalid sources and normalize the passed source into an array
+    // of source objects.
     const sources = filterSource(source);
 
-    // if a source was passed in then it is invalid because
-    // it was filtered to a zero length Array. So we have to
-    // show an error
+    // Show an error if there are no sources after filtering.
     if (!sources.length) {
-      this.setTimeout(function() {
-        this.error({ code: 4, message: this.localize(this.options_.notSupportedMessage) });
-      }, 0);
-      return;
+      return queueError();
     }
 
-    // intial sources
+    // Cache initial sources.
     this.cache_.sources = sources;
-    this.changingSrc_ = true;
-
-    // intial source
     this.cache_.source = sources[0];
 
-    // middlewareSource is the source after it has been changed by middleware
+    // Set the player to a non-ready state while middleware asynchronously
+    // determines the final source.
+    this.isReady_ = false;
+
+    // This asynchronously resolves any configured middleware. The callback is
+    // called when the process completes and given the `middlewareSource`,
+    // which is the final source we'll use to find a compatible tech.
     middleware.setSource(this, sources[0], (middlewareSource, mws) => {
+
+      // Cache middleware.
       this.middleware_ = mws;
 
-      const err = this.src_(middlewareSource);
+      // Attempt to set the source on a tech.
+      const foundTech = this.src_(middlewareSource);
 
-      if (err) {
+      // We could not find a compatible tech.
+      if (foundTech) {
+
+        // If no compatible tech was found and there are still sources we have
+        // not tried, start the process over (including middleware) with the
+        // next source in the sources array.
         if (sources.length > 1) {
           return this.src(sources.slice(1));
         }
 
-        // We need to wrap this in a timeout to give folks a chance to add error event handlers
-        this.setTimeout(function() {
-          this.error({ code: 4, message: this.localize(this.options_.notSupportedMessage) });
-        }, 0);
-
-        // we could not find an appropriate tech, but let's still notify the delegate that this is it
-        // this needs a better comment about why this is needed
+        // If there is no compatible tech, but there are no sources left to
+        // try, we queue up an error state on the player and trigger "ready"
+        // to inform any subscribers that the player is ready.
+        queueError();
         this.triggerReady();
-
         return;
       }
 
-      this.changingSrc_ = false;
-      // video element listed source
+      // At this point, we've found a compatible tech for the middleware source,
+      // so we can tell any subscribers that the player is ready again.
+      this.triggerReady();
+
+      // Cache the source URL.
       this.cache_.src = middlewareSource.src;
 
+      // Notify middlewares of a new tech.
       middleware.setTech(mws, this.tech_);
     });
   }
@@ -2346,37 +2362,34 @@ class Player extends Component {
   src_(source) {
     const sourceTech = this.selectSource([source]);
 
+    // There is no tech available that can play the chosen source.
     if (!sourceTech) {
       return true;
     }
 
+    // The tech that was found is not the tech that is currently loaded into
+    // the player; so, we need to unload the old tech, load a new one, and
+    // set the source on it.
     if (!titleCaseEquals(sourceTech.tech, this.techName_)) {
-      this.changingSrc_ = true;
-
-      // load this technology with the chosen source
       this.loadTech_(sourceTech.tech, sourceTech.source);
       return false;
     }
 
-    // wait until the tech is ready to set the source
-    this.ready(function() {
+    // The existing tech will work with this source; so, we can set the
+    // source on the existing tech.
+    //
+    // The `setSource` method was added with source handlers; so, older techs
+    // won't support it. Also, we need to check the direct prototype for
+    // the case where subclasses of `Tech` do not support source handlers.
+    if (this.tech_.constructor.prototype.hasOwnProperty('setSource')) {
+      this.techCall_('setSource', source);
+    } else {
+      this.techCall_('src', source.src);
+    }
 
-      // The setSource tech method was added with source handlers
-      // so older techs won't support it
-      // We need to check the direct prototype for the case where subclasses
-      // of the tech do not support source handlers
-      if (this.tech_.constructor.prototype.hasOwnProperty('setSource')) {
-        this.techCall_('setSource', source);
-      } else {
-        this.techCall_('src', source.src);
-      }
-
-      if (this.options_.preload === 'auto') {
-        this.load();
-      }
-
-    // Set the source synchronously if possible (#2326)
-    }, true);
+    if (this.options_.preload === 'auto') {
+      this.load();
+    }
 
     return false;
   }

src/js/utils/promise.js

@@ -0,0 +1,28 @@
+
+/**
+ * Returns whether an object is `Promise`-like (i.e. has a `then` method).
+ *
+ * @param  {Object}  value
+ *         An object that may or may not be `Promise`-like.
+ *
+ * @return {Boolean}
+ *         Whether or not the object is `Promise`-like.
+ */
+export function isPromise(value) {
+  return value !== undefined && typeof value.then === 'function';
+}
+
+/**
+ * Silence a Promise-like object.
+ *
+ * This is useful for avoiding non-harmful, but potentially confusing "uncaught
+ * play promise" rejection error messages.
+ *
+ * @param  {Object} value
+ *         An object that may or may not be `Promise`-like.
+ */
+export function silencePromise(value) {
+  if (isPromise(value)) {
+    value.then(null, (e) => {});
+  }
+}

test/unit/player.test.js

@@ -1601,18 +1601,84 @@ QUnit.test('src_ does not call loadTech is name is titleCaseEquals', function(as
         tech: 'html5'
       };
     },
+    options_: {},
+    tech_: {
+      constructor: {
+        prototype: {}
+      }
+    },
+    techCall_() {},
     techName_: 'Html5',
-    ready() {},
+    // ready() {},
+    load() {},
     loadTech_() {
       loadTechCalled++;
     }
   };
 
-  Player.prototype.src_.call(playerProxy);
+  Player.prototype.src_.call(playerProxy, {});
 
   assert.equal(loadTechCalled, 0, 'loadTech was not called');
 });
 
+QUnit.test('subsequent calls to src() will put the player in a non-ready state, so calling ready() immediately after will correctly wait until the new source is set', function(assert) {
+  const tag = TestHelpers.makeTag();
+  const player = videojs(tag);
+  const onReadySpy = sinon.spy();
+  const readySpy = sinon.spy();
+
+  player.on('ready', onReadySpy);
+  player.ready(readySpy);
+
+  assert.equal(onReadySpy.callCount, 0, 'no readiness yet...');
+  assert.equal(readySpy.callCount, 0, 'no readiness yet...');
+
+  this.clock.tick(1);
+
+  assert.equal(onReadySpy.callCount, 1, 'saw an initial "ready"');
+  assert.equal(readySpy.callCount, 1, 'saw an initial ready() callback');
+
+  player.src({
+    src: 'http://example.com/video.mp4',
+    type: 'video/mp4'
+  });
+
+  player.ready(readySpy);
+
+  assert.equal(onReadySpy.callCount, 1, 'did not see a "ready" because source setting is async');
+  assert.equal(readySpy.callCount, 1, 'did not see a ready() callback because source setting is async');
+
+  this.clock.tick(1);
+
+  assert.equal(onReadySpy.callCount, 1, 'did not see a "ready" because middleware queues up another async operation');
+  assert.equal(readySpy.callCount, 1, 'did not see a ready() callback because middleware queues up another async operation');
+
+  this.clock.tick(1);
+
+  assert.equal(onReadySpy.callCount, 2, 'saw second "ready" because source setting and tech selection are complete');
+  assert.equal(readySpy.callCount, 2, 'saw second ready() callback because source setting and tech selection are complete');
+
+  player.src({
+    src: 'http://example.com/video2.mp4',
+    type: 'video/mp4'
+  });
+
+  player.ready(readySpy);
+
+  assert.equal(onReadySpy.callCount, 2, 'did not see a "ready" because source setting is async');
+  assert.equal(readySpy.callCount, 2, 'did not see a ready() callback because source setting is async');
+
+  this.clock.tick(1);
+
+  assert.equal(onReadySpy.callCount, 2, 'did not see a "ready" because middleware queues up another async operation');
+  assert.equal(readySpy.callCount, 2, 'did not see a ready() callback because middleware queues up another async operation');
+
+  this.clock.tick(1);
+
+  assert.equal(onReadySpy.callCount, 3, 'saw third "ready" because source setting and tech selection are complete');
+  assert.equal(readySpy.callCount, 3, 'saw third ready() callback because source setting and tech selection are complete');
+});
+
 QUnit.test('options: plugins', function(assert) {
   const optionsSpy = sinon.spy();
 

test/unit/tracks/text-tracks.test.js

@@ -574,7 +574,7 @@ if (Html5.isSupported()) {
     player.addRemoteTextTrack(track, false);
     player.src({src: 'example.mp4', type: 'video/mp4'});
 
-    this.clock.tick(1);
+    this.clock.tick(2);
     assert.equal(player.textTracks().length, 0, 'we do not have any tracks left');
 
     player.dispose();