From f6fd00b2a202a81f23a3193759f70746a56672e3 Mon Sep 17 00:00:00 2001 From: Dzianis Dashkevich Date: Tue, 5 Nov 2024 12:57:16 -0500 Subject: [PATCH 01/11] chore: clear previous dash media request timeout everytime we clear or update it --- src/dash-playlist-loader.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/dash-playlist-loader.js b/src/dash-playlist-loader.js index 623a8178c..904cf21de 100644 --- a/src/dash-playlist-loader.js +++ b/src/dash-playlist-loader.js @@ -384,6 +384,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; } @@ -553,6 +554,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 @@ -629,6 +631,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 +776,7 @@ export default class DashPlaylistLoader extends EventTarget { handleMain_() { // clear media request + window.clearTimeout(this.mediaRequest_); this.mediaRequest_ = null; const oldMain = this.mainPlaylistLoader_.main; From 626123f45a8e6c470de4988b4a7c66de71b88f76 Mon Sep 17 00:00:00 2001 From: Dzianis Dashkevich Date: Tue, 5 Nov 2024 12:58:43 -0500 Subject: [PATCH 02/11] chore: call fast-quality-switch only when we enable playlist from quality selector --- src/rendition-mixin.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/rendition-mixin.js b/src/rendition-mixin.js index 8a2e35a60..cd0127712 100644 --- 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}); From 0a44defb0e1c4e0bf06d8a52dc9ca13681ffaef4 Mon Sep 17 00:00:00 2001 From: Dzianis Dashkevich Date: Tue, 5 Nov 2024 13:28:50 -0500 Subject: [PATCH 03/11] chore: add isPaused for dash playlist loader to mitigate duplicate playlist trigger for the main segment loader --- src/dash-playlist-loader.js | 11 +++++++++++ src/playlist-controller.js | 11 ++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/dash-playlist-loader.js b/src/dash-playlist-loader.js index 904cf21de..1c061501e 100644 --- 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) { @@ -466,6 +472,7 @@ export default class DashPlaylistLoader extends EventTarget { } dispose() { + this.isPaused_ = true; this.trigger('dispose'); this.stopRequest(); this.loadedPlaylists_ = {}; @@ -571,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; @@ -590,6 +599,8 @@ export default class DashPlaylistLoader extends EventTarget { } load(isFinalRendition) { + this.isPaused_ = false; + window.clearTimeout(this.mediaUpdateTimeout); this.mediaUpdateTimeout = null; diff --git a/src/playlist-controller.js b/src/playlist-controller.js index 6f4f97130..4da5e53bb 100644 --- a/src/playlist-controller.js +++ b/src/playlist-controller.js @@ -701,7 +701,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 From 3ddeea378d6509f3aa43c8e3dba911bf89c219c5 Mon Sep 17 00:00:00 2001 From: Dzianis Dashkevich Date: Tue, 5 Nov 2024 13:46:35 -0500 Subject: [PATCH 04/11] chore: fix fastQualityChange_ tests --- test/rendition-mixin.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/rendition-mixin.test.js b/test/rendition-mixin.test.js index 0997a6894..72be954d6 100644 --- 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'); } ); From d1f43c710c0a69b163ecb2fb33aac0df969ef6c8 Mon Sep 17 00:00:00 2001 From: Dzianis Dashkevich Date: Tue, 5 Nov 2024 14:36:34 -0500 Subject: [PATCH 05/11] chore: add fast quality change debounce --- src/playlist-controller.js | 6 ++++++ src/util/fn.js | 11 +++++++++++ 2 files changed, 17 insertions(+) create mode 100644 src/util/fn.js diff --git a/src/playlist-controller.js b/src/playlist-controller.js index 4da5e53bb..0c17d9d3a 100644 --- 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, diff --git a/src/util/fn.js b/src/util/fn.js new file mode 100644 index 000000000..a5e29048a --- /dev/null +++ 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); + }; +}; From 3c74bd2c52f8a2b88c34d2584583b35e40425e79 Mon Sep 17 00:00:00 2001 From: Dzianis Dashkevich Date: Tue, 5 Nov 2024 15:30:38 -0500 Subject: [PATCH 06/11] chore: add debounce tick to the tests --- test/playlist-controller.test.js | 2 ++ test/videojs-http-streaming.test.js | 3 +++ 2 files changed, 5 insertions(+) diff --git a/test/playlist-controller.test.js b/test/playlist-controller.test.js index 0f555d556..48a6f91c9 100644 --- a/test/playlist-controller.test.js +++ b/test/playlist-controller.test.js @@ -4859,6 +4859,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, @@ -4868,6 +4869,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/videojs-http-streaming.test.js b/test/videojs-http-streaming.test.js index 027ca75b1..cdb975f7e 100644 --- a/test/videojs-http-streaming.test.js +++ b/test/videojs-http-streaming.test.js @@ -4920,6 +4920,9 @@ QUnit.test('eme handles keystatuschange where status is output-restricted', func assert.equal(playlists[0].excludeUntil, Infinity, 'first HD playlist excluded'); assert.equal(playlists[1].excludeUntil, Infinity, 'second HD playlist excluded'); assert.equal(playlists[2].excludeUntil, undefined, 'non-HD playlist not excluded'); + + this.clock.tick(110); + assert.equal(switchMediaCalled, 1, 'switchMedia_ called once'); }); From 4d4b751ade9688d960dae67c24cbfe926f6a556d Mon Sep 17 00:00:00 2001 From: Dzianis Dashkevich Date: Tue, 5 Nov 2024 18:34:58 -0500 Subject: [PATCH 07/11] chore: restart all loaders when we hit fix bad timeline change --- src/playlist-controller.js | 6 ++++++ src/segment-loader.js | 17 +---------------- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/src/playlist-controller.js b/src/playlist-controller.js index 0c17d9d3a..3ccb136b0 100644 --- a/src/playlist-controller.js +++ b/src/playlist-controller.js @@ -977,6 +977,12 @@ 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.setCurrentTime(this.tech_.currentTime()); + }); + this.mainSegmentLoader_.on('earlyabort', (event) => { // never try to early abort with the new ABR algorithm if (this.bufferBasedABR) { diff --git a/src/segment-loader.js b/src/segment-loader.js index 877209e78..aef4e6858 100644 --- 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'); } }; From 231b4bbc02007b02f66d7a6262c5fa2c3a4ea8b8 Mon Sep 17 00:00:00 2001 From: Dzianis Dashkevich Date: Wed, 13 Nov 2024 14:41:16 -0500 Subject: [PATCH 08/11] chore: update segment loader --- src/playlist-controller.js | 13 ++++++- src/segment-loader.js | 10 +++++ test/segment-loader.test.js | 76 ++++++++++++------------------------- 3 files changed, 47 insertions(+), 52 deletions(-) diff --git a/src/playlist-controller.js b/src/playlist-controller.js index 3ccb136b0..2b391de00 100644 --- a/src/playlist-controller.js +++ b/src/playlist-controller.js @@ -1136,7 +1136,18 @@ export class PlaylistController extends videojs.EventTarget { this.mainSegmentLoader_.load(); }); - // don't need to reset audio as it is reset when media changes + if (this.mediaTypes_.AUDIO.activePlaylistLoader) { + this.audioSegmentLoader_.pause(); + this.audioSegmentLoader_.resetEverything(() => { + this.audioSegmentLoader_.load(); + }); + } + if (this.mediaTypes_.SUBTITLES.activePlaylistLoader) { + this.subtitleSegmentLoader_.pause(); + this.subtitleSegmentLoader_.resetEverything(() => { + this.subtitleSegmentLoader_.load(); + }); + } } /** diff --git a/src/segment-loader.js b/src/segment-loader.js index aef4e6858..4ec5f2565 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -857,6 +857,7 @@ export default class SegmentLoader extends videojs.EventTarget { if (this.pendingSegment_) { this.pendingSegment_ = null; } + this.timelineChangeController_.clearPendingTimelineChange(this.loaderType_); return; } @@ -1102,6 +1103,15 @@ export default class SegmentLoader extends videojs.EventTarget { if (!newPlaylist) { 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/test/segment-loader.test.js b/test/segment-loader.test.js index 0d90b2259..24fd672c7 100644 --- a/test/segment-loader.test.js +++ b/test/segment-loader.test.js @@ -10,8 +10,7 @@ import { getTroublesomeSegmentDurationMessage, getSyncSegmentCandidate, segmentInfoString, - shouldFixBadTimelineChanges, - fixBadTimelineChange + shouldFixBadTimelineChanges } from '../src/segment-loader'; import mp4probe from 'mux.js/lib/mp4/probe'; import { @@ -515,37 +514,6 @@ QUnit.test('shouldFixBadTimelineChange returns false when timelineChangeControll assert.notOk(shouldFixBadTimelineChanges(timelineChangeController), 'should not fix a timeline change with no timelineChangeController'); }); -QUnit.module('fixBadTimelineChange'); - -QUnit.test('fixBadTimelineChange calls pause, resetEverything and load on a segmentLoader', function(assert) { - let pauseCalls = 0; - let resetEverythingCalls = 0; - let loadCalls = 0; - let mockSegmentLoader = { - pause() { - pauseCalls++; - }, - resetEverything() { - resetEverythingCalls++; - }, - load() { - loadCalls++; - } - }; - - fixBadTimelineChange(mockSegmentLoader); - assert.equal(pauseCalls, 1, 'calls pause once'); - assert.equal(resetEverythingCalls, 1, 'calls resetEverything once'); - assert.equal(loadCalls, 1, 'calls load once'); - - // early return if undefined. call counts remain the same. - mockSegmentLoader = undefined; - fixBadTimelineChange(mockSegmentLoader); - assert.equal(pauseCalls, 1, 'calls pause once'); - assert.equal(resetEverythingCalls, 1, 'calls resetEverything once'); - assert.equal(loadCalls, 1, 'calls load once'); -}); - QUnit.module('safeBackBufferTrimTime'); QUnit.test('uses 30s before playhead when seekable start is 0', function(assert) { @@ -1668,17 +1636,11 @@ QUnit.module('SegmentLoader', function(hooks) { loader = new SegmentLoader(LoaderCommonSettings.call(this, { loaderType: 'audio' }), {}); - const origPause = loader.pause; const origLoad = loader.load; const origResetEverything = loader.resetEverything; - let pauseCalls = 0; let loadCalls = 0; let resetEverythingCalls = 0; - loader.pause = () => { - pauseCalls++; - origPause.call(loader); - }; loader.load = () => { loadCalls++; origLoad.call(loader); @@ -1701,6 +1663,14 @@ QUnit.module('SegmentLoader', function(hooks) { } }; + let fixBadTimelineChangeCount = 0; + + loader.timelineChangeController_.trigger = (type) => { + if (type === 'fixBadTimelineChange') { + fixBadTimelineChangeCount++; + } + }; + const playlist = playlistWithDuration(20); playlist.discontinuityStarts = [1]; @@ -1716,9 +1686,9 @@ QUnit.module('SegmentLoader', function(hooks) { loader.playlist(playlist); loader.load(); this.clock.tick(1); - assert.equal(pauseCalls, 1, '1 pause call expected'); - assert.equal(loadCalls, 2, '2 load calls expected'); - assert.equal(resetEverythingCalls, 2, '1 load calls expected'); + assert.equal(loadCalls, 1, '1 load calls expected'); + assert.equal(resetEverythingCalls, 1, '1 load calls expected'); + assert.equal(fixBadTimelineChangeCount, 1, '1 fixBadTimelineChangeCount triggered'); }); }); @@ -1727,17 +1697,11 @@ QUnit.module('SegmentLoader', function(hooks) { loader = new SegmentLoader(LoaderCommonSettings.call(this, { loaderType: 'main' }), {}); - const origPause = loader.pause; const origLoad = loader.load; const origResetEverything = loader.resetEverything; - let pauseCalls = 0; let loadCalls = 0; let resetEverythingCalls = 0; - loader.pause = () => { - pauseCalls++; - origPause.call(loader); - }; loader.load = () => { loadCalls++; origLoad.call(loader); @@ -1759,6 +1723,15 @@ QUnit.module('SegmentLoader', function(hooks) { }; } }; + + let fixBadTimelineChangeCount = 0; + + loader.timelineChangeController_.trigger = (type) => { + if (type === 'fixBadTimelineChange') { + fixBadTimelineChangeCount++; + } + }; + this.sourceUpdater_.ready = () => { return true; }; @@ -1783,9 +1756,10 @@ QUnit.module('SegmentLoader', function(hooks) { loader.playlist(playlist); loader.load(); this.clock.tick(1); - assert.equal(pauseCalls, 1, '1 pause call expected'); - assert.equal(loadCalls, 2, '2 load calls expected'); - assert.equal(resetEverythingCalls, 2, '1 load calls expected'); + assert.equal(loadCalls, 1, '1 load calls expected'); + assert.equal(resetEverythingCalls, 1, '1 load calls expected'); + assert.equal(fixBadTimelineChangeCount, 1, '1 fixBadTimelineChangeCount triggered'); + }); }); From 140eec3d70c7fb022f5b54a31ee67208185ccf4d Mon Sep 17 00:00:00 2001 From: Dzianis Dashkevich Date: Wed, 13 Nov 2024 15:46:53 -0500 Subject: [PATCH 09/11] chore: update run-fast-quality-switch and fix bad timeline changes --- src/playlist-controller.js | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/src/playlist-controller.js b/src/playlist-controller.js index 2b391de00..64a25f2b9 100644 --- a/src/playlist-controller.js +++ b/src/playlist-controller.js @@ -980,7 +980,19 @@ export class PlaylistController extends videojs.EventTarget { this.timelineChangeController_.on('fixBadTimelineChange', () => { // pause, reset-everything and load for all segment-loaders this.logger_('Fix bad timeline change. Restarting al segment loaders...'); - this.setCurrentTime(this.tech_.currentTime()); + 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) => { @@ -1130,24 +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(() => { - this.audioSegmentLoader_.load(); - }); + this.audioSegmentLoader_.resetEverything(); } if (this.mediaTypes_.SUBTITLES.activePlaylistLoader) { this.subtitleSegmentLoader_.pause(); - this.subtitleSegmentLoader_.resetEverything(() => { - this.subtitleSegmentLoader_.load(); - }); + this.subtitleSegmentLoader_.resetEverything(); } + + // start segment loader loading in case they are paused + this.load(); } /** From 604a643340e3bcb6ce0227a4be294de525c99369 Mon Sep 17 00:00:00 2001 From: Dzianis Dashkevich Date: Wed, 20 Nov 2024 19:34:46 -0500 Subject: [PATCH 10/11] fix test --- test/playlist-controller.test.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/playlist-controller.test.js b/test/playlist-controller.test.js index ab7fd5dd2..66ea4ce58 100644 --- a/test/playlist-controller.test.js +++ b/test/playlist-controller.test.js @@ -726,8 +726,7 @@ 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(); + doneFn && doneFn(); origRemove.call(segmentLoader, start, end, doneFn); }; From fd15e60d3ee539947b9b4b41fee04b4323c9c018 Mon Sep 17 00:00:00 2001 From: Dzianis Dashkevich Date: Wed, 20 Nov 2024 19:43:43 -0500 Subject: [PATCH 11/11] chore: fix lint issues --- src/segment-loader.js | 1 - test/playlist-controller.test.js | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/segment-loader.js b/src/segment-loader.js index 6e33a6b6f..4ec5f2565 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -1104,7 +1104,6 @@ export default class SegmentLoader extends videojs.EventTarget { return; } - if (this.playlist_ && this.playlist_.endList && newPlaylist.endList && diff --git a/test/playlist-controller.test.js b/test/playlist-controller.test.js index 66ea4ce58..c62813039 100644 --- a/test/playlist-controller.test.js +++ b/test/playlist-controller.test.js @@ -726,7 +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'); - doneFn && doneFn(); + if (doneFn) { + doneFn(); + } origRemove.call(segmentLoader, start, end, doneFn); };