-
Notifications
You must be signed in to change notification settings - Fork 62
feat: cap-rendition-to-player-size #1263
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: cap-rendition-to-player-size #1263
Conversation
…r-react): clean up of rendition capping
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
@cjpillsbury is attempting to deploy a commit to the Mux Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
heff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few problems I see with the API:
We're pulling in the word "level" from HLS.js, but it's a vague term that we don't use anywhere else in the player settings docs. We use "resolution" otherwise.
We're explicitly calling out HLS.js, while otherwise we've only ever mentioned it as "(currently hls.js)". So this hardens the tie to HLS.js to some degree.
There's also not quite enough detail in the docs on the interaction between this, maxAutoResolution, maxResolution (not quite sure how this differs from the auto version), minResolution. Some quick notes in each setting about the interaction could solve this one.
Considering the future of MuxPlayer there's a world where we call this a "quick and advanced" feature and lean harder into that with the messaging: Say "ADVANCED" in the setting description, name it "capHlsLevel..." to be explicit abou the tie in the name.
But I think we should just polish it up.
- Change Level to Resolution
- Get clear on the interaction beteween related settings and make them clear in the docs.
luwes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me but yes def agree with Steve about the naming.
… informative in dev docs, per PR feedback. Remove hls.js references
…cap controller type (build + test issues with prior impl)
heff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API updates look good, thanks 👍
Exposes hls.js's
capRenditionToPlayerSizeoption with a Mux-optimized default.Closes #1238 (based off of initial effort from @spuppo-mux in #1251)
Behavior
undefined(default)truefalseTest plan
npm run dev→ http://localhost:3000/MuxPlayerdocument.querySelector('mux-player').media.nativeEl._hls.config.capLevelToPlayerSize