-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 modern EME support for FairPlay #3776
Conversation
|
||
shaka.polyfill.register(shaka.polyfill.PatchedMediaKeysApple.install); | ||
// Make it configurable ? | ||
// shaka.polyfill.register(shaka.polyfill.PatchedMediaKeysApple.install); |
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 need the native implementation to work for MSE/EME to work properly with Safari. But we should keep native workflow for people who are not ready to switch to MSE.
Any ideas on how making this polyfill configurable on a configuration basis ?
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.
Apple gives guidance on how to use the native implementation with the modern api, wouldn't it make sense to make it work with the modern API?
Look at the zip from #3346
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.
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've tried to load a TS content manifest and a CMAF content manifest directly as file
(without MSE) but encrypted
events aren't fired by the browser, therefore no capability to negociate a licence, the video element still send the webkitneedkey
event. I've tried to replicate as much as possible the Apple examples, but no success at all 🤷♂️ Maybe I am missing something
I've also tried the test contents provided by Apple, without success yet.
Edit: I have been able to have the encrypted event in a very simple env, I'll keep working on 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.
More info from video-js: videojs/videojs-contrib-eme@5897655 and https://github.com/videojs/videojs-contrib-eme/commits/main
lib/media/drm_engine.js
Outdated
|
||
// Explicit init data an offline session is | ||
// sufficient to suppress 'encrypted' events for all streams. | ||
if (!this.offlineSessionIds_.length) { |
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 need to have the encrypted event to get the proper initData, since we can't build from manifest data (at least for HLS implementation)
@valotvince I have seen that you have put NA in TS in media-source, why with TS it will not work? When I was looking at it I remember that with TS the encrypted event was not fired if mux.js was used, maybe it's worth looking: https://github.com/google/shaka-player/blob/master/lib/polyfill/ mediasource.js # L41-L44 |
@avelad You are right, I tried to load TS content without mux.js, I wasn't aware of the Shaka polyfill which rejects TS content in MediaSource. I'll try it out (but if it doesn't work out of the box, I don't know if it's worth to make it work ?) |
maybe it's worth asking the videojs team (@gkatsev) if they work with native TS in Safari with MSE, so we know if it is a limitation of the ShakaPlayer or the browser itself. |
We made that choice due to a browser bug that was never resolved by Apple. |
What I see is that I have tons of legacy content with TS, and through the playlist I am not able to differentiate if it has TS or MP4. So supporting MSE without TS does not help me. Also it would be strange that clear with TS is supported (through mux.js), but not with Fairplay. |
👋 @avelad Sure thing, we still have tons of content in TS, but we are able to know if they are TS or not before starting Shaka, and choose to force native player or not :) I'll test mux.js anyway, could be a non-issue afterall |
I've tried with mux.js and the I've looked a little bit inside mux.js and I didn't find any references to I don't know the insides of TS that much, and if it holds the key inside each segments. If not, mux.js would need to take it from the playlist KEY URI SKD, which could lead to errors since skd doesn't necessarily means keyId. The |
0ca3385
to
8f7f2e6
Compare
👋 @joeyparrish @avelad This PR is ready to be reviewed (again). I wish you a happy Christmas and happy holidays 😄🎄 |
@theodab can you review it? or launch the github action? |
Test Failure:
|
README.md
Outdated
@@ -171,7 +171,9 @@ NOTES: | |||
|HLS |**Y** |**Y** |**Y** ¹ | - | | |||
|
|||
NOTES: | |||
- ¹: We support FairPlay through Apple's native HLS player. | |||
- ¹: We support FairPlay through MSE/EME when available and when | |||
`useNativeHlsOnSafari` is turned off. Otherwise, we are using |
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.
Although, on second thought, does it even make sense for useNativeHlsOnSafari
to be set to true
by default, with this PR? If we change it to be false
by default, we won't even need to have a footnote 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.
Given the limitations of MSE with FPS (eg TS is not supported), I think it is necessary to keep it to true.
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.
And there's still the issue with the multi-keys CMAF HLS, for which I will add a note in the documentation
bb6bd0f
to
5c68876
Compare
@theodab I took your comments into account and committed accordingly. The GHA CI is a nice change 👍 I would say now the only downside is we are always going to run the tests with the latest Safari, meaning we're using the modern EME API in our tests, but never the legacy webkit-prefixed one. Maybe we should add a run on of the tests on Safari < 14 if it's possible ? |
According to caniuse.com usage data, Safari 14 is still about as popular as Safari 15 right now, so it's probably a good idea to test on Safari 14 if possible. We'll look into it. (#3899) Since people will some day read this and find that the data at the link above has changed, today's data shows:
|
@@ -140,6 +126,26 @@ shaka.polyfill.PatchedMediaKeysApple = class { | |||
return Promise.resolve(); | |||
} | |||
|
|||
/** | |||
* | |||
* @param {!MediaKeyEvent} sourceEvent |
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.
This parameter appears to be unused. Could it be removed? Is there a good reason to keep 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.
It is used at l.146 :)
|
||
try { | ||
const base64 = shaka.util.StringUtils.fromCharCode(uint8); | ||
const data = JSON.parse(base64); |
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.
You call it base64
, but then treat it as a JSON string. Perhaps that's the wrong name for it?
const base64 = shaka.util.StringUtils.fromCharCode(uint8); | ||
const data = JSON.parse(base64); | ||
|
||
if (data) { |
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.
Are the contents of data
not needed for the dispatched event at all?
@@ -32,24 +32,20 @@ shaka.polyfill.PatchedMediaKeysWebkit = class { | |||
* @export | |||
*/ | |||
static install() { | |||
if (!shaka.util.Platform.willMediaKeysWebkitBePolyfilled()) { |
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 don't love moving the details of the detection out of the polyfill. If there's a need to detect whether or not a polyfill was installed, perhaps we can compare the implementation to native instead. This appears to work:
const polyfilled =
navigator.requestMediaKeySystemAccess !=
Navigator.prototype.requestMediaKeySystemAccess;
If you need to identify a specific polyfill, you could perhaps have a tag or something on the implementation without needing to depend on the polyfill from some part of the core code.
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.
Well it doesn't really work because there is also the EMEEncryptionScheme polyfill that overrides the requestMediaKeySystemAccess. I'll add a tag on the polyfill implementation so we can detect shaka own implementation
The only issue I'm really concerned about is moving polyfill details from the polyfills themselves to shaka.util.Platform. I'd like to explore alternatives to that before we commit to it. Otherwise, it looks great. Thanks for working on this! |
@joeyparrish I've added a |
If we test Safari 14, we won't test the legacy MediaKeys implementation (Safari < 13 & some iOS based browsers versions), so I think we should have Safari 13 (for legacy MediaKeys), Safari 14 & latest for EME modern API. |
8407cd7
to
159ea32
Compare
👋 @joeyparrish @theodab Ready to be merged (rebased today) :) |
@joeyparrish can you review it? Thanks! |
Will do. I'm out of time for today, but I'll try to look at it tomorrow if I can. |
BTW, I'm hopeful that this, containerless/raw/packed format support for HLS, and HLS latency improvements will all land soon, and then we can push v3.4 out with all of those features. |
The Safari test process hung for some reason and had to be cancelled, but the logs show all tests ran and passed first. |
Fixes an issue where non-encrypted content were unable to playback due to encryption detection done with `this.video.mediaKeys`. It seems we could set MediaKeys even though the content is not encrypted, therefore trying & failing to add information in the init segment and resulting into a `CONTENT_TRANSFORMATION_FAILED`. I am still unsure why this is happening exactly but I think it could be a side-effect of #3776 . I'll keep looking for the real cause of the issue, but I think this is a reasonable fix to rely on stream info I'll test different versions of Tizens we have because I think this workaround isn't needed on all of the versions
Fixes an issue where non-encrypted content were unable to playback due to encryption detection done with `this.video.mediaKeys`. It seems we could set MediaKeys even though the content is not encrypted, therefore trying & failing to add information in the init segment and resulting into a `CONTENT_TRANSFORMATION_FAILED`. I am still unsure why this is happening exactly but I think it could be a side-effect of #3776 . I'll keep looking for the real cause of the issue, but I think this is a reasonable fix to rely on stream info I'll test different versions of Tizens we have because I think this workaround isn't needed on all of the versions
Fixes an issue where non-encrypted content were unable to playback due to encryption detection done with `this.video.mediaKeys`. It seems we could set MediaKeys even though the content is not encrypted, therefore trying & failing to add information in the init segment and resulting into a `CONTENT_TRANSFORMATION_FAILED`. I am still unsure why this is happening exactly but I think it could be a side-effect of #3776 . I'll keep looking for the real cause of the issue, but I think this is a reasonable fix to rely on stream info I'll test different versions of Tizens we have because I think this workaround isn't needed on all of the versions
Description
Close #3346
Close #2999
Add support for HLS
com.apple.streamingkeydelivery
through MSE/EME implementation.Add support for FairPlay encryption into DASH manifests through MSE/EME implementation.
Tests
Tested on:
encrypted
never firedreal MSE:
encrypted
event received, but with incorrectsinf
initData (*1)webkitneedkey
never firedreal MSE: TBD
(video:4) – "failed fetch and append: code=3015"
Support table
4040: HLS_MSE_ENCRYPTED_MP2T_NOT_SUPPORTED
4041: HLS_MSE_ENCRYPTED_LEGACY_APPLE_MEDIA_KEYS_NOT_SUPPORTED
4041: HLS_MSE_ENCRYPTED_LEGACY_APPLE_MEDIA_KEYS_NOT_SUPPORTED
*1
Type of change
not work as expected)
Checklist:
./build/all.py
and the build passes./build/test.py
and all tests pass