feat(VttLoader): a better way of loading vtt.js #4258

gkatsev · 2017-04-05T22:17:43Z

This is the first commit in that feature. Still needs to be exposed
properly on video.js. Also, vtt.js needs to be updated so that the
WebVTT object is properly available on the vttjs object in the CDN
version of the file.
Afterwards, parseCues in TextTrack should be updated to use the new way
of getting vttjs.

This also means that if vttjs is being loaded via script, it won't load until a player is initialized and it is using a tech that requires emulated text tracks. So, if a player loads and only requests native text tracks, it won't ever try to load vttjs.
It also means that a user can disable loading of vttjs if using the novtt script by requesting that the component be unavailable.

const player = videojs('vid', {
  vttLoader: false
});

It is now exported on videojs as an object with two properties, which are functions: get and loaded.

if (videojs.vttjs.loaded()) {
  const vttjs = videojs.vttjs.get();
  /* use vttjs */
}

Todos

expose vttjs on videojs object as videojs.vttjs
add videojs hooks for vttjsloaded
this is so because vttjs isn't really a per-player thing. The only events aren't going away but should be considered deprecated.
if loaded vttjs via script, check to see if it has loaded previously.
Tests!!!
port to 5.x branch

@imbcmdth @gesinger @mjneil this is the start of exporting vttjs properly.

This is the first commit in that feature. Still needs to be exposed properly on video.js. Also, vtt.js needs to be updated so that the WebVTT object is properly available on the vttjs object in the CDN version of the file. Afterwards, parseCues in TextTrack should be updated to use the new way of getting vttjs.

gkatsev · 2017-04-06T22:04:15Z

This requires vttjs updated with this PR: videojs/vtt.js#12

misteroneill · 2017-04-07T14:56:53Z

src/js/video.js

+
+      vttjsLoadedHooks.forEach((hookFn) => hookFn(vttjs));
+      // we notified the listeners, now clear them out
+      vttjsLoadedHooks.length = 0;


So this prevents the hooks from being called multiple times? Because it seems like otherwise you'd call each hook vttjsLoadedHooks.length times.

Yup, it's to make sue that each of the vttjsloaded hooks is only ever called the one time.

brandonocasey · 2017-04-07T15:02:59Z

src/js/tracks/text-track.js

