fix: only save timeline mappings for main loader (#839)

- fix: only save timeline mappings for main loader

Timeline mappings should only be saved for the main loader. This is for multiple
reasons:

1) Only one mapping is saved per timeline, meaning that if both the audio loader
   and the main loader try to save the timeline mapping, whichever comes later
   will overwrite the first. In theory this is OK, as the mappings should be the
   same, however, it breaks for (2)
2) In the event of a live stream, the initial live point will make for a somewhat
   arbitrary mapping. If audio and video streams are not perfectly in-sync, then
   the mapping will be off for one of the streams, dependent on which one was
   first saved (see (1)).
3) Primary timing goes by video in VHS, so the mapping should be video.

Since the audio loader will wait for the main loader to load the first segment,
the main loader will save the first timeline mapping, and ensure that there won't
be a case where audio loads two segments without saving a mapping (thus leading
to missing segment timing info).

- Add playback watcher check to correct cases where seek point ends up just before content.

If audio and video streams are not perfectly aligned, a seek can result in the buffer
starting just after the seek position. In that event, playback watcher should seek into
the buffered contents to resume playback.

src/playback-watcher.js

@@ -21,6 +21,45 @@ const timerCancelEvents = [
   'error'
 ];
 
+/**
+ * Returns whether or not the current time should be considered close to buffered content,
+ * taking into consideration whether there's enough buffered content for proper playback.
+ *
+ * @param {Object} options
+ *        Options object
+ * @param {TimeRange} options.buffered
+ *        Current buffer
+ * @param {number} options.targetDuration
+ *        The active playlist's target duration
+ * @param {number} options.currentTime
+ *        The current time of the player
+ * @return {boolean}
+ *         Whether the current time should be considered close to the buffer
+ */
+export const closeToBufferedContent = ({ buffered, targetDuration, currentTime }) => {
+  if (!buffered.length) {
+    return false;
+  }
+
+  // At least two to three segments worth of content should be buffered before there's a
+  // full enough buffer to consider taking any actions.
+  if (buffered.end(0) - buffered.start(0) < targetDuration * 2) {
+    return false;
+  }
+
+  // It's possible that, on seek, a remove hasn't completed and the buffered range is
+  // somewhere past the current time. In that event, don't consider the buffered content
+  // close.
+  if (currentTime > buffered.start(0)) {
+    return false;
+  }
+
+  // Since target duration generally represents the max (or close to max) duration of a
+  // segment, if the buffer is within a segment of the current time, the gap probably
+  // won't be closed, and current time should be considered close to buffered content.
+  return buffered.start(0) - currentTime < targetDuration;
+};
+
 /**
  * @class PlaybackWatcher
  */
@@ -198,6 +237,23 @@ export default class PlaybackWatcher {
       return true;
     }
 
+    const buffered = this.tech_.buffered();
+
+    if (
+      closeToBufferedContent({
+        buffered,
+        targetDuration: this.media().targetDuration,
+        currentTime
+      })
+    ) {
+      seekTo = buffered.start(0) + Ranges.SAFE_TIME_DELTA;
+      this.logger_(`Buffered region starts (${buffered.start(0)}) ` +
+                   ` just beyond seek point (${currentTime}). Seeking to ${seekTo}.`);
+
+      this.seekTo(seekTo);
+      return true;
+    }
+
     return false;
   }
 

src/segment-loader.js

@@ -2487,7 +2487,27 @@ export default class SegmentLoader extends videojs.EventTarget {
     // finished (and we have its end time).
     this.updateTimingInfoEnd_(segmentInfo);
     if (this.shouldSaveSegmentTimingInfo_) {
-      this.syncController_.saveSegmentTimingInfo(segmentInfo);
+      // Timeline mappings should only be saved for the main loader. This is for multiple
+      // reasons:
+      //
+      // 1) Only one mapping is saved per timeline, meaning that if both the audio loader
+      //    and the main loader try to save the timeline mapping, whichever comes later
+      //    will overwrite the first. In theory this is OK, as the mappings should be the
+      //    same, however, it breaks for (2)
+      // 2) In the event of a live stream, the initial live point will make for a somewhat
+      //    arbitrary mapping. If audio and video streams are not perfectly in-sync, then
+      //    the mapping will be off for one of the streams, dependent on which one was
+      //    first saved (see (1)).
+      // 3) Primary timing goes by video in VHS, so the mapping should be video.
+      //
+      // Since the audio loader will wait for the main loader to load the first segment,
+      // the main loader will save the first timeline mapping, and ensure that there won't
+      // be a case where audio loads two segments without saving a mapping (thus leading
+      // to missing segment timing info).
+      this.syncController_.saveSegmentTimingInfo({
+        segmentInfo,
+        shouldSaveTimelineMapping: this.loaderType_ === 'main'
+      });
     }
 
     this.logger_(segmentInfoString(segmentInfo));

src/sync-controller.js

@@ -367,8 +367,26 @@ export default class SyncController extends videojs.EventTarget {
     }
   }
 
