From 0f5ed778a1249e95ef3c935dc2d8b9d32230c849 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Thu, 29 Jul 2021 15:25:11 -0400 Subject: [PATCH 1/6] fix: Keep media update timeout alive so live playlists can recover --- src/master-playlist-controller.js | 2 +- src/playlist-loader.js | 42 ++++++++++++++----- test/master-playlist-controller.test.js | 39 ----------------- test/playlist-loader.test.js | 56 +++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 51 deletions(-) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index 329523c4d..33ce6130f 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -67,7 +67,7 @@ const shouldSwitchToMedia = function({ // This is because in LIVE, the player plays 3 segments from the end of the // playlist, and if `BUFFER_LOW_WATER_LINE` is greater than the duration availble // in those segments, a viewer will never experience a rendition upswitch. - if (!currentPlaylist || !currentPlaylist.endList) { + if (!currentPlaylist) { log(`${sharedLogLine} as current playlist ` + (!currentPlaylist ? 'is not set' : 'is live')); return true; } diff --git a/src/playlist-loader.js b/src/playlist-loader.js index 9715ab6f7..ace6dbec7 100644 --- a/src/playlist-loader.js +++ b/src/playlist-loader.js @@ -505,13 +505,7 @@ export default class PlaylistLoader extends EventTarget { this.trigger('playlistunchanged'); } - // refresh live playlists after a target duration passes - if (!this.media().endList) { - window.clearTimeout(this.mediaUpdateTimeout); - this.mediaUpdateTimeout = window.setTimeout(() => { - this.trigger('mediaupdatetimeout'); - }, refreshDelay(this.media(), !!update)); - } + this.updateMediaUpdateTimeout_(refreshDelay(this.media(), !!update)); this.trigger('loadedplaylist'); } @@ -619,6 +613,8 @@ export default class PlaylistLoader extends EventTarget { return; } + this.updateMediaUpdateTimeout_(refreshDelay(playlist, true)); + // switching to the active playlist is a no-op if (!mediaChange) { return; @@ -679,8 +675,12 @@ export default class PlaylistLoader extends EventTarget { * pause loading of the playlist */ pause() { + if (this.mediaUpdateTimeout) { + window.clearTimeout(this.mediaUpdateTimeout); + this.mediaUpdateTimeout = null; + } + this.stopRequest(); - window.clearTimeout(this.mediaUpdateTimeout); if (this.state === 'HAVE_NOTHING') { // If we pause the loader before any data has been retrieved, its as if we never // started, so reset to an unstarted state. @@ -705,14 +705,16 @@ export default class PlaylistLoader extends EventTarget { * start loading of the playlist */ load(shouldDelay) { - window.clearTimeout(this.mediaUpdateTimeout); - const media = this.media(); if (shouldDelay) { const delay = media ? ((media.partTargetDuration || media.targetDuration) / 2) * 1000 : 5 * 1000; - this.mediaUpdateTimeout = window.setTimeout(() => this.load(), delay); + this.mediaUpdateTimeout = window.setTimeout(() => { + this.mediaUpdateTimeout = null; + this.load(); + }, delay); + return; } @@ -728,6 +730,24 @@ export default class PlaylistLoader extends EventTarget { } } + updateMediaUpdateTimeout_(delay) { + if (this.mediaUpdateTimeout) { + window.clearTimeout(this.mediaUpdateTimeout); + this.mediaUpdateTimeout = null; + } + + // we only have use mediaupdatetimeout for live playlists. + if (!this.media() || this.media().endList) { + return; + } + + this.mediaUpdateTimeout = window.setTimeout(() => { + this.mediaUpdateTimeout = null; + this.trigger('mediaupdatetimeout'); + this.updateMediaUpdateTimeout_(delay); + }, delay); + } + /** * start loading of the playlist */ diff --git a/test/master-playlist-controller.test.js b/test/master-playlist-controller.test.js index 3ebae6d85..cba310c75 100644 --- a/test/master-playlist-controller.test.js +++ b/test/master-playlist-controller.test.js @@ -2033,36 +2033,6 @@ QUnit.test( mediaChanges.length = 0; - endList = false; - currentTime = 100; - currentPlaylistBandwidth = 1000; - nextPlaylistBandwidth = 1001; - buffered = []; - this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate'); - assert.equal( - mediaChanges.length, - 1, - 'changes live media when no buffer and higher bandwidth playlist' - ); - buffered = [[0, 100], [100, 109]]; - this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate'); - assert.equal( - mediaChanges.length, - 2, - 'changes live media when insufficient forward buffer and higher ' + - 'bandwidth playlist' - ); - buffered = [[0, 100], [100, 130]]; - this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate'); - assert.equal( - mediaChanges.length, - 3, - 'changes live media when sufficient forward buffer and higher ' + - 'bandwidth playlist' - ); - - mediaChanges.length = 0; - endList = true; currentTime = 9; duration = 18; @@ -5884,15 +5854,6 @@ QUnit.test('true if a no current playlist', function(assert) { assert.ok(mpc.shouldSwitchToMedia_(nextPlaylist), 'should switch without currentPlaylist'); }); -QUnit.test('true if current playlist is live', function(assert) { - const mpc = this.masterPlaylistController; - - mpc.masterPlaylistLoader_.media = () => ({endList: false, id: 'bar'}); - const nextPlaylist = {id: 'foo', endList: true}; - - assert.ok(mpc.shouldSwitchToMedia_(nextPlaylist), 'should switch with live currentPlaylist'); -}); - QUnit.test('true if duration < 30', function(assert) { const mpc = this.masterPlaylistController; const nextPlaylist = {id: 'foo', endList: true}; diff --git a/test/playlist-loader.test.js b/test/playlist-loader.test.js index d8a96c6b2..b6631984d 100644 --- a/test/playlist-loader.test.js +++ b/test/playlist-loader.test.js @@ -2101,6 +2101,62 @@ QUnit.module('Playlist Loader', function(hooks) { ); }); + QUnit.test('mediaupdatetimeout works as expeted for live playlists', function(assert) { + const loader = new PlaylistLoader('master.m3u8', this.fakeVhs); + let media = + '#EXTM3U\n' + + '#EXT-X-MEDIA-SEQUENCE:0\n' + + '#EXTINF:5,\n' + + 'low-0.ts\n' + + '#EXTINF:5,\n' + + 'low-1.ts\n'; + + loader.load(); + + this.requests.shift().respond( + 200, null, + '#EXTM3U\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + + 'media.m3u8\n' + + '#EXT-X-STREAM-INF:BANDWIDTH=1\n' + + 'media2.m3u8\n' + ); + + this.requests.shift().respond(200, null, media); + + assert.ok(loader.mediaUpdateTimeout, 'has an initial media update timeout'); + + this.clock.tick(5000); + + media += '#EXTINF:5\nlow-2.ts\n'; + + this.requests.shift().respond(200, null, media); + + assert.ok(loader.mediaUpdateTimeout, 'media update timeout created another'); + + loader.pause(); + assert.notOk(loader.mediaUpdateTimeout, 'media update timeout cleared'); + + loader.media(loader.master.playlists[0]); + + assert.ok(loader.mediaUpdateTimeout, 'media update timeout created again'); + assert.equal(this.requests.length, 0, 'no request'); + + loader.media(loader.master.playlists[1]); + + assert.ok(loader.mediaUpdateTimeout, 'media update timeout created'); + assert.equal(this.requests.length, 1, 'playlist requested'); + + this.requests.shift().respond(500, null, 'fail'); + + assert.ok(loader.mediaUpdateTimeout, 'media update timeout exists after request failure'); + + this.clock.tick(5000); + + assert.ok(loader.mediaUpdateTimeout, 'media update timeout created again'); + assert.equal(this.requests.length, 1, 'playlist re-requested'); + }); + QUnit.module('llhls', { beforeEach() { this.fakeVhs.options_ = {experimentalLLHLS: true}; From 378234a64a5a8917b796100358c5f17d2f7866ef Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Thu, 29 Jul 2021 15:44:24 -0400 Subject: [PATCH 2/6] consistency --- src/playlist-loader.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/playlist-loader.js b/src/playlist-loader.js index ace6dbec7..9e4e81981 100644 --- a/src/playlist-loader.js +++ b/src/playlist-loader.js @@ -705,6 +705,10 @@ export default class PlaylistLoader extends EventTarget { * start loading of the playlist */ load(shouldDelay) { + if (this.mediaUpdateTimeout) { + window.clearTimeout(this.mediaUpdateTimeout); + this.mediaUpdateTimeout = null; + } const media = this.media(); if (shouldDelay) { From e4aa0703cf0321407313c70ae8c18c11b5725ccd Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Thu, 29 Jul 2021 15:50:37 -0400 Subject: [PATCH 3/6] typo --- test/playlist-loader.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/playlist-loader.test.js b/test/playlist-loader.test.js index b6631984d..1ed2250ef 100644 --- a/test/playlist-loader.test.js +++ b/test/playlist-loader.test.js @@ -2101,7 +2101,7 @@ QUnit.module('Playlist Loader', function(hooks) { ); }); - QUnit.test('mediaupdatetimeout works as expeted for live playlists', function(assert) { + QUnit.test('mediaupdatetimeout works as expected for live playlists', function(assert) { const loader = new PlaylistLoader('master.m3u8', this.fakeVhs); let media = '#EXTM3U\n' + From 9ecba52518426885dd806c5a2eb4ccf63b26ffb7 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Thu, 5 Aug 2021 16:38:08 -0400 Subject: [PATCH 4/6] add comment --- src/playlist-loader.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/playlist-loader.js b/src/playlist-loader.js index 9e4e81981..90deaa392 100644 --- a/src/playlist-loader.js +++ b/src/playlist-loader.js @@ -613,6 +613,11 @@ export default class PlaylistLoader extends EventTarget { return; } + // We update/set the timeout here so that live playlists + // that are not a media change will "start" the loader as expected. + // We expect that this function will start the media update timeout + // cycle again. This also prevents a playlist switch failure from + // causing us to stall during live. this.updateMediaUpdateTimeout_(refreshDelay(playlist, true)); // switching to the active playlist is a no-op From 9ea139ff6904bb72f74e7da5d23d6373b74bd8c4 Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Fri, 6 Aug 2021 11:12:43 -0400 Subject: [PATCH 5/6] move live playlist check down, bring back tests --- src/master-playlist-controller.js | 19 +++++++----- test/master-playlist-controller.test.js | 39 +++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/src/master-playlist-controller.js b/src/master-playlist-controller.js index 33ce6130f..1c7678c0a 100644 --- a/src/master-playlist-controller.js +++ b/src/master-playlist-controller.js @@ -63,20 +63,25 @@ const shouldSwitchToMedia = function({ const sharedLogLine = `allowing switch ${currentPlaylist && currentPlaylist.id || 'null'} -> ${nextPlaylist.id}`; - // If the playlist is live, then we want to not take low water line into account. - // This is because in LIVE, the player plays 3 segments from the end of the - // playlist, and if `BUFFER_LOW_WATER_LINE` is greater than the duration availble - // in those segments, a viewer will never experience a rendition upswitch. if (!currentPlaylist) { - log(`${sharedLogLine} as current playlist ` + (!currentPlaylist ? 'is not set' : 'is live')); + log(`${sharedLogLine} as current playlist is not set`); return true; } - // no need to switch playlist is the same + // no need to switch if playlist is the same if (nextPlaylist.id === currentPlaylist.id) { return false; } + // If the playlist is live, then we want to not take low water line into account. + // This is because in LIVE, the player plays 3 segments from the end of the + // playlist, and if `BUFFER_LOW_WATER_LINE` is greater than the duration availble + // in those segments, a viewer will never experience a rendition upswitch. + if (!currentPlaylist.endList) { + log(`${sharedLogLine} as current playlist is live`); + return true; + } + const maxBufferLowWaterLine = experimentalBufferBasedABR ? Config.EXPERIMENTAL_MAX_BUFFER_LOW_WATER_LINE : Config.MAX_BUFFER_LOW_WATER_LINE; @@ -351,7 +356,7 @@ export class MasterPlaylistController extends videojs.EventTarget { checkABR_() { const nextPlaylist = this.selectPlaylist(); - if (this.shouldSwitchToMedia_(nextPlaylist)) { + if (nextPlaylist && this.shouldSwitchToMedia_(nextPlaylist)) { this.switchMedia_(nextPlaylist, 'abr'); } } diff --git a/test/master-playlist-controller.test.js b/test/master-playlist-controller.test.js index cba310c75..3ebae6d85 100644 --- a/test/master-playlist-controller.test.js +++ b/test/master-playlist-controller.test.js @@ -2033,6 +2033,36 @@ QUnit.test( mediaChanges.length = 0; + endList = false; + currentTime = 100; + currentPlaylistBandwidth = 1000; + nextPlaylistBandwidth = 1001; + buffered = []; + this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate'); + assert.equal( + mediaChanges.length, + 1, + 'changes live media when no buffer and higher bandwidth playlist' + ); + buffered = [[0, 100], [100, 109]]; + this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate'); + assert.equal( + mediaChanges.length, + 2, + 'changes live media when insufficient forward buffer and higher ' + + 'bandwidth playlist' + ); + buffered = [[0, 100], [100, 130]]; + this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate'); + assert.equal( + mediaChanges.length, + 3, + 'changes live media when sufficient forward buffer and higher ' + + 'bandwidth playlist' + ); + + mediaChanges.length = 0; + endList = true; currentTime = 9; duration = 18; @@ -5854,6 +5884,15 @@ QUnit.test('true if a no current playlist', function(assert) { assert.ok(mpc.shouldSwitchToMedia_(nextPlaylist), 'should switch without currentPlaylist'); }); +QUnit.test('true if current playlist is live', function(assert) { + const mpc = this.masterPlaylistController; + + mpc.masterPlaylistLoader_.media = () => ({endList: false, id: 'bar'}); + const nextPlaylist = {id: 'foo', endList: true}; + + assert.ok(mpc.shouldSwitchToMedia_(nextPlaylist), 'should switch with live currentPlaylist'); +}); + QUnit.test('true if duration < 30', function(assert) { const mpc = this.masterPlaylistController; const nextPlaylist = {id: 'foo', endList: true}; From 5f2c279743ad5d658351def9ce4c56ffecd7c32b Mon Sep 17 00:00:00 2001 From: brandonocasey Date: Tue, 17 Aug 2021 14:14:17 -0400 Subject: [PATCH 6/6] add a test for load delay --- test/playlist-loader.test.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/test/playlist-loader.test.js b/test/playlist-loader.test.js index 1ed2250ef..bf806bed5 100644 --- a/test/playlist-loader.test.js +++ b/test/playlist-loader.test.js @@ -683,6 +683,22 @@ QUnit.module('Playlist Loader', function(hooks) { ); }); + QUnit.test('can delay load', function(assert) { + const loader = new PlaylistLoader('master.m3u8', this.fakeVhs); + + assert.notOk(loader.mediaUpdateTimeout, 'no media update timeout'); + + loader.load(true); + + assert.ok(loader.mediaUpdateTimeout, 'have a media update timeout now'); + assert.strictEqual(this.requests.length, 0, 'have no requests'); + + this.clock.tick(5000); + + assert.notOk(loader.mediaUpdateTimeout, 'media update timeout is gone'); + assert.strictEqual(this.requests.length, 1, 'playlist request after delay'); + }); + QUnit.test('starts without any metadata', function(assert) { const loader = new PlaylistLoader('master.m3u8', this.fakeVhs);