add an option to ignore player size in selection logic #238

theRealWardo · 2018-09-21T17:07:37Z

Description

Some modern encoding techniques target the best quality at a given bitrate, to some extent irrespective of resolution. In such scenarios, users should be able to tell the player that the player size should be ignored during rendition selection in order to favor a pure bandwidth based decision which will maximize quality for the playback.

Specific Changes proposed

add a limitRenditionByPlayerDimensions configuration option which can be set to false to ignore resolutions when doing rendition selection.

Requirements Checklist

Feature implemented / Bug fixed - awww yea
If necessary, more likely in a feature request than a bug fix
- Unit Tests updated or fixed - wrote some unit tests!
- Docs/guides updated - wrote docs in the README
- Example created - https://jsbin.com/gejugat/4/edit?html,output - I mean this is a pretty boring example as I'm just setting the config variable in it.
Reviewed by Two Core Contributors

welcome · 2018-09-21T17:07:40Z

💖 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.

forbesjo · 2018-09-21T17:10:55Z

src/playlist-selectors.js

@@ -217,6 +230,7 @@ export const simpleSelector = function(master,
    resolutionPlusOneList = haveResolution.filter(
      (rep) => rep.width > playerWidth || rep.height > playerHeight
    );
+    console.log('resolutionPlusOneList', haveResolution, resolutionPlusOneList, playerWidth, playerHeight);


Looks like there's logging left over from debugging

yep you right. removed it.

forbesjo · 2018-09-21T17:12:45Z

Seems reasonable to me, @gesinger thoughts?

forbesjo · 2018-09-21T17:14:20Z

src/playlist-selectors.js

+      bandwidthBestRep ||
+      enabledPlaylistReps[0] ||
+      sortedPlaylistReps[0]
+    );


You'll need a newline here to fix linting errors

yea so like I got a ton of linting errors relating to exceeds the maximum line length of 90, Don't make functions within a loop and Unexpected "todo" comment so I admittedly ignored all of them and just opened a PR... sorry

No worries, when we update our linter I think it should be more clear which of those is a linter error and just a warning

gesinger

Thanks for the contribution! This is definitely a nice flag to have.

gesinger · 2018-09-21T18:05:20Z

README.md

+* Type: `boolean`
+* can be used as an initialization option
+
+When `ignorePlayerSize` is set to true, selection logic will ignore the


selection logic => rendition selection logic

gesinger · 2018-09-21T18:06:10Z

README.md

+* can be used as an initialization option
+
+When `ignorePlayerSize` is set to true, selection logic will ignore the
+player size and rendition resolutions when making a decision.  This should


The should part here is a bit unclear to me, but I may not fully understand the scenario. Any more details on that part?

I am opening this PR because I had a customer using our per-title encoding solution which is marketed at https://mux.com/per-title-encoding/ and described in all kinds of fun technical detail at - https://mux.com/blog/per-title-encoding-scale/ - a specific video was seen to never play anything "reasonably good looking" given the generated rendition set which I put in the unit tests. most people in the industry would look at that ladder and say "what?" so I wrote this comment. but since the comment had a "what?" response, I guess I'll delete it. 😄

gesinger · 2018-09-21T18:08:22Z

README.md

@@ -333,6 +333,16 @@ When `enableLowInitialPlaylist` is set to true, it will be used to select
 the lowest bitrate playlist initially.  This helps to decrease playback start time.
 This setting is `false` by default.

+##### ignorePlayerSize


In terms of naming, I think it might be a bit easier to use a positive and have it be true by default. Also maybe a bit more precise for rendition selection. Maybe something like limitRenditionSelectionByPlayerDimensions, though that name could be too long.

I am more than happy to name this anything the maintainers like. just let me know.

Let's go with limitRenditionByPlayerDimensions, default to true

I believe I have updated all of the things!

gesinger

Minor comment wording, but otherwise LGTM

src/playlist-selectors.js

theRealWardo · 2018-09-21T21:58:39Z

oo there seems to be something wrong according to the tests... taking a look...

theRealWardo · 2018-09-21T22:40:34Z

maybe something is wrong in the defaults here, despite the unit tests saying they work, I saw a bunch of behavior that indicated that limitRenditionByPlayerDimensions was undefined. not sure what to do here as the tests that were failing in CI never pass on my machine even on master.

theRealWardo · 2018-09-24T23:58:29Z

@forbesjo @gesinger - mind taking one last look and merging this if you are comfortable with it please?

forbesjo · 2018-09-26T18:29:48Z

src/videojs-http-streaming.js

@@ -335,6 +332,7 @@ class HlsHandler extends Component {
  setOptions_() {
    // defaults
    this.options_.withCredentials = this.options_.withCredentials || false;
+    this.options_.limitRenditionByPlayerDimensions = this.options_.limitRenditionByPlayerDimensions || true;


Wont this always be true? In which case this option wont take affect if you set this to false in the player options instead of source options

theRealWardo · 2018-10-02T15:36:26Z

patched the silly mistake that @forbesjo found, fix the test that I misconfigured making be believe it was working correctly when it wasn't, and now I think its ready for another look. thanks!!

forbesjo

Looks good to me. @gesinger do you agree?

gesinger · 2018-10-02T16:20:31Z

src/playlist-selectors.js

@@ -261,7 +275,8 @@ export const lastBandwidthSelector = function() {
  return simpleSelector(this.playlists.master,
                        this.systemBandwidth,
                        parseInt(safeGetComputedStyle(this.tech_.el(), 'width'), 10),
-                        parseInt(safeGetComputedStyle(this.tech_.el(), 'height'), 10));
+                        parseInt(safeGetComputedStyle(this.tech_.el(), 'height'), 10),
+                        this.limitRenditionByPlayerDimensions);


Here and below, I don't think this value will ever be set, since limitRenditionByPlayerDimensions is set within the options_ object property and not on the HlsHandler instance directly.

I really should have spent some more time running the code via the debugger to make sure things were getting passed around. I did some more testing using this m3u8 which I created with Mux per title encoding - https://stream.mux.com/d3dYsgwvtxk101D6ExK3HuoKe3aps9IIA.m3u8 - you can pretty clearly spot the difference on the test page when setting limitRenditionByPlayerDimensions: false if you use this manifest.

this is my first PR for this repo so I was all like...

I'm going to update your contribution guidelines with some learnings from this process in another PR

add an option to ignore player size in selection logic

28fa98e

forbesjo reviewed Sep 21, 2018

View reviewed changes

gesinger reviewed Sep 21, 2018

View reviewed changes

theRealWardo added 2 commits September 21, 2018 11:32

review comments

077d9c2

so shall it be limitRenditionByPlayerDimensions

d29f14b

forbesjo approved these changes Sep 21, 2018

View reviewed changes

gesinger reviewed Sep 21, 2018

View reviewed changes

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

we to we're

8a97633

forbesjo reviewed Sep 26, 2018

View reviewed changes

gkatsev mentioned this pull request Sep 26, 2018

The size of the video player determines the HLS video quality videojs/video.js#5451

Closed

theRealWardo added 2 commits October 2, 2018 08:15

more clear and explicit false check

89c2de4

fix the test to test false

9b24234

theRealWardo force-pushed the mw-ignore-player-size branch from c2f70e0 to 9b24234 Compare October 2, 2018 15:30

forbesjo approved these changes Oct 2, 2018

View reviewed changes

gesinger reviewed Oct 2, 2018

View reviewed changes

plumbing

abc3eab

gesinger approved these changes Oct 5, 2018

View reviewed changes

forbesjo merged commit 7ae42b1 into videojs:master Oct 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add an option to ignore player size in selection logic #238

add an option to ignore player size in selection logic #238

theRealWardo commented Sep 21, 2018 •

edited by forbesjo

Loading

welcome bot commented Sep 21, 2018

forbesjo Sep 21, 2018

theRealWardo Sep 21, 2018

forbesjo commented Sep 21, 2018

forbesjo Sep 21, 2018

theRealWardo Sep 21, 2018

forbesjo Sep 21, 2018

gesinger left a comment

gesinger Sep 21, 2018

theRealWardo Sep 21, 2018

gesinger Sep 21, 2018

theRealWardo Sep 21, 2018

gesinger Sep 21, 2018

theRealWardo Sep 21, 2018

forbesjo Sep 21, 2018

theRealWardo Sep 21, 2018

gesinger left a comment

theRealWardo commented Sep 21, 2018

theRealWardo commented Sep 21, 2018

theRealWardo commented Sep 24, 2018

forbesjo Sep 26, 2018 •

edited

Loading

theRealWardo Oct 2, 2018

theRealWardo commented Oct 2, 2018

forbesjo left a comment

gesinger Oct 2, 2018

theRealWardo Oct 2, 2018 •

edited

Loading

add an option to ignore player size in selection logic #238

add an option to ignore player size in selection logic #238

Conversation

theRealWardo commented Sep 21, 2018 • edited by forbesjo Loading

Description

Specific Changes proposed

Requirements Checklist

welcome bot commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forbesjo commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gesinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gesinger left a comment

Choose a reason for hiding this comment

theRealWardo commented Sep 21, 2018

theRealWardo commented Sep 21, 2018

theRealWardo commented Sep 24, 2018

forbesjo Sep 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theRealWardo commented Oct 2, 2018

forbesjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theRealWardo Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

theRealWardo commented Sep 21, 2018 •

edited by forbesjo

Loading

forbesjo Sep 26, 2018 •

edited

Loading

theRealWardo Oct 2, 2018 •

edited

Loading