Start using remote TextTracks because they can be properly removed #118
Conversation
imbcmdth
force-pushed
the
use-remote-text-tracks
branch
from
63a8576
to
66536af
Compare
| @@ -36,6 +37,12 @@ export default class FlashMediaSource extends videojs.EventTarget { | |||
| } | |||
| }); | |||
|
|
|||
| if (this.tech_.hls) { | |||
| this.tech_.hls.on('dispose', () => { | |||
There was a problem hiding this comment.
I know that it looks like video.js' tech doesn't trigger dispose, but maybe we should consider adding it later, so we don't have to check for HLS specifically (or listening to a different type of event).
There was a problem hiding this comment.
Techs do have a dispose but they are only dispose when switching techs (html->flash) but sourceHandlers have a dispose too and they trigger it whenever the source changes (their dispose gets triggered via HLSHandler's superclass in Component#dispose).
So while I agree that this line is kind of cludgy, it would be necessary since video.js will properly clean up remote text tracks whenever the source changes. This is just insurance that they get cleaned up even if using an older version of video.js.
| }); | ||
| sourceBuffer.segmentParser_.trigger('done'); | ||
|
|
||
| QUnit.equal(addedTracks.length, 2, 'created two remote TextTracks'); |
There was a problem hiding this comment.
We should check which ones were created.
There was a problem hiding this comment.
We have tests that catch this scenario already and make sure the proper tracks are created for captions and metadata individually but it is a good idea to check that here for completeness.
|
|
||
| QUnit.equal(addedTracks.length, 2, 'created two remote TextTracks'); | ||
| this.player.tech_.hls.trigger('dispose'); | ||
| QUnit.equal(removedTracks.length, 2, 'removed both TextTracks'); |
There was a problem hiding this comment.
Same for checking which ones removed.
There was a problem hiding this comment.
We have tests that catch this scenario already and make sure the proper tracks are created for captions and metadata individually but it is a good idea to check that here for completeness.
| sourceBuffer.transmuxer_.onmessage(createDataMessage('video', new Uint8Array(1), { | ||
| metadata | ||
| })); | ||
| sourceBuffer.transmuxer_.onmessage(createDataMessage('video', new Uint8Array(1), { |
There was a problem hiding this comment.
Should we check in-between these two messages?
There was a problem hiding this comment.
Nah, in fact I should just collapse the two messages into one. No reason to keep both. Thanks for pointing that out.
| @@ -11,18 +12,27 @@ | |||
| * @private | |||
| */ | |||
| const createTextTracksIfNecessary = function(sourceBuffer, mediaSource, segment) { | |||
There was a problem hiding this comment.
May be worth adding tests for this.
There was a problem hiding this comment.
This is tested already in several places but what isn't tested right now is the removeExistingTrack behavior which is new. Good point.
| QUnit.equal(addedTracks.length, 2, 'created two text tracks'); | ||
| QUnit.equal(addedTracks.filter(t => ['captions', 'metadata'].indexOf(t.kind) === -1).length, | ||
| 0, | ||
| 'created only the expected two remote TextTracks'); |
There was a problem hiding this comment.
Maybe it's overboard, since the remove should be fine, but it might be worth checking to verify that the newly created tracks are the new ones.
We were using
videojs.addTextTrackwhich createsTextTrackobjects in memory and associates them with a video element. These are not removable from the video element once added. This PR switches tovideojs.addRemoteTextTrackand then carefully removes them whenever the MediaSource is closed (in the Html5 case) or the HLS sourceHandler is disposed of (in the Flash case).