-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 support for com.apple.fps keySystem #4474
feat: add support for com.apple.fps keySystem #4474
Conversation
Hi @valotvince, Would you be able to resolve the conflicts in the PR? I know it's been a while. I'll be putting together a working branch for DRM work soon, and think these changes (minus the working files (dist/*)) provide a really nice and baseline for a future release with multi-DRM support. |
Hi @robwalch |
3b5667f
to
3595921
Compare
@@ -702,7 +832,8 @@ class EMEController implements ComponentAPI { | |||
(videoCodec: string | undefined): videoCodec is string => !!videoCodec | |||
); | |||
|
|||
this._attemptKeySystemAccess(KeySystems.WIDEVINE, audioCodecs, videoCodecs); | |||
// TBD We should try a keySystem based on manifest information | |||
this._attemptKeySystemAccess(KeySystems.FAIRPLAY, audioCodecs, videoCodecs); |
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.
For that part, what would be the best way you want it handled ?
Try all the keySystems and take the first one that works ?
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.
We could add a config option called keySystemPreference
to specify which key-system is preferred (see below).
If not set there, we could map each key in _drmSystems
(+_widevineLicenseUrl
) to a key-system. If there is only one, we can assume that's the one we need to access. If more than one, then we could check them all, at this point there should only be one that works and that would be the one we want.
I think it would make more sense to not set this.mediaKeysPromise
until onMediaEncrypted
. At that point the playlist and media are loaded, and from that we could reduce how many calls to requestMediaKeySystemAccess
we need to make, especially if the playlist keys tell us what decrypt method and keysystems can be used.
Let me know your thoughts. I'm making some assumptions looking at the code, but have a limited set of sample streams and license servers to work with at the moment.
### `keySystemPreference`
(default: `void 0`)
The value used to select the right key system. The playlist may signal multiple key systems, selection of the key system is done based on the preference specified here.
Possible values,
- `fairplaystreaming`: com.apple.streamingkeydelivery
- `playready`: com.microsoft.playready
- `widevine`: urn:uuid:edef8ba9-79d6-4ace-a3c8-27dcd51d21ed
- `clearkey`: org.w3.clearkey
- `undefined`: Select the key system from the first compatible EXT-X-KEY tag
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.
Note that for backwards compatibility I cannot merge this change as-is, as it breaks WIDEVINE support. If you can address this, perhaps checking access of both types but using the first that passes then I can take it from there.
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.
Hi @valotvince,
The test branch I posted solves key-system access by selecting the key-system based on key formats found in the playlist or the systems present for the configured license URLs. Access is no longer attempted on ATTACH_MEDIA. It happens when the first key(s) for a segment are loaded. This is when available key formats are known which is mapped to key-systems to request access to. This happens later in the process of loading an HLS stream, so there could be a desire to request access earlier even if it means querying systems the stream does not have keys for. Session keys will be the first option if present in the manifest (#4927). The queries could be done on ATTACH_MEDIA based on licenses in the config, but I prefer there to avoid requesting access to key-systems for HLS streams that are not encrypted, even when emeEnabled
is true
and license urls are defined.
@@ -367,8 +367,10 @@ export default class M3U8Parser { | |||
const decryptkeyformat = | |||
keyAttrs.enumeratedString('KEYFORMAT') ?? 'identity'; | |||
|
|||
// TBD we might need to check if we try to MSE playback mp2t content |
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.
Do we need to throw an handled error if someone tries to read MP2T Fairplay-encrypted content ? Or do we only rely on documentation ?
If there's a need for an error to be thrown, I might need some guidance to see where I should do it :)
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.
No need. We can rely on documented supported features in the README.
I want us to avoid any kind of file type detection as that has tended to be problematic.
👋 @robwalch The branch is rebased, still two things left to do but I'll wait for your input before going further |
@@ -367,8 +367,10 @@ export default class M3U8Parser { | |||
const decryptkeyformat = | |||
keyAttrs.enumeratedString('KEYFORMAT') ?? 'identity'; | |||
|
|||
// TBD we might need to check if we try to MSE playback mp2t content |
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.
No need. We can rely on documented supported features in the README.
I want us to avoid any kind of file type detection as that has tended to be problematic.
if (keySystem === KeySystems.WIDEVINE && this._widevineLicenseUrl) { | ||
return this._widevineLicenseUrl; |
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.
_widevineLicenseUrl
can be replaced with _config.widevineLicenseUrl
.
(remove private _widevineLicenseUrl?: string
)
@@ -702,7 +832,8 @@ class EMEController implements ComponentAPI { | |||
(videoCodec: string | undefined): videoCodec is string => !!videoCodec | |||
); | |||
|
|||
this._attemptKeySystemAccess(KeySystems.WIDEVINE, audioCodecs, videoCodecs); | |||
// TBD We should try a keySystem based on manifest information | |||
this._attemptKeySystemAccess(KeySystems.FAIRPLAY, audioCodecs, videoCodecs); |
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.
We could add a config option called keySystemPreference
to specify which key-system is preferred (see below).
If not set there, we could map each key in _drmSystems
(+_widevineLicenseUrl
) to a key-system. If there is only one, we can assume that's the one we need to access. If more than one, then we could check them all, at this point there should only be one that works and that would be the one we want.
I think it would make more sense to not set this.mediaKeysPromise
until onMediaEncrypted
. At that point the playlist and media are loaded, and from that we could reduce how many calls to requestMediaKeySystemAccess
we need to make, especially if the playlist keys tell us what decrypt method and keysystems can be used.
Let me know your thoughts. I'm making some assumptions looking at the code, but have a limited set of sample streams and license servers to work with at the moment.
### `keySystemPreference`
(default: `void 0`)
The value used to select the right key system. The playlist may signal multiple key systems, selection of the key system is done based on the preference specified here.
Possible values,
- `fairplaystreaming`: com.apple.streamingkeydelivery
- `playready`: com.microsoft.playready
- `widevine`: urn:uuid:edef8ba9-79d6-4ace-a3c8-27dcd51d21ed
- `clearkey`: org.w3.clearkey
- `undefined`: Select the key system from the first compatible EXT-X-KEY tag
@@ -702,7 +832,8 @@ class EMEController implements ComponentAPI { | |||
(videoCodec: string | undefined): videoCodec is string => !!videoCodec | |||
); | |||
|
|||
this._attemptKeySystemAccess(KeySystems.WIDEVINE, audioCodecs, videoCodecs); | |||
// TBD We should try a keySystem based on manifest information | |||
this._attemptKeySystemAccess(KeySystems.FAIRPLAY, audioCodecs, videoCodecs); |
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.
Note that for backwards compatibility I cannot merge this change as-is, as it breaks WIDEVINE support. If you can address this, perhaps checking access of both types but using the first that passes then I can take it from there.
This PR will...
Add support for
com.apple.streamingkeydelivery
KEYFORMAT with MSE (some copy/paste from #2630)It brings several changes that would need be separated in other PRs (but to have a MVP, I chose to make it inside one PR)
keySystem
to licenseXhrSetup & licenseResponseCallback to allow developers to do different treatment to xhr and data depending on the used keySystemwidevineLicenseUrl
session.update
rejectsTo make it work with my own DRMProvider, I had to write that configuration on my app, hope this helps developers when integrating fps keySystem (maybe add it somewhere it the configuration ?)
com.apple.fps
keySystem correctly works with MSE when using fMP4 (or CMAF). It won't work properly when using mp2t with MSE, directly or when using mux.js (it might be different with the hls.js transmuxer, but I didn't took the opportunity to test it). See shaka-project/shaka-player#3776 for more information.Still to be done and I might need tips to go faster 😄 :
Why is this Pull Request needed?
Because it allows to quit Safari's streaming black-box and use MSE/EME to play content
Are there any points in the code the reviewer needs to double check?
yarn link
doesn't work properly with hls.jsResolves issues:
Checklist