-
Notifications
You must be signed in to change notification settings - Fork 424
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: Custom Pixel Ratio #1497
feat: Custom Pixel Ratio #1497
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1497 +/- ##
=======================================
Coverage 86.26% 86.27%
=======================================
Files 43 43
Lines 10877 10884 +7
Branches 2501 2504 +3
=======================================
+ Hits 9383 9390 +7
Misses 1494 1494 ☔ View full report in Codecov by Sentry. |
src/playlist-selectors.js
Outdated
const pixelRatio = this.useDevicePixelRatio ? window.devicePixelRatio || 1 : 1; | ||
let pixelRatio = this.useDevicePixelRatio ? window.devicePixelRatio || 1 : 1; | ||
|
||
pixelRatio = pixelRatio * this.customPixelRatio; |
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.
shouldn't this use the customPixelRatio
directly, unconditionally? That way, there won't be double counting if both useDevicePixelRatio
and customPixelRatio
are set.
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.
This seems like a fairly rare case, but worth mentioning. I figured that if someone set both useDevicePixelRatio = true
and customPixelRatio = 2
, we would want the final pixel ratio to be cumulative. I could see the case where someone would want to save bandwidth or force higher renditions on top of whatever the device pixel ratio is.
Let me know if this seems wrong, but I do agree with @mister-ben that I can document this better.
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.
To me, only using the customPixelRatio
seems like a simpler and more straight forward direction. "If it's set, we're going to use this value".
Completely sidesteps needing to worry about the compounding effect. And if someone wanted to do so, they could set it themselves.
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.
Fair enough, I do not feel strongly either way. I made updates so we overwrite any previous pixel ratio with this value 👍
It might be good to explain in the docs how it works when |
@wseymour15, great work! |
Description
An optional
customPixelRatio
parameter. This enables the ABR algorithm to act as if the player dimensions were multiplied by a number greater than 0. The benefits of this are to manipulate the player to select higher renditions if better quality video is required, or lower renditions if it is desired to save on bandwidth.Example:
if you have a player where the dimension is
540p
, with a custom pixel ratio of2
, a rendition of1080p
or a lower rendition closest to this value will be chosen.Specific Changes proposed
customPixelRatio
option.Requirements Checklist
[ ] Example created (starter template on JSBin)