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 HLS playback support for web browsers #4352

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bhyoo99
Copy link

@bhyoo99 bhyoo99 commented Dec 28, 2024

Summary

Add HLS playback support for web browsers

Motivation

Some web browsers do not support native HLS playback, requiring hls.js implementation for cross-browser compatibility

Changes

  • Add hls.js package
  • Implement HLS stream validation logic
  • Add HLS player initialization for browser compatibility

Test plan

  1. HLS stream playback testing:
    • Verify HLS stream playback in Chrome and Firefox
    • Confirm proper functionality in browsers without native HLS support
    • Check for memory leaks during stream load/unload

Copy link
Member

@KrzysztofMoch KrzysztofMoch left a comment

Choose a reason for hiding this comment

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

Hey, Thank You for the PR!

@@ -26,6 +26,7 @@
"@typescript-eslint/eslint-plugin": "^6.7.4",
"eslint": "^8.19.0",
"eslint-plugin-jest": "^27.4.2",
"hls.js": "^1.5.18",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add this also as optional peer dependency

Copy link
Author

Choose a reason for hiding this comment

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

@KrzysztofMoch like this?

"peerDependencies": {
        "react": "*",
        "react-native": "*",
        "hls.js": "^1.5.18"
 },

Copy link
Collaborator

Choose a reason for hiding this comment

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

@bhyoo99 The best would be to put it in optionnalDependency: https://docs.npmjs.com/cli/v8/configuring-npm/package-json#optionaldependencies
It will allow to not embed the package is not needed.
It would be interesting to put the information in the documentation also.

@freeboub
Copy link
Collaborator

  • ping @zoriya
    I am not sure of the implementation, what will happen when switching from hls to dash and the opposite ...

Copy link
Collaborator

@zoriya zoriya left a comment

Choose a reason for hiding this comment

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

This implementation lacks some options, namely:

  • source containing headers (using hls's xhrSetup)
  • video/audio/subtitles tracks reporting (with the onInfo prop)
  • selection of video/audio/subtitle (selectedVideoTrack, selectedAudioTrack & selectedTextTrack props)
  • error handling
  • startPosition support

For headers, it could be nice to consider #2884.
I also know that error handling is planned to be reworked, idk if there's somewhere we can refer to on how to implement this for hls?

Regarding HLS.js as a whole, it was my first guess since I never needed more than hls for kyoo, but for rnv we might want to consider something a bit more feature-full. I considered both shaka and video.js when I first did #3958 (and I think I preferred shaka, but not sure anymore tbh.) Does anyone have an opinion on this? If not, we might want to research a bit and list drawbacks/advantages of all solutions before actually implementing anything.

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.

4 participants