Update vttjs #4115
Update vttjs #4115
Conversation
|
Yup, I'll make another PR against |
|
You're welcome @gkatsev ! |
video.js will try to load `vtt.js` internally and override `window.VTTCue`. This will break text tracks in dashjs in Chrome beacuse the native `TextTrack.addCue()`` function expects a native `VTTCue()` as it's first argument. If we use the `vtt.js` shimmed version of `VTTCue`, then all calls to `addCue()` will throw an error. This will silently kill playback with using fragmented text captions. If we add a truthy value to for `vtt.js` in `options`, then `video.js` will try to load this url and use that instead of it's internal shim mechanism. Nothing will load, and `video.js` won't try to apply it's own version of `vtt.js` to `window`. This will prevent tracks from breaking in Chrome. We'll provide a space beacuse this won't throw an error or try to actually load anything and it will prevent video.js from breaking dashjs. This can be removed when videojs/video.js#4115 is applied to [email protected].
src/js/tracks/text-track.js
Outdated
| let cue = _cue; | ||
|
|
||
| if (!(_cue instanceof window.vttjs.VTTCue)) { | ||
| cue = new window.vttjs.VTTCue(_cue.startTime, _cue.endTime, _cue.text); |
There was a problem hiding this comment.
would it be possible/better to expose this on videojs rather than window?
There was a problem hiding this comment.
it's more or less exposed globally right now plus we've expected it to be exposed. For the 5.x version, we definitely should keep it exposed on window because it'll be easier to make it backwards compat.
For this PR, we could look into making a version of vttjs that doesn't bind itself to window. Though, I think it should be done as a separate PR rather than part of this work.
There was a problem hiding this comment.
My feeling here is that vttjs is kind of its own thing - we just use it and it should stand alone more or less.
There was a problem hiding this comment.
@misteroneill sort of. Given that we're using a custom fork, we can do what we want with it at this point.
The only thing I can imagine that would be tricky to get right to have vttjs not export itself globally is getting the novtt build working correctly.
There was a problem hiding this comment.
Im open to doing it in another PR
src/js/tracks/text-track.js
Outdated
| @@ -336,7 +336,17 @@ class TextTrack extends Track { | |||
| * @param {TextTrack~Cue} cue | |||
| * The cue to add to our internal list | |||
| */ | |||
| addCue(cue) { | |||
| addCue(_cue) { | |||
| let cue = _cue; | |||
There was a problem hiding this comment.
can we change this to cueCopy and change _cue back to just cue?
There was a problem hiding this comment.
We could. I just thought it would be better to leave the rest of the code as is and keep the change relatively self-contained.
There was a problem hiding this comment.
I think that it's more a self documenting code change that would be worth doing, as _cue doesn't really have any significance to tell you what it actually is.
There was a problem hiding this comment.
What about naming it originalCue?
Fixes #4093