feat: Add support for ManagedMediaSource 'startstreaming' and 'endstream' event handling #1542
Conversation
…ing event handling
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1542 +/- ##
==========================================
+ Coverage 86.37% 86.39% +0.02%
==========================================
Files 43 43
Lines 11155 11165 +10
Branches 2550 2552 +2
==========================================
+ Hits 9635 9646 +11
+ Misses 1520 1519 -1 ☔ View full report in Codecov by Sentry. |
src/playlist-controller.js
Outdated
| @@ -192,6 +192,7 @@ export class PlaylistController extends videojs.EventTarget { | |||
| this.playlistExclusionDuration = playlistExclusionDuration; | |||
| this.maxPlaylistRetries = maxPlaylistRetries; | |||
| this.enableLowInitialPlaylist = enableLowInitialPlaylist; | |||
| this.hasManagedMediaSource_ = false; | |||
There was a problem hiding this comment.
nit: To save a few lines of code, you could change this to:
this.hasManagedMediaSource_ = experimentalUseMMS && window.ManagedMediaSource;
There was a problem hiding this comment.
The intent of the flag was to signal that the ManagedMediaSource has been created and currently exists. If we set it here we should rename the flag to useManagedMediaSource_ or createManagedMediaSource_, as it hasn't been created yet.
src/playlist-controller.js
Outdated
| @@ -232,6 +235,14 @@ export class PlaylistController extends videojs.EventTarget { | |||
| // we don't have to handle sourceclose since dispose will handle termination of | |||
| // everything, and the MediaSource should not be detached without a proper disposal | |||
|
|
|||
| if (this.hasManagedMediaSource_) { | |||
| this.handleStartStreaming_ = this.handleStartStreaming_.bind(this); | |||
There was a problem hiding this comment.
You can add the content of this if statement to the above pre-existing if statement. All nits, but saves a few lines
There was a problem hiding this comment.
You mean to the if (experimentalUseMMS && window.ManagedMediaSource) {...} block? It is more concise but I opted against it because to my eye it made the code slightly less organized by putting the new addEventListener calls inside the block where the MediaSource is set rather than in the subsequent step/section where the other MediaSource event handlers are added. Minor issue, I'm happy to change it
There was a problem hiding this comment.
Yeah, all my comments were basically to save some lines of code by moving this code to where you set the hasManagedMediaSource flag and making the flag unnecessary, but fine with me if it keeps it cleaner in your eyes. 👍
src/playlist-controller.js
Outdated
| @@ -232,6 +235,14 @@ export class PlaylistController extends videojs.EventTarget { | |||
| // we don't have to handle sourceclose since dispose will handle termination of | |||
| // everything, and the MediaSource should not be detached without a proper disposal | |||
|
|
|||
| if (this.hasManagedMediaSource_) { | |||
| this.handleStartStreaming_ = this.handleStartStreaming_.bind(this); | |||
There was a problem hiding this comment.
Yeah, all my comments were basically to save some lines of code by moving this code to where you set the hasManagedMediaSource flag and making the flag unnecessary, but fine with me if it keeps it cleaner in your eyes. 👍
src/playlist-controller.js
Outdated
| this.handleEndStreaming_ = this.handleEndStreaming_.bind(this); | ||
|
|
||
| this.mediaSource.addEventListener('startstreaming', this.handleStartStreaming_); | ||
| this.mediaSource.addEventListener('endstreaming', this.handleEndStreaming_); |
There was a problem hiding this comment.
I guess we can remove the hasManagedMediaSource_ flag and simply add these listeners alongside other listeners; we should not care about the type of the media source
src/playlist-controller.js
Outdated
| handleEndStreaming_() { | ||
| this.pause(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we really need these wrappers for load and pause?
There was a problem hiding this comment.
We don't. If the logic ever becomes more sophisticated that just load/pause in the future, then we would need them. We can cross that bridge if we get there, I'm fine removing them now
There was a problem hiding this comment.
yeah, at this point seems redundant
Description
This PR adds onto the experimental support for
ManagedMediaSource. When the user agent determines that a ManagedMediaSource has enough data buffered to ensure uninterrupted playback, or does not have enough to ensure it, it will fire aendstreamingorstartstreamingevent, indicating that the player should stop loading new segments or start loading new segments, respectively.Specific Changes proposed
Add 2 new event handlers for
startstreamingandendstreaming. The former will callplaylistController.load(), which was a preexisting method that callsload()on all active segment loaders. The latter callsplaylistController.pause(), which is a new method that callspause()on all active segment loaders.Requirements Checklist
References
https://www.w3.org/TR/media-source-2/#dom-managedmediasource-streaming
w3c/media-source#320