fix: more accurate segment choices and logging

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;
 
@@ -374,9 +374,12 @@ 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: We should consider not using TIME_FUDGE_FACTOR at all here, but at
+        // the time where this change was made there wasn't enough time to test without it.
+        // We should make some time to see if we can remove TIME_FUDGE_FACTOR here.
+        if ((time + TIME_FUDGE_FACTOR) > 0) {
           return {
             mediaIndex: segment.segmentIndex,
             startTime: startTime - sumDurations(playlist, startIndex, segment.segmentIndex),
@@ -416,9 +419,12 @@ 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: We should consider not using TIME_FUDGE_FACTOR at all here, but at
+    // the time where this change was made there wasn't enough time to test without it.
+    // We should make some time to see if we can remove TIME_FUDGE_FACTOR here.
+    if ((time - TIME_FUDGE_FACTOR) < 0) {
       return {
         mediaIndex: partSegment.segmentIndex,
         startTime: startTime + sumDurations(playlist, startIndex, partSegment.segmentIndex),

src/segment-loader.js

@@ -31,30 +31,33 @@ import {lastBufferedEnd} from './ranges.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
@@ -135,11 +138,8 @@ const segmentInfoString = (segmentInfo) => {
   const {
     startOfSegment,
     duration,
-    segment: {
-      start,
-      end,
-      parts
-    },
+    segment,
+    part,
     playlist: {
       mediaSequence: seq,
       id,
@@ -159,12 +159,18 @@ const segmentInfoString = (segmentInfo) => {
     selection = 'getSyncSegmentCandidate (isSyncRequest)';
   }
 
+  const hasPartIndex = typeof partIndex === 'number';
   const name = segmentInfo.segment.uri ? 'segment' : 'pre-segment';
-
-  return `${name} [${index}/${segmentLen}]` +
-    (partIndex ? ` part [${partIndex}/${parts.length - 1}]` : '') +
-    ` mediaSequenceNumber [${seq}/${seq + segmentLen}]` +
-    ` start/end [${start} => ${end}]` +
+  const partCount = segment.parts ? segment.parts.length : 0;
+  const preloadPartCount = segment.preloadHints ?
+    segment.preloadHints.filter((h) => h.type === 'PART').length :
+    0;
+  const zeroBasedPartCount = partCount + preloadPartCount - 1 - (preloadPartCount > 0 ? 1 : 0);
+
+  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}]` +
@@ -1382,7 +1388,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;
@@ -2031,6 +2037,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
@@ -2885,8 +2916,6 @@ export default class SegmentLoader extends videojs.EventTarget {
       });
     }
 
-    this.logger_(`Appended ${segmentInfoString(segmentInfo)}`);
-
     const segmentDurationMessage =
       getTroublesomeSegmentDurationMessage(segmentInfo, this.sourceType_);
 
@@ -2904,9 +2933,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/segment-loader.test.js

@@ -79,51 +79,38 @@ SegmentLoader.prototype.addSegmentMetadataCue_ = function() {};
 QUnit.module('SegmentLoader Isolated Functions');
 
 QUnit.test('getSyncSegmentCandidate works as expected', function(assert) {
-  assert.equal(getSyncSegmentCandidate(-1, {}), 0, '-1 timeline, try index 0');
-  assert.equal(getSyncSegmentCandidate(0, {}), 0, '0 timeline, no segments, try index 0');
-
-  assert.equal(
-    getSyncSegmentCandidate(0, {segments: [
-      {timeline: 0},
-      {timeline: 0}
-    ]}),
-    1,
-    '0 timeline, two timeline 0 segments, try index 1'
-  );
-
-  assert.equal(
-    getSyncSegmentCandidate(0, {segments: [
-      {timeline: 0},
-      {timeline: 0},
-      {timeline: 0},
-      {timeline: 0}
-    ]}),
-    1,
-    '0 timeline, four timeline 0 segments, try index 1'
-  );
-
-  assert.equal(
-    getSyncSegmentCandidate(0, {segments: [
-      {timeline: 0},
-      {timeline: 1},
-      {timeline: 1},
-      {timeline: 1}
-    ]}),
-    0,
-    '0 timeline, one timeline 0 segment, three timeline 1 segments, try index 0'
-  );
-
-  assert.equal(
-    getSyncSegmentCandidate(0, {segments: [
-      {timeline: 1},
-      {timeline: 1},
-      {timeline: 1},
-      {timeline: 1}
-    ]}),
-    3,
-    '0 timeline, four timeline 1 segments, try index 3'
-  );
-
+  let segments = [];
+
+  assert.equal(getSyncSegmentCandidate(-1, segments, 0), 0, '-1 timeline, no segments, 0 target');
+  assert.equal(getSyncSegmentCandidate(0, segments, 0), 0, '0 timeline, no segments, 0 target');
+
+  segments = [
+    {timeline: 0, duration: 4},
+    {timeline: 0, duration: 4},
+    {timeline: 0, duration: 4},
+    {timeline: 0, duration: 4}
+  ];
+
+  assert.equal(getSyncSegmentCandidate(-1, segments, 0), 0, '-1 timeline, 4x 0 segments, 0 target');
+  assert.equal(getSyncSegmentCandidate(0, segments, 1), 0, '0 timeline, 4x 0 segments, 1 target');
+  assert.equal(getSyncSegmentCandidate(0, segments, 4), 1, '0 timeline, 4x 0 segments, 4 target');
+  assert.equal(getSyncSegmentCandidate(-1, segments, 8), 0, '-1 timeline, 4x 0 segments, 8 target');
+  assert.equal(getSyncSegmentCandidate(0, segments, 8), 2, '0 timeline, 4x 0 segments, 8 target');
+  assert.equal(getSyncSegmentCandidate(0, segments, 20), 3, '0 timeline, 4x 0 segments, 20 target');
+
+  segments = [
+    {timeline: 1, duration: 4},
+    {timeline: 0, duration: 4},
+    {timeline: 1, duration: 4},
+    {timeline: 0, duration: 4},
+    {timeline: 2, duration: 4},
+    {timeline: 1, duration: 4},
+    {timeline: 0, duration: 4}
+  ];
+
+  assert.equal(getSyncSegmentCandidate(1, segments, 8), 5, '1 timeline, mixed timeline segments, 8 target');
+  assert.equal(getSyncSegmentCandidate(0, segments, 8), 6, '0 timeline, mixed timeline segments, 8 target');
+  assert.equal(getSyncSegmentCandidate(2, segments, 8), 4, '2 timeline, mixed timeline segments, 8 target');
 });
 
 QUnit.test('illegalMediaSwitch detects illegal media switches', function(assert) {
@@ -2967,6 +2954,69 @@ QUnit.module('SegmentLoader', function(hooks) {
       });
     });
 
+    QUnit.test('sync request can be thrown away', function(assert) {
+      const appends = [];
+      const logs = [];
+
+      return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_, {isVideoOnly: true}).then(() => {
+        const origAppendToSourceBuffer = loader.appendToSourceBuffer_.bind(loader);
+
+        // set the mediaSource duration as it is usually set by
+        // master playlist controller, which is not present here
+        loader.mediaSource_.duration = Infinity;
+
+        loader.appendToSourceBuffer_ = (config) => {
+          appends.push(config);
+          origAppendToSourceBuffer(config);
+        };
+
+        loader.playlist(playlistWithDuration(20));
+        loader.load();
+        this.clock.tick(1);
+        standardXHRResponse(this.requests.shift(), videoSegment());
+
+        return new Promise((resolve, reject) => {
+          loader.one('appended', resolve);
+          loader.one('error', reject);
+        });
+      }).then(() => {
+        this.clock.tick(1);
+
+        assert.equal(appends.length, 1, 'one append');
+        assert.equal(appends[0].type, 'video', 'appended to video buffer');
+        assert.ok(appends[0].initSegment, 'appended video init segment');
+
+        loader.playlist(playlistWithDuration(20, { uri: 'new-playlist.m3u8' }));
+        // remove old aborted request
+        this.requests.shift();
+        // get the new request
+        this.clock.tick(1);
+        loader.chooseNextRequest_ = () => ({partIndex: null, mediaIndex: 1});
+        loader.logger_ = (line) => {
+          logs.push(line);
+        };
+        standardXHRResponse(this.requests.shift(), videoSegment());
+
+        // since it's a sync request, wait for the syncinfoupdate event (we won't get the
+        // appended event)
+        return new Promise((resolve, reject) => {
+          loader.one('syncinfoupdate', resolve);
+          loader.one('error', reject);
+        });
+      }).then(() => {
+        this.clock.tick(1);
+        assert.equal(appends.length, 1, 'still only one append');
+        assert.true(
+          logs.some((l) => (/^sync segment was incorrect, not appending/).test(l)),
+          'has log line'
+        );
+        assert.true(
+          logs.some((l) => (/^Throwing away un-appended sync request segment/).test(l)),
+          'has log line'
+        );
+      });
+    });
+
     QUnit.test('re-appends init segments on discontinuity', function(assert) {
       const appends = [];