feat: add experimental pixel diff selector behind a flag #786

brandonocasey · 2020-03-27T21:02:31Z

Simplify and improve our resolution selection by selecting the closest resolution to the player, rather than using one larger or the exact size.

brandonocasey · 2020-03-27T21:06:31Z

src/playlist-selectors.js

-  stableSort(haveResolution, (left, right) => left.width - right.width);
-
-  // if we have the exact resolution as the player use it
-  const resolutionBestRepList = haveResolution.filter((rep) => rep.width === playerWidth && rep.height === playerHeight);


We don't need this now that we use pixelDiff since the pixelDiff will be 0 for such a playlist.

brandonocasey · 2020-03-31T19:05:51Z

test/master-playlist-controller.test.js

+    const playlists = this.masterPlaylistController.master().playlists;
+    const currenstPlaylist = this.masterPlaylistController.media();
+
+    return playlists.find((playlist) => playlist !== currenstPlaylist);


Made these more dynamic

brandonocasey · 2020-03-31T19:05:58Z

test/videojs-http-streaming.test.js

@@ -1059,7 +1059,7 @@ QUnit.test('selects a playlist below the current bandwidth', function(assert) {
 });

 QUnit.test(
-  'selects a primary rendtion when there are multiple rendtions share same attributes',
+  'selects a primary rendition when there are multiple rendtions share same attributes',


brandonocasey · 2020-03-31T19:06:20Z

Will write tests for this if we decide to go with this change.

test/master-playlist-controller.test.js

gesinger · 2020-04-10T13:12:50Z

test/videojs-http-streaming.test.js

@@ -1219,12 +1219,12 @@ QUnit.test('selects the correct rendition by tech dimensions', function(assert)

  assert.deepEqual(
    playlist.attributes.RESOLUTION,
-    {width: 960, height: 540},
+    {width: 396, height: 224},


Although in many cases, particularly when there's a good number of available renditions, selecting the closest rendition rather than the closest plus one rendition makes sense, in a case like this one, the quality will be pretty degraded compared to the previous logic. We may want to make a reasonable allotment for how many fewer pixels we'll allow before just using the higher rendition, especially if there's plenty of bandwidth available to accommodate.

gkatsev · 2020-04-10T19:40:45Z

I think we want to hold off on merging this until we get closer to working on our ABR epic.

stale · 2020-08-22T19:09:24Z

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 · 2020-12-12T21:03:25Z

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.

codecov · 2021-04-08T17:40:22Z

Codecov Report

Merging #786 (f10d133) into main (b3d1ec0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #786      +/-   ##
==========================================
+ Coverage   86.47%   86.49%   +0.01%     
==========================================
  Files          39       39              
  Lines        9577     9590      +13     
  Branches     2210     2214       +4     
==========================================
+ Hits         8282     8295      +13     
  Misses       1295     1295

Impacted Files	Coverage Δ
src/videojs-http-streaming.js	`90.30% <ø> (ø)`
src/master-playlist-controller.js	`94.60% <100.00%> (+<0.01%)`	⬆️
src/playlist-selectors.js	`87.05% <100.00%> (+0.98%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3d1ec0...f10d133. Read the comment docs.

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

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.

brandonocasey · 2021-07-14T18:10:52Z

scripts/index-demo-page.js

+      'liveui',
+      'pixel-diff-selector'
+    ].forEach(function(name) {
+      stateEls[name].addEventListener('change', function(event) {


merged all the handlers that reload the player, since I was adding a new one.

gesinger

Did we want to note this in the README?

src/playlist-selectors.js

gesinger · 2021-07-27T19:10:07Z

src/playlist-selectors.js

+    // find the smallest variant that is larger than the player
+    // if there is no match of exact resolution


This comment is exactly the same as the one for resolutionPlusOneList, but the behavior is different. Maybe each should call out why they're choosing each method?

gesinger · 2021-07-27T19:12:11Z

src/playlist-selectors.js

@@ -296,7 +323,9 @@ export let simpleSelector = function(
  if (chosenRep && chosenRep.playlist) {
    let type = 'sortedPlaylistReps';

-    if (resolutionPlusOneRep) {
+    if (leastPixelDiffRep) {
+      type = 'leastPixelDiffRep';


Since we call it leastPixelDiffRep should we name the property experimentalLeastPixelDiffSelector?

Co-authored-by: Garrett Singer <[email protected]>

brandonocasey · 2021-07-27T19:17:10Z

I don't think that we want to add it to the README. We haven't added any of the other experimental options. I think we just need to do an a/b test with them and make them the default or remove them.

src/playlist-selectors.js

Co-authored-by: Garrett Singer <[email protected]>

brandonocasey commented Mar 27, 2020

View reviewed changes

brandonocasey force-pushed the fix/lowest-pixel-diff branch 3 times, most recently from 124ea78 to 0b58f53 Compare March 31, 2020 19:03

brandonocasey commented Mar 31, 2020

View reviewed changes

brandonocasey force-pushed the fix/lowest-pixel-diff branch 2 times, most recently from 92a04a9 to ddbfb5e Compare April 1, 2020 18:54

gesinger reviewed Apr 10, 2020

View reviewed changes

gkatsev added this to the 2.1 milestone May 21, 2020

gkatsev modified the milestones: 2.1, 2.2 Jun 12, 2020

gkatsev changed the base branch from master to main June 19, 2020 16:49

stale bot added the outdated label Aug 22, 2020

gkatsev removed the outdated label Aug 27, 2020

gkatsev modified the milestones: 2.2, ABR Sep 10, 2020

brandonocasey force-pushed the fix/lowest-pixel-diff branch from abd9354 to deb1c7f Compare September 25, 2020 16:46

stale bot added the outdated label Dec 12, 2020

gkatsev removed the outdated label Dec 13, 2020

brandonocasey added the needs investigation label Dec 15, 2020

stale bot added the outdated label Jun 26, 2021

gkatsev removed the outdated label Jul 2, 2021

brandonocasey force-pushed the fix/lowest-pixel-diff branch from a83a4ee to fdb522c Compare July 13, 2021 19:36

brandonocasey added needs: review and removed needs investigation labels Jul 13, 2021

brandonocasey changed the title ~~fix: use the closest resolution to the player~~ feat: add experimental pixel diff selector Jul 13, 2021

feat: add experimental pixel diff selector

1ebc14d

brandonocasey force-pushed the fix/lowest-pixel-diff branch from fdb522c to 1ebc14d Compare July 13, 2021 19:40

Merge branch 'main' into fix/lowest-pixel-diff

91ae5c8

brandonocasey commented Jul 14, 2021

View reviewed changes

Merge branch 'main' into fix/lowest-pixel-diff

b92f689

brandonocasey changed the title ~~feat: add experimental pixel diff selector~~ feat: add experimental pixel diff selector behind a flag Jul 16, 2021

Merge branch 'main' into fix/lowest-pixel-diff

516d0d0

gesinger reviewed Jul 27, 2021

View reviewed changes

Update src/playlist-selectors.js

8bddc0a

Co-authored-by: Garrett Singer <[email protected]>

brandonocasey added 2 commits July 27, 2021 15:18

rename option

d64a317

update comment

013e21f

gesinger previously approved these changes Jul 27, 2021

View reviewed changes

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

Update src/playlist-selectors.js

f10d133

Co-authored-by: Garrett Singer <[email protected]>

brandonocasey dismissed gesinger’s stale review via f10d133 July 27, 2021 19:44

brandonocasey merged commit a0c0359 into main Jul 27, 2021

brandonocasey deleted the fix/lowest-pixel-diff branch July 27, 2021 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add experimental pixel diff selector behind a flag #786

feat: add experimental pixel diff selector behind a flag #786

brandonocasey commented Mar 27, 2020

brandonocasey Mar 27, 2020

brandonocasey Mar 31, 2020

brandonocasey Mar 31, 2020

brandonocasey commented Mar 31, 2020

gesinger Apr 10, 2020

gkatsev commented Apr 10, 2020

stale bot commented Aug 22, 2020

stale bot commented Dec 12, 2020

codecov bot commented Apr 8, 2021 •

edited

Loading

stale bot commented Jun 26, 2021

brandonocasey Jul 14, 2021

gesinger left a comment

gesinger Jul 27, 2021

gesinger Jul 27, 2021

brandonocasey commented Jul 27, 2021

		// find the smallest variant that is larger than the player
		// if there is no match of exact resolution

feat: add experimental pixel diff selector behind a flag #786

feat: add experimental pixel diff selector behind a flag #786

Conversation

brandonocasey commented Mar 27, 2020

brandonocasey Mar 27, 2020

Choose a reason for hiding this comment

brandonocasey Mar 31, 2020

Choose a reason for hiding this comment

brandonocasey Mar 31, 2020

Choose a reason for hiding this comment

brandonocasey commented Mar 31, 2020

gesinger Apr 10, 2020

Choose a reason for hiding this comment

gkatsev commented Apr 10, 2020

stale bot commented Aug 22, 2020

stale bot commented Dec 12, 2020

codecov bot commented Apr 8, 2021 • edited Loading

Codecov Report

stale bot commented Jun 26, 2021

brandonocasey Jul 14, 2021

Choose a reason for hiding this comment

gesinger left a comment

Choose a reason for hiding this comment

gesinger Jul 27, 2021

Choose a reason for hiding this comment

gesinger Jul 27, 2021

Choose a reason for hiding this comment

brandonocasey commented Jul 27, 2021

codecov bot commented Apr 8, 2021 •

edited

Loading