From 29a8ee1d60160d291ab626815043f6f7d339e1bb Mon Sep 17 00:00:00 2001 From: Gary Katsevman Date: Mon, 12 Feb 2018 17:30:27 -0500 Subject: [PATCH] fix: cache middleware instances per player (#4939) Middleware factories currently get run each time a source is set. Because middleware are assocated with a player, the factories should only run once per player. This PR fixes it so that we associate a middleware instance with a middleware factory per player. Each time a player is disposed, we will clear the cache of the middleware instances for that player. Fixes #4677. --- src/js/player.js | 2 + src/js/tech/middleware.js | 40 +++++++++++- test/unit/tech/middleware.test.js | 100 ++++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+), 1 deletion(-) diff --git a/src/js/player.js b/src/js/player.js index 2003e4de7..c258abe3f 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -523,6 +523,8 @@ class Player extends Component { this.tag = null; } + middleware.clearCacheForPlayer(this); + // the actual .el_ is removed here super.dispose(); } diff --git a/src/js/tech/middleware.js b/src/js/tech/middleware.js index 1ced23d3d..c7b29a409 100644 --- a/src/js/tech/middleware.js +++ b/src/js/tech/middleware.js @@ -2,6 +2,7 @@ import { assign } from '../utils/obj.js'; import toTitleCase from '../utils/to-title-case.js'; const middlewares = {}; +const middlewareInstances = {}; export const TERMINATOR = {}; @@ -102,6 +103,43 @@ function executeRight(mws, method, value, terminated) { } } +export function clearCacheForPlayer(player) { + middlewareInstances[player.id()] = null; +} + +/** + * { + * [playerId]: [[mwFactory, mwInstance], ...] + * } + */ +function getOrCreateFactory(player, mwFactory) { + const mws = middlewareInstances[player.id()]; + let mw = null; + + if (mws === undefined || mws === null) { + mw = mwFactory(player); + middlewareInstances[player.id()] = [[mwFactory, mw]]; + return mw; + } + + for (let i = 0; i < mws.length; i++) { + const [mwf, mwi] = mws[i]; + + if (mwf !== mwFactory) { + continue; + } + + mw = mwi; + } + + if (mw === null) { + mw = mwFactory(player); + mws.push([mwFactory, mw]); + } + + return mw; +} + function setSourceHelper(src = {}, middleware = [], next, player, acc = [], lastRun = false) { const [mwFactory, ...mwrest] = middleware; @@ -112,7 +150,7 @@ function setSourceHelper(src = {}, middleware = [], next, player, acc = [], last // if we have an mwFactory, call it with the player to get the mw, // then call the mw's setSource method } else if (mwFactory) { - const mw = mwFactory(player); + const mw = getOrCreateFactory(player, mwFactory); mw.setSource(assign({}, src), function(err, _src) { diff --git a/test/unit/tech/middleware.test.js b/test/unit/tech/middleware.test.js index ab69cee5b..c1d9643c7 100644 --- a/test/unit/tech/middleware.test.js +++ b/test/unit/tech/middleware.test.js @@ -250,6 +250,9 @@ QUnit.test('setSource is run asynchronously', function(assert) { let acc; middleware.setSource({ + id() { + return 'vid1'; + }, setTimeout: window.setTimeout }, { src: 'foo', type: 'video/foo' }, function(_src, _acc) { src = _src; @@ -281,6 +284,9 @@ QUnit.test('setSource selects a source based on the middleware given', function( middleware.use('video/foo', fooFactory); middleware.setSource({ + id() { + return 'vid1'; + }, setTimeout: window.setTimeout }, {src: 'foo', type: 'video/foo'}, function(_src, _acc) { src = _src; @@ -323,6 +329,9 @@ QUnit.test('setSource can select multiple middleware from multiple types', funct middleware.use('video/bar', barFactory); middleware.setSource({ + id() { + return 'vid1'; + }, setTimeout: window.setTimeout }, {src: 'foo', type: 'video/foo'}, function(_src, _acc) { src = _src; @@ -377,6 +386,9 @@ QUnit.test('setSource will select all middleware of a given type, until src chan middleware.use('video/foo', fooFactory3); middleware.setSource({ + id() { + return 'vid1'; + }, setTimeout: window.setTimeout }, {src: 'foo', type: 'video/foo'}, function(_src, _acc) { src = _src; @@ -409,3 +421,91 @@ QUnit.test('a middleware without a mediator method will not throw an error', fun assert.equal(pauseCalled, 1, 'pauseCalled was called once and no error was thrown'); }); + +QUnit.test('a middleware factory is not called on source change', function(assert) { + let mwfactoryCalled = 0; + const mw = { + setSource(_src, next) { + next(null, { + src: 'http://example.com/video.mp4', + type: 'video/mp4' + }); + } + }; + const fooFactory = () => { + mwfactoryCalled++; + return mw; + }; + + middleware.use('video/foo', fooFactory); + + // set "initial" source" + middleware.setSource({ + id() { + return 'vid1'; + }, + setTimeout: window.setTimeout + }, {src: 'foo', type: 'video/foo'}, function() {}); + + this.clock.tick(1); + + assert.equal(mwfactoryCalled, 1, 'the factory was called once'); + + // "change" source + middleware.setSource({ + id() { + return 'vid1'; + }, + setTimeout: window.setTimeout + }, {src: 'bar', type: 'video/foo'}, function() {}); + + this.clock.tick(1); + + assert.equal(mwfactoryCalled, 1, 'the factory was not called again'); + + middleware.getMiddleware('video/foo').pop(); +}); + +QUnit.test('a middleware factory is called on a new source with a new player', function(assert) { + let mwfactoryCalled = 0; + const mw = { + setSource(_src, next) { + next(null, { + src: 'http://example.com/video.mp4', + type: 'video/mp4' + }); + } + }; + const fooFactory = () => { + mwfactoryCalled++; + return mw; + }; + + middleware.use('video/foo', fooFactory); + + // set "initial" source with player vid1 + middleware.setSource({ + id() { + return 'vid1'; + }, + setTimeout: window.setTimeout + }, {src: 'foo', type: 'video/foo'}, function() {}); + + this.clock.tick(1); + + assert.equal(mwfactoryCalled, 1, 'the factory was called once'); + + // set "initial" source with player vid2 + middleware.setSource({ + id() { + return 'vid2'; + }, + setTimeout: window.setTimeout + }, {src: 'bar', type: 'video/foo'}, function() {}); + + this.clock.tick(1); + + assert.equal(mwfactoryCalled, 2, 'the factory was called again'); + + middleware.getMiddleware('video/foo').pop(); +});