Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 7 commits into from
Nov 9, 2016
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cb0df09
Added the ability to flag remoteTextTracks to be automatically 'garba…
Nov 3, 2016
08a1eb6
Added deprecation notice to doc comment
Nov 4, 2016
8a5fd49
Fix the auto-managed remoteTexttTacks to work correctly when using bo…
Nov 7, 2016
402a126
Added doc comments for createRemoteTextTrack
Nov 7, 2016
b65b872
Renamed addWebVttScript function and made it private
Nov 8, 2016
ad7008a
Update jsdoc. Move default options to tech
gkatsev Nov 9, 2016
9359fa3
update player.js method signature
gkatsev Nov 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions src/js/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2521,13 +2521,20 @@ class Player extends Component {
}

/**
* Add a remote text track
*
* @param {Object} options Options for remote text track
*/
addRemoteTextTrack(options) {
* Creates a remote text track object and returns an html track element.
*
* @param {Object} options The object should contain values for
* kind, language, label, and src (location of the WebVTT file)
* @param {Boolean} [manualCleanup=true] if set to false, the TextTrack will be
* automatically removed from the video element whenever the source changes
* @return {HTMLTrackElement} An Html Track Element.
* This can be an emulated {@link HTMLTrackElement} or a native one.
* @deprecated The default value of the "manualCleanup" parameter will default
* to "false" in upcoming versions of Video.js
*/
addRemoteTextTrack(options, manualCleanup) {
if (this.tech_) {
return this.tech_.addRemoteTextTrack(options);
return this.tech_.addRemoteTextTrack(options, manualCleanup);
}
}

Expand Down
41 changes: 23 additions & 18 deletions src/js/tech/html5.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,17 +608,16 @@ class Html5 extends Tech {
}

/**
* Creates a remote text track object and returns a html track element
* Creates either native TextTrack or an emulated TextTrack depending
* on the value of `featuresNativeTextTracks`
*
* @param {Object} options The object should contain values for
* kind, language, label and src (location of the WebVTT file)
* @return {HTMLTrackElement}
*/
addRemoteTextTrack(options = {}) {
createRemoteTextTrack(options) {
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be private.

if (!this.featuresNativeTextTracks) {
return super.addRemoteTextTrack(options);
return super.createRemoteTextTrack(options);
}

const htmlTrackElement = document.createElement('track');

if (options.kind) {
Expand All @@ -640,11 +639,25 @@ class Html5 extends Tech {
htmlTrackElement.src = options.src;
}

this.el().appendChild(htmlTrackElement);
return htmlTrackElement;
}

/**
* Creates a remote text track object and returns an html track element.
*
* @param {Object} options The object should contain values for
* kind, language, label, and src (location of the WebVTT file)
* @param {Boolean} [manualCleanup=true] if set to false, the TextTrack will be
* automatically removed from the video element whenever the source changes
* @return {HTMLTrackElement} An Html Track Element.
* This can be an emulated {@link HTMLTrackElement} or a native one.
* @deprecated The default value of the "manualCleanup" parameter will default
* to "false" in upcoming versions of Video.js
*/
addRemoteTextTrack(options, manualCleanup) {
const htmlTrackElement = super.addRemoteTextTrack(options, manualCleanup);

// store HTMLTrackElement and TextTrack to remote list
this.remoteTextTrackEls().addTrackElement_(htmlTrackElement);
this.remoteTextTracks().addTrack_(htmlTrackElement.track);
this.el().appendChild(htmlTrackElement);

return htmlTrackElement;
}
Expand All @@ -655,15 +668,7 @@ class Html5 extends Tech {
* @param {TextTrackObject} track Texttrack object to remove
*/
removeRemoteTextTrack(track) {
if (!this.featuresNativeTextTracks) {
return super.removeRemoteTextTrack(track);
}

const trackElement = this.remoteTextTrackEls().getTrackElementByTrack_(track);

// remove HTMLTrackElement and TextTrack from remote list
this.remoteTextTrackEls().removeTrackElement_(trackElement);
this.remoteTextTracks().removeTrack_(track);
super.removeRemoteTextTrack(track);

const tracks = this.$$('track');

Expand Down
134 changes: 100 additions & 34 deletions src/js/tech/tech.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,11 @@ class Tech extends Component {
}

if (!this.featuresNativeTextTracks) {
this.on('ready', this.emulateTextTracks);
this.emulateTextTracks();
}

this.autoRemoteTextTracks_ = new TextTrackList();

this.initTextTrackListeners();
this.initTrackListeners();

Expand Down Expand Up @@ -291,6 +293,23 @@ class Tech extends Component {
});
}

/**
* Remove any TextTracks added via addRemoteTextTrack that are
* flagged for automatic garbage collection
*
* @method cleanupAutoTextTracks
*/
cleanupAutoTextTracks() {
const list = this.autoRemoteTextTracks_ || [];
let i = list.length;

while (i--) {
const track = list[i];

this.removeRemoteTextTrack(track);
}
}

/**
* Reset the tech. Removes all sources and resets readyState.
*
Expand Down Expand Up @@ -394,17 +413,11 @@ class Tech extends Component {
}

/**
* Emulate texttracks
* Add vtt.js if necessary
*
* @method emulateTextTracks
* @private
*/
emulateTextTracks() {
const tracks = this.textTracks();

if (!tracks) {
return;
}

addWebVttScript_() {
if (!window.WebVTT && this.el().parentNode !== null && this.el().parentNode !== undefined) {
const script = document.createElement('script');

Expand All @@ -424,6 +437,32 @@ class Tech extends Component {
window.WebVTT = true;
this.el().parentNode.appendChild(script);
}
}

/**
* Emulate texttracks
*
* @method emulateTextTracks
*/
emulateTextTracks() {
const tracks = this.textTracks();

if (!tracks) {
return;
}

this.remoteTextTracks().on('addtrack', (e) => {
this.textTracks().addTrack_(e.track);
});

this.remoteTextTracks().on('removetrack', (e) => {
this.textTracks().removeTrack_(e.track);
});

// Initially, Tech.el_ is a child of a dummy-div wait until the Component system
// signals that the Tech is ready at which point Tech.el_ is part of the DOM
// before inserting the WebVTT script
this.on('ready', this.addWebVttScript_);

const updateDisplay = () => this.trigger('texttrackchange');
const textTracksChanges = () => {
Expand Down Expand Up @@ -527,26 +566,51 @@ class Tech extends Component {
}

/**
* Creates a remote text track object and returns a emulated html track element
* Create an emulated TextTrack for use by addRemoteTextTrack
*
* This is intended to be overridden by classes that inherit from
* Tech in order to create native or custom TextTracks.
*
* @param {Object} options The object should contain values for
* kind, language, label and src (location of the WebVTT file)
* @return {HTMLTrackElement}
* @method addRemoteTextTrack
*/
addRemoteTextTrack(options) {
createRemoteTextTrack(options) {
Copy link
Member

Choose a reason for hiding this comment

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

private

const track = mergeOptions(options, {
tech: this
});

const htmlTrackElement = new HTMLTrackElement(track);
return new HTMLTrackElement(track);
}

/**
* Creates a remote text track object and returns an html track element.
*
* @param {Object} options The object should contain values for
* kind, language, label, and src (location of the WebVTT file)
* @param {Boolean} [manualCleanup=true] if set to false, the TextTrack will be
* automatically removed from the video element whenever the source changes
* @return {HTMLTrackElement} An Html Track Element.
* This can be an emulated {@link HTMLTrackElement} or a native one.
* @deprecated The default value of the "manualCleanup" parameter will default
* to "false" in upcoming versions of Video.js
*/
addRemoteTextTrack(options = {}, manualCleanup) {
const htmlTrackElement = this.createRemoteTextTrack(options);

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');
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

manualCleanup = true;
}

// store HTMLTrackElement and TextTrack to remote list
this.remoteTextTrackEls().addTrackElement_(htmlTrackElement);
this.remoteTextTracks().addTrack_(htmlTrackElement.track);

// must come after remoteTextTracks()
this.textTracks().addTrack_(htmlTrackElement.track);
if (manualCleanup !== true) {
// create the TextTrackList if it doesn't exist
this.autoRemoteTextTracks_.addTrack_(htmlTrackElement.track);
}

return htmlTrackElement;
}
Expand All @@ -558,13 +622,12 @@ class Tech extends Component {
* @method removeRemoteTextTrack
*/
removeRemoteTextTrack(track) {
this.textTracks().removeTrack_(track);

const trackElement = this.remoteTextTrackEls().getTrackElementByTrack_(track);

// remove HTMLTrackElement and TextTrack from remote list
this.remoteTextTrackEls().removeTrackElement_(trackElement);
this.remoteTextTracks().removeTrack_(track);
this.autoRemoteTextTracks_.removeTrack_(track);
}

/**
Expand Down Expand Up @@ -685,7 +748,7 @@ Tech.prototype.featuresNativeTextTracks = false;
*
* ##### EXAMPLE:
*
* Tech.withSourceHandlers.call(MyTech);
* Tech.withSourceHandlers(MyTech);
*
*/
Tech.withSourceHandlers = function(_Tech) {
Expand Down Expand Up @@ -820,25 +883,14 @@ Tech.withSourceHandlers = function(_Tech) {
this.disposeSourceHandler();
this.off('dispose', this.disposeSourceHandler);

// if we have a source and get another one
// then we are loading something new
// than clear all of our current tracks
if (this.currentSource_) {
this.clearTracks(['audio', 'video']);

this.currentSource_ = null;
}

if (sh !== _Tech.nativeSourceHandler) {

this.currentSource_ = source;

// Catch if someone replaced the src without calling setSource.
// If they do, set currentSource_ to null and dispose our source handler.
this.off(this.el_, 'loadstart', _Tech.prototype.firstLoadStartListener_);
this.off(this.el_, 'loadstart', _Tech.prototype.successiveLoadStartListener_);
this.one(this.el_, 'loadstart', _Tech.prototype.firstLoadStartListener_);

}

this.sourceHandler_ = sh.handleSource(source, this, this.options_);
Expand All @@ -854,7 +906,6 @@ Tech.withSourceHandlers = function(_Tech) {

// On successive loadstarts when setSource has not been called again
_Tech.prototype.successiveLoadStartListener_ = function() {
this.currentSource_ = null;
this.disposeSourceHandler();
this.one(this.el_, 'loadstart', _Tech.prototype.successiveLoadStartListener_);
};
Expand All @@ -863,10 +914,25 @@ Tech.withSourceHandlers = function(_Tech) {
* Clean up any existing source handler
*/
_Tech.prototype.disposeSourceHandler = function() {
if (this.sourceHandler_ && this.sourceHandler_.dispose) {
// if we have a source and get another one
// then we are loading something new
// than clear all of our current tracks
if (this.currentSource_) {
this.clearTracks(['audio', 'video']);
this.currentSource_ = null;
}

// always clean up auto-text tracks
this.cleanupAutoTextTracks();

if (this.sourceHandler_) {
this.off(this.el_, 'loadstart', _Tech.prototype.firstLoadStartListener_);
this.off(this.el_, 'loadstart', _Tech.prototype.successiveLoadStartListener_);
this.sourceHandler_.dispose();

if (this.sourceHandler_.dispose) {
this.sourceHandler_.dispose();
}

this.sourceHandler_ = null;
}
};
Expand Down
Loading