Fix 'ended' event not firing after replay (#842)

* add eos check in checkbuffer

* don't rely on 'this' in checkBuffer

* make use of detectEndOfStream in fill buffer

src/segment-loader.js

@@ -54,10 +54,9 @@ const updateSegmentMetadata = function(playlist, segmentIndex, segmentEnd) {
  * @param {Object} playlist a media playlist object
  * @param {Object} mediaSource the MediaSource object
  * @param {Number} segmentIndex the index of segment we last appended
- * @param {Object} currentBuffered buffered region that currentTime resides in
  * @returns {Boolean} do we need to call endOfStream on the MediaSource
  */
-const detectEndOfStream = function(playlist, mediaSource, segmentIndex, currentBuffered) {
+const detectEndOfStream = function(playlist, mediaSource, segmentIndex) {
   if (!playlist) {
     return false;
   }
@@ -66,16 +65,14 @@ const detectEndOfStream = function(playlist, mediaSource, segmentIndex, currentB
 
   // determine a few boolean values to help make the branch below easier
   // to read
-  let appendedLastSegment = (segmentIndex === segments.length - 1);
-  let bufferedToEnd = (currentBuffered.length &&
-    segments[segments.length - 1].end <= currentBuffered.end(0));
+  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 || bufferedToEnd);
+    appendedLastSegment;
 };
 
 /**
@@ -363,10 +360,14 @@ export default class SegmentLoader extends videojs.EventTarget {
    * @param {TimeRanges} buffered - the state of the buffer
    * @param {Object} playlist - the playlist object to fetch segments from
    * @param {Number} currentTime - the playback position in seconds
+   * @param {Boolean} hasPlayed - if the player has played before
+   * @param {Number} expired - the seconds expired off the playlist
+   * @param {Number} timeCorrection - correction value to add to current time when
+   *  when determining media index to use
    * @returns {Object} a segment info object that describes the
    * request that should be made or null if no request is necessary
    */
-  checkBuffer_(buffered, playlist, currentTime) {
+  checkBuffer_(buffered, playlist, currentTime, hasPlayed, expired, timeCorrection) {
     let currentBuffered = Ranges.findRange(buffered, currentTime);
 
     // There are times when MSE reports the first segment as starting a
@@ -379,18 +380,17 @@ export default class SegmentLoader extends videojs.EventTarget {
 
     let bufferedTime;
     let currentBufferedEnd;
-    let segment;
     let mediaIndex;
 
     if (!playlist.segments.length) {
-      return;
+      return null;
     }
 
     if (currentBuffered.length === 0) {
       // find the segment containing currentTime
       mediaIndex = getMediaIndexForTime(playlist,
-                                        currentTime + this.timeCorrection_,
-                                        this.expired_);
+                                        currentTime + timeCorrection,
+                                        expired);
     } else {
       // find the segment adjacent to the end of the current
       // buffered region
@@ -399,48 +399,21 @@ export default class SegmentLoader extends videojs.EventTarget {
 
       // if the video has not yet played only, and we already have
       // one segment downloaded do nothing
-      if (!this.hasPlayed_() && bufferedTime >= 1) {
+      if (!hasPlayed && bufferedTime >= 1) {
         return null;
       }
 
       // if there is plenty of content buffered, and the video has
       // been played before relax for awhile
-      if (this.hasPlayed_() && bufferedTime >= Config.GOAL_BUFFER_LENGTH) {
+      if (hasPlayed && bufferedTime >= Config.GOAL_BUFFER_LENGTH) {
         return null;
       }
       mediaIndex = getMediaIndexForTime(playlist,
-                                        currentBufferedEnd + this.timeCorrection_,
-                                        this.expired_);
-    }
-
-    if (mediaIndex < 0 || mediaIndex === playlist.segments.length) {
-      return null;
+                                        currentBufferedEnd + timeCorrection,
+                                        expired);
     }
 
-    segment = playlist.segments[mediaIndex];
-    return {
-      // resolve the segment URL relative to the playlist
-      uri: segment.resolvedUri,
-      // the segment's mediaIndex at the time it was requested
-      mediaIndex,
-      // the segment's playlist
-      playlist,
-      // unencrypted bytes of the segment
-      bytes: null,
-      // when a key is defined for this segment, the encrypted bytes
-      encryptedBytes: null,
-      // the state of the buffer before a segment is appended will be
-      // stored here so that the actual segment duration can be
-      // determined after it has been appended
-      buffered: null,
-      // The target timestampOffset for this segment when we append it
-      // to the source buffer
-      timestampOffset: NaN,
-      // The timeline that the segment is in
-      timeline: segment.timeline,
-      // The expected duration of the segment in seconds
-      duration: segment.duration
-    };
+    return mediaIndex;
   }
 
   /**
@@ -480,36 +453,81 @@ export default class SegmentLoader extends videojs.EventTarget {
       return;
     }
 
+    let buffered = this.sourceUpdater_.buffered();
+    let playlist = this.playlist_;
+    let currentTime = this.currentTime_();
+    let hasPlayed = this.hasPlayed_();
+    let expired = this.expired_;
+    let timeCorrection = this.timeCorrection_;
+
     // see if we need to begin loading immediately
-    let segmentInfo = this.checkBuffer_(this.sourceUpdater_.buffered(),
-                                        this.playlist_,
-                                        this.currentTime_());
+    let requestIndex = this.checkBuffer_(buffered,
+                                         playlist,
+                                         currentTime,
+                                         hasPlayed,
+                                         expired,
+                                         timeCorrection);
+
+    if (requestIndex === null) {
+      return;
+    }
 
-    if (!segmentInfo) {
+    let isEndOfStream = detectEndOfStream(playlist,
+                                          this.mediaSource_,
+                                          requestIndex);
+
+    if (isEndOfStream) {
+      this.mediaSource_.endOfStream();
       return;
     }
 
-    if (segmentInfo.mediaIndex === this.playlist_.segments.length - 1 &&
+    if (requestIndex === playlist.segments.length - 1 &&
         this.mediaSource_.readyState === 'ended' &&
         !this.seeking_()) {
       return;
     }
 
-    let segment = this.playlist_.segments[segmentInfo.mediaIndex];
-    let startOfSegment = duration(this.playlist_,
-                                  this.playlist_.mediaSequence + segmentInfo.mediaIndex,
-                                  this.expired_);
+    if (requestIndex < 0 || requestIndex >= playlist.segments.length) {
+      return;
+    }
+
+    let segment = this.playlist_.segments[requestIndex];
+
+    let request = {
+      // resolve the segment URL relative to the playlist
+      uri: segment.resolvedUri,
+      // the segment's mediaIndex at the time it was requested
+      mediaIndex: requestIndex,
+      // the segment's playlist
+      playlist,
+      // unencrypted bytes of the segment
+      bytes: null,
+      // when a key is defined for this segment, the encrypted bytes
+      encryptedBytes: null,
+      // the state of the buffer before a segment is appended will be
+      // stored here so that the actual segment duration can be
+      // determined after it has been appended
+      buffered: null,
+      // The target timestampOffset for this segment when we append it
+      // to the source buffer
+      timestampOffset: NaN,
+      // The timeline that the segment is in
+      timeline: segment.timeline,
+      // The expected duration of the segment in seconds
+      duration: segment.duration
+    };
+
+    let startOfSegment = duration(playlist,
+                                  playlist.mediaSequence + request.mediaIndex,
+                                  expired);
 
-    // We will need to change timestampOffset of the sourceBuffer if either of
-    // the following conditions are true:
-    // - The segment.timeline !== this.currentTimeline
-    //   (we are crossing a discontinuity)
+    //   (we are crossing a discontinuity somehow)
     // - The "timestampOffset" for the start of this segment is less than
     //   the currently set timestampOffset
-    segmentInfo.timestampOffset = this.sourceUpdater_.timestampOffset();
+    request.timestampOffset = this.sourceUpdater_.timestampOffset();
     if (segment.timeline !== this.currentTimeline_ ||
         startOfSegment < this.sourceUpdater_.timestampOffset()) {
-      segmentInfo.timestampOffset = startOfSegment;
+      request.timestampOffset = startOfSegment;
     }
 
     // Sanity check the segment-index determining logic by calcuating the
@@ -518,15 +536,15 @@ export default class SegmentLoader extends videojs.EventTarget {
     // any way
     let percentBuffered = Ranges.getSegmentBufferedPercent(startOfSegment,
                                                           segment.duration,
-                                                          this.currentTime_(),
-                                                          this.sourceUpdater_.buffered());
+                                                          currentTime,
+                                                          buffered);
 
     if (percentBuffered >= 90) {
       // Increment the timeCorrection_ variable to push the fetcher forward
       // in time and hopefully skip any gaps or flaws in our understanding
       // of the media
       let correctionApplied =
-        this.incrementTimeCorrection_(this.playlist_.targetDuration / 2, 1);
+        this.incrementTimeCorrection_(playlist.targetDuration / 2, 1);
 
       if (correctionApplied && !this.paused()) {
         this.fillBuffer_();
@@ -535,7 +553,7 @@ export default class SegmentLoader extends videojs.EventTarget {
       return;
     }
 
-    this.loadSegment_(segmentInfo);
+    this.loadSegment_(request);
   }
 
   /**
@@ -916,8 +934,7 @@ export default class SegmentLoader extends videojs.EventTarget {
     // fire if playback reaches that point.
     let isEndOfStream = detectEndOfStream(segmentInfo.playlist,
                                           this.mediaSource_,
-                                          currentMediaIndex,
-                                          currentBuffered);
+                                          currentMediaIndex + 1);
 
     if (isEndOfStream) {
       this.mediaSource_.endOfStream();

test/segment-loader.test.js

@@ -736,6 +736,7 @@ QUnit.test('fires ended at the end of a playlist', function() {
     sourceBuffers: mediaSource.sourceBuffers,
     endOfStream() {
       endOfStreams++;
+      this.readyState = 'ended';
     }
   };
 
@@ -1072,9 +1073,14 @@ QUnit.module('Segment Loading Calculation', {
 QUnit.test('requests the first segment with an empty buffer', function() {
   loader.mimeType(this.mimeType);
 
-  let segmentInfo = loader.checkBuffer_(videojs.createTimeRanges(),
-                                        playlistWithDuration(20),
+  let playlist = playlistWithDuration(20);
+  let segmentIndex = loader.checkBuffer_(videojs.createTimeRanges(),
+                                        playlist,
+                                        0,
+                                        false,
+                                        0,
                                         0);
+  let segmentInfo = playlist.segments[segmentIndex];
 
   QUnit.ok(segmentInfo, 'generated a request');
   QUnit.equal(segmentInfo.uri, '0.ts', 'requested the first segment');
@@ -1109,6 +1115,7 @@ QUnit.test('does not download the next segment if the buffer is full', function(
 QUnit.test('downloads the next segment if the buffer is getting low', function() {
   let buffered;
   let segmentInfo;
+  let segmentIndex;
   let playlist = playlistWithDuration(30);
 
   loader.mimeType(this.mimeType);
@@ -1116,7 +1123,8 @@ QUnit.test('downloads the next segment if the buffer is getting low', function()
 
   playlist.segments[1].end = 19.999;
   buffered = videojs.createTimeRanges([[0, 19.999]]);
-  segmentInfo = loader.checkBuffer_(buffered, playlist, 15);
+  segmentIndex = loader.checkBuffer_(buffered, playlist, 15, true, 0, 0);
+  segmentInfo = playlist.segments[segmentIndex];
 
   QUnit.ok(segmentInfo, 'made a request');
   QUnit.equal(segmentInfo.uri, '2.ts', 'requested the third segment');
@@ -1125,16 +1133,20 @@ QUnit.test('downloads the next segment if the buffer is getting low', function()
 QUnit.test('buffers based on the correct TimeRange if multiple ranges exist', function() {
   let buffered;
   let segmentInfo;
+  let segmentIndex;
+  let playlist = playlistWithDuration(40);
 
   loader.mimeType(this.mimeType);
 
   buffered = videojs.createTimeRanges([[0, 10], [20, 30]]);
-  segmentInfo = loader.checkBuffer_(buffered, playlistWithDuration(40), 8);
+  segmentIndex = loader.checkBuffer_(buffered, playlist, 8, true, 0, 0);
+  segmentInfo = playlist.segments[segmentIndex];
 
   QUnit.ok(segmentInfo, 'made a request');
   QUnit.equal(segmentInfo.uri, '1.ts', 'requested the second segment');
 
-  segmentInfo = loader.checkBuffer_(buffered, playlistWithDuration(40), 20);
+  segmentIndex = loader.checkBuffer_(buffered, playlist, 20, true, 0, 0);
+  segmentInfo = playlist.segments[segmentIndex];
   QUnit.ok(segmentInfo, 'made a request');
   QUnit.equal(segmentInfo.uri, '3.ts', 'requested the fourth segment');
 });
@@ -1171,6 +1183,7 @@ QUnit.test('adjusts calculations based on expired time', function() {
   let buffered;
   let playlist;
   let segmentInfo;
+  let segmentIndex;
 
   loader.mimeType(this.mimeType);
 
@@ -1179,9 +1192,13 @@ QUnit.test('adjusts calculations based on expired time', function() {
 
   loader.expired(10);
 
-  segmentInfo = loader.checkBuffer_(buffered,
+  segmentIndex = loader.checkBuffer_(buffered,
                                     playlist,
-                                    40 - Config.GOAL_BUFFER_LENGTH);
+                                    40 - Config.GOAL_BUFFER_LENGTH,
+                                    true,
+                                    loader.expired_,
+                                    0);
+  segmentInfo = playlist.segments[segmentIndex];
 
   QUnit.ok(segmentInfo, 'fetched a segment');
   QUnit.equal(segmentInfo.uri, '2.ts', 'accounted for expired time');