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

feat: Add support for ManagedMediaSource 'startstreaming' and 'endstream' event handling #1542

Merged
merged 5 commits into from
Oct 9, 2024

Conversation

alex-barstow
Copy link
Contributor

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 a endstreaming or startstreaming event, indicating that the player should stop loading new segments or start loading new segments, respectively.

Specific Changes proposed

Add 2 new event handlers for startstreaming and endstreaming. The former will call playlistController.load(), which was a preexisting method that calls load() on all active segment loaders. The latter calls playlistController.pause(), which is a new method that calls pause() on all active segment loaders.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
  • Reviewed by Two Core Contributors

References

https://www.w3.org/TR/media-source-2/#dom-managedmediasource-streaming
w3c/media-source#320

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.39%. Comparing base (bfc17b4) to head (30e2fe5).

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.
📢 Have feedback on the report? Share it here.

@@ -192,6 +192,7 @@ export class PlaylistController extends videojs.EventTarget {
this.playlistExclusionDuration = playlistExclusionDuration;
this.maxPlaylistRetries = maxPlaylistRetries;
this.enableLowInitialPlaylist = enableLowInitialPlaylist;
this.hasManagedMediaSource_ = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: To save a few lines of code, you could change this to:

this.hasManagedMediaSource_ = experimentalUseMMS && window.ManagedMediaSource;

Copy link
Contributor Author

@alex-barstow alex-barstow Oct 3, 2024

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add the content of this if statement to the above pre-existing if statement. All nits, but saves a few lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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. 👍

Copy link
Contributor

@wseymour15 wseymour15 left a comment

Choose a reason for hiding this comment

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

Good work! Looks good to me

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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. 👍

this.handleEndStreaming_ = this.handleEndStreaming_.bind(this);

this.mediaSource.addEventListener('startstreaming', this.handleStartStreaming_);
this.mediaSource.addEventListener('endstreaming', this.handleEndStreaming_);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

handleEndStreaming_() {
this.pause();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need these wrappers for load and pause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, at this point seems redundant

@alex-barstow alex-barstow merged commit ae1ae70 into main Oct 9, 2024
15 checks passed
@alex-barstow alex-barstow deleted the feat/mms-startstreaming-endstreaming branch October 9, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants