[ABR] Store bandwidth and throughput values in local storage and use them o… #1142

gesinger · 2017-06-01T23:29:18Z

…n startup

Description

#1113

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

…n startup

mjneil · 2017-06-05T15:16:56Z

Didn't we want to turn on the storing of local storage but leave the using of it off for now?

Also do you think this would be a good thing to allow turning off and on through the player options?

…and useBandwidthFromLocalStorage options

mjneil · 2017-06-09T21:09:29Z

README.md

@@ -306,6 +308,19 @@ When the `bandwidth` property is set (bits per second), it will be used in
 the calculation for initial playlist selection, before more bandwidth
 information is seen by the player.

+##### storeBandwidthInLocalStorage


Is this option needed? Any reason to not just have storage always enabled, and allow the setting of whether its actually used?

Since local storage is limited in size (before access to more space must be granted by the site visitor), it is possible that a user of videojs-contrib-hls doesn't want the project using some of the space if they've carefully accounted for the amount allotted to them.

Can't useBandwidthFromLocalStorage do both?

It's only storing 2 ints though, so I don't think it will be a big issue. Either way, I agree with @forbesjo , we should just use one flag to turn off both storing and using. I know previously we've discussed turning on the storing but not the using, but I don't think that matters much, especially since we are wanting to go the usage event route

Sounds good 👍 . Will change to just one.

…romLocalStorage

mjneil · 2017-06-13T17:48:10Z

We should add the usage tracking event for when localstorage values have been used

OshinKaramian

A couple of minor suggestions.

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

src/videojs-contrib-hls.js

