Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Keep media update timeout alive so live playlists can recover #1176

Merged
merged 8 commits into from
Aug 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions src/master-playlist-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 || !currentPlaylist.endList) {
log(`${sharedLogLine} as current playlist ` + (!currentPlaylist ? 'is not set' : 'is live'));
if (!currentPlaylist) {
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;

Expand Down Expand Up @@ -351,7 +356,7 @@ export class MasterPlaylistController extends videojs.EventTarget {
checkABR_() {
const nextPlaylist = this.selectPlaylist();

if (this.shouldSwitchToMedia_(nextPlaylist)) {
if (nextPlaylist && this.shouldSwitchToMedia_(nextPlaylist)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor fix here. I saw that we throw an error if nextPlaylist is undefined.

this.switchMedia_(nextPlaylist, 'abr');
}
}
Expand Down
51 changes: 40 additions & 11 deletions src/playlist-loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
Expand Down Expand Up @@ -619,6 +613,13 @@ 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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be worth a comment above here saying why we update the timeout even if it's switching to the active playlist

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added


// switching to the active playlist is a no-op
if (!mediaChange) {
return;
Expand Down Expand Up @@ -679,8 +680,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.
Expand All @@ -705,14 +710,20 @@ export default class PlaylistLoader extends EventTarget {
* start loading of the playlist
*/
load(shouldDelay) {
window.clearTimeout(this.mediaUpdateTimeout);

if (this.mediaUpdateTimeout) {
window.clearTimeout(this.mediaUpdateTimeout);
this.mediaUpdateTimeout = null;
}
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(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo this should be called something else, but I think we should save it for the playlist-loader base class refactor as dash-playlist-loader does the same thing.

this.mediaUpdateTimeout = null;
this.load();
gkatsev marked this conversation as resolved.
Show resolved Hide resolved
}, delay);

return;
}

Expand All @@ -728,6 +739,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
*/
Expand Down
72 changes: 72 additions & 0 deletions test/playlist-loader.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -2101,6 +2117,62 @@ QUnit.module('Playlist Loader', function(hooks) {
);
});

QUnit.test('mediaupdatetimeout works as expected 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};
Expand Down