Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(middleware): cache grows even if no middleware created #8674

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(middleware): cache grows even if no middleware created #8674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf3a61a
chore(middleware): add sandbox to show issue with growing cache
GiuseppePiscopo Apr 8, 2024
d189f9b
fix(middleware): when player is removed and no middleware was set, pr…
GiuseppePiscopo Apr 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
71 changes: 71 additions & 0 deletions sandbox/middleware-instances.html.example
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />
<title>Video.js Sandbox</title>
<link href="../dist/video-js.css" rel="stylesheet" type="text/css">
<script src="../dist/video.js"></script>
<link rel="icon" href="data:;base64,=">
</head>
<body>
<div style="background-color:#eee; border: 1px solid #777; padding: 10px; margin-bottom: 20px; font-size: .8em; line-height: 1.5em; font-family: Verdana, sans-serif;">
<p>You can use /sandbox/ for writing and testing your own code. Nothing in /sandbox/ will get checked into the repo, except files that end in .example (so don't edit or add those files). To get started run `npm start` and open the index.html</p>
<pre>npm start</pre>
<pre>open http://localhost:9999/sandbox/index.html</pre>
</div>

<div style="background-color:#eee; border: 1px solid #777; padding: 10px; margin-bottom: 20px; font-size: 1em; line-height: 1.5em; font-family: Verdana, sans-serif;">
<p>
In developer console, Sources tab, look for <code>clearCacheForPlayer</code> function.
Place a logpoint at function closing. Logpoint content should be:
</p>
<pre style="font-size: 1.2em;">'middlewareInstances nr', Object.keys(middlewareInstances).length</pre>
<p>
When one or more players are removed, the number of instances should *NOT* grow.
</p>
</div>

<div>
<button id="add-player" style="min-height: 36px;">Add player</button>
<button id="remove-all" style="min-height: 36px;">Remove all players</button>
</div>

<div id="player-container"></div>

<script>
let playerList = [];

document.querySelector('#add-player').addEventListener('click', addPlayer);
document.querySelector('#remove-all').addEventListener('click', removeAll);

function addPlayer() {
const videoJsElem = document.createElement('video-js');
videoJsElem.setAttribute('controls', '');
videoJsElem.setAttribute('preload', 'auto');
videoJsElem.setAttribute('width', '640');
videoJsElem.setAttribute('height', '264');
videoJsElem.setAttribute('poster', 'https://vjs.zencdn.net/v/oceans.png');

document.querySelector('#player-container').appendChild(videoJsElem);

const player = videojs(videoJsElem);
player.src({
src: 'https://vjs.zencdn.net/v/oceans.mp4',
type: 'video/mp4',
});

playerList.push(player);

player.log('Video.js player created', player.id());
}

function removeAll() {
for (const player of playerList) {
player.dispose();
}
playerList.length = 0;
}
</script>

</body>
</html>
4 changes: 3 additions & 1 deletion src/js/tech/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,9 @@ function executeRight(mws, method, value, terminated) {
* A {@link Player} instance.
*/
export function clearCacheForPlayer(player) {
middlewareInstances[player.id()] = null;
if (middlewareInstances.hasOwnProperty(player.id())) {
middlewareInstances[player.id()] = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if delete is preferable, if the object having null properties was the issue being addressed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the issue being addressed is that middlewareInstances grows with new keys having null values. Not sure why the original code prefers setting to null instead of deleting.

Just as a speculation: in player.js there's the global enumeration of players, Player.players, and in that case it is explicitly mentioned that a disposed player should be left as a key with null value. Maybe that same behaviour has been applied in middlewareInstances, even if there is no actual use of it.

}
}

/**
Expand Down