Skip to content

Commit

Permalink
fix: auto-removal remote text tracks being removed when not supposed …
Browse files Browse the repository at this point in the history
…to (videojs#4450)

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:
```js
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.

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


Fixes videojs#4403 and videojs#4315.
  • Loading branch information
gkatsev authored Jun 28, 2017
1 parent f5cc165 commit 82c8b80
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/js/tech/tech.js
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ class Tech extends Component {

if (manualCleanup !== true) {
// create the TextTrackList if it doesn't exist
this.autoRemoteTextTracks_.addTrack(htmlTrackElement.track);
this.ready(() => this.autoRemoteTextTracks_.addTrack(htmlTrackElement.track));
}

return htmlTrackElement;
Expand Down
5 changes: 5 additions & 0 deletions test/unit/tech/tech.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -214,18 +214,22 @@ QUnit.test('switching sources should clear all remote tracks that are added with

const tech = new MyTech();

tech.triggerReady();

// set the initial source
tech.setSource({src: 'foo.mp4', type: 'mp4'});

// default value for manualCleanup is true
tech.addRemoteTextTrack({});
this.clock.tick(1);

assert.equal(warning,
'Calling addRemoteTextTrack without explicitly setting the "manualCleanup" parameter to `true` is deprecated and default to `false` in future version of video.js',
'we log a warning when `addRemoteTextTrack` is called without a manualCleanup argument');

// should be automatically cleaned up when source changes
tech.addRemoteTextTrack({}, false);
this.clock.tick(1);

assert.equal(tech.textTracks().length, 2, 'should have two text tracks at the start');
assert.equal(tech.remoteTextTrackEls().length,
Expand All @@ -238,6 +242,7 @@ QUnit.test('switching sources should clear all remote tracks that are added with

// change source to force cleanup of auto remote text tracks
tech.setSource({src: 'bar.mp4', type: 'mp4'});
this.clock.tick(1);

assert.equal(tech.textTracks().length,
1,
Expand Down
52 changes: 52 additions & 0 deletions test/unit/tracks/text-tracks.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -528,3 +528,55 @@ QUnit.test('removeRemoteTextTrack should be able to take both a track and the re
'the track element was removed correctly');
player.dispose();
});

if (Html5.isSupported()) {
QUnit.test('auto remove tracks should not clean up tracks added while source is being added', function(assert) {
const player = TestHelpers.makePlayer({
techOrder: ['html5'],
html5: {
nativeTextTracks: false
}
});

const track = {
kind: 'kind',
src: 'src',
language: 'language',
label: 'label',
default: 'default'
};

player.src({src: 'example.mp4', type: 'video/mp4'});
player.addRemoteTextTrack(track, false);

this.clock.tick(1);
assert.equal(player.textTracks().length, 1, 'we have one text track');

player.dispose();
});

QUnit.test('auto remove tracks added right before a source change will be cleaned up', function(assert) {
const player = TestHelpers.makePlayer({
techOrder: ['html5'],
html5: {
nativeTextTracks: false
}
});

const track = {
kind: 'kind',
src: 'src',
language: 'language',
label: 'label',
default: 'default'
};

player.addRemoteTextTrack(track, false);
player.src({src: 'example.mp4', type: 'video/mp4'});

this.clock.tick(1);
assert.equal(player.textTracks().length, 0, 'we do not have any tracks left');

player.dispose();
});
}

0 comments on commit 82c8b80

Please sign in to comment.