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

Consider ManagedSourceBuffer in MSE support detection #6846

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

niklaskorz
Copy link

@niklaskorz niklaskorz commented Nov 15, 2024

This PR will...

Consider ManagedSourceBuffer as well when detecting MSE support.

Why is this Pull Request needed?

On iOS Safari, isMSESupported(), and thus isSupported(), will currently always return false as the prototype checks on SourceBuffer fail, because self.SourceBuffer (and self.WebKitSourceBuffer) does not exist on iOS Safari.

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

The change is rather simple, so no.

Resolves issues:

No issue has been reported in this regard, but #6161 might be related.

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable) => there were no tests for isSupported() to begin with
  • API or design changes are documented in API.md => no API changes (getSourceBuffer is not exported)

@robwalch
Copy link
Collaborator

Hi @niklaskorz,

On iOS Safari, isMSESupported(), and thus isSupported(), will currently always return false

In which versions of iOS are you able to reproduce this issue?

I've been running HLS.js on iOS 17.1-18+ without this issue.

the prototype checks on SourceBuffer fail, because self.SourceBuffer (and self.WebKitSourceBuffer) does not exist on iOS Safari.

While correct, there are other platforms that do not expose SourceBuffer globally, therefor isMSESupported() and isSupported() still pass when getSourceBuffer() returns undefined:

// if SourceBuffer is exposed ensure its API is valid
// Older browsers do not expose SourceBuffer globally so checking SourceBuffer.prototype is impossible
const sourceBuffer = getSourceBuffer();
return (
!sourceBuffer ||

@niklaskorz
Copy link
Author

niklaskorz commented Nov 15, 2024

@robwalch I now realize I indeed misunderstood the purpose of the SourceBuffer check. It makes sense that needing to check for these functions on Managed MSE is not required in the first place, as the only (current) implementation of Managed MSE (WebKit) is feature-complete.

I'll double check on Monday how I came upon this in the first place, sorry for the noise!

@niklaskorz niklaskorz closed this Nov 15, 2024
@robwalch
Copy link
Collaborator

@niklaskorz no worries. The changes make sense. I had to debug by connecting an iPhone to understand why it was working.

I think we can take this change. I'll want to run them on the device too before giving a formal review.

@robwalch robwalch reopened this Nov 15, 2024
@robwalch robwalch self-requested a review November 15, 2024 19:44
@robwalch robwalch added this to the 1.6.0 milestone Nov 15, 2024
@robwalch
Copy link
Collaborator

robwalch commented Nov 15, 2024

As for #6161, the simulator is another story. I don't use iOS simulators for media testing Problems with playback in simulators should be reported to Apple via Feedback Assistant. Maybe there's a change we should make (in addition to this one) that would help developers know that they should test on a real device (or a real bug that needs to be fixes for hls.js? - let me know if you identify any).

Comment on lines +5 to +6
function getSourceBuffer(
preferManagedSourceBuffer = true,
Copy link
Collaborator

@robwalch robwalch Nov 18, 2024

Choose a reason for hiding this comment

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

The only place getSourceBuffer is called is in this module by isMSESupported. Adding the preferManagedSourceBuffer argument is unnecessary, as it is never passed in. That argument is used by player instances calling getMediaSource and not the static method isMSESupported used to check for the presence of MSE or compatible Managed Media Source.

Comment on lines +5 to +11
function getSourceBuffer(
preferManagedSourceBuffer = true,
): typeof self.SourceBuffer {
const msb =
(preferManagedSourceBuffer || !self.SourceBuffer) &&
((self as any).ManagedSourceBuffer as undefined | typeof self.SourceBuffer);
return msb || self.SourceBuffer || (self as any).WebKitSourceBuffer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be willing to merge with the following change. See previous comment for reasoning:

Suggested change
function getSourceBuffer(
preferManagedSourceBuffer = true,
): typeof self.SourceBuffer {
const msb =
(preferManagedSourceBuffer || !self.SourceBuffer) &&
((self as any).ManagedSourceBuffer as undefined | typeof self.SourceBuffer);
return msb || self.SourceBuffer || (self as any).WebKitSourceBuffer;
function getSourceBuffer(): typeof self.SourceBuffer {
return self.SourceBuffer || (self as any).ManagedSourceBuffer || (self as any).WebKitSourceBuffer;

Copy link
Collaborator

Choose a reason for hiding this comment

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

@niklaskorz would you be willing to make the requested changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants