Abr Improvements #1176

mjneil · 2017-06-21T23:28:02Z

Description

This PR contains the following improvements to the ABR algorithm:

Use a starting bandwidth value of 0.0625 MB/s on Android devices
Do not allow an up-switch in quality until a certain amount of forward buffer has been filled, BUFFER_LOW_WATER_LINE
Dynamically increase the BUFFER_LOW_WATER_LINE and GOAL_BUFFER_LENGTH from 0 - > 30 and 30 -> 60 respectively during the first 30 seconds of playback
Abort segment requests before completion if bandwidth reported by the XHR progress event shows that network conditions are not fast enough to complete the request without causing rebuffering

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

imbcmdth · 2017-06-22T15:04:17Z

Good thing git can now squash and merge.. This might be the ugliest commit list in the history of videojs! 😆

mjneil · 2017-06-28T15:04:59Z

src/master-playlist-controller.js

+        buffered.end(buffered.length - 1) - this.tech_.currentTime() : 0;
+
+      const currentTime = this.tech_.currentTime();
+      const initial = Config.BUFFER_LOW_WATER_LINE;


We should make getter functions on MPC for low water line and goal buffer length, and pass the GBL one down to the segment loader to use

mjneil · 2017-06-28T19:55:07Z

src/media-segment-request.js

