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

Consider object-fit when selecting playlist by player size #1051

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tf
Copy link

@tf tf commented Jan 25, 2021

Description

So far, when limitRenditionByPlayerDimensions is true,
simpleSelector tried to either find a rendition with a resolution
that matches the size of the player exactly or, if that does not
exist, a rendition with the smallest resolution that has either
greater width or greater height than the player. This makes sense
since by default the video will be scaled to fit inside the media
element. So every resolution that exceeds player size in at least one
dimension will be scaled down.

Most browsers support customizing this scaling behavior via the
object-fit CSS property. If it set to cover, the video will
instead be scaled up if video and player aspect ratio do not match.

The previous behavior caused renditions with low resolution to be
selected for players with small width (e.g. portrait phone aspect
ratio) even when videos were then scaled up to cover the whole player.

We therefore detect if object-fit is set to cover and instead
select the smallest rendition with a resolution that exceeds player
dimensions in both width and height.

Specific Changes proposed

The first two commits are preparatory refactorings. The third commit contains the change itself.

Requirements Checklist

  • Feature implemented
  • If necessary, more likely in a feature request than a bug fix
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (see comment below)
  • Reviewed by Two Core Contributors

@welcome
Copy link

welcome bot commented Jan 25, 2021

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@tf
Copy link
Author

tf commented Jan 25, 2021

Both CI_TEST_TYPE=playback npm run test and CI_TEST_TYPE=unit npm run test pass locally. Not sure if failure of playback test in CI is related.

@tf
Copy link
Author

tf commented Jan 25, 2021

Regarding docs: A note about the feature could be added to the docs of limitRenditionByPlayerDimensions in the readme. The bitrate switching guide also seems relevant, feels a bit outdated already, though.

@tf
Copy link
Author

tf commented Jan 25, 2021

Here's an example showcasing the new behavior using a build that includes the changes from the PR branch.

https://jsbin.com/wuxabonezo/1/edit?html,output

@tf
Copy link
Author

tf commented Feb 19, 2021

Could somebody take a look at this PR?

