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

Add forceSafariHLS option for FilePlayer #1560

Merged
merged 3 commits into from
Mar 7, 2023

Conversation

nabeards
Copy link
Contributor

@nabeards nabeards commented Jan 5, 2023

I have a need for the Safari native HLS player to load on macOS so I've added an option to do so.

The reason for this is because hls.js loads the <video> element with a src that's a blob. This breaks certain <canvas> features when working with the <video> DOM element on Safari 15.x.

Question: is there a way to locally install this as an NPM module for local development? Other modules I've worked with have been able to work that way, but trying with react-player doesn't work that way.

@cookpete cookpete changed the title Add forceAppleHLS option for FilePlayer Add forceSafariHLS option for FilePlayer Mar 7, 2023
@cookpete
Copy link
Owner

cookpete commented Mar 7, 2023

Updated prop name to forceSafariHLS – "apple" is a bit ambiguous, and now the prop is consistent with the IS_SAFARI matching logic.

Also tidied the logic a bit to match the forceHLS logic.

Nice work!

@cookpete cookpete merged commit 80fbb16 into cookpete:master Mar 7, 2023
@nabeards
Copy link
Contributor Author

Thanks @cookpete !

@nabeards
Copy link
Contributor Author

nabeards commented Mar 21, 2023

@cookpete Was about to integrate the changes, but I just realized that I think you reversed the logic here. The idea is to force the native HLS player on macOS Safari. Instead, you've made it force the hls.js player.

@nabeards
Copy link
Contributor Author

@cookpete just following up on this message for feedback. Should I submit a new PR for my use case?

@lvnam96
Copy link

lvnam96 commented Jun 15, 2023

@nabeards If you submit new PR, I think you should name the prop to forceNativeHls then have your own isSafariOnMacOS boolean passing to that prop, & react-player should not init hls.js instance if the boolean is truthy.

@cookpete is this prop forceSafariHLS a bit redundant? As I understand, shouldUseHls() will only skip iOS devices. I don't know why we need this prop to bypass HLS extension check at the end of this method.

@nabeards
Copy link
Contributor Author

nabeards commented Aug 2, 2023

@lvnam96 my original name was forceSafariHls but forceNativeHls is good as well. Would love to hear from @cookpete if they are open to this change. 🙂

@cookpete
Copy link
Owner

cookpete commented Aug 31, 2023

@cookpete Was about to integrate the changes, but I just realized that I think you reversed the logic here. The idea is to force the native HLS player on macOS Safari. Instead, you've made it force the hls.js player.

@nabeards Yep, you’re right. I misinterpreted forceAppleHLS to mean "force Apple desktop (Safari) to use hls.js". My bad! forceNativeHLS is much more clear.

@cookpete is this prop forceSafariHLS a bit redundant? As I understand, shouldUseHls() will only skip iOS devices. I don't know why we need this prop to bypass HLS extension check at the end of this method.

@lvnam96 I think you’re right. The question is – would removing it be considered a breaking change that requires a major version bump? I am tempted to say no for quality of life, but happy to hear opinions.

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