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: support check persistent-license to remove the mediakeysession #7050

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

JackPu
Copy link
Contributor

@JackPu JackPu commented Feb 26, 2025

This PR will...

It will check if the session type is persistent-license and then make the decision to execute mediakeySession.remove.

Why is this Pull Request needed?

I mentioned the issue here: #6806

We have found the issue on the Samsung and Comcast platforms. It seems we cannot resolve the method mediaKeySession.remove(). Because we cannot close the media session, it will cause the next time session request. It will throw an error like this

TypeError-Failed to execute 'generateRequest' on 'MediaKeySession': Request generation failed. . (99999)

Are there any points in the code the reviewer needs to double check?

Resolves issues:

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

return mediaKeysSession
.remove()
const { drmSystemOptions } = this.config;
const removePromise = isPersistentSessionType(drmSystemOptions)
Copy link
Contributor Author

@JackPu JackPu Feb 26, 2025

Choose a reason for hiding this comment

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

If we don't want to check this, we can also try using a config like disableMediaKeySessionRemove to skip this. Any ideas?

Copy link
Collaborator

@robwalch robwalch Mar 4, 2025

Choose a reason for hiding this comment

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

What would making this configurable solve?

There are many cases where removeSession is called. It makes sense to leave persistent sessions active in the happy path, but it might not in all the places where removeSession is called - like when cleaning up after a fatal error.

Would a callback function that accepts the key session context and removal conditions and returns whether or not to dispose of the key session work?
Something like:

removeMediaKeySessionFunc: (
  keyContext: MediaKeySessionContext,
  reasonForRemoval: 'key-renewal' | 'license-release' | 'destroy' | 'error' | <other message?>,
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this PR resolve #6806, or is additional work needed to unblock removeSession (like wrapping it in a timeout or additional error handling for mediaKeySession.remove)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @JackPu,

Can you respond to these comments?

  1. Should we follow up on these changes by passing in EME "message" Event messageType to removeSession (or Error/Event itself), because in some cases the persistent session should still be removed?
  2. Do we still need to catch or timeout the removeSession promises in CDMCleanupPromise for Long EMEController. destroy block the next time setMediaKeys #6806?

Copy link
Contributor Author

@JackPu JackPu Mar 18, 2025

Choose a reason for hiding this comment

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

Does this PR resolve #6806,

I think the PR can resolve one case of #6806. But wrapping a timeout is a more general solution to prevent some other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we follow up on these changes by passing in EME "message" Event messageType to removeSession (or Error/Event itself), because in some cases the persistent session should still be removed?

I think we may need this if we haven't any better solution.

Do we still need to catch or timeout the removeSession promises in CDMCleanupPromise
I think adding this is a good feature to catch the other cases. As you mentioned you can add additional error handling for this. In fact, I only met the issue on a small OTT platform, I am not sure If it is worth doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can merge this firstly until we find some other rereuirements for improve the condition of removeSession

@JackPu
Copy link
Contributor Author

JackPu commented Feb 26, 2025

I will fix the unit tests later

@JackPu JackPu requested a review from robwalch March 4, 2025 08:13
robwalch
robwalch previously approved these changes Mar 5, 2025
@robwalch robwalch requested a review from cjpillsbury March 5, 2025 23:42
@robwalch robwalch added this to the 1.6.0 milestone Mar 5, 2025
@robwalch robwalch dismissed their stale review March 14, 2025 22:24

Waiting for a response to the comments above.

@robwalch robwalch removed this from the 1.6.0 milestone Mar 14, 2025
@JackPu JackPu requested a review from robwalch March 19, 2025 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants