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.

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

fix(safari): video not starting because key are never considered usable for a track #1479

Merged
merged 7 commits into from
Aug 5, 2024

Conversation

Florent-Bouisset
Copy link
Collaborator

We observed an issue with encrypted HLS playlist on Safari with the legacy EME API:
If the HLS playlist has multiple audio with the same encryption key, and the default lang of the playlist is NOT the lang of the OS, the video will stall.

This appears to be a race condition within Safari, that occurs when the licence is received at about the same time the audio tracks are added.

Safari will continuously fire events "webkitneedkey" for the init-data we just processed, the RxPlayer will not be re-creating a session for an already handled init-data.
Forcing to recreate a session even if the init-data is already handled fixed the issue.

video.addEventListener("webkitneedkey", onEncrypted);

function onEncrypted(event) {
   const video = event.target;

   // check if initData is already processed
   if(alreadyProcessed(initData)) {
      // don't re-create a session, there is already an active session
      // with this init-data
      return 
   }

   // create a session
   ...
   const session = video.webkitKeys.createSession("video/mp4", initData)

   // perform the licence request
   ...
}

@Florent-Bouisset
Copy link
Collaborator Author

Maybe related to issue #1326

@peaBerberian peaBerberian added bug This is an RxPlayer issue (unexpected result when comparing to the API) DRM Relative to DRM (EncryptedMediaExtensions) DirectFile Relative to "directfile", i.e. direct content playback without MediaSourceExtensions involved Compatibility Relative to the RxPlayer's compatibility with various devices and environments Priority: 1 (High) This issue or PR has a high priority. labels Jul 11, 2024
@peaBerberian peaBerberian added this to the 4.2.0 milestone Jul 11, 2024
@peaBerberian peaBerberian added Priority: 0 (Very high) This issue or PR has a very high priority. Efforts should be concentrated on it first. and removed Priority: 1 (High) This issue or PR has a high priority. labels Jul 26, 2024
@peaBerberian
Copy link
Collaborator

To propose an alternative solution for this one: Maybe we could add something to the payload when receiving the event, like a forceSessionCreation boolean attribute for example where a MediaKeySession would be re-created?

Also maybe when doing this we should close the previous MediaKeySession or at least close it. I'm afraid that we could be mixing things up if we had two alive MediaKeySession linked to the same key id + init data.

this.error = null;

eme.onEncrypted(
mediaElement,
(evt) => {
log.debug("DRM: Encrypted event received from media element.");
const initData = getInitData(evt as MediaEncryptedEvent);
const initData = getInitData(evt as ICustomMediaEncryptedEvent);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want to better type onEncrypted instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that still necessary if we update onEncrypted's type in IEmeApiImplementation like this?:

  onEncrypted: (
    target: IEventTargetLike,
    listener: (evt: ICustomMediaEncryptedEvent) => void,
    cancelSignal: CancellationSignal,
  ) => void;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes because createCompatibleEventListener returns a listener of type (event?: Event) => void
and will not satisfies type (evt: ICustomMediaEncryptedEvent) => void

Copy link
Collaborator

Choose a reason for hiding this comment

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

Event should be compatible to an ICustomMediaEncryptedEvent though as forceSessionRecreation is optional no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yet maybe Event is not compatible with a MediaEncryptedEvent, in which case we might improve on the type?
Through createCompatibleEventListener event overloading?

@peaBerberian
Copy link
Collaborator

Also, does it work?

compatibleEventListener(
target,
(event?: Event) => {
const patchedEvent = object_assign(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Weird that eslint is not telling us about this weird function naming

@peaBerberian peaBerberian force-pushed the fix/safari-stuck-webkitneedkey branch from 9fd3962 to d563ea2 Compare August 5, 2024 09:38
@peaBerberian peaBerberian merged commit eea2f84 into dev Aug 5, 2024
6 checks passed
peaBerberian added a commit that referenced this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This is an RxPlayer issue (unexpected result when comparing to the API) Compatibility Relative to the RxPlayer's compatibility with various devices and environments DirectFile Relative to "directfile", i.e. direct content playback without MediaSourceExtensions involved DRM Relative to DRM (EncryptedMediaExtensions) Priority: 0 (Very high) This issue or PR has a very high priority. Efforts should be concentrated on it first.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants