fix: Append valid syncRequests, better sync request choice, less getM…

…ediaInfoForTime rounding (#1127)

src/playlist.js

@@ -5,8 +5,8 @@
  */
 import videojs from 'video.js';
 import window from 'global/window';
-import {TIME_FUDGE_FACTOR} from './ranges.js';
 import {isAudioCodec} from '@videojs/vhs-utils/es/codecs.js';
+import {TIME_FUDGE_FACTOR} from './ranges.js';
 
 const {createTimeRange} = videojs;
 
@@ -391,9 +391,10 @@ export const getMediaInfoForTime = function(
       for (let i = startIndex - 1; i >= 0; i--) {
         const segment = partsAndSegments[i];
 
-        time += (segment.duration + TIME_FUDGE_FACTOR);
+        time += segment.duration;
 
-        if (time > 0) {
+        // TODO: consider not using TIME_FUDGE_FACTOR at all here
+        if ((time + TIME_FUDGE_FACTOR) > 0) {
           return {
             mediaIndex: segment.segmentIndex,
             startTime: startTime - sumDurations(playlist, startIndex, segment.segmentIndex),
@@ -433,9 +434,10 @@ export const getMediaInfoForTime = function(
   for (let i = startIndex; i < partsAndSegments.length; i++) {
     const partSegment = partsAndSegments[i];
 
-    time -= partSegment.duration + TIME_FUDGE_FACTOR;
+    time -= partSegment.duration;
 
-    if (time < 0) {
+    // TODO: consider not using TIME_FUDGE_FACTOR at all here
+    if ((time - TIME_FUDGE_FACTOR) < 0) {
       return {
         mediaIndex: partSegment.segmentIndex,
         startTime: startTime + sumDurations(playlist, startIndex, partSegment.segmentIndex),

src/segment-loader.js

@@ -32,30 +32,33 @@ import {getKnownPartCount} from './playlist.js';
  * generate a syncPoint. This function returns a good candidate index
  * for that process.
  *
- * @param {Object} playlist - the playlist object to look for a
+ * @param {Array} segments - the segments array from a playlist.
  * @return {number} An index of a segment from the playlist to load
  */
-export const getSyncSegmentCandidate = function(currentTimeline, {segments = []} = {}) {
-  // if we don't currently have a real timeline yet.
-  if (currentTimeline === -1) {
-    return 0;
-  }
+export const getSyncSegmentCandidate = function(currentTimeline, segments, targetTime) {
+  segments = segments || [];
+  const timelineSegments = [];
+  let time = 0;
+
+  for (let i = 0; i < segments.length; i++) {
+    const segment = segments[i];
+
+    if (currentTimeline === segment.timeline) {
+      timelineSegments.push(i);
+      time += segment.duration;
 
-  const segmentIndexArray = segments.reduce((acc, s, i) => {
-    if (s.timeline === currentTimeline) {
-      acc.push(i);
+      if (time > targetTime) {
+        return i;
+      }
     }
-    return acc;
-  }, []);
+  }
 
-  if (segmentIndexArray.length) {
-    // TODO: why do we do this? Basically we choose index 0 if
-    // segmentIndexArray.length is 1 and index = 1 if segmentIndexArray.length
-    // is greater then 1
-    return segmentIndexArray[Math.min(segmentIndexArray.length - 1, 1)];
+  if (timelineSegments.length === 0) {
+    return 0;
   }
 
-  return Math.max(segments.length - 1, 0);
+  // default to the last timeline segment
+  return timelineSegments[timelineSegments.length - 1];
 };
 
 // In the event of a quota exceeded error, keep at least one second of back buffer. This
@@ -137,6 +140,7 @@ const segmentInfoString = (segmentInfo) => {
     startOfSegment,
     duration,
     segment,
+    part,
     playlist: {
       mediaSequence: seq,
       id,
@@ -155,15 +159,15 @@ const segmentInfoString = (segmentInfo) => {
   } else if (segmentInfo.isSyncRequest) {
     selection = 'getSyncSegmentCandidate (isSyncRequest)';
   }
-  const {start, end} = segment;
+
   const hasPartIndex = typeof partIndex === 'number';
   const name = segmentInfo.segment.uri ? 'segment' : 'pre-segment';
-  const totalParts = hasPartIndex ? getKnownPartCount({preloadSegment: segment}) - 1 : 0;
+  const zeroBasedPartCount = hasPartIndex ? getKnownPartCount({preloadSegment: segment}) - 1 : 0;
 
-  return `${name} [${index}/${segmentLen}]` +
-    (hasPartIndex ? ` part [${partIndex}/${totalParts}]` : '') +
-    ` mediaSequenceNumber [${seq}/${seq + segmentLen}]` +
-    ` start/end [${start} => ${end}]` +
+  return `${name} [${seq + index}/${seq + segmentLen}]` +
+    (hasPartIndex ? ` part [${partIndex}/${zeroBasedPartCount}]` : '') +
+    ` segment start/end [${segment.start} => ${segment.end}]` +
+    (hasPartIndex ? ` part start/end [${part.start} => ${part.end}]` : '') +
     ` startOfSegment [${startOfSegment}]` +
     ` duration [${duration}]` +
     ` timeline [${timeline}]` +
@@ -1381,7 +1385,7 @@ export default class SegmentLoader extends videojs.EventTarget {
     };
 
     if (next.isSyncRequest) {
-      next.mediaIndex = getSyncSegmentCandidate(this.currentTimeline_, this.playlist_);
+      next.mediaIndex = getSyncSegmentCandidate(this.currentTimeline_, segments, bufferedEnd);
     } else if (this.mediaIndex !== null) {
       const segment = segments[this.mediaIndex];
       const partIndex = typeof this.partIndex === 'number' ? this.partIndex : -1;
@@ -2030,6 +2034,31 @@ export default class SegmentLoader extends videojs.EventTarget {
     // as we use the start of the segment to offset the best guess (playlist provided)
     // timestamp offset.
     this.updateSourceBufferTimestampOffset_(segmentInfo);
+
+    // if this is a sync request we need to determine whether it should
+    // be appended or not.
+    if (segmentInfo.isSyncRequest) {
+      // first save/update our timing info for this segment.
+      // this is what allows us to choose an accurate segment
+      // and the main reason we make a sync request.
+      this.updateTimingInfoEnd_(segmentInfo);
+      this.syncController_.saveSegmentTimingInfo({
+        segmentInfo,
+        shouldSaveTimelineMapping: this.loaderType_ === 'main'
+      });
+
+      const next = this.chooseNextRequest_();
+
+      // If the sync request isn't the segment that would be requested next
+      // after taking into account its timing info, do not append it.
+      if (next.mediaIndex !== segmentInfo.mediaIndex || next.partIndex !== segmentInfo.partIndex) {
+        this.logger_('sync segment was incorrect, not appending');
+        return;
+      }
+      // otherwise append it like any other segment as our guess was correct.
+      this.logger_('sync segment was correct, appending');
+    }
+
     // Save some state so that in the future anything waiting on first append (and/or
     // timestamp offset(s)) can process immediately. While the extra state isn't optimal,
     // we need some notion of whether the timestamp offset or other relevant information
@@ -2884,8 +2913,6 @@ export default class SegmentLoader extends videojs.EventTarget {
       });
     }
 
-    this.logger_(`Appended ${segmentInfoString(segmentInfo)}`);
-
     const segmentDurationMessage =
       getTroublesomeSegmentDurationMessage(segmentInfo, this.sourceType_);
 
@@ -2903,9 +2930,18 @@ export default class SegmentLoader extends videojs.EventTarget {
 
     if (segmentInfo.isSyncRequest) {
       this.trigger('syncinfoupdate');
-      return;
+      // if the sync request was not appended
+      // then it was not the correct segment.
+      // throw it away and use the data it gave us
+      // to get the correct one.
+      if (!segmentInfo.hasAppendedData_) {
+        this.logger_(`Throwing away un-appended sync request ${segmentInfoString(segmentInfo)}`);
+        return;
+      }
     }
 
+    this.logger_(`Appended ${segmentInfoString(segmentInfo)}`);
+
     this.addSegmentMetadataCue_(segmentInfo);
     this.fetchAtBuffer_ = true;
     if (this.currentTimeline_ !== segmentInfo.timeline) {

test/loader-common.js

@@ -870,7 +870,7 @@ export const LoaderCommonFactory = ({
       });
 
       QUnit.test('drops partIndex if playlist update drops parts', function(assert) {
-        assert.timeout(100000000000000000000);
+        loader.duration_ = () => Infinity;
         return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => {
           loader.playlist(playlistWithDuration(50, {
             mediaSequence: 0,
@@ -899,8 +899,8 @@ export const LoaderCommonFactory = ({
 
             assert.equal(
               this.requests[0].url,
-              '1.ts',
-              'requested mediaIndex 1 only'
+              '0.ts',
+              'requested mediaIndex 0 only'
             );
           });
       });

test/playlist-loader.test.js

@@ -2143,6 +2143,7 @@ QUnit.module('Playlist Loader', function(hooks) {
     this.requests.shift().respond(
       200, null,
       '#EXTM3U\n' +
+      '#EXT-X-PART-INF:PART-TARGET=1\n' +
       '#EXT-X-MEDIA-SEQUENCE:0\n' +
       '#EXTINF:2\n' +
       'low-1.ts\n' +
@@ -2163,6 +2164,7 @@ QUnit.module('Playlist Loader', function(hooks) {
     this.requests.shift().respond(
       200, null,
       '#EXTM3U\n' +
+      '#EXT-X-PART-INF:PART-TARGET=1\n' +
       '#EXT-X-MEDIA-SEQUENCE:0\n' +
       '#EXTINF:2\n' +
       'low-1.ts\n' +
@@ -2182,6 +2184,7 @@ QUnit.module('Playlist Loader', function(hooks) {
     this.requests.shift().respond(
       200, null,
       '#EXTM3U\n' +
+      '#EXT-X-PART-INF:PART-TARGET=1\n' +
       '#EXT-X-MEDIA-SEQUENCE:0\n' +
       '#EXTINF:2\n' +
       'segment0.ts\n' +
@@ -2208,6 +2211,7 @@ QUnit.module('Playlist Loader', function(hooks) {
     this.requests.shift().respond(
       200, null,
       '#EXTM3U\n' +
+      '#EXT-X-PART-INF:PART-TARGET=1\n' +
       '#EXT-X-MEDIA-SEQUENCE:0\n' +
       '#EXT-X-SERVER-CONTROL:CAN-SKIP-UNTIL=3\n' +
       '#EXTINF:2\n' +
@@ -2242,6 +2246,7 @@ QUnit.module('Playlist Loader', function(hooks) {
     this.requests.shift().respond(
       200, null,
       '#EXTM3U\n' +
+      '#EXT-X-PART-INF:PART-TARGET=1\n' +
       '#EXT-X-MEDIA-SEQUENCE:0\n' +
       '#EXT-X-SERVER-CONTROL:CAN-SKIP-UNTIL=3,CAN-SKIP-DATERANGES=YES\n' +
       '#EXTINF:2\n' +
@@ -2276,6 +2281,7 @@ QUnit.module('Playlist Loader', function(hooks) {
     this.requests.shift().respond(
       200, null,
       '#EXTM3U\n' +
+      '#EXT-X-PART-INF:PART-TARGET=1\n' +
       '#EXT-X-MEDIA-SEQUENCE:0\n' +
       '#EXT-X-SERVER-CONTROL:CAN-BLOCK-RELOAD=YES\n' +
       '#EXTINF:2\n' +
@@ -2309,6 +2315,7 @@ QUnit.module('Playlist Loader', function(hooks) {
     this.requests.shift().respond(
       200, null,
       '#EXTM3U\n' +
+      '#EXT-X-PART-INF:PART-TARGET=1\n' +
       '#EXT-X-MEDIA-SEQUENCE:0\n' +
       '#EXT-X-SERVER-CONTROL:CAN-BLOCK-RELOAD=YES\n' +
       '#EXTINF:2\n' +
@@ -2341,6 +2348,7 @@ QUnit.module('Playlist Loader', function(hooks) {
     this.requests.shift().respond(
       200, null,
       '#EXTM3U\n' +
+      '#EXT-X-PART-INF:PART-TARGET=1\n' +
       '#EXT-X-MEDIA-SEQUENCE:0\n' +
       '#EXT-X-SERVER-CONTROL:CAN-BLOCK-RELOAD=YES\n' +
       '#EXTINF:2\n' +
@@ -2373,6 +2381,7 @@ QUnit.module('Playlist Loader', function(hooks) {
     this.requests.shift().respond(
       200, null,
       '#EXTM3U\n' +
+      '#EXT-X-PART-INF:PART-TARGET=1\n' +
       '#EXT-X-MEDIA-SEQUENCE:0\n' +
       '#EXT-X-SERVER-CONTROL:CAN-BLOCK-RELOAD=YES\n' +
       '#EXTINF:2\n' +
@@ -2407,6 +2416,7 @@ QUnit.module('Playlist Loader', function(hooks) {
     this.requests.shift().respond(
       200, null,
       '#EXTM3U\n' +
+      '#EXT-X-PART-INF:PART-TARGET=1\n' +
       '#EXT-X-MEDIA-SEQUENCE:0\n' +
       '#EXT-X-SERVER-CONTROL:CAN-BLOCK-RELOAD=YES,CAN-SKIP-UNTIL=3\n' +
       '#EXTINF:2\n' +