+
+  // record the time that we receive the first byte of data
+  if (!segment.stats.firstByteRoundTripTime && segment.stats.bytesReceived) {
+    segment.stats.firstByteRoundTripTime = Date.now();


firstByteRoundTripTime is probably not the most accurate property name

mjneil · 2017-06-28T20:12:16Z

test/master-playlist-controller.test.js

@@ -814,6 +817,134 @@ QUnit.test('re-triggers bandwidthupdate events on the tech', function(assert) {
  assert.equal(bandwidthupdateEvents, 2, 'triggered bandwidthupdate');
 });

+// TODO: Add test for ignoring low waterline in LIVE playback


let's do this todo

gesinger

Still need to review tests. Not reviewing network simulator files.

gesinger · 2017-06-30T19:57:31Z

src/master-playlist-controller.js

@@ -548,7 +550,30 @@ export class MasterPlaylistController extends videojs.EventTarget {
    this.mainSegmentLoader_.on('bandwidthupdate', () => {
      // figure out what stream the next segment should be downloaded from


This comment is a bit extraneous now

gesinger · 2017-06-30T20:04:37Z

src/master-playlist-controller.js

+
+      const bufferLowWaterLine = this.bufferLowWaterLine();
+
+      // we want to switch down to lower resolutions quickly to continue playback, but


I think at least for this top piece, we can move it above the relevant lines below.

// switch down to lower resolutions quickly to continue playback nextPlaylist.attributes.BANDWIDTH < currentPlaylist.attributes.BANDWIDTH || // ensure we have some buffer before we switch up to prevent us running out of // buffer while loading a higher rendition forwardBuffer >= bufferLowWaterLine) {

gesinger · 2017-06-30T21:30:45Z

src/segment-loader.js

+   * updated to trigger a playlist switch.
+   *
+   * @param {Object} stats
+   *        Object continaing stats about the request timing and size


gesinger · 2017-06-30T21:40:53Z

src/segment-loader.js

+                                                          this.duration_(),
+                                                          this.currentTimeline_,
+                                                          currentTime);
+      // There is no sync point for this playlist, so switching to it will require a


If there is no

gesinger · 2017-06-30T21:42:17Z

src/segment-loader.js

+    }
+
+    // set the bandwidth to that of the desired playlist
+    // (Being sure to scale by BANDWIDTH_VARIANCE and add one so the


Can remove parenthesis

gesinger · 2017-06-30T21:45:48Z

src/segment-loader.js

@@ -776,7 +898,7 @@ export default class SegmentLoader extends videojs.EventTarget {
        seekable.start(0) < currentTime) {
      removeToTime = seekable.start(0);
    } else {
-      removeToTime = currentTime - 60;
+      removeToTime = currentTime - 30;


Comment for 1 minute is out of date (above). And we should probably mention that we have to keep it lower than max LWL. Though we should probably calculate based on LWL, particularly if people set their own values.

I think maybe we should calculate based on GBL. The reason we lowered this was we increased GBL from 30 -> 60, so we decreased the trim. Maybe we should have a rule of only having 90 (or something similar, while also having some minimum trim incase someone sets GBL very high, we still want to trim content) seconds buffered max and depending on GBL, trim the remaining?

Makes sense.

deciding to go back to 60 second trim. Comment in code above stating 150mb should actually be 150MB which is ~5 minutes of playback, so we should be safe keeping 60 sec of back buffer

gesinger · 2017-06-30T21:47:10Z

src/videojs-contrib-hls.js

@@ -58,6 +58,98 @@ Object.defineProperty(Hls, 'GOAL_BUFFER_LENGTH', {
  }
 });

+Object.defineProperty(Hls, 'MAX_GOAL_BUFFER_LENGTH', {


A lot of these are simply copies. We should have a method that generates them, and make all of them one line function calls.

I would also add that you could shove the property string 'MAX_GOAL_BUFFER_LENGTH' into a variable somewhere.

If these functions are all similar enough you could probably dump them all in an array and run through it to define these objects.

Should we make them all have the same constraints (e.g. positive integer) and remove the low waterline constraints for being below GBL? We can just make sure to check for valid numbers whenever we use them and make no assumptions about what the numbers are, which we already do I believe

I think since the variable name is only used in one spot, it might make more sense to keep it as a string inline. If we end up using it in more than one spot, then we should consider making a constant for it.

I think we can remove the various constraints for now. Most people should realize that setting a LWL above GBL just won't work. If people run into trouble with them in the future, then we can add in the config specific constraints again.

gesinger · 2017-06-30T21:47:58Z

src/config.js

+  // How much of the buffer must be filled before we consider upswitching
+  BUFFER_LOW_WATER_LINE: 0,
+  MAX_BUFFER_LOW_WATER_LINE: 30,
+  BUFFER_LOW_WATER_RATE: 1


For consistency, maybe BUFFER_LOW_WATER_LINE_RATE?

should GOAL_BUFFER_RATE be updated to GOAL_BUFFER_LENGTH_RATE then?

Makes sense to me

OshinKaramian · 2017-07-03T16:19:32Z

src/master-playlist-controller.js

+   *
+   * @return {Number} Desired forward buffer length in seconds
+   */
+  goalBufferLength() {


There is some duplication of this function in the test suite. You could:

1.Take currentTime in as a variable into the function and use that for testing OR
2. Take currentTime in and set it to a default value, then change it/set it in the test.

This would also let you write a couple of tiny unit tests to check the internal logic of this function if you wanted to.

You could probably do the same for bufferLowWaterLine if you wanted to test it similarly.

In the test suite i decided to set this.goalBufferLength = MasterPlaylistController.prototype.goalBufferLength.bind(this) to eliminate the duplication. How are you with that approach?

OshinKaramian · 2017-07-03T16:23:47Z

src/playlist-selectors.js

-  let resolutionBestVariantList = [];
-
-  stableSort(sortedPlaylists, comparePlaylistBandwidth);
+const simpleSelector = function(master, playerBandwidth, playerWidth, playerHeight) {


Given that I am a big n00b it would be nice to have this function commented, at the moment I'm guessing it selects the proper rendition based on the criteria that's passed in.

Yea it selects the best rendition available given the bandwidth and player size constraints

OshinKaramian · 2017-07-03T16:25:51Z

src/playlist-selectors.js

+  let haveResolution = bandwidthPlaylistReps.filter((rep) => rep.width && rep.height);
+
+  // sort variants by resolution
+  stableSort(haveResolution, (left, right) => left.width - right.width);


This seems to be sorting and then filtering it, is there any reason not to sort the final output from whatever filters you've applied?

One reason is that we reuse haveResolution in different filters to get different values, so it's probably better to sort once then filter twice than filter twice and sort twice. We only ever sort twice, once by bandwidth and once by resolution and I think its easier to reason about the filters when they are working on sorted lists

Ah, makes sense.

OshinKaramian · 2017-07-03T16:32:15Z

src/segment-loader.js

+   * @private
+   */
+  abortRequestEarly_(stats) {
+    if (this.hls_.tech_.paused() ||


Anyway to break this almost 100 line function down into a couple of smaller functions?

Maybe some of the "ifs" could be moved elsewhere? Or some of the logic in the sort functions?

OshinKaramian · 2017-07-03T16:33:12Z

src/segment-loader.js

+      return;
+    }
+
+    if (this.abortRequestEarly_(simpleSegment.stats)) {


Could just move this condition up into the if statement above.

OshinKaramian · 2017-07-03T16:34:58Z

src/sync-controller.js

@@ -46,7 +46,7 @@ export const syncPointStrategies = [
  {
    name: 'Segment',
    run: (syncController, playlist, duration, currentTimeline, currentTime) => {
-      let segments = playlist.segments;
+      let segments = playlist.segments || [];


Could do something like let { segments = [] } = playlist;

OshinKaramian · 2017-07-03T16:37:05Z

src/videojs-contrib-hls.js

@@ -58,6 +58,98 @@ Object.defineProperty(Hls, 'GOAL_BUFFER_LENGTH', {
  }
 });

+Object.defineProperty(Hls, 'MAX_GOAL_BUFFER_LENGTH', {


I would also add that you could shove the property string 'MAX_GOAL_BUFFER_LENGTH' into a variable somewhere.

If these functions are all similar enough you could probably dump them all in an array and run through it to define these objects.

OshinKaramian · 2017-07-03T16:37:58Z

src/videojs-contrib-hls.js

-      this.options_.bandwidth = 4194304;
+      // only use Android for mobile because iOS does not support MSE (and uses
+      // native HLS)
+      this.options_.bandwidth = videojs.browser.IS_ANDROID ? 500000 : 4194304;


Would be neat to store these constants in a couple of variables.

OshinKaramian · 2017-07-03T16:38:47Z

test/loader-common.js

+        playbackRate: () => this.playbackRate
+      }
+    };
+    this.goalBufferLength = () => {


This is the function I mentioned before that you may be able to avoid duplicating here.

OshinKaramian · 2017-07-03T16:41:37Z

test/loader-common.js

+      loader.playlist(playlist1, xhrOptions);
+      loader.load();
+
+      this.clock.tick(1);


If the order of each test here is unimportant (one test doesn't rely on another) you could break it up into a few smaller/more specific tests. It might make it easier to debug if one specific case fails but the rest work.

This test is actually just one test, order is very important here. There is only one segment request in flight for this test, each dispatchEvent call is mocking a progress event for the xhr request. using the value on loaded within the event and the number of clock ticks that have passed since the request has been made is what is being used for the estimated bandwidth

Gotcha, that makes sense.

OshinKaramian · 2017-07-03T22:14:18Z

src/segment-loader.js

+      return false;
+    }
+
+    const switchCandidate = minRebufferMaxBandwidthSelector({


…ack buffer (#1120)

… of downloading the segment in a reasonable time

gesinger · 2017-07-03T22:33:09Z

src/playlist.js

+
+  const size = segmentDuration * playlist.attributes.BANDWIDTH;
+
+  return (size - (bytesReceived * 8)) / bandwidth;


Should this be bitsReceived

No, thats why we multiply by 8 to convert to bits

gesinger · 2017-07-03T22:34:32Z

src/segment-loader.js

+      return false;
+    }
+
+    const switchCandidate = minRebufferMaxBandwidthSelector({


gesinger · 2017-07-03T22:38:20Z

src/segment-loader.js

@@ -898,7 +883,7 @@ export default class SegmentLoader extends videojs.EventTarget {
        seekable.start(0) < currentTime) {
      removeToTime = seekable.start(0);
    } else {
-      removeToTime = currentTime - 30;
+      removeToTime = currentTime - 60;


This may be too high.

gesinger · 2017-07-03T22:39:21Z

src/videojs-contrib-hls.js

-  }
-});
+      if (typeof value !== 'number' || value < 0) {
+        videojs.log.warn(`value passed to Hls.${prop} must be a positive number or 0`);


Might be easier to read as must be greater than or equal to 0

gesinger · 2017-07-03T22:40:24Z

src/videojs-contrib-hls.js

@@ -374,7 +279,8 @@ class HlsHandler extends Component {
    if (typeof this.options_.bandwidth !== 'number') {
      // only use Android for mobile because iOS does not support MSE (and uses
      // native HLS)
-      this.options_.bandwidth = videojs.browser.IS_ANDROID ? 500000 : 4194304;
+      this.options_.bandwidth =
+        videojs.browser.IS_ANDROID ? BANDWIDTH_MOBILE : BANDWIDTH_BROADBAND;


Might be easier read/more precise as INITIAL_BANDWIDTH_MOBILE and INITIAL_BANDWIDTH_DESKTOP

You just like making staying under 90 chars per line difficult

edit: this fits perfectly

gesinger · 2017-07-03T23:06:39Z

src/playlist-selectors.js

+  });
+
+  const noRebufferingPlaylists = rebufferingEstimates.filter(
+    (estimate) => estimate.rebufferingImpact < 0);


Should this be <=?

gesinger

Would be good to add tests for:

goalBufferLength()
bufferLowWaterLine()
firstBytesReceivedAt added to pending segment object
minRebufferMaxBandwidthSelector
hasAttribute
estimateSegmentRequestTime
timeUntilRebuffer

gesinger · 2017-07-05T19:07:35Z

test/configuration.test.js

  assert.equal(this.env.log.warn.calls, 2, 'logged two warnings');

  assert.equal(Config.GOAL_BUFFER_LENGTH, 30, 'default');
 });

+QUnit.test('MAX_GOAL_BUFFER_LENGTH get warning', function(assert) {


Although I'm usually for non shared tests, these are similar enough that it might be worth abstracting the test logic, and also checking the warning messages.

gesinger · 2017-07-05T19:24:26Z

test/master-playlist-controller.test.js

+  this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate');
+  assert.equal(mediaChanges.length,
+               1,
+               'changes media when no buffer and equal bandwidth playlist');


I don't think that a change should happen when the bandwidths are equal (either by how the code works or by what we should do).

It looks like it succeeds here because currentTime === 0 and initial BUFFER_LOW_WATER_LINE is 0, so the check forwardBuffer >= bufferLowWaterLine is true.

gesinger · 2017-07-05T19:27:58Z

test/master-playlist-controller.test.js

+  this.masterPlaylistController.mainSegmentLoader_.trigger('bandwidthupdate');
+  assert.equal(mediaChanges.length,
+               1,
+               'changes media when sufficient forward buffer and higher ' +


Might be worth having another for forward buffer equal to dynamic LWL

gesinger · 2017-07-05T19:28:36Z

test/master-playlist-controller.test.js

+               3,
+               'changes live media when sufficient forward buffer and higher ' +
+               'bandwidth playlist');
+});


Would be good to have some tests for always switching when duration < LWL.

mjneil added the in progress label Jun 21, 2017

mjneil commented Jun 28, 2017

View reviewed changes

gesinger reviewed Jun 30, 2017

View reviewed changes

OshinKaramian suggested changes Jul 3, 2017

View reviewed changes

OshinKaramian reviewed Jul 3, 2017

View reviewed changes

gesinger and others added 22 commits July 3, 2017 18:15

Change default GOAL_BUFFER_LENGTH to 60 and keep only 30 seconds of b…

b89d1ff

…ack buffer (#1120)

update dist

aa45879

Add forward buffer low water line before upswitching rendition

01556bd

Add tests for buffer low water line and fix tests the change broke

a1d24b8

add low-water line to network simulator

4b6929a

chart results

822bb35

update okr page to use new simulator output

69c166b

allow 0 low water line

ffe518b

add buffer_low_water_line test

5a2bc8c

actually use fuzzinput function

7ae07ea

use goal buffer length of 60, low water line of 30

2b2013c

ignore low-water-line for live playlists

76c8f77

ignore waterline when vod duration is less than waterline

054bdd3

Abort on progress events during a segment request if there is no hope…

ce527a4

… of downloading the segment in a reasonable time

add timestamp stat for when the first byte of data was receieved

a1f55b1

only abort early if we risk rebuffering

78388bc

some fixes

44cbded

Use dynamic changed low-water-line and GOAL_BUFFER_LENGTH

adc21b0

add abort early function that estimates rebuffering potential

950d6e9

cleanup

f5187f3

remove simple approach

83fa1fb

fix unit test

43862da

mjneil added 7 commits July 3, 2017 18:19

use sync controller to add extra request estimation

a0971de

prioritize no rebuffering

e16edf3

fix comment

de122b6

update dist

6ec21bb

fix some undefined bugs

c025fff

ignore dist again

1b4a1b4

refactor and add unit tests

5c7c688

OshinKaramian approved these changes Jul 3, 2017

View reviewed changes

refactor of things and fixing up tests

c632ab1

mjneil force-pushed the abr/rc branch from 4366e0e to c632ab1 Compare July 3, 2017 22:22

mjneil added 2 commits July 3, 2017 18:24

remove dists

b4f3530

put dist back on ignore

1eb0c76

gesinger reviewed Jul 3, 2017

View reviewed changes

backbuffer trim at 30 sec

4a571d7

gesinger reviewed Jul 3, 2017

View reviewed changes

fix lint errors

e852559

forbesjo self-assigned this Jul 5, 2017

gesinger reviewed Jul 5, 2017

View reviewed changes

mjneil added 2 commits July 6, 2017 11:09

add more unit tests

6cc7992

add clarifying comment

51b0e05

gesinger approved these changes Jul 6, 2017

View reviewed changes

mjneil merged commit 8a229d3 into master Jul 6, 2017

mjneil removed the in progress label Jul 6, 2017

forbesjo removed their assignment Jul 6, 2017

forbesjo deleted the abr/rc branch July 2, 2018 19:11

		@@ -548,7 +550,30 @@ export class MasterPlaylistController extends videojs.EventTarget {
		this.mainSegmentLoader_.on('bandwidthupdate', () => {
		// figure out what stream the next segment should be downloaded from


		const bufferLowWaterLine = this.bufferLowWaterLine();

		// we want to switch down to lower resolutions quickly to continue playback, but


		const size = segmentDuration * playlist.attributes.BANDWIDTH;

		return (size - (bytesReceived * 8)) / bandwidth;

Abr Improvements #1176

Abr Improvements #1176

Conversation

mjneil commented Jun 21, 2017 • edited Loading

Description

Requirements Checklist

imbcmdth commented Jun 22, 2017

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

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 Jul 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjneil Jul 3, 2017 • edited Loading

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

mjneil commented Jun 21, 2017 •

edited

Loading

mjneil Jul 3, 2017 •

edited

Loading

mjneil Jul 3, 2017 •

edited

Loading