Add option to override native audio and video to html5 tech #5074

OshinKaramian · 2018-04-04T22:47:02Z

Description

Adding an option to override native audio/video tracks. This is being added to ease enabling and disabling it within https://github.com/videojs/http-streaming. This is partly to make enabling/disabling native playback easier when integrating videojs/http-streaming into this project.

I'm in the process of adding tests. I'd like eyes on this while I'm doing that to ensure that the implementation makes sense/if I'm missing anything that might break playback that I'm unaware of.

Specific Changes proposed

Adds 2 flags on the Html5 tech, overrideNativeAudioTracks and overrideNativeVideoTracks that can be set at any time. The change will take effect any time the source is set.

One question I'm unclear on... what should the version compatibility be with video.js with this change? I can modify things to be backwards compatible (checking to see if the APIs exist and handling things one way or another). Do we care about this? Or are we okay removing this (since leaving it in for backwards compatibility purposes would add a little bit of tech debt).

Requirements Checklist

Feature implemented / Bug fixed
If necessary, more likely in a feature request than a bug fix
- Change has been verified in an actual browser (Verified in Edge with VHS)
- Unit Tests updated or fixed
- Docs/guides updated
- Example created (starter template on JSBin)
Reviewed by Two Core Contributors

forbesjo · 2018-04-09T14:57:06Z

src/js/tech/html5.js

@@ -200,6 +200,14 @@ class Html5 extends Tech {
    });
  }

