Skip to content

Commit

Permalink
fix: cache middleware instances per player (#4939)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
gkatsev committed Feb 12, 2018
1 parent 6189baa commit 29a8ee1
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 1 deletion.
2 changes: 2 additions & 0 deletions src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,8 @@ class Player extends Component {
this.tag = null;
}

middleware.clearCacheForPlayer(this);

// the actual .el_ is removed here
super.dispose();
}
Expand Down
40 changes: 39 additions & 1 deletion src/js/tech/middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};

Expand Down Expand Up @@ -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;

Expand All @@ -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) {

Expand Down
100 changes: 100 additions & 0 deletions test/unit/tech/middleware.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
});

0 comments on commit 29a8ee1

Please sign in to comment.