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

tf · 2021-01-25T12:23:28Z

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 · 2021-01-25T12:23:30Z

💖 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 · 2021-01-25T12:59:45Z

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 · 2021-01-25T13:07:57Z

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 · 2021-01-25T13:40:13Z

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 · 2021-02-19T07:44:06Z

Could somebody take a look at this PR?

src/playlist-selectors.js

test/playlist-selectors.test.js

src/playlist-selectors.js

brandonocasey

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?

src/playlist-selectors.js

test/playlist-selectors.test.js

tf · 2021-03-02T15:55:40Z

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

stale · 2021-06-26T03:05:11Z

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 · 2022-01-08T22:56:52Z

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.

adrums86

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 · 2024-01-17T06:20:45Z

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

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

codecov · 2025-02-05T05:08:41Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.00%. Comparing base (c1d3186) to head (4b5a9d9).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1051   +/-   ##
=======================================
  Coverage   83.99%   84.00%           
=======================================
  Files          44       44           
  Lines       11700    11707    +7     
  Branches     2618     2623    +5     
=======================================
+ Hits         9827     9834    +7     
  Misses       1873     1873

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

welcome · 2025-02-05T05:22:08Z

Congrats on merging your first pull request! 🎉🎉🎉

dzianis-dashkevich · 2025-02-05T05:22:47Z

@tf thanks! < 5 years :)

gkatsev reviewed Mar 1, 2021

View reviewed changes

src/playlist-selectors.js Show resolved Hide resolved

src/playlist-selectors.js Show resolved Hide resolved

test/playlist-selectors.test.js Outdated Show resolved Hide resolved

src/playlist-selectors.js Outdated Show resolved Hide resolved

tf force-pushed the object-fit-selector branch from 4dc68f6 to d63a062 Compare March 2, 2021 08:48

brandonocasey reviewed Mar 2, 2021

View reviewed changes

src/playlist-selectors.js Outdated Show resolved Hide resolved

test/playlist-selectors.test.js Outdated Show resolved Hide resolved

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

stale bot added the outdated label Jun 26, 2021

gkatsev removed the outdated label Jul 2, 2021

stale bot added the outdated label Jan 8, 2022

misteroneill added needs: review and removed outdated labels Feb 14, 2022

adrums86 approved these changes Jan 16, 2024

View reviewed changes

tf added 4 commits January 17, 2024 06:22

Add usePlayerObjectFit option

c6ea01a

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 force-pushed the object-fit-selector branch from 00ebda7 to c6ea01a Compare January 17, 2024 05:50

Merge branch 'main' into object-fit-selector

bc12f4c

dzianis-dashkevich approved these changes Feb 5, 2025

View reviewed changes

chore: add object-fit option to the demo page

4b5a9d9

dzianis-dashkevich merged commit 2c52076 into videojs:main Feb 5, 2025
7 checks passed

tf deleted the object-fit-selector branch February 5, 2025 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tf commented Jan 25, 2021 •

edited

Loading

welcome bot commented Jan 25, 2021

tf commented Jan 25, 2021 •

edited

Loading

tf commented Jan 25, 2021

tf commented Jan 25, 2021 •

edited

Loading

tf commented Feb 19, 2021

brandonocasey left a comment

tf commented Mar 2, 2021

stale bot commented Jun 26, 2021

stale bot commented Jan 8, 2022

adrums86 left a comment

tf commented Jan 17, 2024

codecov bot commented Feb 5, 2025 •

edited

Loading

welcome bot commented Feb 5, 2025

dzianis-dashkevich commented Feb 5, 2025

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

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

Conversation

tf commented Jan 25, 2021 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

welcome bot commented Jan 25, 2021

tf commented Jan 25, 2021 • edited Loading

tf commented Jan 25, 2021

tf commented Jan 25, 2021 • edited Loading

tf commented Feb 19, 2021

brandonocasey left a comment

Choose a reason for hiding this comment

tf commented Mar 2, 2021

stale bot commented Jun 26, 2021

stale bot commented Jan 8, 2022

adrums86 left a comment

Choose a reason for hiding this comment

tf commented Jan 17, 2024

codecov bot commented Feb 5, 2025 • edited Loading

Codecov Report

welcome bot commented Feb 5, 2025

dzianis-dashkevich commented Feb 5, 2025

tf commented Jan 25, 2021 •

edited

Loading

tf commented Jan 25, 2021 •

edited

Loading

tf commented Jan 25, 2021 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading