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

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

Merged
merged 7 commits into from
Oct 5, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,15 @@ 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.

##### limitRenditionByPlayerDimensions
* Type: `boolean`
* can be used as an initialization option

When `limitRenditionByPlayerDimensions` is set to true, rendition
selection logic will take into account the player size and rendition
resolutions when making a decision.
This setting is `true` by default.

### Runtime Properties
Runtime properties are attached to the tech object when HLS is in
use. You can get a reference to the HLS source handler like this:
Expand Down
2 changes: 2 additions & 0 deletions src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ export default {
GOAL_BUFFER_LENGTH: 30,
MAX_GOAL_BUFFER_LENGTH: 60,
GOAL_BUFFER_LENGTH_RATE: 1,
// 0.5 MB/s
INITIAL_BANDWIDTH: 4194304,
// A fudge factor to apply to advertised playlist bitrates to account for
// temporary flucations in client bandwidth
BANDWIDTH_VARIANCE: 1.2,
Expand Down
22 changes: 19 additions & 3 deletions src/playlist-selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -124,14 +124,17 @@ export const comparePlaylistResolution = function(left, right) {
* Current width of the player element
* @param {Number} playerHeight
* Current height of the player element
* @param {Boolean} limitRenditionByPlayerDimensions
* True if the player width and height should be used during the selection, false otherwise
* @return {Playlist} the highest bitrate playlist less than the
* currently detected bandwidth, accounting for some amount of
* bandwidth variance
*/
export const simpleSelector = function(master,
playerBandwidth,
playerWidth,
playerHeight) {
playerHeight,
limitRenditionByPlayerDimensions) {
// convert the playlists to an intermediary representation to make comparisons easier
let sortedPlaylistReps = master.playlists.map((playlist) => {
let width;
Expand Down Expand Up @@ -190,6 +193,17 @@ export const simpleSelector = function(master,
(rep) => rep.bandwidth === highestRemainingBandwidthRep.bandwidth
)[0];

// if we not going to limit renditions by player size, make an early decision.
theRealWardo marked this conversation as resolved.
Show resolved Hide resolved
if (!limitRenditionByPlayerDimensions) {
let chosenRep = (
bandwidthBestRep ||
enabledPlaylistReps[0] ||
sortedPlaylistReps[0]
);
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need a newline here to fix linting errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


return chosenRep ? chosenRep.playlist : null;
}

// filter out playlists without resolution information
let haveResolution = bandwidthPlaylistReps.filter((rep) => rep.width && rep.height);

Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@theRealWardo theRealWardo Oct 2, 2018

Choose a reason for hiding this comment

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

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

};

/**
Expand Down Expand Up @@ -294,7 +309,8 @@ export const movingAverageBandwidthSelector = function(decay) {
return simpleSelector(this.playlists.master,
average,
parseInt(safeGetComputedStyle(this.tech_.el(), 'width'), 10),
parseInt(safeGetComputedStyle(this.tech_.el(), 'height'), 10));
parseInt(safeGetComputedStyle(this.tech_.el(), 'height'), 10),
this.limitRenditionByPlayerDimensions);
};
};

Expand Down
10 changes: 4 additions & 6 deletions src/videojs-http-streaming.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,6 @@ const Hls = {
xhr: xhrFactory()
};

// 0.5 MB/s
const INITIAL_BANDWIDTH = 4194304;

// Define getter/setters for config properites
[
'GOAL_BUFFER_LENGTH',
Expand Down Expand Up @@ -335,6 +332,7 @@ class HlsHandler extends Component {
setOptions_() {
// defaults
this.options_.withCredentials = this.options_.withCredentials || false;
this.options_.limitRenditionByPlayerDimensions = this.options_.limitRenditionByPlayerDimensions || true;
Copy link
Contributor

@forbesjo forbesjo Sep 26, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://media3.giphy.com/media/14aUO0Mf7dWDXW/giphy.gif


if (typeof this.options_.blacklistDuration !== 'number') {
this.options_.blacklistDuration = 5 * 60;
Expand All @@ -343,17 +341,17 @@ class HlsHandler extends Component {
// start playlist selection at a reasonable bandwidth for
// broadband internet (0.5 MB/s) or mobile (0.0625 MB/s)
if (typeof this.options_.bandwidth !== 'number') {
this.options_.bandwidth = INITIAL_BANDWIDTH;
this.options_.bandwidth = Config.INITIAL_BANDWIDTH;
}

// If the bandwidth number is unchanged from the initial setting
// then this takes precedence over the enableLowInitialPlaylist option
this.options_.enableLowInitialPlaylist =
this.options_.enableLowInitialPlaylist &&
this.options_.bandwidth === INITIAL_BANDWIDTH;
this.options_.bandwidth === Config.INITIAL_BANDWIDTH;

// grab options passed to player.src
['withCredentials', 'bandwidth'].forEach((option) => {
['withCredentials', 'limitRenditionByPlayerDimensions', 'bandwidth'].forEach((option) => {
if (typeof this.source_[option] !== 'undefined') {
this.options_[option] = this.source_[option];
}
Expand Down
5 changes: 5 additions & 0 deletions test/configuration.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ const options = [{
default: false,
test: true,
alt: false
}, {
name: 'limitRenditionByPlayerDimensions',
default: true,
test: true,
alt: false
}, {
name: 'bandwidth',
default: 4194304,
Expand Down
34 changes: 33 additions & 1 deletion test/playlist-selectors.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,39 @@ test('simpleSelector switches up even without resolution information', function(
{ attributes: { BANDWIDTH: 1000 } }
];

const selectedPlaylist = simpleSelector(master, 2000, 1, 1);
const selectedPlaylist = simpleSelector(master, 2000, 1, 1, false);

assert.equal(selectedPlaylist, master.playlists[1], 'selected the correct playlist');
});

// A set of playlists that were defined using non-traditional encoding.
// The resolutions were selected using a per-title encoding technique
// that ensures the resolution maximizes quality at a given bitrate.
const trickyPlaylists = [
{ attributes: { BANDWIDTH: 2362080, RESOLUTION: { width: 1280, height: 720 } } },
{ attributes: { BANDWIDTH: 1390830, RESOLUTION: { width: 1280, height: 720 } } },
{ attributes: { BANDWIDTH: 866114, RESOLUTION: { width: 1024, height: 576 } } },
{ attributes: { BANDWIDTH: 573028, RESOLUTION: { width: 768, height: 432 } } },
{ attributes: { BANDWIDTH: 3482070, RESOLUTION: { width: 1920, height: 1080 } } },
{ attributes: { BANDWIDTH: 6151620, RESOLUTION: { width: 1920, height: 1080 } } }
];

test('simpleSelector limits using resolution information when it exists', function(assert) {
let master = this.hls.playlists.master;

master.playlists = trickyPlaylists;

const selectedPlaylist = simpleSelector(master, Config.INITIAL_BANDWIDTH, 444, 790, true);

assert.equal(selectedPlaylist, master.playlists[3], 'selected the playlist with the lowest bandwidth higher than player resolution');
});

test('simpleSelector can not limit based on resolution information', function(assert) {
let master = this.hls.playlists.master;

master.playlists = trickyPlaylists;

const selectedPlaylist = simpleSelector(master, Config.INITIAL_BANDWIDTH, 444, 790, false);

assert.equal(selectedPlaylist, master.playlists[4], 'selected a playlist based solely on bandwidth');
});