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.

Sign up for GitHub

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

Jump to bottom

Bugfix: preferManagedMediaSource:false when MSE is not present #6338

Merged
merged 3 commits into from
Apr 10, 2024
Merged

Bugfix: preferManagedMediaSource:false when MSE is not present #6338

merged 3 commits into from
Apr 10, 2024

Conversation

Asen-O-Nikolov
Copy link
Contributor

@Asen-O-Nikolov Asen-O-Nikolov commented Apr 9, 2024 •
edited
Loading

This PR will...

Flip the BufferController.appendSource flag, in cases where preferManagedMediaSource config is set to false but MSE is not present.

Why is this Pull Request needed?

The API Docs state:

preferManagedMediaSource
...
Setting this to false will only use ManagedMediaSource when MediaSource is undefined.

This is indeed the case but the appendSource flag is never updated to handle the scenario as it's only set within the constructor:

this.appendSource =
      hls.config.preferManagedMediaSource &&
      typeof self !== 'undefined' &&
      (self as any).ManagedMediaSource;

, thus blocks like:

      if (this.appendSource) {
        ms.addEventListener('startstreaming', this._onStartStreaming);
        ms.addEventListener('endstreaming', this._onEndStreaming);
      }

and

if (this.appendSource) {
        try {
          media.removeAttribute('src');
          // ManagedMediaSource will not open without disableRemotePlayback set to false or source alternatives
          const MMS = (self as any).ManagedMediaSource;
          media.disableRemotePlayback =
            media.disableRemotePlayback || (MMS && ms instanceof MMS);
         ....

never get invoked.

What this PR aims to do is check if the elected MediaSource is MMS and ensure appendSource adequately represent the state of the world.

Are there any points in the code the reviewer needs to double check?

Not to my knowledge

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

robwalch
robwalch requested changes Apr 9, 2024
View reviewed changes
Copy link
Collaborator

@robwalch robwalch left a comment

Choose a reason for hiding this comment

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

Thank you for identifying this issue and providing a fix. Please see the requested changes.

src/utils/mediasource-helper.ts Outdated
@@ -15,3 +15,7 @@ export function getMediaSource(
((self as any).WebKitMediaSource as typeof MediaSource)
);
}

export function isManagedMediaSource(source: typeof MediaSource | undefined) {
return source === (self as any).ManagedMediaSource;
Copy link
Collaborator

Choose a reason for hiding this comment

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

self may be undefined in some environments.

Suggested change
return source === (self as any).ManagedMediaSource;
return typeof self !== 'undefined' &&
source === (self as any).ManagedMediaSource;

Copy link
Collaborator

Choose a reason for hiding this comment

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

With this change we should update the constructor to use this function

  constructor(hls: Hls, fragmentTracker: FragmentTracker) {
    super('buffer-controller', hls.logger);
    this.hls = hls;
    this.fragmentTracker = fragmentTracker;
    this.appendSource = isManagedMediaSource(
      getMediaSource(hls.config.preferManagedMediaSource)
    );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this, thank you

src/controller/buffer-controller.ts Outdated
@@ -207,6 +210,8 @@ export default class BufferController extends Logger implements ComponentAPI {
) {
const media = (this.media = data.media);
const MediaSource = getMediaSource(this.appendSource);
this.appendSource ||= isManagedMediaSource(MediaSource);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need the OR here. If it's not ManagedMediaSource for any reason, appendSource should be set to false.

Suggested change
this.appendSource ||= isManagedMediaSource(MediaSource);
this.appendSource = isManagedMediaSource(MediaSource);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When taking a step back and taking into consideration this comment.
This line is no longer needed thus I decided to remove it all together

@robwalch robwalch added this to the 1.5.8 milestone Apr 9, 2024
@robwalch robwalch merged commit 0d00e7e into video-dev:master Apr 10, 2024
12 checks passed
robwalch pushed a commit that referenced this pull request Apr 10, 2024
@Asen-O-Nikolov @robwalch
Bugfix: preferManagedMediaSource:false when MSE is not present (#6338)
54d62c7 
(cherry picked from commit 0d00e7e)
@Asen-O-Nikolov Asen-O-Nikolov deleted the bugfix/preferManagedMediaSource_selection branch April 11, 2024 05:30
robwalch added a commit that referenced this pull request Apr 11, 2024
@robwalch
Merge branch 'patch/v1.5.x'
f1a73b6 
* patch/v1.5.x:
  Bugfix: Patch for light player exception with audio groups (#6342)
  Do not use "levelCodec" to instantiate SourceBuffer when multiple audio codec strings are present in the Multivariant Playlist variant CODECS attribute with muxed content (#6341)
  Bugfix: preferManagedMediaSource:false when MSE is not present  (#6338)
  Determine canSkip based on age of last playlist request (#6300)
  API.md update removeLevel
@Tisankan Tisankan mentioned this pull request Sep 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robwalch robwalch robwalch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.5.8
Development

Successfully merging this pull request may close these issues.
2 participants
@Asen-O-Nikolov @robwalch