From 969589e6dbe477f97e43868a646e0976d34b4c60 Mon Sep 17 00:00:00 2001 From: Garrett Singer Date: Fri, 18 Nov 2022 16:54:32 -0500 Subject: [PATCH] fix: resume loading on segment timeout for `bufferBasedABR` (#1333) Previously, if there was a segment timeout and `experimentalBufferBasedABR` was set to `true`, then the main segment loader would stay in a `READY` state and never resume loading segments. This is because the buffer based ABR path doesn't follow the same flow as non buffer based ABR, where a new playlist would be loaded immediately and it would trigger a load. Buffer based ABR may determine that no rendition change should be made, despite the timeout, leading to nothing happening. This change makes the call to load explicit, but only for buffer based ABR on timeouts. --- src/playlist-controller.js | 33 +++++++++++++++---------- src/segment-loader.js | 1 + test/loader-common.js | 16 +++++++++++++ test/playlist-controller.test.js | 41 ++++++++++++++++++++------------ 4 files changed, 64 insertions(+), 27 deletions(-) diff --git a/src/playlist-controller.js b/src/playlist-controller.js index 5723f4c9a..e37ea48af 100644 --- a/src/playlist-controller.js +++ b/src/playlist-controller.js @@ -364,14 +364,14 @@ export class PlaylistController extends videojs.EventTarget { /** * Run selectPlaylist and switch to the new playlist if we should * + * @param {string} [reason=abr] a reason for why the ABR check is made * @private - * */ - checkABR_() { + checkABR_(reason = 'abr') { const nextPlaylist = this.selectPlaylist(); if (nextPlaylist && this.shouldSwitchToMedia_(nextPlaylist)) { - this.switchMedia_(nextPlaylist, 'abr'); + this.switchMedia_(nextPlaylist, reason); } } @@ -766,17 +766,26 @@ export class PlaylistController extends videojs.EventTarget { * @private */ setupSegmentLoaderListeners_() { - if (!this.bufferBasedABR) { - this.mainSegmentLoader_.on('bandwidthupdate', () => { - const nextPlaylist = this.selectPlaylist(); - - if (this.shouldSwitchToMedia_(nextPlaylist)) { - this.switchMedia_(nextPlaylist, 'bandwidthupdate'); - } + this.mainSegmentLoader_.on('bandwidthupdate', () => { + // Whether or not buffer based ABR or another ABR is used, on a bandwidth change it's + // useful to check to see if a rendition switch should be made. + this.checkABR_('bandwidthupdate'); + this.tech_.trigger('bandwidthupdate'); + }); - this.tech_.trigger('bandwidthupdate'); - }); + this.mainSegmentLoader_.on('timeout', () => { + if (this.bufferBasedABR) { + // If a rendition change is needed, then it would've be done on `bandwidthupdate`. + // Here the only consideration is that for buffer based ABR there's no guarantee + // of an immediate switch (since the bandwidth is averaged with a timeout + // bandwidth value of 1), so force a load on the segment loader to keep it going. + this.mainSegmentLoader_.load(); + } + }); + // `progress` events are not reliable enough of a bandwidth measure to trigger buffer + // based ABR. + if (!this.bufferBasedABR) { this.mainSegmentLoader_.on('progress', () => { this.trigger('progress'); }); diff --git a/src/segment-loader.js b/src/segment-loader.js index b36509ca8..3b26a2f04 100644 --- a/src/segment-loader.js +++ b/src/segment-loader.js @@ -2635,6 +2635,7 @@ export default class SegmentLoader extends videojs.EventTarget { this.bandwidth = 1; this.roundTrip = NaN; this.trigger('bandwidthupdate'); + this.trigger('timeout'); } /** diff --git a/test/loader-common.js b/test/loader-common.js index 9964fa5c2..6681c6094 100644 --- a/test/loader-common.js +++ b/test/loader-common.js @@ -364,6 +364,22 @@ export const LoaderCommonFactory = ({ assert.ok(isNaN(loader.roundTrip), 'reset round trip time'); }); + QUnit.test('segment request timeout triggers timeout event', function(assert) { + let timeoutEvents = 0; + + loader.on('timeout', () => timeoutEvents++); + loader.playlist(playlistWithDuration(10)); + + loader.load(); + this.clock.tick(1); + + this.requests[0].timedout = true; + // arbitrary length of time that should lead to a timeout + this.clock.tick(100 * 1000); + + assert.equal(timeoutEvents, 1, 'triggered timeout event'); + }); + QUnit.test('progress on segment requests are redispatched', function(assert) { let progressEvents = 0; diff --git a/test/playlist-controller.test.js b/test/playlist-controller.test.js index 2fb13d0ee..9d9806ea6 100644 --- a/test/playlist-controller.test.js +++ b/test/playlist-controller.test.js @@ -1790,11 +1790,8 @@ QUnit.test('selects a playlist after main/combined segment downloads', function( }); QUnit.test('does not select a playlist after segment downloads if only one playlist', function(assert) { - const origWarn = videojs.log.warn; let calls = 0; - const warnings = []; - videojs.log.warn = (text) => warnings.push(text); this.playlistController.selectPlaylist = () => { calls++; return null; @@ -1809,15 +1806,6 @@ QUnit.test('does not select a playlist after segment downloads if only one playl // "downloaded" a segment this.playlistController.mainSegmentLoader_.trigger('bandwidthupdate'); assert.strictEqual(calls, 2, 'selects after the initial segment'); - - assert.equal(warnings.length, 1, 'one warning logged'); - assert.equal( - warnings[0], - 'We received no playlist to switch to. Please check your stream.', - 'we logged the correct warning' - ); - - videojs.log.warn = origWarn; }); QUnit.test('re-triggers bandwidthupdate events on the tech', function(assert) { @@ -5759,17 +5747,40 @@ QUnit.test('Determines if playlist should change on bandwidthupdate/progress fro // "downloaded" a segment this.playlistController.mainSegmentLoader_.trigger('bandwidthupdate'); - assert.strictEqual(calls, 1, 'does not select after segment download'); + assert.strictEqual(calls, 2, 'selects after segment download'); this.clock.tick(250); - assert.strictEqual(calls, 2, 'selects after clock tick'); + assert.strictEqual(calls, 3, 'selects after clock tick'); this.clock.tick(1000); - assert.strictEqual(calls, 6, 'selects after clock tick, 1000 is 4x250'); + assert.strictEqual(calls, 7, 'selects after clock tick, 1000 is 4x250'); // verify stats assert.equal(this.player.tech_.vhs.stats.bandwidth, 4194304, 'default bandwidth'); }); +QUnit.test('loads main segment loader on timeout', function(assert) { + const mainSegmentLoader = this.playlistController.mainSegmentLoader_; + + this.playlistController.mediaSource.trigger('sourceopen'); + + // master + this.standardXHRResponse(this.requests.shift()); + // media + this.standardXHRResponse(this.requests.shift()); + + let loadCalls = 0; + + mainSegmentLoader.load = () => loadCalls++; + + this.playlistController.mainSegmentLoader_.trigger('bandwidthupdate'); + + assert.equal(loadCalls, 0, 'does not call load'); + + this.playlistController.mainSegmentLoader_.trigger('timeout'); + + assert.equal(loadCalls, 1, 'calls load'); +}); + QUnit.module('PlaylistController shouldSwitchToMedia', sharedHooks); QUnit.test('true if a no current playlist', function(assert) {