+
+      track.tech_.on('vttjsloaded', loadHandler);
+      track.tech_.on('vttjserror', () => {
+        log.error(`vttjs failed to load, stopping trying to process ${track.src}`);


I think this error is a bit hard to understand, maybe "vttjs failed to load. Unable to process ${track.src}."

you could even just use src here too, I think.

Probably a good change, though, it's been around for a while now :P

brandonocasey · 2017-04-07T15:05:03Z

src/js/tracks/vtt.js

+ * @fires VttLoader#vttjserror
+ */
+class VttLoader extends Component {
+  constructor(player, options, ready) {


should this be a component or just an event target?

Having it be a component gives us a lot of things for free. For example, the ability to turn off the inclusion of vttjs when using the novtt build.
There's also precedent with the MediaLoader.

brandonocasey · 2017-04-07T16:02:21Z

src/js/tracks/vtt.js

+ */
+class VttLoader extends Component {
+  constructor(player, options, ready) {
+    const options_ = mergeOptions({createEl: false}, options);


I think this is named settings in other places, this probably makes more sense, but for consistency we may want to do that.

Makes sense to change. I just copied this from MediaLoader.

brandonocasey · 2017-04-07T16:03:07Z

src/js/tracks/vtt.js

+     * @event VttLoader#vttjsloaded
+     * @type {EventTarget~Event}
+     */
+    this.trigger('vttjsloaded');


Do we want to trigger this event here? seems linke we only listen for this on tech

Yeah, we probably don't need it. I was thinking that maybe users can get the instance and listen to it but with the hooks feature it's redundant.

brandonocasey · 2017-04-07T16:07:58Z

src/js/tracks/vtt.js

+     * @event VttLoader#vttjserror
+     * @type {EventTarget~Event}
+     */
+    this.trigger('vttjserror');


Do we want to trigger this event here? seems linke we only listen for this on tech

brandonocasey · 2017-04-07T16:15:28Z

src/js/video.js

@@ -147,6 +148,15 @@ videojs.hooks_ = {};
 */
 videojs.hooks = function(type, fn) {
  videojs.hooks_[type] = videojs.hooks_[type] || [];
+  if (fn && type === 'vttjsloaded') {
+    vttjsOnLoad((vttjs) => {


The onload function seems a bit weird in the vttjsloader module. Maybe we should just move all the logic for that here? I don't think that it would be used anywhere else.

it can't be outside of vttjsloader because only that module knows if and when vttjs has loaded. Keeping it there, I think, it good because it contains all (except this little bit, but this mostly deals with hooks) the things that work with loading vttjs.

brandonocasey · 2017-04-24T21:32:34Z

So fas this LGTM in my tests, we will have to release videojs-vtt.js and update the package.json here to get this to work.

brandonocasey · 2017-04-25T20:36:04Z

So everything except for ie8 work, I don't know if we are doing work to support ie8 anymore anyway. This LGTM if thats the case. IE8 may have broken from another change, I can't even get videos to play.

gkatsev · 2017-04-25T20:39:39Z

@brandonocasey @gesinger noticed that grunt connect isn't serving the swf up correctly anymore. If you can change the local sandbox to point to the cdn (or delete the override setting) it potentially should work, assuming that's the issue.

gkatsev · 2018-03-29T20:53:52Z

This isn't going to go in any time soon, closing.

gkatsev added 12 commits April 5, 2017 18:09

fix linting

0501b8e

expose a way to know whether vttjs is loaded and to get vttjs

5532676

don't load vttjs multiple times

89470d7

set the boolean first

0cb9a94

add a vttjsloaded hooks

249e880

disable the vttjsLoaded if not set explicitly in tests

ce610a1

Update TextTrack.js to use the new vttjs import method

a139fd3

fix tracks are parsed if vttjs is loaded test

37f0745

fix tracks are parsed once vttjs is loaded test

6f86b33

fix stops processing if vttjs loading errored out test

737d84a

use vttjs from git for now

fd34569

fix lint

65022a8

misteroneill reviewed Apr 7, 2017

View reviewed changes

brandonocasey requested changes Apr 7, 2017

View reviewed changes

gkatsev added 2 commits April 7, 2017 14:37

CR updates

ea942fa

fix test

104dda8

brandonocasey approved these changes Apr 25, 2017

View reviewed changes

gkatsev added the on-hold label May 25, 2017

gkatsev closed this Mar 29, 2018

gkatsev deleted the videojs-vttjs branch March 29, 2018 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(VttLoader): a better way of loading vtt.js #4258

feat(VttLoader): a better way of loading vtt.js #4258

gkatsev commented Apr 5, 2017 •

edited

Loading

gkatsev commented Apr 6, 2017

misteroneill Apr 7, 2017

gkatsev Apr 7, 2017

brandonocasey Apr 7, 2017

brandonocasey Apr 7, 2017

gkatsev Apr 7, 2017

brandonocasey Apr 7, 2017

gkatsev Apr 7, 2017

brandonocasey Apr 7, 2017

gkatsev Apr 7, 2017

brandonocasey Apr 7, 2017

gkatsev Apr 7, 2017

brandonocasey Apr 7, 2017

brandonocasey Apr 7, 2017

gkatsev Apr 7, 2017

brandonocasey commented Apr 24, 2017

brandonocasey commented Apr 25, 2017

gkatsev commented Apr 25, 2017

gkatsev commented Mar 29, 2018

feat(VttLoader): a better way of loading vtt.js #4258

feat(VttLoader): a better way of loading vtt.js #4258

Conversation

gkatsev commented Apr 5, 2017 • edited Loading

Todos

gkatsev commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonocasey commented Apr 24, 2017

brandonocasey commented Apr 25, 2017

gkatsev commented Apr 25, 2017

gkatsev commented Mar 29, 2018

gkatsev commented Apr 5, 2017 •

edited

Loading