+  overrideNativeAudioTracks(override) {


Needs a jsDoc explaining the intent of these functions and how it will take affect on source change

forbesjo · 2018-04-09T18:16:44Z

We should make sure these options are compatible with the player options https://github.com/videojs/video.js/blob/master/docs/guides/options.md#html5

gkatsev

not a huge fan of adding a bunch of new flags, also, have some follow up questions about this.

gkatsev · 2018-04-09T20:23:45Z

src/js/tech/html5.js

@@ -605,6 +639,9 @@ class Html5 extends Tech {
   * @deprecated Since version 5.
   */
  src(src) {
+    this.forceNativeAudioOverride_ = this.forceNativeAudioOverride;


why the two separate flags?

It seemed more flexible to have two flags over one flag to rule them all. I can merge them together if that's preferred.

gkatsev · 2018-04-09T20:26:12Z

src/js/tech/html5.js

+      };
+
+      listeners.addtrack = (e) => {
+        if (this[`forceNative${props.capitalName}Override_`] && elTracks.addTrack) {


why would elTracks not have an addTrack method?

I'm not sure, it seemed safer to check to see if a method existed on something before using it.

gkatsev · 2018-04-09T21:01:26Z

So, the idea here is that VHS would call overrideNativeVideoTracks and overrideNativeVideoTracks with true to tell the the tech not to use native stuff?
If I'm remembering proxyNativeTracks_ correctly, would the best solution be to just remove the listeners when we disable native track support? Though, that may require a larger refactor.

Also, the override methods should probably be added to tech.js as empty methods.

gkatsev · 2018-04-09T21:05:23Z

src/js/tech/html5.js

+
+      listeners.removetrack = (e) => {
+        if (this[`forceNative${props.capitalName}Override_`] && elTracks.addTrack) {
+          elTracks.removeTrack(e.track);


so, I think we don't want to do this part of the if statement for addTrack and removeTrack, since the event comes from the native elTracks object already.
I think best case scenario is that we should be removing the listeners when we disable native support.

gkatsev · 2018-04-09T21:06:20Z

src/js/tech/html5.js

+   * otherwise native video will potentially be used.
+   */
+  overrideNativeAudioTracks(override) {
+    this.forceNativeAudioOverride = override;


should we just override featuresNativeAudioTracks and featuresNativeVideoTracks? While a lot of the docs language around it says it's around "support" it also means whether the tech will be using that feature or not.

we may also need to do something like call proxyNativeTracks_ after the fact if this is called with false so that that process kicks in, especially if we originally start with them disabled.

OshinKaramian · 2018-04-09T21:33:34Z

Unfortunately I can't comment on single comments, so re: removing listeners:

I explored this initially but had also determined it would be a pretty large refactor, I was attempting to go in with less work/more of a scalpel. I can go down this route, but I have a feeling this is going to be a bit more work to ensure things do not break.

gkatsev

Thanks, I think this is better reflects what should happen.

gkatsev · 2018-04-10T14:56:03Z

src/js/tech/html5.js

@@ -217,22 +251,26 @@ class Html5 extends Tech {
          !elTracks.addEventListener) {
        return;
      }
-      const listeners = {


would be good to revert this object's change to make the PR more streamlined.

gkatsev · 2018-04-10T14:56:11Z

src/js/tech/html5.js

@@ -613,6 +661,10 @@ class Html5 extends Tech {
    this.setSrc(src);
  }

+  setSrc(src) {


this shouldn't be necessary

gkatsev · 2018-04-10T15:06:14Z

src/js/tech/html5.js

+          this.trackListeners[props.capitalName] = [];
+        }
+
+        this.trackListeners[props.capitalName].push({ eventName, listener });


what about instead of making an array of stuff, do something like the following outside of this Object.keys().forEach:

this[props.getterName + 'Listeners_'] = listeners;

then to remove these, outside of this method we can do something like:

Object.keys(this.audioTracksListeners_).forEach((eventName) => { elTracks.removeEventListener(eventName, this.audioTracksListeners_[eventName] }); Object.keys(this.videoTracksListeners_).forEach((eventName) => { elTracks.removeEventListener(eventName, this.videoTracksListeners_[eventName] });

gkatsev · 2018-04-10T15:06:50Z

src/js/tech/html5.js

+   * @param {Boolean} override - If set to true native video will be overridden,
+   * otherwise native video will potentially be used.
+   */
+  overrideNativeTracks(override) {


might be good to make this a no-op if we aren't changing the override.

gkatsev · 2018-04-10T15:07:31Z

src/js/tech/html5.js

+    this.trackListeners.Video = [];
+    this.trackListeners.Audio = [];
+
+    this.proxyNativeTracks_();


so basically, everything above will remove listeners and then we call proxyNativeTracks_ so that if we turn native back on it'll run but if we turned it off, proxyNativeTracks_ will exit early and not run?

That sounds right to me.

gkatsev · 2018-04-10T15:07:48Z

src/js/tech/html5.js

+   * Attempt to force override of native video tracks.
+   *
+   * @param {Boolean} override - If set to true native video will be overridden,
+   * otherwise native video will potentially be used.


would be good to update this to mention that this is just for audio and video tracks

gkatsev · 2018-04-10T15:08:43Z

src/js/tech/html5.js

+   * @param {Boolean} override - If set to true native video will be overridden,
+   * otherwise native video will potentially be used.
+   */
+  overrideNativeTracks(override) {


we should add either an empty method or a default implementation in tech.js for this.

gkatsev · 2018-04-10T18:32:50Z

src/js/tech/html5.js

@@ -260,6 +301,9 @@ class Html5 extends Tech {
        const listener = listeners[eventName];

        elTracks.addEventListener(eventName, listener);
+
+        this[props.getterName + 'Listeners_'] = listeners;


this can move outside of the forEach (line 300).

gkatsev · 2018-04-10T18:35:37Z

src/js/tech/html5.js

+   */
+  overrideNativeTracks(override) {
+    // If there is no behavioral change don't add/remove listeners
+    if (override === this.override_) {


could we compare this against the values of featuresNativeVideoTracks and featuresNativeAudioTracks instead? that way we don't have another flag to worry about.

Given a brief chat with @gkatsev I'll set this to check against featuresNativeVideoTracks && featuresNativeAudioTracks.

forbesjo · 2018-04-10T19:43:01Z

src/js/tech/html5.js

+   */
+  overrideNativeTracks(override) {
+    // If there is no behavioral change don't add/remove listeners
+    if (override !== (this.featuresNativeAudioTracks && this.featuresNativeAudioTracks)) {


That took me a minute to spot, good 👀 s

gkatsev · 2018-04-11T15:58:46Z

src/js/tech/html5.js

+    this.featuresNativeVideoTracks = !override;
+    this.featuresNativeAudioTracks = !override;
+
+    this.audioTracksListeners_ = [];


should be set to null probably.

brandonocasey · 2018-04-11T16:32:17Z

src/js/tech/html5.js

@@ -200,6 +200,43 @@ class Html5 extends Tech {
    });
  }

+  /**
+   * Attempt to force override of native audiio/video tracks.


audiio -> audio

brandonocasey · 2018-04-11T16:33:19Z

src/js/tech/html5.js

+   */
+  overrideNativeTracks(override) {
+    // If there is no behavioral change don't add/remove listeners
+    if (override !== (this.featuresNativeAudioTracks && this.featuresNativeVideoTracks)) {


should audio and video tracks be overridden in a separate check?

at this time, we're assuming they'll always match, we can always change it later if necessary.

Add option to override native audio and video to html5 tech

9c20c37

OshinKaramian mentioned this pull request Apr 4, 2018

feat: Utilize option to override native on tech videojs/http-streaming#76

Merged

6 tasks

Oshin Karamian added 2 commits April 5, 2018 20:41

Add tests for override properties

1f2461b

Get rid of rogue only

e9ed558

forbesjo reviewed Apr 9, 2018

View reviewed changes

Add jsdoc for overrideAudio/Video Tracks

397354d

gkatsev reviewed Apr 9, 2018

View reviewed changes

Oshin Karamian added 2 commits April 9, 2018 21:32

Rework to remove listeners instead of leaving them and ignoring logic

51f2330

Get rid of stored local variables

ec99367

gkatsev reviewed Apr 10, 2018

View reviewed changes

Oshin Karamian added 5 commits April 10, 2018 14:22

Code review comments

a158145

Get rid of extraneous white space

59f90d1

Another rogue white space

c2216ff

Doc update

99b2105

More tiny doc updates

49a226f

gkatsev reviewed Apr 10, 2018

View reviewed changes

Oshin Karamian added 3 commits April 10, 2018 15:06

Code review updates

58cd557

Eliminate white space

1afa8b2

Small change in if clause

c140236

forbesjo reviewed Apr 10, 2018

View reviewed changes

Oshin Karamian added 2 commits April 10, 2018 15:56

Fix copy pasta issue

c359c94

Whitespace consistency

cb15e8f

gkatsev reviewed Apr 11, 2018

View reviewed changes

brandonocasey reviewed Apr 11, 2018

View reviewed changes

Code review updates

5af08c9

gkatsev approved these changes Apr 17, 2018

View reviewed changes

gkatsev added the needs: LGTM Needs one or more additional approvals label Apr 17, 2018

brandonocasey approved these changes Apr 19, 2018

View reviewed changes

gkatsev added confirmed major and removed needs: LGTM Needs one or more additional approvals labels Apr 19, 2018

gkatsev merged commit 22bbbc9 into videojs:master Apr 19, 2018

gkatsev mentioned this pull request Apr 23, 2018

Changelog for 7.0.0? #5119

Closed

thijstriemstra mentioned this pull request May 15, 2018

Tech breaks other video.js players on same page collab-project/videojs-wavesurfer#60

Closed

OshinKaramian mentioned this pull request May 24, 2018

Update overrideNative flag to take in feature from VideoJS 7 videojs/http-streaming#101

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to override native audio and video to html5 tech #5074

Add option to override native audio and video to html5 tech #5074

OshinKaramian commented Apr 4, 2018 •

edited

Loading

forbesjo Apr 9, 2018

forbesjo commented Apr 9, 2018

gkatsev left a comment

gkatsev Apr 9, 2018

OshinKaramian Apr 9, 2018

gkatsev Apr 9, 2018

OshinKaramian Apr 9, 2018

gkatsev commented Apr 9, 2018

gkatsev Apr 9, 2018

gkatsev Apr 9, 2018

gkatsev Apr 9, 2018

OshinKaramian commented Apr 9, 2018

gkatsev left a comment

gkatsev Apr 10, 2018

gkatsev Apr 10, 2018

gkatsev Apr 10, 2018

gkatsev Apr 10, 2018

gkatsev Apr 10, 2018

OshinKaramian Apr 10, 2018

gkatsev Apr 10, 2018

gkatsev Apr 10, 2018

gkatsev Apr 10, 2018

gkatsev Apr 10, 2018

OshinKaramian Apr 10, 2018

forbesjo Apr 10, 2018

OshinKaramian Apr 10, 2018

gkatsev Apr 11, 2018

brandonocasey Apr 11, 2018

brandonocasey Apr 11, 2018

gkatsev Apr 11, 2018

Add option to override native audio and video to html5 tech #5074

Add option to override native audio and video to html5 tech #5074

Conversation

OshinKaramian commented Apr 4, 2018 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

Choose a reason for hiding this comment

forbesjo commented Apr 9, 2018

gkatsev 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

gkatsev commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OshinKaramian commented Apr 9, 2018

gkatsev 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

OshinKaramian commented Apr 4, 2018 •

edited

Loading