-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: Add CDM detection module #98
Conversation
src/plugin.js
Outdated
// Pass a promise polyfill from the player options for IE support. If none | ||
// exists, native Promises will be used and the function won't be supported in IE | ||
detectSupportedCDMs: createDetectSupportedCDMsFunc(player.options().Promise), | ||
getSupportedCDMs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why have a synchronous function for an asynchronous process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could just get rid of it. Curious whether others think it makes sense to remove getSupportedCDMs()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think getting rid of getSupportedCDMs
is the right call here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I think removing the sync function is the right call. There also might be some warnings in MDNs docs regarding when to call requestMediaKeySystemAccess
that we may want to bubble up to our documentation for this module. https://developer.mozilla.org/en-US/docs/Web/API/Navigator/requestMediaKeySystemAccess
src/plugin.js
Outdated
// Pass a promise polyfill from the player options for IE support. If none | ||
// exists, native Promises will be used and the function won't be supported in IE | ||
detectSupportedCDMs: createDetectSupportedCDMsFunc(player.options().Promise), | ||
getSupportedCDMs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think getting rid of getSupportedCDMs
is the right call here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR adds two utility methods:
player.eme.getSupportedCDMs
andplayer.eme.detectSupportedCDMs
.The latter uses
window.navigator.requestMediaKeySystemAccess()
to asynchronously detect which CDMs are available in the browser. When that EME API method is unavailable, such as in older Safari versions, other feature detection is used.The former is synchronous and returns either 1) the last result of
detectSupportedCDMs()
, or if that hasn't been called, 2) a best guess based solely on the user agent and presence of EME API features.