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

@@ -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');
       });

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');
   }
 
   /**

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;
 

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) {