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

fix: auto-removal remote text tracks being removed when not supposed to #4434

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Jun 22, 2017

Description

We added a feature so that remote text tracks can auto-removed when a source changes. However, in 6.x we changed the source behavior to be asynchronous meaning that some text tracks were accidentally being removed when they weren't supposed to be.
For example:

var player = videojs('my-player');
player.src({src: 'video.mp4', type: 'video/mp4'});
 // set second arg to false so that they get auto-removed on source change
player.addRemoteTextTrack({kind: 'captions', src: 'text.vtt', srclang: 'en'}, false);

Now when the player loads, this captions track is actually missing because it was removed.

Specific Changes proposed

Instead of adding auto-removal tracks immediately to the list, wait until we've selected a source before adding them in.
This probably needs a test.

Fixes #4403 and #4315.

techOrder: ['html5']
});

console.log(player.isReady_, player.tech_.isReady_);
Copy link
Member

Choose a reason for hiding this comment

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

Linter is complaining about this.

techOrder: ['html5']
});

console.log(player.isReady_, player.tech_.isReady_);
Copy link
Member

Choose a reason for hiding this comment

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

Linter is complaining about this.

@misteroneill misteroneill dismissed their stale review June 23, 2017 15:38

No longer valid.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

LGTM once we figure out why tests are failing on Travis.

@gkatsev
Copy link
Member Author

gkatsev commented Jun 27, 2017

Closing this PR for the moment so I can re-open it via upstream so tests will run against browserstack.

@gkatsev gkatsev closed this Jun 27, 2017
@gkatsev gkatsev deleted the fix-auto-removed-tracks-being-auto-removed branch June 27, 2017 23:59
@gkatsev
Copy link
Member Author

gkatsev commented Jun 28, 2017

New PR: #4450

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants