fix(experimentalBufferBasedABR): call selectPlaylist and change media on an interval #978

gkatsev · 2020-10-23T19:21:58Z

This is a followup to #886. There, I noticed that if we get sustained bad network we will time out because we will an opportunity for earlyabort. At first, I tried working around by delaying earlyabort in that case (#966) but as I was getting that ready I kept finding edge cases that we needed to account for. Specifically, the issue is that we only run our ABR algorithm on progress and bandwidthchange events from the segment loader. This means that if the download stalls, we stop run our algorithm and will eventually stall playback. Instead, we should remove earlyabort altogether (f897dde) and set up an interval (currently 250ms) to re-run our ABR algorithm. This means that if the network becomes bad for a sustained period, we will drop down once the buffer allows us but if the network recovers, we will switch up appropriately as well.

Also, the interval is stopped while we're paused.

Fixes #964.

…vents

brandonocasey

A few comments for now, still thinking about this. I think we might need to start the abr timer in a few other places just to be safe. Going to do some testing.

src/master-playlist-controller.js

brandonocasey · 2020-10-30T15:11:49Z

src/master-playlist-controller.js

@@ -258,6 +259,12 @@ export class MasterPlaylistController extends videojs.EventTarget {

    this.setupSegmentLoaderListeners_();

+    if (this.experimentalBufferBasedABR) {
+      this.startABRTimer_();
+      this.tech_.on('pause', () => this.stopABRTimer_());


I wonder if we should handle seeks that cause a pause differently here. Should they be ignored so that we can switch playlists during a seek? Maybe that already happens?

Was thinking about this a bit more and perhaps we just want to call this.checkABR_ on seeking so that we can change playlist during a seek rather than afterwards?

If we're just seeking by clicking, the pause is so small as to not matter. If we're scrubbing, we'll get a whole bunch of seeking events, so, we'd want to debounce it or something.
Maybe stopABRTimer_ should check whether scrubbing is set? So, we only stop it if we're paused for not scrubbing.

Updated the stop handler to check tech.scrubbing() if it exists, which is being added here videojs/video.js#6920

brandonocasey · 2020-10-30T15:13:17Z

src/master-playlist-controller.js

+      });
+
+      this.mainSegmentLoader_.on('progress', () => {
+        if (this.experimentalBufferBasedABR) {


I think we can remove this if statement and everything inside of it now.

yeah, this is unnecessary now.

brandonocasey · 2020-10-30T15:14:16Z

src/master-playlist-controller.js

@@ -1066,6 +1091,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
   */
  pauseLoading() {
    this.delegateLoaders_('all', ['abort', 'pause']);
+    this.stopABRTimer_();


Should we start the abr timer in load()?

brandonocasey · 2020-10-30T15:15:05Z

src/master-playlist-controller.js

@@ -258,6 +259,12 @@ export class MasterPlaylistController extends videojs.EventTarget {

    this.setupSegmentLoaderListeners_();

+    if (this.experimentalBufferBasedABR) {
+      this.startABRTimer_();


Should we be starting the timer before the first play?

we probably need to update this based on the preload setting. If preload is none or metedata (i.e. when we shouldn't have downloaded any segments) there's no point to it but with preload auto, I think we should.

I'll add updating this as part of the story to fix preload=none

brandonocasey · 2020-10-30T15:52:49Z

After some testing I think that we start/stop the timers in a fine places. Everything seems to work very smoothly now too. This solution seems way better than earlyabort, and it will keep all of the logic in masterPlaylistController where it belongs 👍

The getter will be added via videojs/video.js#6920

gesinger · 2020-11-02T15:02:25Z

test/master-playlist-controller.test.js

@@ -5534,53 +5534,6 @@ QUnit.module('MasterPlaylistController experimentalBufferBasedABR', {
  }
 });

-QUnit.test('Determines if playlist should be aborted on earlyabort', function(assert) {


Should this test be left in just when experimentalBufferBasedABR is false?

Maybe, though, this test was added as part of #886.

Actually, looks like we do have a similar test that already

http-streaming/test/master-playlist-controller.test.js

Lines 1847 to 1887 in ca7655e

QUnit.test('blacklists playlist on earlyabort', function(assert) {

this.masterPlaylistController.mediaSource.trigger('sourceopen');

// master

this.standardXHRResponse(this.requests.shift());

// media

this.standardXHRResponse(this.requests.shift());

const mediaChanges = [];

const playlistLoader = this.masterPlaylistController.masterPlaylistLoader_;

const currentMedia = playlistLoader.media();

const origMedia = playlistLoader.media.bind(playlistLoader);

const origWarn = videojs.log.warn;

const warnings = [];

this.masterPlaylistController.masterPlaylistLoader_.media = (media) => {

if (media) {

mediaChanges.push(media);

}

return origMedia(media);

};

videojs.log.warn = (text) => warnings.push(text);

assert.notOk(currentMedia.excludeUntil > 0, 'playlist not blacklisted');

assert.equal(mediaChanges.length, 0, 'no media change');

this.masterPlaylistController.mainSegmentLoader_.trigger('earlyabort');

assert.ok(currentMedia.excludeUntil > 0, 'playlist blacklisted');

assert.equal(mediaChanges.length, 1, 'one media change');

assert.equal(warnings.length, 1, 'one warning logged');

assert.equal(

warnings[0],

`Problem encountered with playlist ${currentMedia.id}. ` +

'Aborted early because there isn\'t enough bandwidth to complete the ' +

`request without rebuffering. Switching to playlist ${mediaChanges[0].id}.`,

'warning message is correct'

);

videojs.log.warn = origWarn;

});

. This test is a variation of that that accounts for shouldSwitchToMedia variations with experimentalBufferBasedABR

Updates based on late comments from #978.

gkatsev added 2 commits October 23, 2020 15:15

remove earlyabort altogether for the experimentalBufferBasedABR

f897dde

use an interval for ABR algorithm

e800203

gkatsev mentioned this pull request Oct 23, 2020

[Question] Does the Segment Loader have a retry mechanism? #961

Closed

gkatsev added 3 commits October 26, 2020 18:12

*stop*ABRTimer

278dab6

earlyabort is completely turned off

10fe516

update test to account for interval and no progress/bandwidthupdate e…

d6d121f

…vents

gkatsev changed the title ~~fix: call selectPlaylist and change media on an interval~~ fix(experimentalBufferBasedABR): call selectPlaylist and change media on an interval Oct 28, 2020

brandonocasey reviewed Oct 30, 2020

View reviewed changes

gkatsev added 3 commits October 30, 2020 11:57

remove unusued code

904094a

move not doing earlyabort to MPC

8d42faa

Don't stop ABR timer if we're scrubbing.

0c23d9d

The getter will be added via videojs/video.js#6920

brandonocasey approved these changes Oct 30, 2020

View reviewed changes

gkatsev merged commit 200c87b into main Oct 30, 2020

gkatsev deleted the use-a-timer branch October 30, 2020 22:58

gesinger reviewed Nov 2, 2020

View reviewed changes

gkatsev added a commit that referenced this pull request Nov 2, 2020

test(playback-watcher): subtitle test refactor

7ddd035

Updates based on late comments from #978.

gkatsev added this to the ABR milestone Nov 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(experimentalBufferBasedABR): call selectPlaylist and change media on an interval #978

fix(experimentalBufferBasedABR): call selectPlaylist and change media on an interval #978

gkatsev commented Oct 23, 2020 •

edited

Loading

brandonocasey left a comment

brandonocasey Oct 30, 2020

brandonocasey Oct 30, 2020

gkatsev Oct 30, 2020

gkatsev Oct 30, 2020

brandonocasey Oct 30, 2020

gkatsev Oct 30, 2020

brandonocasey Oct 30, 2020

brandonocasey Oct 30, 2020

gkatsev Oct 30, 2020

brandonocasey commented Oct 30, 2020

gesinger Nov 2, 2020

gkatsev Nov 2, 2020

gkatsev Nov 2, 2020

	QUnit.test('blacklists playlist on earlyabort', function(assert) {
	this.masterPlaylistController.mediaSource.trigger('sourceopen');
	// master
	this.standardXHRResponse(this.requests.shift());
	// media
	this.standardXHRResponse(this.requests.shift());

	const mediaChanges = [];
	const playlistLoader = this.masterPlaylistController.masterPlaylistLoader_;
	const currentMedia = playlistLoader.media();
	const origMedia = playlistLoader.media.bind(playlistLoader);
	const origWarn = videojs.log.warn;
	const warnings = [];

	this.masterPlaylistController.masterPlaylistLoader_.media = (media) => {
	if (media) {
	mediaChanges.push(media);
	}
	return origMedia(media);
	};

	videojs.log.warn = (text) => warnings.push(text);

	assert.notOk(currentMedia.excludeUntil > 0, 'playlist not blacklisted');
	assert.equal(mediaChanges.length, 0, 'no media change');

	this.masterPlaylistController.mainSegmentLoader_.trigger('earlyabort');

	assert.ok(currentMedia.excludeUntil > 0, 'playlist blacklisted');
	assert.equal(mediaChanges.length, 1, 'one media change');
	assert.equal(warnings.length, 1, 'one warning logged');
	assert.equal(
	warnings[0],
	`Problem encountered with playlist ${currentMedia.id}. ` +
	'Aborted early because there isn\'t enough bandwidth to complete the ' +
	`request without rebuffering. Switching to playlist ${mediaChanges[0].id}.`,
	'warning message is correct'
	);

	videojs.log.warn = origWarn;
	});

fix(experimentalBufferBasedABR): call selectPlaylist and change media on an interval #978

fix(experimentalBufferBasedABR): call selectPlaylist and change media on an interval #978

Conversation

gkatsev commented Oct 23, 2020 • edited Loading

brandonocasey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Oct 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Oct 23, 2020 •

edited

Loading