fix(demo): dispose the player after closing the modal

[jboix]

Description

Closes #113

Calling player.reset() before loading a new source was preventing the webkitneedkey event from being listened by videojs-contrib-eme resulting in the inability to play DRM protected content.

Changes made

This commit creates a new player everytime the modal is open in the demo and disposes the player when closing the modal. Additionally it adds a KNOWN_ISSUES document describing the problem with player.reset() and how to mitigate it during the integration of Pillarbox for tracking purposes.

[github-actions[bot]]

Coverage report

St.:grey_question:	Category	Percentage	Covered / Total
🟢	Statements	98.37%	482/490
🟢	Branches	93.44%	228/244
🟢	Functions	100%	136/136
🟢	Lines	99.15%	466/470

Test suite run success

146 tests passing in 9 suites.

Report generated by 🧪jest coverage report action from 99f96e9a3abc6b0a197ae964c5c504c4550e21a4

[amtins]

@jboix I think this problem could be solved if #113 is made instead. It also brings back the problem of the poster when transitioning from one content to another, or when playing content that doesn't have a poster.

[jboix]

@jboix I think this problem could be solved if #113 is made instead. It also brings back the problem of the poster when transitioning from one content to another, or when playing content that doesn't have a poster.

My bad ! I completely forgot about the poster bug. I reverted the fix in the demo from this PR and I will implement #113 here. The issues with calling player.reset() still exists, I think it is good to have it documented though.

Potential fixes that I see in pillarbox are :

1. Reproduce the fix that was implemented in the letterbox middleware, for reference:

 if (videojs.browser.IS_ANY_SAFARI && Drm.hasDrm(resourcesList)) {
  player.eme = videojs.getPlugin('eme');
  player.eme();
}

2. Submit a change request to videojs-contrib-eme that listens to the playerreset and reattaches the webkitneedskey event to the tech (videojs-contrib-eme/src/plugin.js#L234).

[jboix]

Updated the description of the pull request to reflect the new changes that are implemented as suggests in #113.

[amtins]

@jboix I don't really like the Letterbox approach, and I'm pretty sure I wrote it. 🥲

To address all the issue we can follow video.js recommendations, please refer to Showing and Hiding a Player

With this in mind, we can imagine doing something like the following:

demo/src/player/player.js

import Pillarbox from '../../../src/pillarbox.js';
import '../../../src/middleware/srgssr.js';

// ... I removed the rest for the sake of simplicity

export const disposePlayer = () => {
  Pillarbox.getPlayer('player').dispose();
};

export const createPlayer = () => {
  window.player = new Pillarbox('player', {
    fill: true,
    html5: {
      vhs: { useForcedSubtitles: true },
    },
    liveTracker: {
      trackingThreshold: 120,
      liveTolerance: 15,
    },
    liveui: true,
    muted: true,
    playsinline: true,
    plugins: { eme: true },
    responsive: true,
    restoreEl: true
  });

  return window.player;
};

Then:

demo/src/player/player-dialog.js

import { createPlayer, disposePlayer } from './player';

// ... I removed the rest for the sake of simplicity

dialog.addEventListener('close', () => {
  disposePlayer();
});

export const openPlayerModal = ({ src, type, keySystems }) => {
  const player = createPlayer();

  player.src({ src, type, keySystems });

  dialog.showModal();
  dialog.classList.toggle('slide-up-fade-in', true);
};

With this, there is no more DRM issues with Safari, and we also address the #113.

Disclaimer, I didn't test the code above 😅

[jboix]

@amtins That's exactly what I did. I changed this PR right before your comment:

Updated the description of the pull request to reflect the new changes that are implemented as suggests in #113.

Indeed your code didn't work during my testing since player.dispose() got rid of the HTML element, that's why I resorted to create the element using videojs DOM utilities right before initialising it.

I agree that the letterbox fix is not clean, my suggestion is to keep this issue documented in KNOWN_ISSUES until we can find a cleaner solution.

I've updated the description and the title of this PR to reflect the new changes. Sorry for the confusion.

[amtins]

@jboix sorry, I should have add a comment about the property restoreEl set to true in my example. Adding this property to the options allows to avoid recreating manually a videoElement each time. See restoreel

[jboix]

@jboix sorry, I should have add a comment about the property restoreEl set to true in my example. Adding this property to the options allows to avoid recreating manually a videoElement each time. See restoreel

That works! I've changed the code. Thanks for the feedback 👍