fix: Restart masterPlaylistLoader after media change #1339
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1339 +/- ##
=======================================
Coverage 86.32% 86.32%
=======================================
Files 39 39
Lines 9856 9857 +1
Branches 2298 2298
=======================================
+ Hits 8508 8509 +1
Misses 1348 1348
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
We do call load on the main segment loader just below, so, this sounds good to me. |
| @@ -614,6 +614,8 @@ export class MasterPlaylistController extends videojs.EventTarget { | |||
| this.requestOptions_.timeout = requestTimeout; | |||
| } | |||
|
|
|||
| this.masterPlaylistLoader_.load(); | |||
There was a problem hiding this comment.
Shouldn't this happen in the dash/hls playlist loader itself?
There was a problem hiding this comment.
You mean like add a 'mediachange' listener to the constructor? I think we could. Does the same go for the mainSegmentLoader_ method calls below this? Should they be moved as well?
There was a problem hiding this comment.
So the master playlist loader fires mediachange. It should probably start to load by itself after that too? Could totally be wrong, as I haven't looked at the code in a bit now. I think having the mainSegmentLoader_ call load here is fine.
Description
Whenever a playlist is excluded for any reason, we pause the
masterPlaylistLoaderand never restart it. We have noticed this causing timeouts with live DASH playback which relies on regular MPD refreshes.If, for example, a single MPD or segment request fails for whatever reason, the
minimumUpdatePeriodtimeout gets cleared permanently, which halts any further MPD refreshes. This is a side-effect ofblacklistCurrentPlaylist(), which gets called on all MPL and MSL errors. The MPC eventually picks up on the fact that the MPD is no longer updating and starts a cascade of futile playlist switches which ultimately result in a timeout (futile because the lack of MPD refreshes means none of the playlists are updating).This may also be causing problems with HLS, although they are less apparent since the media playlist loaders continue fetching media playlists even when the
masterPlaylistLoaderis paused. I haven't spent much time yet thinking through the implications for HLS-- I'd appreciate some input there.Tentative Changes Proposed
Other loaders that are paused during the playlist exclusion process (ex.
AUDIO,SUBTITLES) get restarted on'mediachange', so it seems to make sense to do the same for themasterPlaylistLoader.Note: There was already a bit of discussion on Slack about this and there was some skepticism that this
'mediachange'handler is the best place to put theload()call. I've spent some time investigating better alternatives, but haven't yet come up with anything. I'm leaving this as a draft for now so we can discuss further.