-
Notifications
You must be signed in to change notification settings - Fork 62
feat: Expose capLevelToPlayerSize property #1251
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
Conversation
|
@spuppo-mux is attempting to deploy a commit to the Mux Team on Vercel. A member of the Team first needs to authorize it. |
✅ 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. |
87a05b5 to
73eb9b5
Compare
5a64c0e to
3e53889
Compare
| value="" | ||
| checked={value === undefined} | ||
| /> | ||
| <label htmlFor={`${name}-none-control`}>None</label> |
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.
praise: 😎 glad you acknowledged the "3rd state" of "optional booleans" (aka "unset")
cjpillsbury
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.
I'm realizing this isn't in line with what's discussed in #1238. We shouldn't be using string values for booleans, and we shouldn't be adding disable-cap-level-to-player-size yet unless there is no way or relying on JavaScript to set the value to false. This should resolve our current pain points, and we can add additional props if/when needed. If this won't work or if you have questions or confusions here, please reach out.
| type="radio" | ||
| onChange={() => { | ||
| const changeValue = enumValue; | ||
| console.log("Selecting value:", changeValue, toChangeObject(name, changeValue)); |
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.
nitpick(blocking): remove the console.log before merging
| backward-seek-offset={10} | ||
| max-auto-resolution="720p" | ||
| cap-level-to-player-size={capLevelToPlayerSize ? true : undefined} | ||
| disable-cap-level-to-player-size={capLevelToPlayerSize === false ? true : undefined} |
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.
nitpick(blocking): should this still exist? I thought we were descoping this for the initial implementation.
| // envKey="mux-data-env-key" | ||
| controls | ||
| capLevelToPlayerSize={capLevelToPlayerSize === true} | ||
| disableCapLevelToPlayerSize={capLevelToPlayerSize === false} |
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.
nitpick(blocking): same as above
Exposes hls.js's `capRenditionToPlayerSize` option with a Mux-optimized default. Closes #1238 (based off of initial effort from @spuppo-mux in #1251) ### Behavior | Value | Behavior | |-------|----------| | `undefined` (default) | Caps to player size, 720p minimum floor | | `true` | Standard hls.js capping (may go below 720p) | | `false` | No capping | ### Test plan - [ ] `npm run dev` → http://localhost:3000/MuxPlayer - [ ] Toggle "Cap Level To Player Size" between None/true/false - [ ] Verify no console errors - [ ] Verify config: `document.querySelector('mux-player').media.nativeEl._hls.config.capLevelToPlayerSize` - [ ] Trigger a rendition switch/re-selection via ABR - e.g. open Chrome Dev Tools, resize player size, seek backwards during playback to prompt segment reloading, and confirm that rendition does(n't) get capped under different permutations --------- Co-authored-by: Santiago Puppo <[email protected]>
|
Closing as this was fixed on 9dfac1b |
Closes #1238
The purpose of this PR is to expose
hls.capLevelToPlayerSizeproperty while maintaining the current behavior (if nothing is set,hls.capLevelToPlayerSize = trueand we use ourMinCapLevelControllerinstead of HLS's default controller).Added two new attributes
cap-level-to-player-sizeanddisable-cap-level-to-player-size; the latter mainly to be used on React components.Web Components (
<mux-video>and<mux-player>)cap-level-to-player-sizehls.capLevelToPlayerSizewhen the HLS element is initialized using its own defaultCapLevelController. Note: If present will be true unless set to the string"false".hls.capLevelToPlayerSizeis set to true andMinCapLevelControlleris used instead.disable-cap-level-to-player-sizeIf present (and not set to"false"), will have the same behavior as settingcap-level-to-player-size="false". Takes precedence overcap-level-to-player-size.React Components (
MuxVideoandMuxPlayer)reactAttribute={false}is equivalent to settingreact-attribute=undefinedon the Web Components.capLevelToPlayerSize : boolean | undefinedtrue): setshls.capLevelToPlayerSizetotrueand sets HLS's default CapLevelController.falseorundefined):hls.capLevelToPlayerSizeis set to true andMinCapLevelControlleris used instead.disableCapLevelToPlayerSize : boolean | undefinedtrue): setshls.capLevelToPlayerSizetofalseand sets HLS's default CapLevelController.falseorundefined): Does nothing.For both cases (React and Web Components), getting the
capLevelToPlayerSizeproperty will return:falseif disableCapLevelToPlayerSize=true;_hlsConfig.capLevelToPlayerSizeif that value is defined;cap-level-to-player-sizeattribute !=="false"otherwise.