fix: Add exclude reason and skip duplicate playlist-unchanged #1082
Conversation
src/master-playlist-controller.js
Outdated
| @@ -1066,6 +1075,9 @@ export class MasterPlaylistController extends videojs.EventTarget { | |||
|
|
|||
| // Blacklist this playlist | |||
| currentPlaylist.excludeUntil = Date.now() + (blacklistDuration * 1000); | |||
| if (error.reason) { | |||
| currentPlaylist.lastExcludeReason = error.reason; | |||
There was a problem hiding this comment.
For the short term, do you think we should add this as lastExcludeReason_ to prevent others from using it? Or maybe we should decide if it would make sense to have an array of reasons/what we want a public API on playlists should be.
There was a problem hiding this comment.
I think making it an underscore works as I think our whole exclusion mechanism needs a rework and I don't want anyone to rely on it.
There was a problem hiding this comment.
yeah, let's keep it private for now. We should think about this more formally for the longer term.
src/dash-playlist-loader.js
Outdated
| @@ -705,6 +705,9 @@ export default class DashPlaylistLoader extends EventTarget { | |||
| } | |||
|
|
|||
| updateMinimumUpdatePeriodTimeout_() { | |||
| if (!this.isMaster_) { | |||
There was a problem hiding this comment.
only add a minimum update period timer on the master playlist object.
src/dash-playlist-loader.js
Outdated
| @@ -728,7 +731,7 @@ export default class DashPlaylistLoader extends EventTarget { | |||
| // If we haven't yet selected a playlist, wait until then so we know the | |||
| // target duration | |||
| if (!this.media()) { | |||
| this.one('loadedplaylist', () => { | |||
| this.one('loadedmetadata', () => { | |||
There was a problem hiding this comment.
When media loads on the master playlist for the first time it's called loadedmetadata. loadedplaylist signifies the load of the master playlist.
src/dash-playlist-loader.js
Outdated
| @@ -791,9 +794,10 @@ export default class DashPlaylistLoader extends EventTarget { | |||
| this.trigger('playlistunchanged'); | |||
| } | |||
|
|
|||
| if (!this.media().endList) { | |||
| if (!this.media().endList && !this.mediaUpdateTimeout) { | |||
There was a problem hiding this comment.
only have one mediaupdatetimeout running at a time for an object. Mutex's ftw
src/dash-playlist-loader.js
Outdated
| } | ||
| } | ||
|
|
||
| // if minimumUpdatePeriod is invalid or <= zero skip creating a timeout |
There was a problem hiding this comment.
In what cases can this happen? Removed to become a VOD?
It might be worth some log messages here.
There was a problem hiding this comment.
i'm honestly not sure, that's just what the original code assumed. I would think that it is removed as the source becomes a VOD, and I will clarify that in the comment.
| // ignore unchanged playlists that have already been | ||
| // excluded for not-changing. We likely just have a really slowly updating | ||
| // playlist. | ||
| if (updatedPlaylist.lastExcludeReason_ === 'playlist-unchanged') { |
There was a problem hiding this comment.
Is it also worth checking the last excludeUntil date to see if we're passed the point where the exclusion should be considered?
There was a problem hiding this comment.
🤷🏻 It might be but I think that playlist-unchanged excludes are not likely to stop happening no matter how much time has passed.
| // if minimumUpdatePeriod is invalid or <= zero, which | ||
| // can happen when a live video becomes VOD. skip timeout | ||
| // creation. |
There was a problem hiding this comment.
I think we can use the DASH Interop as a reference:
https://dashif.org/docs/DASH-IF-IOP-v4.2-clean.htm
4.6.4. Transition Phase between Live and On-Demand
In the scenario for which the same MPD URL is used for live and On-Demand content, once the URL and publish time of the last Segment is known for the live service, and the duration of the service is known as well, the service provider acts as defined in clause 4.4.3.1, i.e.,
- adds the attribute MPD@mediaPresentationDuration
- removes the attribute MPD@minimumUpdatePeriod
If the value is < 0 we should probably issue a warning, as I don't think that should be valid.
src/dash-playlist-loader.js
Outdated
| @@ -752,6 +754,9 @@ export default class DashPlaylistLoader extends EventTarget { | |||
| // can happen when a live video becomes VOD. skip timeout | |||
| // creation. | |||
| if (typeof mup !== 'number' || mup <= 0) { | |||
| if (mup < 0) { | |||
| this.logger_('minimumUpdatePeriod is less then 0 and invalid'); | |||
There was a problem hiding this comment.
this.logger_(`found invalid minimumUpdatePeriod of ${mup}, not setting a timeout`);
|
Ran a live stream for ~20 minutes and it's working fine. |
Add a reason for exclusions and skip
playlist-unchangedif we have already seen it.