fix: multi-period DASH VOD fixes (#1551)

* chore: clear previous dash media request timeout everytime we clear or update it * chore: call fast-quality-switch only when we enable playlist from quality selector * chore: add isPaused for dash playlist loader to mitigate duplicate playlist trigger for the main segment loader * chore: fix fastQualityChange_ tests * chore: add fast quality change debounce * chore: add debounce tick to the tests * chore: restart all loaders when we hit fix bad timeline change * chore: update segment loader * chore: update run-fast-quality-switch and fix bad timeline changes * fix test * chore: fix lint issues --------- Co-authored-by: Dzianis Dashkevich <[email protected]>
videojs · Nov 21, 2024 · 6fe7d9c · 6fe7d9c
1 parent 2f8d0af
commit 6fe7d9c
Show file tree

Hide file tree

Showing 9 changed files with 117 additions and 77 deletions.
diff --git a/src/dash-playlist-loader.js b/src/dash-playlist-loader.js
@@ -304,6 +304,8 @@ export default class DashPlaylistLoader extends EventTarget {
   constructor(srcUrlOrPlaylist, vhs, options = { }, mainPlaylistLoader) {
     super();
 
+    this.isPaused_ = true;
+
     this.mainPlaylistLoader_ = mainPlaylistLoader || this;
     if (!mainPlaylistLoader) {
       this.isMain_ = true;
@@ -345,6 +347,10 @@ export default class DashPlaylistLoader extends EventTarget {
     }
   }
 
+  get isPaused() {
+    return this.isPaused_;
+  }
+
   requestErrored_(err, request, startingState) {
     // disposed
     if (!this.request) {
@@ -384,6 +390,7 @@ export default class DashPlaylistLoader extends EventTarget {
     // playlist lacks sidx or sidx segments were added to this playlist already.
     if (!playlist.sidx || !sidxKey || this.mainPlaylistLoader_.sidxMapping_[sidxKey]) {
       // keep this function async
+      window.clearTimeout(this.mediaRequest_);
       this.mediaRequest_ = window.setTimeout(() => cb(false), 0);
       return;
     }
@@ -465,6 +472,7 @@ export default class DashPlaylistLoader extends EventTarget {
   }
 
   dispose() {
+    this.isPaused_ = true;
     this.trigger('dispose');
     this.stopRequest();
     this.loadedPlaylists_ = {};
@@ -553,6 +561,7 @@ export default class DashPlaylistLoader extends EventTarget {
   haveMetadata({startingState, playlist}) {
     this.state = 'HAVE_METADATA';
     this.loadedPlaylists_[playlist.id] = playlist;
+    window.clearTimeout(this.mediaRequest_);
     this.mediaRequest_ = null;
 
     // This will trigger loadedplaylist
@@ -569,6 +578,8 @@ export default class DashPlaylistLoader extends EventTarget {
   }
 
   pause() {
+    this.isPaused_ = true;
+
     if (this.mainPlaylistLoader_.createMupOnMedia_) {
       this.off('loadedmetadata', this.mainPlaylistLoader_.createMupOnMedia_);
       this.mainPlaylistLoader_.createMupOnMedia_ = null;
@@ -588,6 +599,8 @@ export default class DashPlaylistLoader extends EventTarget {
   }
 
   load(isFinalRendition) {
+    this.isPaused_ = false;
+
     window.clearTimeout(this.mediaUpdateTimeout);
     this.mediaUpdateTimeout = null;
 
@@ -629,6 +642,7 @@ export default class DashPlaylistLoader extends EventTarget {
     // We don't need to request the main manifest again
     // Call this asynchronously to match the xhr request behavior below
     if (!this.isMain_) {
+      window.clearTimeout(this.mediaRequest_);
       this.mediaRequest_ = window.setTimeout(() => this.haveMain_(), 0);
       return;
     }
@@ -773,6 +787,7 @@ export default class DashPlaylistLoader extends EventTarget {
 
   handleMain_() {
     // clear media request
+    window.clearTimeout(this.mediaRequest_);
     this.mediaRequest_ = null;
 
     const oldMain = this.mainPlaylistLoader_.main;

diff --git a/src/playlist-controller.js b/src/playlist-controller.js
@@ -29,6 +29,7 @@ import {merge, createTimeRanges} from './util/vjs-compat';
 import { addMetadata, createMetadataTrackIfNotExists, addDateRangeMetadata } from './util/text-tracks';
 import ContentSteeringController from './content-steering-controller';
 import { bufferToHexString } from './util/string.js';
+import {debounce} from './util/fn';
 
 const ABORT_EARLY_EXCLUSION_SECONDS = 10;
 
@@ -152,6 +153,11 @@ export class PlaylistController extends videojs.EventTarget {
   constructor(options) {
     super();
 
+    // Adding a slight debounce to avoid duplicate calls during rapid quality changes, for example:
+    // When selecting quality from the quality list,
+    // where we may have multiple bandwidth profiles for the same vertical resolution.
+    this.fastQualityChange_ = debounce(this.fastQualityChange_.bind(this), 100);
+
     const {
       src,
       withCredentials,
@@ -701,7 +707,16 @@ export class PlaylistController extends videojs.EventTarget {
 
       if (this.sourceType_ === 'dash') {
         // we don't want to re-request the same hls playlist right after it was changed
-        this.mainPlaylistLoader_.load();
+
+        // Initially it was implemented as workaround to restart playlist loader for live
+        // when playlist loader is paused because of playlist exclusions:
+        // see: https://github.com/videojs/http-streaming/pull/1339
+        // but this introduces duplicate "loadedplaylist" event.
+        // Ideally we want to re-think playlist loader life-cycle events,
+        // but simply checking "paused" state should help a lot
+        if (this.mainPlaylistLoader_.isPaused) {
+          this.mainPlaylistLoader_.load();
+        }
       }
 
       // TODO: Create a new event on the PlaylistLoader that signals
@@ -962,6 +977,24 @@ export class PlaylistController extends videojs.EventTarget {
       this.tech_.setCurrentTime(newTime);
     });
 
+    this.timelineChangeController_.on('fixBadTimelineChange', () => {
+      // pause, reset-everything and load for all segment-loaders
+      this.logger_('Fix bad timeline change. Restarting al segment loaders...');
+      this.mainSegmentLoader_.pause();
+      this.mainSegmentLoader_.resetEverything();
+      if (this.mediaTypes_.AUDIO.activePlaylistLoader) {
+        this.audioSegmentLoader_.pause();
+        this.audioSegmentLoader_.resetEverything();
+      }
+      if (this.mediaTypes_.SUBTITLES.activePlaylistLoader) {
+        this.subtitleSegmentLoader_.pause();
+        this.subtitleSegmentLoader_.resetEverything();
+      }
+
+      // start segment loader loading in case they are paused
+      this.load();
+    });
+
     this.mainSegmentLoader_.on('earlyabort', (event) => {
       // never try to early abort with the new ABR algorithm
       if (this.bufferBasedABR) {
@@ -1109,13 +1142,19 @@ export class PlaylistController extends videojs.EventTarget {
 
   runFastQualitySwitch_() {
     this.waitingForFastQualityPlaylistReceived_ = false;
-    // Delete all buffered data to allow an immediate quality switch.
     this.mainSegmentLoader_.pause();
-    this.mainSegmentLoader_.resetEverything(() => {
-      this.mainSegmentLoader_.load();
-    });
+    this.mainSegmentLoader_.resetEverything();
+    if (this.mediaTypes_.AUDIO.activePlaylistLoader) {
+      this.audioSegmentLoader_.pause();
+      this.audioSegmentLoader_.resetEverything();
+    }
+    if (this.mediaTypes_.SUBTITLES.activePlaylistLoader) {
+      this.subtitleSegmentLoader_.pause();
+      this.subtitleSegmentLoader_.resetEverything();
+    }
 
-    // don't need to reset audio as it is reset when media changes
+    // start segment loader loading in case they are paused
+    this.load();
   }
 
   /**

diff --git a/src/rendition-mixin.js b/src/rendition-mixin.js
@@ -39,8 +39,9 @@ const enableFunction = (loader, playlistID, changePlaylistFn) => (enable) => {
 
   if (enable !== currentlyEnabled && !incompatible) {
     // Ensure the outside world knows about our changes
-    changePlaylistFn(playlist);
     if (enable) {
+      // call fast quality change only when the playlist is enabled
+      changePlaylistFn(playlist);
       loader.trigger({ type: 'renditionenabled', metadata});
     } else {
       loader.trigger({ type: 'renditiondisabled', metadata});

diff --git a/src/segment-loader.js b/src/segment-loader.js
@@ -423,21 +423,6 @@ export const shouldFixBadTimelineChanges = (timelineChangeController) => {
   return false;
 };
 
-/**
- * Fixes certain bad timeline scenarios by resetting the loader.
- *
- * @param {SegmentLoader} segmentLoader
- */
-export const fixBadTimelineChange = (segmentLoader) => {
-  if (!segmentLoader) {
-    return;
-  }
-
-  segmentLoader.pause();
-  segmentLoader.resetEverything();
-  segmentLoader.load();
-};
-
 /**
  * Check if the pending audio timeline change is behind the
  * pending main timeline change.
@@ -480,7 +465,7 @@ const checkAndFixTimelines = (segmentLoader) => {
       return;
     }
 
-    fixBadTimelineChange(segmentLoader);
+    segmentLoader.timelineChangeController_.trigger('fixBadTimelineChange');
   }
 };
 
@@ -872,6 +857,7 @@ export default class SegmentLoader extends videojs.EventTarget {
       if (this.pendingSegment_) {
         this.pendingSegment_ = null;
       }
+      this.timelineChangeController_.clearPendingTimelineChange(this.loaderType_);
       return;
     }
 
@@ -1118,6 +1104,14 @@ export default class SegmentLoader extends videojs.EventTarget {
       return;
     }
 
+    if (this.playlist_ &&
+      this.playlist_.endList &&
+      newPlaylist.endList &&
+      this.playlist_.uri === newPlaylist.uri) {
+      // skip update if both prev and new are vod and have the same URI
+      return;
+    }
+
     const oldPlaylist = this.playlist_;
     const segmentInfo = this.pendingSegment_;
 

diff --git a/src/util/fn.js b/src/util/fn.js
@@ -0,0 +1,11 @@
+export const debounce = (callback, wait) => {
+  let timeoutId = null;
+
+  return (...args) => {
+    clearTimeout(timeoutId);
+
+    timeoutId = setTimeout(() => {
+      callback.apply(null, args);
+    }, wait);
+  };
+};
diff --git a/test/playlist-controller.test.js b/test/playlist-controller.test.js
@@ -726,8 +726,9 @@ QUnit.test('resets everything for a fast quality change then calls load', functi
 
   segmentLoader.remove = (start, end, doneFn) => {
     assert.equal(end, Infinity, 'on a remove all, end should be Infinity');
-    assert.ok(doneFn);
-    doneFn();
+    if (doneFn) {
+      doneFn();
+    }
     origRemove.call(segmentLoader, start, end, doneFn);
   };
 
@@ -4871,6 +4872,7 @@ QUnit.test('can pass or select a playlist for fastQualityChange', function(asser
   };
 
   pc.fastQualityChange_(pc.main().playlists[1]);
+  this.clock.tick(110);
   pc.runFastQualitySwitch_();
   assert.deepEqual(calls, {
     resetEverything: 1,
@@ -4880,6 +4882,7 @@ QUnit.test('can pass or select a playlist for fastQualityChange', function(asser
   }, 'calls expected function when passed a playlist');
 
   pc.fastQualityChange_();
+  this.clock.tick(110);
   pc.runFastQualitySwitch_();
   assert.deepEqual(calls, {
     resetEverything: 2,

diff --git a/test/rendition-mixin.test.js b/test/rendition-mixin.test.js
@@ -294,7 +294,7 @@ QUnit.test(
 
     renditions[1].enabled(false);
 
-    assert.equal(pc.fastQualityChange_.calls, 2, 'fastQualityChange_ was called twice');
+    assert.equal(pc.fastQualityChange_.calls, 1, 'fastQualityChange_ was called once');
   }
 );