Add option to select lowest rendition available on startup #1212

OshinKaramian · 2017-07-26T19:36:33Z

Description

I'm still working through unit tests because many of them broke, but would like eyes on this sooner rather than later in case I went down a bizarre path.

This PR will select the lowest bitrate playlist with video on startup. If no such playlist exists, it picks the lowest bitrate playlist (assuming everything else is audo only).

This feature is gated behind an options flag. In order to enable it set options to:

{
  html5: {
    hls: {
      enableLowInitialPlaylist: true
   }
}

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

OshinKaramian · 2017-07-26T19:41:09Z

src/master-playlist-controller.js

@@ -66,36 +67,6 @@ const objectChanged = function(a, b) {
 };

 /**
- * Parses a codec string to retrieve the number of codecs specified,


This function doesn't seem to be reliant on being here (there are a few other functions here that could be moved as well). I shifted this into a util module as I wanted to use this to check whether a playlist contained video or not in the playlist selection algorithm below.

OshinKaramian · 2017-07-26T19:42:04Z

src/playlist-selectors.js

+    return playlistsWithVideo[0];
+  }
+
+  return playlists[0];


Made an assumption here that may or may not be true, where the playlist are sorted by bitrate. If that's not true I can add a sort in here.

You'll have to sort, no guarantee of order

mjneil · 2017-07-26T20:10:30Z

src/videojs-contrib-hls.js

@@ -328,6 +330,10 @@ class HlsHandler extends Component {
      this.selectPlaylist ?
        this.selectPlaylist.bind(this) : Hls.STANDARD_PLAYLIST_SELECTOR.bind(this);

+    this.masterPlaylistController_.initialSelectPlaylist =
+      this.selectPlaylist ?


After a source change, this will set the initialSelectPlaylist to be the standard selector and not the initial one. You should add another properties to the source handler for selectInitialPlaylist and then this becomes this.selectInitialPlaylist ? this.selectInitialPlaylist.bind(this) : Hls.INITIAL_PLAYLIST_SELECTOR.bind(this).

side note, by my snippet you'll see i personally prefer selectInitialPlaylist over initialSelectPlaylist, but either is descriptive and fine, so feel free to change or keep the naming. @gesinger probably has his own preference

Good catch, I reworked things a bit (I set the selection in the master-playlist-controller) and must have mucked that up.

mjneil · 2017-07-27T20:59:23Z

src/playlist-selectors.js

+ *         exists pick the lowest audio rendition.
+ */
+export const lowestBitrateCompatibleVariantSelector = function() {
+  const { playlists } = this.playlists.master;


since sort messes with the array, you should use a copy of playslists with slice

mjneil · 2017-07-27T21:01:00Z

src/playlist-selectors.js

+  const playlistsWithVideo =
+    playlists.filter(playlist => parseCodecs(playlist.attributes.CODECS).videoCodec);
+
+  const selectedPlaylists = playlistsWithVideo.length !== 0 ? playlistsWithVideo : playlists;


you could omit this and just return playlistsWithVideo[0] || playlists[0];

mjneil · 2017-07-27T21:04:36Z

test/master-playlist-controller.test.js

@@ -212,6 +212,50 @@ QUnit.test('resets SegmentLoader when seeking in flash for both in and out of bu

  });

+QUnit.only('selects lowest bitrate rendition when enableLowInitialPlaylist is set',
+  function(assert) {
+    this.player = createPlayer({ html5: { hls: { enableLowInitialPlaylist: true } } });


since the initial selector is called within the loadedplaylist event handler which fires everytime a live playlist is refreshed, I think it would be worth simulating a live refresh and confirming that the initial selector is not called a second time

mjneil · 2017-07-27T21:17:48Z

test/master-playlist-controller.test.js

+    let numCallsToSelectPlaylist = 0;
+
+    // master
+    this.standardXHRResponse(this.requests.shift());


These two responses should go after the sourceopen trigger, also I would expect that the numCallsToSelectInitialPlaylistCalls should increment after just the master response

mjneil · 2017-07-27T22:18:23Z

src/master-playlist-controller.js

+          this.enableLowInitialPlaylist ?
+            this.selectInitialPlaylist : this.selectPlaylist;
+
+        this.initialMedia_ = initialPlaylistSelector();


You can do this without needing to create the initialPlaylistSelector variable

mjneil · 2017-07-28T19:41:19Z

test/master-playlist-controller.test.js

+
+    this.clock.tick(1);
+
+    // Trigger playlist event which should utilize selectInitialPlaylist and


this comment is a bit outdated now

mjneil · 2017-07-28T19:42:34Z

test/master-playlist-controller.test.js

+
+    // Simulate a live reload
+    this.masterPlaylistController.masterPlaylistLoader_.trigger('loadedplaylist');
+    this.clock.tick(5);


shouldn't need this tick

mjneil · 2017-07-28T19:44:11Z

src/playlist-selectors.js

+/**
+ * Chooses the appropriate media playlist, which in this case is the lowest bitrate
+ * one with video.  If no renditions with video exist, return the lowest audio rendition.
+ *


Can you add Expects to be called within the context of an instance of HlsHandler to this. see lastBandwidthSelector jsdoc

mjneil · 2017-07-28T20:50:52Z

Do you think it would be appropriate to move the other codec functions/constants within MasterPlaylistController.js into utils/codecs.js?

specifically

defaultCodecs
mapLegacyAvcCodecs_
makeMimeTypeString
getContainerType
getCodecs
mimeTypesForPlaylist_

OshinKaramian · 2017-07-31T15:31:51Z

I do think so as I kind of started doing that then pulled back. I was trying to go with a softer touch on this PR (also it's unclear if this will end up getting folded into the greater project). I think doing that in a separate PR might make the most sense.

OshinKaramian · 2017-07-31T20:30:57Z

@mjneil I opened up #1217 in response to your comment about moving some more generic things out of master-playlist-controller.

forbesjo · 2017-08-01T17:30:41Z

src/master-playlist-controller.js

@@ -283,7 +254,8 @@ export class MasterPlaylistController extends videojs.EventTarget {
      bandwidth,
      externHls,
      useCueTags,
-      blacklistDuration
+      blacklistDuration,
+      enableLowInitialPlaylist


We'll want this added to the README eventually

OshinKaramian · 2017-08-03T21:04:13Z

src/master-playlist-controller.js

@@ -429,8 +402,17 @@ export class MasterPlaylistController extends videojs.EventTarget {
      let updatedPlaylist = this.masterPlaylistLoader_.media();

      if (!updatedPlaylist) {
-        // select the initial variant
-        this.initialMedia_ = this.selectPlaylist();
+        let selectedMedia;


Just a note here.

One of the tests was breaking on this (https://github.com/videojs/videojs-contrib-hls/blob/fast-start-time/test/videojs-contrib-hls.test.js#L1221) (I didn't have the temp variable, and instead set this.initialMedia_ twice, once in each if block).

Once I set things to the temporary variable the problem resolved itself. I'm assuming there are side effects off of setting this.initialMedia_... I didn't really dig deep enough to understand what was going on. It seems a little dangerous to have side effects off of setting a variable (when the setter isn't explicitly a function) if this is the case.

We should probably dig into why this was happening since I don't think it should cause the test to break

Looked into this a bit more, its because if you set this.initialMedia_ and then try and load that media, but the request fails e.g. 404, when on the final rendition, we get back to here, and updatedPlaylist is still undefined because we haven't successfully loaded one yet, but initialMedia_ has been set from the previous time here. I think it would make sense to set initialMedia_ in the loadedmetadata handler if it hasnt been set yet as the first time we get into loadedmetadata is after the first media playlist has been loaded.

That loop seems like a code smell to me, though I can't think of a better way to deal with it in the current context.

I believe I handled the other scenario in your comment below with the latest commit. I'll look at this one as well.

OshinKaramian · 2017-08-03T21:08:09Z

test/videojs-contrib-hls.test.js

@@ -1786,7 +1787,7 @@ QUnit.test('uses mobile default bandwidth if browser is Android', function(asser
  openMediaSource(this.player, this.clock);

  assert.equal(this.player.tech_.hls.bandwidth,
-               500000,
+               4194304,


We discussed that we were going to use the desktop bandwidth number everywhere, so this test had to be updated to account for that (I figured I would update over removing).

mjneil · 2017-08-04T21:49:45Z

src/playlist-selectors.js

@@ -375,5 +375,5 @@ export const lowestBitrateCompatibleVariantSelector = function() {
  const playlistsWithVideo =
    playlists.filter(playlist => parseCodecs(playlist.attributes.CODECS).videoCodec);

-  return playlistsWithVideo[0] || playlists[0];
+  return playlistsWithVideo[0] || null;


instead of returning null, you could call the last bandwidth selector with the current context
return playlistsWithVideo[0] || lasBandwidthSelector.call(this); I think this would also remove the need to for the temporary selectedMedia

If possible I'd like to avoid coupling this function to a separate algorithm, and allow the consumer to decide the alternative. I think long term adding the complexity here over elsewhere will be a bit confusing, as well as making unit testing and changes harder/more complex.

makes sense 👍

mjneil · 2017-08-07T15:25:07Z

src/playlist-selectors.js

+  const playlists = this.playlists.master.playlists.slice();
+
+  // Sort ascending by bitrate
+  stableSort(playlists,


Based on the findings from the 404 unit test failing (discussion in other comment thread), this selector should probably filter by isEnabled similar to the simpleSelector since this may try and re-select a playlist that just 404'd

Good catch, I believe the latest commit should deal with that scenario.

mjneil · 2017-08-08T17:11:09Z

src/playlist-selectors.js

+
+  // filter out any playlists that have been excluded due to
+  // incompatible configurations or playback errors
+  const enabledPlaylists = playlists.filter(


this could be one line const enabledPlaylists = playlists.filter(Playlist.isEnabled);

mjneil · 2017-08-08T17:14:44Z

src/videojs-contrib-hls.js

+    // then this takes precedence over the enableLowInitialPlaylist option
+    this.options_.enableLowInitialPlaylist =
+      (this.options_.enableLowInitialPlaylist &&
+       this.options_.bandwidth === INITIAL_BANDWIDTH) || false;


the || false is not necessary a && b will just be the value of b when a == true

mjneil · 2017-08-08T17:19:27Z

test/playlist-selectors.test.js

+      'Selected lowest compatible playlist with video assets');
+  });
+
+test('lowestBitrateCompatibleVariantSelector picks lowest audio rendition if no video exists',


The description is no longer accurate (now returns null) Same goes for the assertion description below

…aylists

mjneil · 2017-08-08T21:15:53Z

src/playlist-selectors.js

+ *         exists pick the lowest audio rendition.
+ */
+export const lowestBitrateCompatibleVariantSelector = function() {
+  const playlists = this.playlists.master.playlists.slice();


could replace this slice call with the enabled filter, then you wouldn't need to do it later or have the local const

Select lowest rendition available

ba8b1d1

brandonocasey added the in progress label Jul 26, 2017

OshinKaramian commented Jul 26, 2017

View reviewed changes

mjneil reviewed Jul 26, 2017

View reviewed changes

Oshin Karamian added 4 commits July 26, 2017 17:37

Code review updates

01bc928

Sort playlist selector descending by bitrate

6a5bd11

Add option to enable low rendition selection, set to false by default

382ad9a

Add test for triggering loadedplaylist

06d4814

mjneil reviewed Jul 27, 2017

View reviewed changes

Update test so that it checks live reload case and works correctly

b4a89b2

mjneil reviewed Jul 28, 2017

View reviewed changes

Code review updates

a2ca2fa

mjneil added the needs:testing label Jul 28, 2017

Add tests that check lowest bitrate rendition selection algorithm

8ccd7e2

OshinKaramian mentioned this pull request Jul 31, 2017

Split generic-seeming functionality into their own files #1217

Closed

6 tasks

forbesjo reviewed Aug 1, 2017

View reviewed changes

Add enableLowInitialPlaylist to README

da8b0c5

mjneil approved these changes Aug 1, 2017

View reviewed changes

Oshin Karamian added 2 commits August 2, 2017 11:33

Additional link to enableLowIinitalPlaylist

362c246

Merge branch 'master' into fast-start-time

f1e3550

Fix issues with codec-less but valid manifests

4f431fe

OshinKaramian commented Aug 3, 2017

View reviewed changes

mjneil reviewed Aug 4, 2017

View reviewed changes

mjneil reviewed Aug 7, 2017

View reviewed changes

Oshin Karamian added 2 commits August 8, 2017 10:49

Handle code review comment to take care of disabled playlists

8afae79

Fix copy/paste error

024f74a

mjneil reviewed Aug 8, 2017

View reviewed changes

Code review updates

c51469a

mjneil approved these changes Aug 8, 2017

View reviewed changes

Add logic to minRebufferMaxBandwidthSelector to ignore blacklisted pl…

c08c114

…aylists

mjneil reviewed Aug 8, 2017

View reviewed changes

Oshin Karamian added 3 commits August 8, 2017 17:27

Remove slice in favor of an existing filter

4139386

Commit changes from master and resolve conflicts

bb79b6e

Add back newline after merge conflict

39f8e44

OshinKaramian changed the title ~~[HOLD ON MERGE] Select lowest rendition available on startup~~ Select lowest rendition available on startup Aug 16, 2017

OshinKaramian changed the title ~~Select lowest rendition available on startup~~ Add option to select lowest rendition available on startup Aug 16, 2017

mjneil approved these changes Aug 16, 2017

View reviewed changes

mjneil merged commit 2d9b3ad into master Aug 16, 2017

forbesjo deleted the fast-start-time branch July 2, 2018 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to select lowest rendition available on startup #1212

Add option to select lowest rendition available on startup #1212

OshinKaramian commented Jul 26, 2017 •

edited

Loading

OshinKaramian Jul 26, 2017 •

edited

Loading

OshinKaramian Jul 26, 2017

mjneil Jul 26, 2017

mjneil Jul 26, 2017

OshinKaramian Jul 26, 2017

mjneil Jul 27, 2017

mjneil Jul 27, 2017

mjneil Jul 27, 2017

mjneil Jul 27, 2017

mjneil Jul 27, 2017

mjneil Jul 28, 2017

mjneil Jul 28, 2017

mjneil Jul 28, 2017

mjneil commented Jul 28, 2017

OshinKaramian commented Jul 31, 2017

OshinKaramian commented Jul 31, 2017

forbesjo Aug 1, 2017

OshinKaramian Aug 3, 2017 •

edited

Loading

mjneil Aug 4, 2017

mjneil Aug 7, 2017

OshinKaramian Aug 8, 2017 •

edited

Loading

OshinKaramian Aug 3, 2017

mjneil Aug 4, 2017

OshinKaramian Aug 8, 2017

mjneil Aug 8, 2017

mjneil Aug 7, 2017

OshinKaramian Aug 8, 2017

mjneil Aug 8, 2017

mjneil Aug 8, 2017

mjneil Aug 8, 2017

mjneil Aug 8, 2017


		this.clock.tick(1);

		// Trigger playlist event which should utilize selectInitialPlaylist and

Add option to select lowest rendition available on startup #1212

Add option to select lowest rendition available on startup #1212

Conversation

OshinKaramian commented Jul 26, 2017 • edited Loading

Description

Requirements Checklist

OshinKaramian Jul 26, 2017 • edited Loading

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

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

mjneil commented Jul 28, 2017

OshinKaramian commented Jul 31, 2017

OshinKaramian commented Jul 31, 2017

Choose a reason for hiding this comment

OshinKaramian Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OshinKaramian Aug 8, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OshinKaramian commented Jul 26, 2017 •

edited

Loading

OshinKaramian Jul 26, 2017 •

edited

Loading

OshinKaramian Aug 3, 2017 •

edited

Loading

OshinKaramian Aug 8, 2017 •

edited

Loading