+      // broadband internet
+      // 0.5 MB/s
+      if (typeof this.options_.bandwidth !== 'number') {
+        this.options_.bandwidth = 4194304;


Might make sense to put this magic number into a const somewhere.

OshinKaramian · 2017-07-03T16:09:42Z

test/videojs-contrib-hls.test.js

@@ -2687,6 +2689,108 @@ QUnit.test('populates quality levels list when available', function(assert) {
  assert.ok(this.player.tech_.hls.qualityLevels_, 'added quality levels from video with source');
 });

+QUnit.test('stores bandwidth and throughput in localStorage', function(assert) {
+  const origHlsOptions = videojs.options.hls;


This is a big test... it might make sense (if possible) to break it down into a few smaller tests. Tiny tests make debugging and finding issues a little bit easier.

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

test/videojs-contrib-hls.test.js

+  });
+
+  assert.equal(this.player.tech_.hls.bandwidth,
+               4194304,


If you export the const you might be able to reference it here.

OshinKaramian · 2018-01-31T16:26:14Z

src/videojs-contrib-hls.js

-    // broadband internet (0.5 MB/s) or mobile (0.0625 MB/s)
+    if (typeof this.options_.bandwidth !== 'number') {
+      if (this.options_.useBandwidthFromLocalStorage && window.localStorage) {
+        let storedBandwidth =


Nit: Is there any reason this can't be a const?

squarebracket · 2018-01-31T17:29:21Z

src/videojs-contrib-hls.js

+    }
+
+    // if bandwidth was not set by options or pulled from local storage, start playlist
+    // selection at a reasonable bandwidth
    if (typeof this.options_.bandwidth !== 'number') {


It's semantically equivalent, but I'm wondering why you didn't include this as an else in the clause above. i.e. if use local storage, grab it, otherwise set the bandwidth from a constant.

It's possible that the user did not specify a bandwidth value and set useBandwidthFromLocalStorage to true, but there was nothing in local storage, leaving bandwidth undefined.

ah good call

gkatsev · 2018-02-06T15:42:53Z

test/videojs-contrib-hls.test.js

@@ -2932,6 +2934,108 @@ QUnit.test('populates quality levels list when available', function(assert) {
  assert.ok(this.player.tech_.hls.qualityLevels_, 'added quality levels from video with source');
 });

+QUnit.test('stores bandwidth and throughput in localStorage when option is true',
+function(assert) {
+  videojs.options.hls = {


should there be a test that sets it in the player directly?

gkatsev · 2018-02-06T15:43:23Z

README.md

+* Type: `boolean`
+* can be used as an initialization option
+
+If true, `bandwidth` and `throughput` values are stored in and retrieved from local


would be good to add explanation of default behavior

forbesjo · 2018-08-06T15:28:09Z

Let's open this against VHS

forbesjo · 2018-08-10T15:17:49Z

@gkatsev what are your thoughts on plugins using local storage? Should there be a common API provided by video.js or should the plugins do this directly?

gkatsev · 2018-08-10T15:42:09Z

I think we shouldn't store things in local storage by default but have an option to enable it. This is because of the EU cookie laws where users have to be informed if things are stored locally. While local storage isn't specifically called out, it's best to play it safe. In Video.js, we have an option to persist caption settings for the user but it is disabled by default.
Then, when a site that has the cookie notification uses Video.js, they could enable the option to persist this information.

With ABR, we'd have two things that are stored in local storage, optionally, I don't think it quite warrants a combined API, but we should keep our eyes out, maybe we'll want to combine both of these into a common videojs api and a common allowPersistence option that enables or disabled all persistence via that new API.

gesinger · 2018-10-26T20:18:23Z

Closing in favor of videojs/http-streaming#275

Store bandwidth and throughput values in local storage and use them o…

aea8f99

…n startup

gesinger added the in progress label Jun 1, 2017

mjneil changed the title ~~Store bandwidth and throughput values in local storage and use them o…~~ [ABR] Store bandwidth and throughput values in local storage and use them o… Jun 2, 2017

Only enable local storage features with storeBandwidthInLocalStorage …

720ef87

…and useBandwidthFromLocalStorage options

mjneil reviewed Jun 9, 2017

View reviewed changes

Remove storeBandwidthInLocalStorage option and only use useBandwidthF…

e751b00

…romLocalStorage

OshinKaramian suggested changes Jul 3, 2017

View reviewed changes

gesinger added 3 commits January 26, 2018 16:01

Merge branch 'master' into abr-local-storage-bandwidth-throughput

18d4519

Resolve merge conflicts

40d1ac3

Split local storage setting test into two tests

c5203d7

OshinKaramian approved these changes Jan 31, 2018

View reviewed changes

Change storedBandwidth and storedThroughput lets to consts

7832ac6

squarebracket reviewed Jan 31, 2018

View reviewed changes

gkatsev reviewed Feb 6, 2018

View reviewed changes

gesinger added 2 commits February 6, 2018 11:22

Add test for useBandwidthFromLocalStorage setting on player instance

cc49dd3

Add usage events for local storage retrieved bandwidth and throughput

90d19ac

forbesjo mentioned this pull request Jul 6, 2018

Use localstorage for ABR videojs/http-streaming#148

Closed

forbesjo assigned OshinKaramian Jul 6, 2018

forbesjo unassigned OshinKaramian Aug 6, 2018

gesinger mentioned this pull request Oct 26, 2018

feat: Add useBandwidthFromLocalStorage option videojs/http-streaming#275

Merged

6 tasks

gesinger closed this Oct 26, 2018

brandonocasey removed the in progress label Oct 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ABR] Store bandwidth and throughput values in local storage and use them o… #1142

[ABR] Store bandwidth and throughput values in local storage and use them o… #1142

gesinger commented Jun 1, 2017 •

edited by forbesjo

Loading

mjneil commented Jun 5, 2017

mjneil Jun 9, 2017

gesinger Jun 9, 2017

forbesjo Jun 12, 2017

mjneil Jun 12, 2017

gesinger Jun 12, 2017

mjneil commented Jun 13, 2017

OshinKaramian left a comment

OshinKaramian Jul 3, 2017

OshinKaramian Jul 3, 2017

OshinKaramian Jul 3, 2017

OshinKaramian Jan 31, 2018

squarebracket Jan 31, 2018

gesinger Jan 31, 2018

squarebracket Jan 31, 2018

gkatsev Feb 6, 2018

gkatsev Feb 6, 2018

forbesjo commented Aug 6, 2018

forbesjo commented Aug 10, 2018

gkatsev commented Aug 10, 2018

gesinger commented Oct 26, 2018

[ABR] Store bandwidth and throughput values in local storage and use them o… #1142

[ABR] Store bandwidth and throughput values in local storage and use them o… #1142

Conversation

gesinger commented Jun 1, 2017 • edited by forbesjo Loading

Description

Requirements Checklist

mjneil commented Jun 5, 2017

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 13, 2017

OshinKaramian 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

forbesjo commented Aug 6, 2018

forbesjo commented Aug 10, 2018

gkatsev commented Aug 10, 2018

gesinger commented Oct 26, 2018

gesinger commented Jun 1, 2017 •

edited by forbesjo

Loading