feat: remove experimental options #1301

kchang-brightcove · 2022-07-28T16:35:40Z

Description

Removed some experimental options and make them default.

welcome · 2022-07-28T16:35:42Z

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

Run npm run lint -- --errors locally to catch formatting errors earlier.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

gkatsev · 2022-07-28T17:23:16Z

This probably should go against the next branch? Also, should we enable llhls by default here as well? @misteroneill?

misteroneill

Yeah, we should, I think. The others are blocked by lack of enough real world data.

misteroneill · 2022-08-01T20:53:41Z

src/manifest.js

@@ -23,7 +23,7 @@ export const createPlaylistID = (index, uri) => {
 *        An array of custom tag parsers for the m3u8-parser instance
 * @param {Object[]} [customTagMappers]
 *        An array of custom tag mappers for the m3u8-parser instance
- * @param {boolean} [experimentalLLHLS=false]
+ * @param {boolean} [llhls=false]


This should default to true now.

misteroneill · 2022-08-03T13:46:29Z

src/manifest.js

@@ -34,7 +34,7 @@ export const parseManifest = ({
  manifestString,
  customTagParsers = [],
  customTagMappers = [],
-  experimentalLLHLS
+  llhls


The change to the JSDoc isn't enough to default it to true. It would need to be changed here as well

Suggested change

llhls

llhls = true

misteroneill

LGTM. @gesinger would you mind having a quick look?

gesinger · 2022-08-03T16:53:30Z

src/playlist-loader.js

-    // force experimentalLLHLS for IE 11
+    // force llhls for IE 11
    if (videojs.browser.IE_VERSION) {
-      this.experimentalLLHLS = false;
+      this.llhls = false;
    }


I think this whole block can be removed.

gesinger · 2022-08-03T16:56:54Z

src/manifest.js

@@ -34,7 +34,7 @@ export const parseManifest = ({
  manifestString,
  customTagParsers = [],
  customTagMappers = [],
-  experimentalLLHLS
+  llhls = true


I think this default should be at a higher level (probably in src/videojs-http-streaming.js), as, as it stands, it looks like playlist-loader will set it to false if left unspecified.

gesinger · 2022-08-03T20:34:36Z

src/playlist-loader.js

-    if (videojs.browser.IE_VERSION) {
-      this.llhls = false;
-    }
+    this.llhls = (vhsOptions && vhsOptions.llhls) || true;


This line will always evaluate to true. But I think the option should to be set at a higher level (in src/videojs-http-streaming.js, probably in setOptions_ https://github.com/videojs/http-streaming/blob/main/src/videojs-http-streaming.js#L622 ).

gesinger · 2022-08-03T20:35:32Z

src/manifest.js

@@ -23,7 +23,7 @@ export const createPlaylistID = (index, uri) => {
 *        An array of custom tag parsers for the m3u8-parser instance
 * @param {Object[]} [customTagMappers]
 *        An array of custom tag mappers for the m3u8-parser instance
- * @param {boolean} [experimentalLLHLS=false]
+ * @param {boolean} [llhls=true]


Suggested change

* @param {boolean} [llhls=true]

* @param {boolean} [llhls]

gesinger · 2022-08-04T14:15:22Z

src/playlist-loader.js

@@ -395,7 +395,7 @@ export default class PlaylistLoader extends EventTarget {

    this.customTagParsers = (vhsOptions && vhsOptions.customTagParsers) || [];
    this.customTagMappers = (vhsOptions && vhsOptions.customTagMappers) || [];
-    this.llhls = (vhsOptions && vhsOptions.llhls) || true;
+    this.llhls = (vhsOptions && vhsOptions.llhls) || false;


Since the option is set at a higher level, I think we can just use:

Suggested change

this.llhls = (vhsOptions && vhsOptions.llhls) || false;

this.llhls = vhsOptions && vhsOptions.llhls;

gesinger · 2022-08-04T14:16:44Z

src/videojs-http-streaming.js

@@ -597,6 +597,7 @@ class VhsHandler extends Component {
    this.options_.customTagParsers = this.options_.customTagParsers || [];
    this.options_.customTagMappers = this.options_.customTagMappers || [];
    this.options_.cacheEncryptionKeys = this.options_.cacheEncryptionKeys || false;
+    this.options_.llhls = this.options_.llhls === false ? false : true;


I know we use this style above, but I think it's clearer as:

Suggested change

this.options_.llhls = this.options_.llhls === false ? false : true;

this.options_.llhls = Boolean(this.options_.llhls);

Coming back to this, Kevin has restored the previous state:

this.options_.llhls === false ? false : true

This is because simply doing Boolean(this.options_.llhls) was not defaulting to true, which is what was wanted.

gesinger

I think the build is failing due to some linting issues in scripts/index.js

src/playlist-loader.js

codecov · 2022-08-11T15:04:59Z

Codecov Report

Merging #1301 (d6d4ed1) into next (206f099) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head d6d4ed1 differs from pull request most recent head 4b1d2a3. Consider uploading reports for the commit 4b1d2a3 to get more accurate results

@@            Coverage Diff             @@
##             next    #1301      +/-   ##
==========================================
- Coverage   86.27%   86.23%   -0.04%     
==========================================
  Files          39       39              
  Lines        9791     9757      -34     
  Branches     2275     2276       +1     
==========================================
- Hits         8447     8414      -33     
+ Misses       1344     1343       -1

Impacted Files	Coverage Δ
src/manifest.js	`93.45% <100.00%> (ø)`
src/master-playlist-controller.js	`94.95% <100.00%> (ø)`
src/playlist-loader.js	`95.00% <100.00%> (-0.03%)`	⬇️
src/playlist-selectors.js	`87.05% <100.00%> (ø)`
src/playlist.js	`94.56% <100.00%> (ø)`
src/segment-loader.js	`96.36% <100.00%> (-0.01%)`	⬇️
src/videojs-http-streaming.js	`90.46% <100.00%> (+0.05%)`	⬆️
src/decrypter-worker.js	`84.73% <0.00%> (-0.64%)`	⬇️
src/util/container-request.js	`82.85% <0.00%> (-0.48%)`	⬇️
src/transmuxer-worker.js	`73.54% <0.00%> (-0.25%)`	⬇️
... and 5 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

gkatsev · 2022-08-16T16:54:40Z

scripts/old-index.js

@@ -398,9 +398,9 @@
            vhs: {


I removed this entire file in #1308

misteroneill · 2022-08-16T17:15:08Z

src/videojs-http-streaming.js

@@ -597,6 +597,7 @@ class VhsHandler extends Component {
    this.options_.customTagParsers = this.options_.customTagParsers || [];
    this.options_.customTagMappers = this.options_.customTagMappers || [];
    this.options_.cacheEncryptionKeys = this.options_.cacheEncryptionKeys || false;
+    this.options_.llhls = Boolean(this.options_.llhls);


I could be wrong, but this would still default to false, right?

I think we want it to default to true, though. Maybe something like this?

Suggested change

this.options_.llhls = Boolean(this.options_.llhls);

this.options_.llhls = this.options_.hasOwnProperty('llhls') ? this.options_.llhls : true;

misteroneill · 2022-08-16T17:17:34Z

src/videojs-http-streaming.js

@@ -641,11 +642,11 @@ class VhsHandler extends Component {
      'initialPlaylistSelector',
      'experimentalBufferBasedABR',


Also, we do want to rename this to bufferBasedABR - but leave it defaulting to false for the time being.

misteroneill

The netlify preview is broken. Also, I noticed that fields in the "Options" tabs still say "[EXPERIMENTAL]".

misteroneill · 2022-08-17T17:09:50Z

scripts/index.js

+    [
+      'exact-manifest-timings',
+      'pixel-diff-selector',
+      'bufferBasedABR'


This is throwing an error on the Netlify preview build:

Suggested change

'bufferBasedABR'

'buffer-water'

misteroneill · 2022-08-17T17:15:59Z

src/videojs-http-streaming.js

@@ -597,6 +597,7 @@ class VhsHandler extends Component {
    this.options_.customTagParsers = this.options_.customTagParsers || [];
    this.options_.customTagMappers = this.options_.customTagMappers || [];
    this.options_.cacheEncryptionKeys = this.options_.cacheEncryptionKeys || false;
+    this.options_.llhls = Boolean(this.options_.llhls);


I think we want it to default to true, though. Maybe something like this?

Suggested change

this.options_.llhls = Boolean(this.options_.llhls);

this.options_.llhls = this.options_.hasOwnProperty('llhls') ? this.options_.llhls : true;

misteroneill · 2022-08-18T19:53:45Z

src/videojs-http-streaming.js

@@ -597,6 +597,7 @@ class VhsHandler extends Component {
    this.options_.customTagParsers = this.options_.customTagParsers || [];
    this.options_.customTagMappers = this.options_.customTagMappers || [];
    this.options_.cacheEncryptionKeys = this.options_.cacheEncryptionKeys || false;
+    this.options_.llhls = this.options_.llhls === false ? false : true;


Coming back to this, Kevin has restored the previous state:

this.options_.llhls === false ? false : true

This is because simply doing Boolean(this.options_.llhls) was not defaulting to true, which is what was wanted.

welcome · 2022-08-19T14:19:51Z

Congrats on merging your first pull request! 🎉🎉🎉

kchang-brightcove requested a review from gesinger July 28, 2022 16:37

kchang-brightcove changed the base branch from main to next August 1, 2022 19:38

kchang-brightcove force-pushed the wp-529-remove-experimental-options branch 2 times, most recently from 47d1261 to b8da6d6 Compare August 1, 2022 19:54

misteroneill requested changes Aug 1, 2022

View reviewed changes

kchang-brightcove requested a review from misteroneill August 3, 2022 12:48

misteroneill requested changes Aug 3, 2022

View reviewed changes

misteroneill approved these changes Aug 3, 2022

View reviewed changes

gesinger reviewed Aug 3, 2022

View reviewed changes

kchang-brightcove requested a review from gesinger August 3, 2022 17:39

gesinger reviewed Aug 3, 2022

View reviewed changes

kchang-brightcove requested a review from gesinger August 4, 2022 13:37

gesinger reviewed Aug 4, 2022

View reviewed changes

kchang-brightcove requested a review from gesinger August 4, 2022 15:00

gesinger reviewed Aug 4, 2022

View reviewed changes

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

gkatsev reviewed Aug 16, 2022

View reviewed changes

scripts/old-index.js Outdated

@@ -398,9 +398,9 @@

vhs: {

Copy link

Member

gkatsev Aug 16, 2022

Choose a reason for hiding this comment

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

I removed this entire file in #1308

misteroneill reviewed Aug 16, 2022

View reviewed changes

misteroneill requested changes Aug 16, 2022

View reviewed changes

kchang-brightcove force-pushed the wp-529-remove-experimental-options branch from c99b0ca to d6d4ed1 Compare August 16, 2022 20:46

misteroneill added the major label Aug 17, 2022

misteroneill requested changes Aug 17, 2022

View reviewed changes

kchang-brightcove added 5 commits August 18, 2022 13:02

remove experimental options

ce19e7d

remove default true for experimentalBufferBasedABR

5c90c75

fix llhls default value

de74d3b

make llhls default true

67aef06

set llhls defaut in playlist-loader

7999e6b

kchang-brightcove added 6 commits August 18, 2022 13:03

set llhls as default in setOptions_

044756c

fix inappropriate lines

0a8b83b

fix linting

62604d6

remove unnecessary parens

b1e3e1b

rename experimentalBufferBasedABR to bufferBasedABR

461da4c

fix: fix netlify preview build

4b1d2a3

kchang-brightcove force-pushed the wp-529-remove-experimental-options branch from d6d4ed1 to 4b1d2a3 Compare August 18, 2022 17:06

misteroneill approved these changes Aug 18, 2022

View reviewed changes

gesinger approved these changes Aug 19, 2022

View reviewed changes

misteroneill merged commit 02c3c77 into next Aug 19, 2022

misteroneill deleted the wp-529-remove-experimental-options branch August 19, 2022 14:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove experimental options #1301

feat: remove experimental options #1301

kchang-brightcove commented Jul 28, 2022 •

edited by misteroneill

Loading

welcome bot commented Jul 28, 2022

gkatsev commented Jul 28, 2022

misteroneill left a comment

misteroneill Aug 1, 2022

misteroneill Aug 3, 2022

misteroneill left a comment

gesinger Aug 3, 2022

gesinger Aug 3, 2022

gesinger Aug 3, 2022

gesinger Aug 3, 2022

gesinger Aug 4, 2022

gesinger Aug 4, 2022

misteroneill Aug 18, 2022

gesinger left a comment

codecov bot commented Aug 11, 2022 •

edited

Loading

gkatsev Aug 16, 2022

misteroneill Aug 16, 2022

kchang-brightcove Aug 16, 2022

misteroneill Aug 17, 2022

misteroneill Aug 16, 2022

misteroneill left a comment

misteroneill Aug 17, 2022

misteroneill Aug 17, 2022

misteroneill Aug 18, 2022

welcome bot commented Aug 19, 2022

	this.llhls = (vhsOptions && vhsOptions.llhls) \|\| false;
	this.llhls = vhsOptions && vhsOptions.llhls;

	this.options_.llhls = this.options_.llhls === false ? false : true;
	this.options_.llhls = Boolean(this.options_.llhls);

	this.options_.llhls = Boolean(this.options_.llhls);
	this.options_.llhls = this.options_.hasOwnProperty('llhls') ? this.options_.llhls : true;

		@@ -641,11 +642,11 @@ class VhsHandler extends Component {
		'initialPlaylistSelector',
		'experimentalBufferBasedABR',

feat: remove experimental options #1301

feat: remove experimental options #1301

Conversation

kchang-brightcove commented Jul 28, 2022 • edited by misteroneill Loading

Description

welcome bot commented Jul 28, 2022

gkatsev commented Jul 28, 2022

misteroneill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misteroneill 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

gesinger left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 11, 2022 • edited Loading

Codecov Report

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

misteroneill 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

welcome bot commented Aug 19, 2022

kchang-brightcove commented Jul 28, 2022 •

edited by misteroneill

Loading

codecov bot commented Aug 11, 2022 •

edited

Loading