Add option to have remoteTextTracks automatically 'garbage-collected' when sources change #3736

imbcmdth · 2016-11-03T18:39:53Z

Description

Right now "remoteTextTracks" persist between source changes and must be removed manually by the user. This is problematic in many cases. It means plugins that change sources, such as the playlist API or videojs-contrib-ads, have to be extremely careful about changes the source and take care to cleanup remoteTextTracks.

We do this because the Video.js API aligns very closely with the media element's native API. I propose that we could be doing more to iron-out the rough patches in the native API. In my opinion, the fact that previous changes to the media-element (adding text track elements) may change the behavior of a source is one of those rough patches.

Specific Changes proposed

Tech#addRemoteTextTrack now accepts a second parameter - a boolean named manualCleanup which defaults to true preserving backwards compatibility. When that value is set to false the text track will be removed from the video element whenever a source change occurs.

A new instance property named autoRemoteTextTracks_ is added to Tech that keeps track of which TextTracks have automatic life-cycle management.

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 · 2016-11-03T18:46:04Z

src/js/tech/tech.js

    if (!sh) {
      // Fall back to a native source hander when unsupported sources are
      // deliberately set
      if (_Tech.nativeSourceHandler) {
        sh = _Tech.nativeSourceHandler;
      } else {
        log.error('No source hander found for the current source.');
+        return;


The addition of this return and moving of lines 843-844 to above this conditional block are to fix a tiny issue that may have never been manifest in the wild but which I discovered during a test run.

If a Tech has no nativeSourceHandler property and selectSourceHandler returned null because no matching source handlers were found, then the line later on: this.sourceHandler_ = sh.handleSource(source, this, this.options_); would throw an exception because sh would stay null.

misteroneill · 2016-11-03T19:08:43Z

Seems reasonable to me. I agree that it feels like the expected behavior when changing sources would be to clear out remote text tracks. And backward-compatibility is a plus as that would've been my big concern here.

I wonder if we ought to do something similar with non-remote tracks? Disable them... or something. Not for this PR, but a general thought.

imbcmdth · 2016-11-03T21:22:24Z

src/js/tech/tech.js

    const htmlTrackElement = new HTMLTrackElement(track);

+    if (manualCleanup !== true && manualCleanup !== false) {
+      // deprecation warning
+      log.warn('Calling addRemoteTextTrack without explicitly setting the "manualCleanup" parameter to `true` is deprecated and default to `false` in future version of video.js');


I am open to some nice word-smithing here. I feel like coming up with error/warnings is not my strong suit.

is there any reason why we need to deprecate this and not just make the change? This almost seems like a bug. If we decide not to deprecate and to just make the change we can probably make this change much smaller.

This is because currently, you have to manually cleanup, we're wanting to change it so that by default tracks are cleaned up automatically.

brandonocasey · 2016-11-04T20:12:58Z

src/js/tech/tech.js

   * @return {HTMLTrackElement}
   * @method addRemoteTextTrack
   */
-  addRemoteTextTrack(options) {
+  addRemoteTextTrack(options, manualCleanup) {


I would make this , manualCleanup = true) here.

Having a default option here would make it difficult to do the deprecation warning.

brandonocasey · 2016-11-04T20:13:40Z

src/js/tech/tech.js

@@ -531,20 +548,33 @@ class Tech extends Component {
   *
   * @param {Object} options The object should contain values for
   * kind, language, label and src (location of the WebVTT file)
+   * @param {Boolean} manualCleanup if set to false, the TextTrack will be


this should be * @param {boolean} [manualCleanup=true], since its optional and defaults to true

also you should add an @depricated tag to the bottom stating that "The manualCleanup parameter will be removed in favor of always doing automatic cleanup in the future."

brandonocasey · 2016-11-04T20:23:55Z

src/js/tech/tech.js

    const htmlTrackElement = new HTMLTrackElement(track);

+    if (manualCleanup !== true && manualCleanup !== false) {
+      // deprecation warning
+      log.warn('Calling addRemoteTextTrack without explicitly setting the "manualCleanup" parameter to `true` is deprecated and default to `false` in future version of video.js');


is there any reason why we need to deprecate this and not just make the change? This almost seems like a bug. If we decide not to deprecate and to just make the change we can probably make this change much smaller.

gkatsev

Html5 tech needs to update addRemoteTextTrack as well because it overrides the method and calls super, so, the manualCleanup option gets lost.

gkatsev · 2016-11-07T23:19:17Z

src/js/tech/html5.js

-   * @return {HTMLTrackElement}
-   */
-  addRemoteTextTrack(options = {}) {
+  createRemoteTextTrack(options) {


this should probably be private.

gkatsev · 2016-11-07T23:20:12Z

src/js/tech/tech.js

-      return;
-    }
-
+  addWebVttScript() {


gkatsev · 2016-11-07T23:20:32Z

src/js/tech/tech.js

@@ -543,6 +560,14 @@ class Tech extends Component {
    return createTrackHelper(this, kind, label, language);
  }

+  createRemoteTextTrack(options) {


misteroneill

LGTM

…ge-collected' when the source changes

…th emulated and native TextTracks. * Remove the redundancy in Html5 & Tech around remote text tracks. * Changed Tech construction to call emulateTextTracks immediately but defer the creation of the webvtt to the `ready` event, if necessary. * Player now passes along all arguments to the underlying Tech on addRemoteTextTrack.

gkatsev

Still need to test it but code-wise, LGTM.

brandonocasey

Pending doc commet/arg changes LGTM

brandonocasey · 2016-11-09T19:14:36Z

src/js/player.js

@@ -2525,9 +2525,9 @@ class Player extends Component {
   *
   * @param {Object} options    Options for remote text track


this should be {TextTrackList[]} theArgs track to add as remote text tracks, also theArgs is a weird name, shouldn't it be tracks?

brandonocasey · 2016-11-09T19:16:02Z

src/js/tech/html5.js

+   * Creates a remote text track object and returns a html track element
+   *
+   * @param {Object} options The object should contain values for
+   * kind, language, label and src (location of the WebVTT file)


theRest needs to be added to the doc comment, and the name is a bit weird (maybe it will make more sense to me with a doc comment.)

brandonocasey · 2016-11-09T20:23:00Z

src/js/player.js

+   * @deprecated The default value of the "manualCleanup" parameter will default
+   * to "false" in upcoming versions of Video.js
+   */
+  addRemoteTextTrack(...theArgs) {


this needs to be changed from ...theArgs

brandonocasey

LGTM

imbcmdth commented Nov 3, 2016

View reviewed changes

imbcmdth force-pushed the auto-cleaup-texttracks branch from 7b59df9 to 838e64a Compare November 3, 2016 20:37

imbcmdth commented Nov 3, 2016

View reviewed changes

gkatsev added this to the 5.13 milestone Nov 4, 2016

brandonocasey requested changes Nov 4, 2016

View reviewed changes

imbcmdth force-pushed the auto-cleaup-texttracks branch from 838e64a to b59701b Compare November 4, 2016 22:22

gkatsev requested changes Nov 4, 2016

View reviewed changes

dmlap mentioned this pull request Nov 7, 2016

Changing source URL leads to closed captioning track duplicate videojs/videojs-contrib-hls#890

Closed

gkatsev reviewed Nov 7, 2016

View reviewed changes

imbcmdth force-pushed the auto-cleaup-texttracks branch from 4de4731 to 2c584df Compare November 8, 2016 16:41

misteroneill approved these changes Nov 9, 2016

View reviewed changes

jrivera added 5 commits November 9, 2016 12:10

Added the ability to flag remoteTextTracks to be automatically 'garba…

cb0df09

…ge-collected' when the source changes

Added deprecation notice to doc comment

08a1eb6

Added doc comments for createRemoteTextTrack

402a126

Renamed addWebVttScript function and made it private

b65b872

gkatsev force-pushed the auto-cleaup-texttracks branch from 2c584df to b65b872 Compare November 9, 2016 17:11

gkatsev approved these changes Nov 9, 2016

View reviewed changes

brandonocasey requested changes Nov 9, 2016

View reviewed changes

brandonocasey approved these changes Nov 9, 2016

View reviewed changes

Update jsdoc. Move default options to tech

ad7008a

gkatsev force-pushed the auto-cleaup-texttracks branch from 3e24eb5 to ad7008a Compare November 9, 2016 20:15

brandonocasey requested changes Nov 9, 2016

View reviewed changes

update player.js method signature

9359fa3

brandonocasey approved these changes Nov 9, 2016

View reviewed changes

gkatsev merged commit f05a927 into master Nov 9, 2016

gkatsev deleted the auto-cleaup-texttracks branch November 9, 2016 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to have remoteTextTracks automatically 'garbage-collected' when sources change #3736

Add option to have remoteTextTracks automatically 'garbage-collected' when sources change #3736

imbcmdth commented Nov 3, 2016 •

edited

Loading

imbcmdth Nov 3, 2016

misteroneill commented Nov 3, 2016

imbcmdth Nov 3, 2016

brandonocasey Nov 4, 2016

gkatsev Nov 4, 2016

brandonocasey Nov 4, 2016

gkatsev Nov 4, 2016

brandonocasey Nov 4, 2016

brandonocasey Nov 4, 2016

imbcmdth Nov 4, 2016

brandonocasey Nov 4, 2016

gkatsev left a comment

gkatsev Nov 7, 2016

gkatsev Nov 7, 2016

gkatsev Nov 7, 2016

misteroneill left a comment

gkatsev left a comment

brandonocasey left a comment

brandonocasey Nov 9, 2016

brandonocasey Nov 9, 2016

brandonocasey Nov 9, 2016

brandonocasey left a comment

		@@ -2525,9 +2525,9 @@ class Player extends Component {
		*
		* @param {Object} options Options for remote text track

Add option to have remoteTextTracks automatically 'garbage-collected' when sources change #3736

Add option to have remoteTextTracks automatically 'garbage-collected' when sources change #3736

Conversation

imbcmdth commented Nov 3, 2016 • edited Loading

Description

Specific Changes proposed

Requirements Checklist

Choose a reason for hiding this comment

misteroneill commented Nov 3, 2016

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

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

misteroneill left a comment

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

brandonocasey 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

brandonocasey left a comment

Choose a reason for hiding this comment

imbcmdth commented Nov 3, 2016 •

edited

Loading