fix: do not always set segmentloader ended to false on sourceopen (#512)

src/master-playlist-controller.js

@@ -453,6 +453,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
     });
 
     this.mainSegmentLoader_.on('ended', () => {
+      this.logger_('main segment loader ended');
       this.onEndOfStream();
     });
 
@@ -486,6 +487,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
     });
 
     this.audioSegmentLoader_.on('ended', () => {
+      this.logger_('audioSegmentLoader ended');
       this.onEndOfStream();
     });
   }
@@ -653,7 +655,12 @@ export class MasterPlaylistController extends videojs.EventTarget {
       this.tryToCreateSourceBuffers_();
     } catch (e) {
       videojs.log.warn('Failed to create Source Buffers', e);
-      return this.mediaSource.endOfStream('decode');
+      if (this.mediaSource.readyState !== 'open') {
+        this.trigger('error');
+      } else {
+        this.sourceUpdater_.endOfStream('decode');
+      }
+      return;
     }
 
     // if autoplay is enabled, begin playback. This is duplicative of
@@ -723,14 +730,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
       return;
     }
 
-    this.logger_(`calling mediaSource.endOfStream()`);
-    // on chrome calling endOfStream can sometimes cause an exception,
-    // even when the media source is in a valid state.
-    try {
-      this.mediaSource.endOfStream();
-    } catch (e) {
-      videojs.log.warn('Failed to call media source endOfStream', e);
-    }
+    this.sourceUpdater_.endOfStream();
   }
 
   /**
@@ -800,11 +800,13 @@ export class MasterPlaylistController extends videojs.EventTarget {
     if (!currentPlaylist) {
       this.error = error;
 
-      try {
-        return this.mediaSource.endOfStream('network');
-      } catch (e) {
-        return this.trigger('error');
+      if (this.mediaSource.readyState !== 'open') {
+        this.trigger('error');
+      } else {
+        this.sourceUpdater_.endOfStream('network');
       }
+
+      return;
     }
 
     let isFinalRendition =
@@ -1186,7 +1188,12 @@ export class MasterPlaylistController extends videojs.EventTarget {
 
       videojs.log.warn(error);
       this.error = error;
-      return this.mediaSource.endOfStream('decode');
+
+      if (this.mediaSource.readyState !== 'open') {
+        this.trigger('error');
+      } else {
+        this.sourceUpdater_.endOfStream('decode');
+      }
     }
 
     try {
@@ -1196,7 +1203,12 @@ export class MasterPlaylistController extends videojs.EventTarget {
 
       videojs.log.warn(error);
       this.error = error;
-      return this.mediaSource.endOfStream('decode');
+      if (this.mediaSource.readyState !== 'open') {
+        this.trigger('error');
+      } else {
+        this.sourceUpdater_.endOfStream('decode');
+      }
+      return;
     }
 
     this.excludeIncompatibleVariants_(media);

src/segment-loader.js

@@ -25,36 +25,6 @@ import { gopsSafeToAlignWith, removeGopBuffer, updateGopBuffer } from './util/go
 
 // in ms
 const CHECK_BUFFER_DELAY = 500;
-
-/**
- * Determines if we should call endOfStream on the media source based
- * on the state of the buffer or if appened segment was the final
- * segment in the playlist.
- *
- * @param {Object} playlist a media playlist object
- * @param {Object} mediaSource the MediaSource object
- * @param {Number} segmentIndex the index of segment we last appended
- * @returns {Boolean} do we need to call endOfStream on the MediaSource
- */
-const detectEndOfStream = function(playlist, mediaSource, segmentIndex) {
-  if (!playlist || !mediaSource) {
-    return false;
-  }
-
-  let segments = playlist.segments;
-
-  // determine a few boolean values to help make the branch below easier
-  // to read
-  let appendedLastSegment = segmentIndex === segments.length;
-
-  // if we've buffered to the end of the video, we need to call endOfStream
-  // so that MediaSources can trigger the `ended` event when it runs out of
-  // buffered data instead of waiting for me
-  return playlist.endList &&
-    mediaSource.readyState === 'open' &&
-    appendedLastSegment;
-};
-
 const finite = (num) => typeof num === 'number' && isFinite(num);
 
 export const illegalMediaSwitch = (loaderType, startingMedia, trackInfo) => {
@@ -246,7 +216,9 @@ export default class SegmentLoader extends videojs.EventTarget {
     this.syncController_.on('syncinfoupdate', () => this.trigger('syncinfoupdate'));
 
     this.mediaSource_.addEventListener('sourceopen', () => {
-      this.ended_ = false;
+      if (!this.isEndOfStream_()) {
+        this.ended_ = false;
+      }
     });
 
     // ...for determining the fetch location
@@ -836,11 +808,6 @@ export default class SegmentLoader extends videojs.EventTarget {
       return;
     }
 
-    if (this.isEndOfStream_(segmentInfo.mediaIndex)) {
-      this.endOfStream();
-      return;
-    }
-
     if (segmentInfo.mediaIndex === this.playlist_.segments.length - 1 &&
         this.mediaSource_.readyState === 'ended' &&
         !this.seeking_()) {
@@ -885,18 +852,26 @@ export default class SegmentLoader extends videojs.EventTarget {
   }
 
   /**
-   * Determines if this segment loader is at the end of it's stream.
+   * Determines if we should call endOfStream on the media source based
+   * on the state of the buffer or if appened segment was the final
+   * segment in the playlist.
    *
-   * @param {Number} mediaIndex the index of segment we last appended
-   * @param {Object} [playlist=this.playlist_] a media playlist object
-   * @returns {Boolean} true if at end of stream, false otherwise.
+   * @param {Number} [mediaIndex] the media index of segment we last appended
+   * @param {Object} [playlist] a media playlist object
+   * @returns {Boolean} do we need to call endOfStream on the MediaSource
    */
-  isEndOfStream_(mediaIndex, playlist = this.playlist_) {
-    return detectEndOfStream(
-      playlist,
-      this.mediaSource_,
-      mediaIndex
-    ) && !this.sourceUpdater_.updating();
+  isEndOfStream_(mediaIndex = this.mediaIndex, playlist = this.playlist_) {
+    if (!playlist || !this.mediaSource_) {
+      return false;
+    }
+
+    // mediaIndex is zero based but length is 1 based
+    const appendedLastSegment = (mediaIndex + 1) === playlist.segments.length;
+
+    // if we've buffered to the end of the video, we need to call endOfStream
+    // so that MediaSources can trigger the `ended` event when it runs out of
+    // buffered data instead of waiting for me
+    return playlist.endList && this.mediaSource_.readyState === 'open' && appendedLastSegment;
   }
 
   /**
@@ -1842,9 +1817,7 @@ export default class SegmentLoader extends videojs.EventTarget {
     // state away from loading until we are officially done loading the segment data.
     this.state = 'APPENDING';
 
-    const isEndOfStream = detectEndOfStream(segmentInfo.playlist,
-                                            this.mediaSource_,
-                                            segmentInfo.mediaIndex + 1);
+    const isEndOfStream = this.isEndOfStream_(segmentInfo.mediaIndex, segmentInfo.playlist);
     const isWalkingForward = this.mediaIndex !== null;
     const isDiscontinuity = segmentInfo.timeline !== this.currentTimeline_ &&
       // TODO verify this behavior
@@ -2107,13 +2080,6 @@ export default class SegmentLoader extends videojs.EventTarget {
 
     const isWalkingForward = this.mediaIndex !== null;
 
-    // any time an update finishes and the last segment is in the
-    // buffer, end the stream. this ensures the "ended" event will
-    // fire if playback reaches that point.
-    if (this.isEndOfStream_(segmentInfo.mediaIndex + 1, segmentInfo.playlist)) {
-      this.endOfStream();
-    }
-
     // Don't do a rendition switch unless we have enough time to get a sync segment
     // and conservatively guess
     if (isWalkingForward) {
@@ -2123,6 +2089,13 @@ export default class SegmentLoader extends videojs.EventTarget {
 
     this.mediaIndex = segmentInfo.mediaIndex;
 
+    // any time an update finishes and the last segment is in the
+    // buffer, end the stream. this ensures the "ended" event will
+    // fire if playback reaches that point.
+    if (this.isEndOfStream_(segmentInfo.mediaIndex, segmentInfo.playlist)) {
+      this.endOfStream();
+    }
+
     // used for testing
     this.trigger('appended');
 

src/source-updater.js

@@ -105,6 +105,8 @@ const actions = {
   appendBuffer: (bytes) => (type, sourceUpdater) => {
     const sourceBuffer = sourceUpdater[`${type}Buffer`];
 
+    sourceUpdater.logger_(`Appending ${bytes.length} to ${type}Buffer`);
+
     sourceBuffer.appendBuffer(bytes);
   },
   remove: (start, end) => (type, sourceUpdater) => {
@@ -112,17 +114,33 @@ const actions = {
 
     sourceBuffer.removing = true;
 
+    sourceUpdater.logger_(`Removing ${start} to ${end} from ${type}Buffer`);
     sourceBuffer.remove(start, end);
   },
   timestampOffset: (offset) => (type, sourceUpdater) => {
     const sourceBuffer = sourceUpdater[`${type}Buffer`];
 
+    sourceUpdater.logger_(`Setting ${type}timestampOffset to ${offset}`);
+
     sourceBuffer.timestampOffset = offset;
   },
   callback: (callback) => (type, sourceUpdater) => {
     callback();
   },
+  endOfStream: (error) => (sourceUpdater) => {
+    if (sourceUpdater.mediaSource.readyState !== 'open') {
+      return;
+    }
+    sourceUpdater.logger_(`Calling mediaSource endOfStream(${error || ''})`);
+
+    try {
+      sourceUpdater.mediaSource.endOfStream(error);
+    } catch (e) {
+      videojs.log.warn('Failed to call media source endOfStream', e);
+    }
+  },
   duration: (duration) => (sourceUpdater) => {
+    sourceUpdater.logger_(`Setting mediaSource duration to ${duration}`);
     try {
       sourceUpdater.mediaSource.duration = duration;
     } catch (e) {
@@ -313,6 +331,23 @@ export default class SourceUpdater extends videojs.EventTarget {
     });
   }
 
+  endOfStream(error = null, doneFn = noop) {
+    if (typeof error !== 'string') {
+      error = undefined;
+    }
+    // In order to set the duration on the media source, it's necessary to wait for all
+    // source buffers to no longer be updating. "If the updating attribute equals true on
+    // any SourceBuffer in sourceBuffers, then throw an InvalidStateError exception and
+    // abort these steps." (source: https://www.w3.org/TR/media-source/#attributes).
+    pushQueue({
+      type: 'mediaSource',
+      sourceUpdater: this,
+      action: actions.endOfStream(error),
+      name: 'endOfStream',
+      doneFn
+    });
+  }
+
   /**
    * Queue an update to remove a time range from the buffer.
    *

test/playback.test.js

@@ -28,6 +28,7 @@ QUnit.module('Playback', {
     let done = assert.async();
     let video = document.createElement('video-js');
 
+    videojs.log.level('debug');
     video.style = 'display: none;';
 
     video.width = 600;
@@ -192,10 +193,7 @@ QUnit.test('DASH sidx', function(assert) {
   });
 });
 
-// This is a potentially existing issue where a combination of things
-// results in no ended event. We should fix this and start running this
-// test with 100% pass rate after that.
-QUnit.skip('DASH sidx with alt audio should end', function(assert) {
+QUnit.test('DASH sidx with alt audio should end', function(assert) {
 
   let done = assert.async();
   let player = this.player;

test/segment-loader.test.js

@@ -891,45 +891,6 @@ QUnit.module('SegmentLoader', function(hooks) {
       });
     });
 
-    QUnit.test('endOfStream does not happen while sourceUpdater is updating', function(assert) {
-      let endOfStreams = 0;
-      let bandwidthupdates = 0;
-
-      return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => {
-
-        loader.on('ended', () => endOfStreams++);
-
-        loader.on('bandwidthupdate', () => {
-          bandwidthupdates++;
-          // Simulate a rendition switch
-          loader.resetEverything();
-        });
-
-        loader.playlist(playlistWithDuration(20));
-        loader.load();
-        this.clock.tick(1);
-
-        standardXHRResponse(this.requests.shift(), muxedSegment());
-        return new Promise((resolve, reject) => {
-          loader.one('appended', resolve);
-          loader.one('error', reject);
-        });
-      }).then(() => {
-        this.clock.tick(10);
-
-        loader.sourceUpdater_.updating = () => true;
-        standardXHRResponse(this.requests.shift(), muxedSegment());
-
-        return new Promise((resolve, reject) => {
-          loader.one('appended', resolve);
-          loader.one('error', reject);
-        });
-      }).then(() => {
-        assert.equal(bandwidthupdates, 1, 'triggered bandwidthupdate');
-        assert.equal(endOfStreams, 0, 'triggered ended');
-      });
-    });
-
     QUnit.test('live playlists do not trigger ended', function(assert) {
       let endOfStreams = 0;
 

test/source-updater.test.js

@@ -845,6 +845,116 @@ function(assert) {
   assert.ok(Number.isNaN(this.mediaSource.duration), 'duration set to NaN at start');
 });
 
+QUnit.test('endOfStream processes immediately if not waiting on source buffers',
+function(assert) {
+  this.sourceUpdater.createSourceBuffers({
+    audio: 'mp4a.40.2',
+    video: 'avc1.4D001E'
+  });
+
+  assert.equal(this.mediaSource.readyState, 'open', 'media source is open');
+  this.sourceUpdater.endOfStream();
+  assert.equal(this.mediaSource.readyState, 'ended', 'media source is ended');
+});
+
+QUnit.test('endOfStream can be called with an error string',
+function(assert) {
+  this.sourceUpdater.createSourceBuffers({
+    audio: 'mp4a.40.2',
+    video: 'avc1.4D001E'
+  });
+
+  assert.equal(this.mediaSource.readyState, 'open', 'media source is open');
+  this.sourceUpdater.endOfStream('network');
+  // some browsers mark it as ended, others as closed
+  assert.ok((/^ended|closed$/).test(this.mediaSource.readyState), 'media source is ended');
+});
+
+QUnit.test('endOfStream waits for audio buffer to finish updating', function(assert) {
+  const done = assert.async();
+
+  assert.expect(5);
+
+  this.sourceUpdater.createSourceBuffers({
+    audio: 'mp4a.40.2',
+    video: 'avc1.4D001E'
+  });
+
+  assert.notOk(this.sourceUpdater.updating(), 'not updating by default');
+
+  this.sourceUpdater.appendBuffer({type: 'audio', bytes: mp4Audio()}, () => {
+    assert.equal(this.mediaSource.readyState, 'open', 'media source is open');
+  });
+  this.sourceUpdater.endOfStream(null, () => {
+    assert.equal(this.mediaSource.readyState, 'ended', 'media source is ended');
+    done();
+  });
+
+  assert.equal(this.mediaSource.readyState, 'open', 'media source is open');
+  assert.ok(this.sourceUpdater.updating(), 'updating during appends');
+});
+
+QUnit.test('endOfStream waits for video buffer to finish updating', function(assert) {
+  const done = assert.async();
+
+  assert.expect(5);
+
+  this.sourceUpdater.createSourceBuffers({
+    audio: 'mp4a.40.2',
+    video: 'avc1.4D001E'
+  });
+
+  assert.notOk(this.sourceUpdater.updating(), 'not updating by default');
+
+  this.sourceUpdater.appendBuffer({type: 'video', bytes: mp4Video()}, () => {
+    assert.equal(this.mediaSource.readyState, 'open', 'media source is open');
+  });
+  this.sourceUpdater.endOfStream(null, () => {
+    assert.equal(this.mediaSource.readyState, 'ended', 'media source is ended');
+    done();
+  });
+
+  assert.equal(this.mediaSource.readyState, 'open', 'media source is open');
+  assert.ok(this.sourceUpdater.updating(), 'updating during appends');
+});
+
+QUnit.test('endOfStream waits for both audio and video buffers to finish updating',
+function(assert) {
+  const done = assert.async();
+  let appendsFinished = 0;
+
+  assert.expect(7);
+
+  this.sourceUpdater.createSourceBuffers({
+    audio: 'mp4a.40.2',
+    video: 'avc1.4D001E'
+  });
+
+  assert.notOk(this.sourceUpdater.updating(), 'not updating by default');
+
+  const checkDuration = () => {
+    assert.equal(this.mediaSource.readyState, 'open', 'media source is open');
+
+    if (appendsFinished === 0) {
+      this.sourceUpdater.endOfStream(null, () => {
+        assert.equal(this.mediaSource.readyState, 'ended', 'media source is ended');
+        done();
+      });
+    }
+
+    appendsFinished++;
+  };
+
+  this.sourceUpdater.appendBuffer({type: 'video', bytes: mp4Video()}, checkDuration);
+  this.sourceUpdater.appendBuffer({type: 'audio', bytes: mp4Audio()}, checkDuration);
+  this.sourceUpdater.endOfStream(null, () => {
+    assert.equal(this.mediaSource.readyState, 'ended', 'media source is ended');
+  });
+
+  assert.equal(this.mediaSource.readyState, 'open', 'media source is open');
+  assert.ok(this.sourceUpdater.updating(), 'updating during appends');
+});
+
 QUnit.test('dispose removes sourceopen listener', function(assert) {
   // create fake media source so we can detect event listeners being added and removed
   const addEventListenerCalls = [];

test/videojs-http-streaming.test.js

@@ -630,17 +630,11 @@ function(assert) {
   });
   openMediaSource(this.player, this.clock);
 
-  this.player.tech_.hls.masterPlaylistController_.mediaSource.endOfStream = (type) => {
-    endOfStreams.push(type);
-    throw new Error();
-  };
+  this.player.tech_.hls.masterPlaylistController_.mediaSource.readyState = 'closed';
 
   this.player.on('error', () => {
     const error = this.player.error();
 
-    assert.equal(endOfStreams.length, 1, 'one endOfStream called');
-    assert.equal(endOfStreams[0], 'network', 'endOfStream called with network');
-
     assert.equal(error.code, 2, 'error has correct code');
     assert.equal(error.message,
                  'HLS playlist request error at URL: manifest/master.m3u8.',