-  saveSegmentTimingInfo(segmentInfo) {
-    if (this.calculateSegmentTimeMapping_(segmentInfo, segmentInfo.timingInfo)) {
+  /**
+   * Calculates and saves timeline mappings, playlist sync info, and segment timing values
+   * based on the latest timing information.
+   *
+   * @param {Object} options
+   *        Options object
+   * @param {SegmentInfo} options.segmentInfo
+   *        The current active request information
+   * @param {boolean} options.shouldSaveTimelineMapping
+   *        If there's a timeline change, determines if the timeline mapping should be
+   *        saved in timelines.
+   */
+  saveSegmentTimingInfo({ segmentInfo, shouldSaveTimelineMapping }) {
+    const didCalculateSegmentTimeMapping = this.calculateSegmentTimeMapping_(
+      segmentInfo,
+      segmentInfo.timingInfo,
+      shouldSaveTimelineMapping
+    );
+
+    if (didCalculateSegmentTimeMapping) {
       this.saveDiscontinuitySyncInfo_(segmentInfo);
 
       // If the playlist does not have sync information yet, record that information
@@ -405,10 +423,13 @@ export default class SyncController extends videojs.EventTarget {
    *        The current active request information
    * @param {Object} timingInfo
    *        The start and end time of the current segment in "media time"
+   * @param {boolean} shouldSaveTimelineMapping
+   *        If there's a timeline change, determines if the timeline mapping should be
+   *        saved in timelines.
    * @return {boolean}
    *          Returns false if segment time mapping could not be calculated
    */
-  calculateSegmentTimeMapping_(segmentInfo, timingInfo) {
+  calculateSegmentTimeMapping_(segmentInfo, timingInfo, shouldSaveTimelineMapping) {
     const segment = segmentInfo.segment;
     let mappingObj = this.timelines[segmentInfo.timeline];
 
@@ -417,11 +438,13 @@ export default class SyncController extends videojs.EventTarget {
         time: segmentInfo.startOfSegment,
         mapping: segmentInfo.startOfSegment - timingInfo.start
       };
-      this.timelines[segmentInfo.timeline] = mappingObj;
-      this.trigger('timestampoffset');
+      if (shouldSaveTimelineMapping) {
+        this.timelines[segmentInfo.timeline] = mappingObj;
+        this.trigger('timestampoffset');
 
-      this.logger_(`time mapping for timeline ${segmentInfo.timeline}: ` +
-        `[time: ${mappingObj.time}] [mapping: ${mappingObj.mapping}]`);
+        this.logger_(`time mapping for timeline ${segmentInfo.timeline}: ` +
+          `[time: ${mappingObj.time}] [mapping: ${mappingObj.mapping}]`);
+      }
 
       segment.start = segmentInfo.startOfSegment;
       segment.end = timingInfo.end + mappingObj.mapping;

test/playback-watcher.test.js

@@ -7,7 +7,10 @@ import {
   openMediaSource,
   standardXHRResponse
 } from './test-helpers.js';
-import PlaybackWatcher from '../src/playback-watcher';
+import {
+  default as PlaybackWatcher,
+  closeToBufferedContent
+} from '../src/playback-watcher';
 // needed for plugin registration
 import '../src/videojs-http-streaming';
 
@@ -475,7 +478,8 @@ QUnit.test('fixes bad seeks', function(assert) {
   playbackWatcher.tech_ = {
     off: () => {},
     seeking: () => seeking,
-    currentTime: () => currentTime
+    currentTime: () => currentTime,
+    buffered: () => videojs.createTimeRanges()
   };
   this.player.vhs.setCurrentTime = (time) => seeks.push(time);
 
@@ -674,6 +678,70 @@ QUnit.test('calls fixesBadSeeks_ on seekablechanged', function(assert) {
   assert.equal(fixesBadSeeks_, 1, 'fixesBadSeeks_ was called');
 });
 
+QUnit.test('jumps to buffered content if seeking just before', function(assert) {
+  // target duration is 10 for this manifest
+  this.player.src({
+    src: 'liveStart30sBefore.m3u8',
+    type: 'application/vnd.apple.mpegurl'
+  });
+
+  // start playback normally
+  this.player.tech_.triggerReady();
+  this.clock.tick(1);
+  standardXHRResponse(this.requests.shift());
+  openMediaSource(this.player, this.clock);
+  this.player.tech_.trigger('play');
+  this.player.tech_.trigger('playing');
+  this.clock.tick(1);
+
+  const playbackWatcher = this.player.tech_.hls.playbackWatcher_;
+  const seeks = [];
+  let currentTime;
+  let buffered;
+
+  playbackWatcher.seekable = () => videojs.createTimeRanges([[10, 100]]);
+  playbackWatcher.tech_ = {
+    off: () => {},
+    seeking: () => true,
+    currentTime: () => currentTime,
+    buffered: () => buffered
+  };
+  this.player.vhs.setCurrentTime = (time) => seeks.push(time);
+
+  currentTime = 10;
+  // target duration is 10
+  buffered = videojs.createTimeRanges([[20, 39]]);
+  assert.notOk(playbackWatcher.fixesBadSeeks_(), 'does nothing when too far from buffer');
+  assert.equal(seeks.length, 0, 'did not seek');
+
+  buffered = videojs.createTimeRanges([[19, 38.9]]);
+  assert.notOk(playbackWatcher.fixesBadSeeks_(), 'does nothing when not enough buffer');
+  assert.equal(seeks.length, 0, 'did not seek');
+
+  buffered = videojs.createTimeRanges([[19, 39]]);
+  assert.ok(
+    playbackWatcher.fixesBadSeeks_(),
+    'acts when close enough to, and enough, buffer'
+  );
+  assert.equal(seeks.length, 1, 'seeked');
+  assert.equal(seeks[0], 19.1, 'player seeked to start of buffer');
+
+  currentTime = 20;
+  assert.notOk(
+    playbackWatcher.fixesBadSeeks_(),
+    'does nothing when current time after buffer start'
+  );
+  assert.equal(seeks.length, 1, 'did not seek');
+
+  // defers to fixing the bad seek over seeking into the buffer when seeking outside of
+  // seekable range
+  currentTime = 10;
+  playbackWatcher.seekable = () => videojs.createTimeRanges([[11, 100]]);
+  assert.ok(playbackWatcher.fixesBadSeeks_(), 'fixed bad seek');
+  assert.equal(seeks.length, 2, 'seeked');
+  assert.equal(seeks[1], 11.1, 'seeked to seekable range');
+});
+
 QUnit.module('PlaybackWatcher isolated functions', {
   beforeEach() {
     monitorCurrentTime_ = PlaybackWatcher.prototype.monitorCurrentTime_;
@@ -977,3 +1045,60 @@ QUnit.test('respects allowSeeksWithinUnsafeLiveWindow flag', function(assert) {
     'false if within seekable range'
   );
 });
+
+QUnit.module('closeToBufferedContent');
+
+QUnit.test('false if no buffer', function(assert) {
+  assert.notOk(
+    closeToBufferedContent({
+      buffered: videojs.createTimeRanges(),
+      targetDuration: 4,
+      currentTime: 10
+    }),
+    'returned false'
+  );
+});
+
+QUnit.test('false if buffer less than two times target duration', function(assert) {
+  assert.notOk(
+    closeToBufferedContent({
+      buffered: videojs.createTimeRanges([[11, 18.9]]),
+      targetDuration: 4,
+      currentTime: 10
+    }),
+    'returned false'
+  );
+});
+
+QUnit.test('false if buffer is beyond target duration from current time', function(assert) {
+  assert.notOk(
+    closeToBufferedContent({
+      buffered: videojs.createTimeRanges([[14.1, 30]]),
+      targetDuration: 4,
+      currentTime: 10
+    }),
+    'returned false'
+  );
+});
+
+QUnit.test('true if enough buffer and close to current time', function(assert) {
+  assert.ok(
+    closeToBufferedContent({
+      buffered: videojs.createTimeRanges([[13.9, 22]]),
+      targetDuration: 4,
+      currentTime: 10
+    }),
+    'returned true'
+  );
+});
+
+QUnit.test('false if current time beyond buffer start', function(assert) {
+  assert.notOk(
+    closeToBufferedContent({
+      buffered: videojs.createTimeRanges([[13.9, 22]]),
+      targetDuration: 4,
+      currentTime: 14
+    }),
+    'returned false'
+  );
+});

test/segment-loader.test.js

@@ -1426,9 +1426,12 @@ QUnit.module('SegmentLoader', function(hooks) {
       const origSaveSegmentTimingInfo =
         syncController.saveSegmentTimingInfo.bind(syncController);
 
-      syncController.saveSegmentTimingInfo = (segmentInfo) => {
+      syncController.saveSegmentTimingInfo = ({
+        segmentInfo,
+        shouldSaveTimelineMapping
+      }) => {
         saveSegmentTimingInfoCalls++;
-        origSaveSegmentTimingInfo(segmentInfo);
+        origSaveSegmentTimingInfo({ segmentInfo, shouldSaveTimelineMapping });
       };
 
       return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => {
@@ -1448,6 +1451,55 @@ QUnit.module('SegmentLoader', function(hooks) {
       });
     });
 
+    QUnit.test('main loader saves timeline mapping', function(assert) {
+      const syncController = loader.syncController_;
+
+      return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => {
+        loader.playlist(playlistWithDuration(20));
+        loader.load();
+        this.clock.tick(1);
+        standardXHRResponse(this.requests.shift(), muxedSegment());
+
+        assert.notOk(syncController.mappingForTimeline(0), 'no mapping for timeline 0');
+
+        return new Promise((resolve, reject) => {
+          loader.one('appended', resolve);
+          loader.one('error', reject);
+        });
+      }).then(() => {
+        assert.ok(syncController.mappingForTimeline(0), 'saved mapping for timeline 0');
+      });
+    });
+
+    QUnit.test('audio loader doesn\'t save timeline mapping', function(assert) {
+      loader.dispose();
+      loader = new SegmentLoader(LoaderCommonSettings.call(this, {
+        loaderType: 'audio'
+      }), {});
+
+      // Fake the last timeline change for main so audio loader has enough info to append
+      // the first segment.
+      this.timelineChangeController.lastTimelineChange({ type: 'main', from: -1, to: 0 });
+
+      const syncController = loader.syncController_;
+
+      return setupMediaSource(loader.mediaSource_, loader.sourceUpdater_).then(() => {
+        loader.playlist(playlistWithDuration(20));
+        loader.load();
+        this.clock.tick(1);
+        standardXHRResponse(this.requests.shift(), audioSegment());
+
+        assert.notOk(syncController.mappingForTimeline(0), 'no mapping for timeline 0');
+
+        return new Promise((resolve, reject) => {
+          loader.one('appended', resolve);
+          loader.one('error', reject);
+        });
+      }).then(() => {
+        assert.notOk(syncController.mappingForTimeline(0), 'no mapping for timeline 0');
+      });
+    });
+
     QUnit.test('tracks segment end times as they are buffered', function(assert) {
       const playlist = playlistWithDuration(20);
 

test/sync-controller.test.js

@@ -286,7 +286,7 @@ QUnit.test('saves segment timing info', function(assert) {
   };
 
   updateTimingInfo(segmentInfo);
-  syncCon.saveSegmentTimingInfo(segmentInfo);
+  syncCon.saveSegmentTimingInfo({ segmentInfo, shouldSaveTimelineMapping: true });
   assert.ok(syncCon.timelines[0], 'created mapping object for timeline 0');
   assert.deepEqual(
     syncCon.timelines[0], { time: 0, mapping: -5 },
@@ -307,7 +307,7 @@ QUnit.test('saves segment timing info', function(assert) {
   segmentInfo.segment = segment;
 
   updateTimingInfo(segmentInfo);
-  syncCon.saveSegmentTimingInfo(segmentInfo);
+  syncCon.saveSegmentTimingInfo({ segmentInfo, shouldSaveTimelineMapping: true });
   assert.equal(segment.start, 10, 'correctly calculated segment start');
   assert.equal(segment.end, 20, 'correctly calculated segment end');
   assert.deepEqual(
@@ -323,7 +323,7 @@ QUnit.test('saves segment timing info', function(assert) {
   segmentInfo.segment = segment;
 
   updateTimingInfo(segmentInfo);
-  syncCon.saveSegmentTimingInfo(segmentInfo);
+  syncCon.saveSegmentTimingInfo({ segmentInfo, shouldSaveTimelineMapping: true });
   assert.ok(syncCon.timelines[1], 'created mapping object for timeline 1');
   assert.deepEqual(
     syncCon.timelines[1], { time: 30, mapping: -11 },

test/vtt-segment-loader.test.js

@@ -779,9 +779,12 @@ QUnit.module('VTTSegmentLoader', function(hooks) {
       const origSaveSegmentTimingInfo =
         syncController.saveSegmentTimingInfo.bind(syncController);
 
-      syncController.saveSegmentTimingInfo = (segmentInfo) => {
+      syncController.saveSegmentTimingInfo = ({
+        segmentInfo,
+        shouldSaveTimelineMapping
+      }) => {
         saveSegmentTimingInfoCalls++;
-        origSaveSegmentTimingInfo(segmentInfo);
+        origSaveSegmentTimingInfo({ segmentInfo, shouldSaveTimelineMapping });
       };
 
       loader.playlist(playlist);