@@ -133,27 +133,35 @@ export const comparePlaylistResolution = function(left, right) {
/**
* Chooses the appropriate media playlist based on bandwidth and player size
*
* @param {Object} master
* @param {Object} settings
Copy link
Member

Choose a reason for hiding this comment

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

thanks for updating the jsdoc!

limitRenditionByPlayerDimensions
) {
export const simpleSelector = function(settings) {
const {
Copy link
Member

Choose a reason for hiding this comment

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

At first, I wasn't sure about this PR because there were a lot of changes that seemed unrelated to the requested feature, however, upon closer inspection, I see that it's only changing a lot of arguments into an options object. Given that simpleSelector is only exported for tests, that seems fine to do.

@@ -203,11 +203,28 @@ test('simpleSelector limits using resolution information when it exists', functi
master,
bandwidth: 4194304,
playerWidth: 444,
playerHeight: 790,
playerHeight: 500,
Copy link
Member

Choose a reason for hiding this comment

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

why change the player height here?

Copy link
Author

@tf tf Mar 2, 2021

Choose a reason for hiding this comment

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

For the object-fit case, I wanted to test that it selects the smallest rendition that has both at least player width and player height. With the old player height value there would have been no rendition with greater height than the player height. So I needed a smaller player height to make sure it does not select a too big rendition.

I wanted the both test cases to only differ regarding "object-fit", to make it clear that that's what's causing the different result. Lowering the value here does not change anything since the rendition is chosen based on player width in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps it would be better to change behavior in a new test and keep this test as (except for the change to an options object for simpleSelector)

bandwidth: this.systemBandwidth,
playerWidth: parseInt(safeGetComputedStyle(this.tech_.el(), 'width'), 10) * pixelRatio,
playerHeight: parseInt(safeGetComputedStyle(this.tech_.el(), 'height'), 10) * pixelRatio,
playerObjectFit: safeGetComputedStyle(this.tech_.el(), 'objectFit'),
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 playerObjectFit should be a config option like limitRenditionByPlayerDimensions or useDevicePixelRatio for people to opt into. Changes in the ABR algorithm like this could potentially affect people's bandwidth usage and we try to have it be opt-in first and then eventually can be turned on by default (or made non-configurable).

Copy link
Author

@tf tf Mar 2, 2021

Choose a reason for hiding this comment

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

I've pushed a commit to add a usePlayerObjectFit option.

Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

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

I also see the tests failing due to a linter error here, not sure why it would fail in scripts/index-demo-page.js as you didn't make any changes there. Perhaps a rebase would fix that?

bandwidth: this.systemBandwidth,
playerWidth: parseInt(safeGetComputedStyle(this.tech_.el(), 'width'), 10) * pixelRatio,
playerHeight: parseInt(safeGetComputedStyle(this.tech_.el(), 'height'), 10) * pixelRatio,
playerObjectFit: safeGetComputedStyle(this.tech_.el(), 'objectFit'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we might be better to pass an empty string for playerObjectFit if usePlayerObjectFit is false and pass the actual value if usePlayerObjectFit is true. That keeps simpleSelector from having to worry about it at all.

Copy link
Author

@tf tf Mar 2, 2021

Choose a reason for hiding this comment

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

This would mean duplicating the logic in movingAverageBandwidthSelector and lastBandwidthSelector, though, where it is not covered by a test - like the pixel ratio logic. Happy to change it if you feel strongly about it.

@@ -203,11 +203,28 @@ test('simpleSelector limits using resolution information when it exists', functi
master,
bandwidth: 4194304,
playerWidth: 444,
playerHeight: 790,
playerHeight: 500,
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps it would be better to change behavior in a new test and keep this test as (except for the change to an options object for simpleSelector)

@tf tf force-pushed the object-fit-selector branch 2 times, most recently from c395d50 to 00ebda7 Compare March 2, 2021 15:54
@tf
Copy link
Author

tf commented Mar 2, 2021

Changed the test case to use the original value again. Rebased onto main for linting fix.

@stale
Copy link

stale bot commented Jun 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the outdated label Jun 26, 2021
@gkatsev gkatsev removed the outdated label Jul 2, 2021
@stale
Copy link

stale bot commented Jan 8, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Copy link
Contributor

@adrums86 adrums86 left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable addition. I think we should get this into the main branch.

Refactor from positional parameters to a single `settings`
argument. This makes it clearer what each argument means in calls of
the function. Additional parameters can now be added without making
the argument list overly long.

Key names were chosen to match those of
`minRebufferMaxBandwidthSelector` to align the signatures.
Inline the passed bandwidth value instead of referencing the config
constant since the concrete value is needed to understand why the
expected playlist is chosen. Also if the config constant should ever
change the test will fail for no good reason.
So far, when `limitRenditionByPlayerDimensions` is `true`,
`simpleSelector` tried to either find a rendition with a resolution
that matches the size of the player exactly or, if that does not
exist, a rendition with the smallest resolution that has either
greater width or greater height than the player. This makes sense
since by default the video will be scaled to fit inside the media
element. So every resolution that exceeds player size in at least one
dimension will be scaled down.

Most browsers support [1] customizing this scaling behavior via the
`object-fit` CSS property [2]. If it set to `cover`, the video will
instead be scaled up if video and player aspect ratio do not match.

The previous behavior caused renditions with low resolution to be
selected for players with small width (e.g. portrait phone aspect
ratio) even when videos were then scaled up to cover the whole player.

We therefore detect if `object-fit` is set to `cover` and instead
select the smallest rendition with a resolution that exceeds player
dimensions in both width and height.

[1] https://caniuse.com/?search=object-fit
[2] https://developer.mozilla.org/de/docs/Web/CSS/object-fit
Only consider `object-fit` CSS property when selecting playlists based
on play size when `usePlayerObjectFit` option is `true` to make new
behavior an opt-in.
@tf
Copy link
Author

tf commented Jan 17, 2024

Rebased onto main. Updated jsbin showcasing the new behavior using the latest version of Video.js:

https://jsbin.com/vipikadeni/edit?html,output

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants