-
Notifications
You must be signed in to change notification settings - Fork 425
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
Changes from 2 commits
0f5ed77
378234a
e4aa070
9ecba52
9ea139f
a600a69
a812826
5f2c279
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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,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(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
|
@@ -728,6 +734,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 | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to remove these tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These only tested that live content always switches as far as I could tell. We might have to add one more back in depending on changes that I have to make in shouldSwitchToMedia. |
||
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}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the comment above, will this mean that we should have a way of ignoring the low water for live in the rest of this function?
And we may need to update this line and the comment above