feat: middleware #3788

gkatsev · 2016-11-16T19:46:26Z

Description

This is the initial PR for middleware which hopes to solve the problems from #3734.
Middleware are objects with two special functions setSource and setTech. On top of these special methods, it can have getters and setters. Currently, the only allowed ones are currentTime, duration, and setCurrentTime. In the future it can be extended to allow more. Also, methods like play could probably be treated as setters and events originating from the video element can be treated as getters and events triggered on the player could be treated as setters. But that's more in the future.

The core of middleware is that video types are basically a routing tree. So, you can "use" a middleware on a particular type but you can also "use" a tech on a particular type using special syntax since techs need a bit extra handling.

// example HLS usage
videojs.use('application/x-mpegurl', require('contrib-hls'));
videojs.use('application/vnd.apple.mpegurl', require('contrib-hls'));

// example tech usage
videojs.use('video/mp4', 'videojs/html5');
videojs.use('application/x-mpegurl', 'videojs/html5');

The contrib-hls object could look something like this:

module.exports = {
  setSource(srcObj, next) {
    // next expects an error as a first argument to let the middleware know whether you can handle the current srcObj
    // you can also modify this srcObj and return something else instead which will continue down the middleware tree
    next(false, srcObj);
  },

  setTech(tech) {
    // this gets the tech that ends up getting chosen to back everything. Store it if you need it.
    // the tech still acts as the final source of truth for everything
  },

  currentTime(previousCt) {
    // getters get the value from further up the chain and should return a transformed value, if necessary.
    // In a simple case, the middleware would get the value from the tech and would expect to return a value for the player to use
    return previousCt / 2;
  },

  setCurrentTime(newCt) {
    // setters get the value from further up the chain and should return a transformed value, if necessary.
   // In a simple case, the middleware would get the value from the player and would expect to return a value for the tech to use
  return newCt;
  }
};

I have a simple live example here: http://plnkr.co/edit/Ap9Fvg21lpYW3Rnlk9ty?p=preview

boushley · 2016-11-16T19:53:39Z

I haven't dug into the code yet, but from the description it seems odd that for setSource and setCurrentTime follow different patterns.

What is the purpose of the error argument passed to next in setSource?

gkatsev · 2016-11-16T19:48:02Z

package.json

@@ -20,9 +20,15 @@
    "build": "grunt dist",
    "change": "grunt chg-add",
    "clean": "grunt clean",
+    "grunt": "grunt",


Most of this will get removed before it's ready. Makes working with this locally a bit easier.
It'll probably get pulled into a separate PR.

gkatsev · 2016-11-16T19:48:26Z

src/js/player.js

+  }
+
+  middlewareGet(method) {
+    return this.middleware_.reduceRight(this.middlewareIterator(method), undefined);


Finally, I can use reduce and reduceRight!

gkatsev · 2016-11-16T19:49:06Z

src/js/video.js

- */
-if (typeof define === 'function' && define.amd) {
-  define('videojs', [], () => videojs);
+module.exports = videojs;


This will get pulled into a separate PR.

gkatsev · 2016-11-16T19:59:40Z

src/js/video.js

-  module.exports = videojs;
-}
+videojs.use = middlewareUse;
+videojs.use('video/mp4', 'videojs/html5');


How to register the techs for native functionality is still something I'm a bit unsure about.

Definitely something we need to think about. Because as it stands you can never add a middleware that handles setSource for 'video/mp4' since there is a tech registered as the first item always.

boushley

This is a great start. There are a couple things we need to work through though, like techs abruptly ending the middleware chain as talked about in slack.

Also, we might want to see if we can unify this syntax in some way so that setSource is more like a generic setter. Although to do that you would have to make the setCurrentTime setter async instead of sync which would likely have some bigger impacts.

boushley · 2016-11-16T19:59:36Z

src/js/tech/middleware.js

+    });
+
+  // something weird happened, try the next middleware on the current level
+  } else if (middleware.length > 1) {


Would probably be good to at least log a warning that middleware for this src was undefined.

boushley · 2016-11-16T20:06:06Z

src/js/tech/middleware.js

+
+      // we've succeeded, now we need to go deeper
+      acc.push(mw);
+      ssh(_src, middlewares[_src.type] || [], next, acc);


We need some way in this case to prevent us from calling the same middleware over and over. If the middleware leaves the src.type unchanged and doesn't send along an error, we will keep calling the same middleware over and over.

boushley · 2016-11-16T20:09:15Z

src/js/tech/middleware.js

+
+      // something happened, try the next middleware on the current level
+      if (err) {
+        return ssh(src, middleware.slice(1), next, acc);


Instead of doing a return here which implies that we care about the return value, can we just put an else on the if statement and add the following two lines to it.

This will also make the code more consistent with other recursive calls in this function which don't return.

boushley · 2016-11-16T20:12:08Z

src/js/tech/middleware.js

+
+    // we've reached a fork, so, we need go deeper
+    } else {
+      ssh(src, middlewares[mw] || [], next, acc);


So this makes it possible to do something like videojs.use('video/foo', 'my-standard-middleware-stack') and when that "middleware" is hit it would then start processing the my-standard-middleware-stack set of middlewares, right? Seems interesting.

Although assuming that set of middleware eventually changes the type back to something standard we need to determine if we will ever call the same middleware twice.

boushley · 2016-11-16T20:15:24Z

src/js/tech/middleware.js

+const middlewares = {};
+
+export function use(type, middleware) {
+  (middlewares[type] = middlewares[type] || []).push(middleware);


I'm too new to the project to know the general code style guidelines, but I find that lines like this while clever add an unnecessary obstacle to reading the code.

I feel that dividing this up into two lines makes it more idiomatic, and thus easier to read:

middlewares[type] = middlewares[type] || [] middlewares[type].push(middleware)

but my one lines...

If we're picking this nit, I generally prefer concat to || [].

middlewares[type] = [].concat(middlewares[type], middlware)

This is a little bit of nonsense here since we're ok with mutating the original array, so ¯_(ツ)_/¯

I think I agree with @boushley, but @mmcc would be fine too.

I also think that "Type" should be renamed to mimeType. I am also wondering if it would make more sense for middleware to come first in the arguments order. I want to use() hls for x type(s). I also thing that type should be able to be an array.

@mmcc The only problem with concat in that case is that if middlewares[type] is undefined, you'll get an array like [undefined, middleware] which I think is probably undesirable.

I went for the two line method.

boushley · 2016-11-16T20:20:30Z

src/js/player.js

+  }
+
+  middlewareSet(method, arg) {
+    return this.middleware_.reduce(this.middlewareIterator(method), arg);


Hooray for the right tool for the job! (reduce here and reduceRight below!)

boushley · 2016-11-16T20:21:27Z

src/js/player.js

+
+  middlewareGet(method) {
+    return this.middleware_.reduceRight(this.middlewareIterator(method), undefined);
+  }


It feels like these three middleware functions might belong in the middleware.js file instead. Just be that they accept a list of middlewares to process that would be passed in.

The methods should be private. The reason why they aren't in middleware.js and why setSource is special is because setSource traverses through our middleware tree and sets up the specific middleware to be used for the player at this time. Though, I guess it can be moved into middleware.js and then this.middleware_ could be passed in.

Right, I was assuming that this.middleware_ would be passed in.

The player.js file is big enough as it is. Seems like iterating over a list of middlewares should be in the middlewares file instead.

gkatsev · 2016-11-17T02:37:44Z

Should middleware be given a reference to the player?

boushley · 2016-11-17T02:45:17Z

It seems like it could add some new capabilities. In fact to implement the use case I talked about before (unmute sets volume to above 0) you would need to handle the mute setter and be able to set volume on the player.

This could also cause problems if someone does something silly like tries to set currentTime on the player while in a middleware that is handling currentTime.

mmcc

Silly nitpicks aside, I really, really like this concept. A lot. Huge 👍

mmcc · 2016-11-18T18:37:44Z

src/js/player.js

+      this.middleware_ = mws;
+      this.loadTech_(tech.name, src_);
+      middleware.setTech(mws, this.tech_);
+    });


If tech.name is the only thing used here, we could destructure in the args. I don't feel strongly about this one, just something I've started doing a little more of lately.

middleware.setSource(src, ({ name }, src_, mws) => { this.middleware_ = mws; this.loadTech_(name, src_); middleware.setTech(mws, this.tech_); });

mmcc · 2016-11-18T18:46:00Z

src/js/tech/middleware.js

+const middlewares = {};
+
+export function use(type, middleware) {
+  (middlewares[type] = middlewares[type] || []).push(middleware);


If we're picking this nit, I generally prefer concat to || [].

middlewares[type] = [].concat(middlewares[type], middlware)

This is a little bit of nonsense here since we're ok with mutating the original array, so ¯_(ツ)_/¯

mmcc · 2016-11-18T18:47:02Z

src/js/tech/middleware.js

+}
+
+export function setSource(src, next) {
+  setTimeout(()=>ssh(src, middlewares[src.type] || [], next, []), 1);


I thought Standard required spaces here (i.e. () => foo())?

I don't feel crazy strong either way, but on line 16 there are spaces so we're not even being consistent within the same file.

Apparently not but yeah, I agree that there should be more whitespace. Originally added it in to verify that async works.

brandonocasey

This PR could also really use a tech guide update, that would certainly make it more understandable for me.

brandonocasey · 2016-12-09T18:43:14Z

package.json

+    "predev": "babel src/js -d es5",
+    "dev": "run-p babel browserify",
+    "dev-test": "run-p dev test-watch",
+    "babel": "babel -w src/js -d es5",


do we want to keep all of these in here? Most of them seem useful, so I wouldn't mind it.

I'm going to pull these out of this PR when getting ready to merge and make a separate PR out of it.

brandonocasey · 2016-12-09T18:45:43Z

src/js/player.js

@@ -829,6 +834,8 @@ class Player extends Component {
      'techId': `${this.id()}_${techName}_api`,
      'videoTracks': this.videoTracks_,
      'textTracks': this.textTracks_,
+      'remoteTextTracks': this.remoteTextTracks_,
+      'remoteTextTrackEls_': this.remoteTextTrackEls_,


I made a change that is basically the same as this in the Unify TextTrack API PR. Does this change need to be in this PR? (if so we just need to be aware that this will have to be merged first, and the rebase could be a bit messy.)

Unfortunately, this change does need to be in this PR, some of the tests rely on tracks and will fail without it because techs are now async.

brandonocasey · 2016-12-09T18:46:51Z

src/js/player.js

@@ -920,6 +923,8 @@ class Player extends Component {
    // Save the current text tracks so that we can reuse the same text tracks with the next tech
    this.videoTracks_ = this.videoTracks();
    this.textTracks_ = this.textTracks();
+    this.remoteTextTracks_ = this.remoteTextTracks();


Same as the other comment with a new link to the unify texttrack api PR.

brandonocasey · 2016-12-09T18:52:38Z

src/js/player.js

+    return this;
+  }
+
+  src_(source) {


this will need a jsdoc comment

brandonocasey · 2016-12-09T18:53:26Z

src/js/player.js

-    if (this.tech_) {
-      return this.tech_.textTracks();
+    if (!this.tech_) {
+      this.textTracks_ = this.textTracks_ || new TextTrackList();


See Other comments on Unify TextTrack API PR

brandonocasey · 2016-12-09T21:29:46Z

src/js/player.js

+    if (method in middleware.allowedSetters) {
+      return middleware.set(this.middleware_, this.tech_, method, arg);
+    }
+


seems like we should check do this here, to prevent checking this.tech_ in two places below.

if (!this.tech_) { return }

Also it seems like this.tech_.ready() already checks if the tech is ready? Seems like we could merge a lot of this code together to make it much simpler.

You're right, we probably want to put the middleware section into one of the ifs.
It's possible that just doing .ready() works, in which case, I can see if it's worth trying to simplify it for this PR.

brandonocasey · 2016-12-09T21:33:25Z

src/js/player.js

@@ -1562,6 +1572,10 @@ class Player extends Component {
  techGet_(method) {
    if (this.tech_ && this.tech_.isReady_) {

+      if (method in middleware.allowedGetters) {


This function could also use some similar cleanup, maybe we can do both in another PR, but its always bothered me that it seems so messy an nested.

Same here. I'd prefer to do the cleanup in a separate PR but might do it if it helps make the middleware part more clear as well.

we can't actually do this cleanup here because we need to return a value from this function and this.ready() doesn't guarantee a value, even if you're running it synchronously. However, I think I was able to simplify techCall.

brandonocasey · 2016-12-09T21:38:33Z

src/js/tech/middleware.js

+const middlewares = {};
+
+export function use(type, middleware) {
+  (middlewares[type] = middlewares[type] || []).push(middleware);


I also think that "Type" should be renamed to mimeType. I am also wondering if it would make more sense for middleware to come first in the arguments order. I want to use() hls for x type(s). I also thing that type should be able to be an array.

brandonocasey · 2016-12-09T21:39:18Z

src/js/tech/middleware.js

+}
+
+export function setSource(setTimeout, src, next) {
+  setTimeout(() => ssh(src, middlewares[src.type], next), 1);


will we get this far if a src does not have a type? Should we be checking for this somewhere?

should this be async?

brandonocasey · 2016-12-09T21:40:35Z

src/js/tech/middleware.js

+  setTimeout(() => ssh(src, middlewares[src.type], next), 1);
+}
+
+export function setTech(middleware, tech) {


middlewareList since it seems like we want it to be an array here?

Or middlewares.

kind of tricky since middleware is both a singular and a plural noun.

misteroneill

Looking good. Some minor feedback/comments.

misteroneill · 2016-12-21T19:30:33Z

src/js/video.js


-export default videojs;
+videojs.use = middlewareUse;


There a reason this comes after module.exports?

misteroneill · 2016-12-21T19:31:37Z

test/index.html

@@ -9,6 +9,9 @@
 <body>
  <div id="qunit"></div>
  <script src="../node_modules/qunitjs/qunit/qunit.js"></script>
+  <script>
+    QUnit.config.reorder = false;


I don't think so. I think setting reorder to false is really used to mask bad cleanup in tests. I think @gkatsev may have cleaned that up in another PR as well.

misteroneill · 2016-12-21T19:32:17Z

test/unit/player.test.js

@@ -283,6 +281,7 @@ QUnit.test('should asynchronously fire error events during source selection', fu
  });

  this.clock.tick(1);
+  this.clock.tick(1);


Could we add an explanation as to why we need to tick 2ms instead of 1ms?

It's because the error is triggered async, so, we need one for it, but we need an additional one for the player creation itself.

I added a comment

gkatsev · 2016-12-28T21:42:16Z

I've moved the grunt and package.json changes into a separate PR: #3896 and thus I reverted the changes in this PR.

gkatsev · 2016-12-28T21:43:48Z

@mmcc @boushley @misteroneill I've changed the PR from WIP to RFC. I think it's in decent shape now just needs closer inspection.

brandonocasey · 2017-01-17T18:20:48Z

@gkatsev It would be a good idea to run the jsdoc linter over this code

misteroneill · 2017-01-19T19:34:55Z

src/js/tech/middleware.js

+    setSourceHelper(src, mwrest, next, acc);
+  } else {
+    // const techs = [].concat(middlewares['*']);
+    // const tech = techs.filter((tech) => !!tech.tech.canPlayType(src.type))[0].name;


Should these commented lines just be removed?

gkatsev commented Nov 16, 2016

View reviewed changes

boushley reviewed Nov 16, 2016

View reviewed changes

mmcc reviewed Nov 18, 2016

View reviewed changes

gkatsev force-pushed the middleware branch from c40e4a8 to 0b40c4c Compare December 2, 2016 23:40

brandonocasey approved these changes Dec 9, 2016

View reviewed changes

brandonocasey added major needs: addressing labels Dec 15, 2016

misteroneill reviewed Dec 21, 2016

View reviewed changes

gkatsev added 19 commits December 21, 2016 15:35

middleware

b8c3b53

setSource middleware

004fbf8

updates

d77cbe2

move middleware to separate module

5e67631

refactor and comment middleware

2ffe208

accumulate the middleware on setSource

6669ea3

refactor middleware set/get to use reduce/reduceRight

340ffc8

fix middleware iterator

afa43a0

move allowed getters and setters to middleware.js

d2505aa

add a setTech method

7888bf9

some better handling of input to Player#src

eeb87e2

fix linting

64870c5

fix tests

b8083ee

fix linting

b7c9dfe

eliminate an infinite loop around same type

d624137

move get and set methods to middleware.js

2872fea

Techs as * middleware

ffad7e3

run babel pre-dev

ba40c00

Add static canPlayType and canPlaySource methods to techs

250a7f0

gkatsev added 2 commits December 28, 2016 16:30

better dev script

e48a90d

revert build changes

7429e65

gkatsev changed the title ~~[WIP] Middleware~~ [RFC] Middleware Dec 28, 2016

gkatsev added 11 commits January 11, 2017 16:29

Merge branch 'master' into middleware

3b5a62f

add a private getMiddleware method to middleware.js

29a389b

add tests for middleware.js

5e887a3

fix linting

acc65cd

add player level tests for allowed setters/getters

f02d0c8

add setVolume stub to tech faker

7a120fd

switch to techFaker tech

453a19b

add source-middleware-tech selection

1eeb265

switch to a custom tech instead of flash tech

be76373

typo

cf8d924

fake html5 tech

6a214f8

gkatsev added 3 commits January 19, 2017 11:35

Merge branch 'master' into middleware

bdab83b

make sure to unset 'changingSrc'

cfcb529

update play promise test to use a source

73a4cd1

misteroneill reviewed Jan 19, 2017

View reviewed changes

remove unnecessary comments

4fd421f

misteroneill approved these changes Jan 19, 2017

View reviewed changes

gkatsev changed the title ~~[RFC] Middleware~~ feat: middleware Jan 19, 2017

Merge branch 'master' into middleware

8d11a8c

gkatsev removed the needs: addressing label Jan 19, 2017

Merge master

cb35153

gkatsev merged commit 34aab3f into master Jan 19, 2017

gkatsev deleted the middleware branch January 19, 2017 22:29

jthomerson mentioned this pull request Nov 22, 2017

Setting src is not synchronous - causes mobile bug #4765

Open

feat: middleware #3788

feat: middleware #3788

Conversation

gkatsev commented Nov 16, 2016

Description

boushley commented Nov 16, 2016

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

boushley left a 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

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

gkatsev commented Nov 17, 2016

boushley commented Nov 17, 2016

mmcc left a 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 left a 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

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

misteroneill left a 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

gkatsev commented Dec 28, 2016

gkatsev commented Dec 28, 2016

brandonocasey commented Jan 17, 2017

Choose a reason for hiding this comment