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.

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

Prevent non native text tracks from leaking cuechange event listeners #2849

Closed
wants to merge 1 commit into from

Conversation

gesinger
Copy link
Contributor

Due to a new updateDisplay function being generated on each invocation of the textTracksChanges function within emulateTextTracks of Tech, the cuechange event listener which called updateDisplay was not getting removed when looping through text tracks. This created a leak where updateDisplay could be called many times on a single track cuechange event.

An example of this can be seen by running the following:

var player = videojs('my-video', {
  html5: {
    nativeTextTracks: false
  }
});

player.on(‘ready’, function() {  
  var track = player.addTextTrack('captions');
  track.addCue(new VTTCue(1, 2, 'Test'));
  track.mode = 'showing';
  track.mode = 'showing';
  track.mode = 'showing';
  player.tech_.on('texttrackchange', function() { console.log('Text track change'); });
  player.play();
});

When a cuechange is reached (the first being at 1 second), texttrackchange will be triggered 3 times (one for each set of track.mode).

This change moves the updateDisplay function outside of the textTracksChanges function, so that it will not be regenerated on each call of textTracksChanges.

@pam
Copy link

pam commented Nov 24, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 32a4b8dcd649be2b9e6448d835f76511a9d6cbf3
Build details: https://travis-ci.org/pam/video.js/builds/92882743

(Please note that this is a fully automated comment.)

let textTracksChanges = Fn.bind(this, function() {
let updateDisplay = () => this.trigger('texttrackchange');

let updateDisplay = Fn.bind(this, () => this.trigger('texttrackchange'));
Copy link
Member

Choose a reason for hiding this comment

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

no need to bind the error function (not that you technically can do that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Thanks!

@gkatsev gkatsev added patch This PR can be added to a patch release. needs: LGTM Needs one or more additional approvals labels Nov 24, 2015
@pam
Copy link

pam commented Nov 25, 2015

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 24d7953
Build details: https://travis-ci.org/pam/video.js/builds/93072923

(Please note that this is a fully automated comment.)

@gkatsev
Copy link
Member

gkatsev commented Nov 25, 2015

LGTM

1 similar comment
@mmcc
Copy link
Member

mmcc commented Nov 25, 2015

LGTM

@gkatsev gkatsev closed this in 72f77d7 Nov 25, 2015
@gkatsev gkatsev mentioned this pull request Nov 25, 2015
@gkatsev gkatsev removed the needs: LGTM Needs one or more additional approvals label Dec 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants