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

feat: add modern EME support for FairPlay #3776

Merged
merged 30 commits into from Feb 7, 2022

Conversation

valotvince
Copy link
Contributor

@valotvince valotvince commented Nov 29, 2021

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:

  • Mac 11.6 Safari 15.2
  • iOS 15.2 Safari 15.2
  • Mac 11.6 Chrome 96 (for potential regressions on Widevine keySystem)
Mode DRM API  TS  CMAF (mono-key and multi-keys)
file EME
file Legacy-prefixed
media-source EME mux-js: encrypted never fired
real MSE: encrypted event received, but with incorrect sinf initData (*1)
media-source Legacy-prefixed mux-js: webkitneedkey never fired
real MSE: TBD
🔴 fails to append media segment to SourceBuffer (init segment ok) (video:4) – "failed fetch and append: code=3015"

Support table

Mode DRM API  TS  CMAF (mono-key and multi-keys)
file EME
file Legacy-prefixed
media-source EME 🚫 4040: HLS_MSE_ENCRYPTED_MP2T_NOT_SUPPORTED
media-source Legacy-prefixed 🚫 4041: HLS_MSE_ENCRYPTED_LEGACY_APPLE_MEDIA_KEYS_NOT_SUPPORTED 🚫 4041: HLS_MSE_ENCRYPTED_LEGACY_APPLE_MEDIA_KEYS_NOT_SUPPORTED

⚠️ Use EME APIs with multi-keys CMAF makes the video stalling with the audio continuing alone after a short time (~3 minutes in the stream, could be shorter, could be longer). Didn't find an explanation to that yet. I've observed the same behaviour with hls.js code so I don't think this is a player issue.

*1

ewogICJjb250IiA6ICJtcHRzIiwKICAiY29kYyIgOiAyMDUzMjA3NjUxLAogICJtdHlwIiA6IDE5ODY2MTg0NjkKfQ==

{
  "cont" : "mpts",
  "codc" : 2053207651,
  "mtyp" : 1986618469
}

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update

Checklist:

  • I have signed the Google CLA https://cla.developers.google.com
  • My code follows the style guidelines of this project
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have verified my change on multiple browsers on different platforms
  • I have run ./build/all.py and the build passes
  • I have run ./build/test.py and all tests pass


shaka.polyfill.register(shaka.polyfill.PatchedMediaKeysApple.install);
// Make it configurable ?
// shaka.polyfill.register(shaka.polyfill.PatchedMediaKeysApple.install);
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 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 ?

Copy link
Collaborator

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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'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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@valotvince valotvince changed the title feat: add support for HLS EME com.apple.fps keySystem feat: add HLS com.apple.streamingkeydelivery through MSE/EME Nov 29, 2021

// Explicit init data an offline session is
// sufficient to suppress 'encrypted' events for all streams.
if (!this.offlineSessionIds_.length) {
Copy link
Contributor Author

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 valotvince changed the title feat: add HLS com.apple.streamingkeydelivery through MSE/EME feat: add HLS com.apple.streamingkeydelivery support through MSE/EME Nov 30, 2021
@valotvince valotvince changed the title feat: add HLS com.apple.streamingkeydelivery support through MSE/EME feat: add modern EME support for HLS FairPlay Dec 1, 2021
@avelad
Copy link
Collaborator

avelad commented Dec 2, 2021

@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

@valotvince
Copy link
Contributor Author

@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 ?)

@avelad
Copy link
Collaborator

avelad commented Dec 2, 2021

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.

@joeyparrish
Copy link
Member

We made that choice due to a browser bug that was never resolved by Apple.

@avelad
Copy link
Collaborator

avelad commented Dec 3, 2021

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.

@valotvince
Copy link
Contributor Author

👋 @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

@valotvince
Copy link
Contributor Author

I've tried with mux.js and the webkitneedkey / encrypted events are never fired.

I've looked a little bit inside mux.js and I didn't find any references to schi > tenc boxes which should hold the keyId for mp4 encryption, therefore making the browser trigger encryption events. That could explain why it works well with clear content, but not with encrypted content. But I could have missed something, let me know if you check on your side !

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 useNativeHlsOnSafari is still configured to true per Shaka's default, so integrators could de-activate it if they're sure to play fMP4 content.

@valotvince valotvince marked this pull request as ready for review December 21, 2021 15:29
@valotvince
Copy link
Contributor Author

👋 @joeyparrish @avelad This PR is ready to be reviewed (again).

I wish you a happy Christmas and happy holidays 😄🎄

@avelad
Copy link
Collaborator

avelad commented Jan 12, 2022

@theodab can you review it? or launch the github action?

@shaka-bot
Copy link
Collaborator

Test Failure:

Chrome 97.0.4692.71 (Linux x86_64)
  DrmEngine ClearKey plays encrypted content with the ClearKey CDM FAILED
    Failed: Shaka Error DRM.FAILED_TO_GENERATE_LICENSE_REQUEST (Failed to execute 'generateRequest' on 'MediaKeySession': No supported PSSH box found.,NotSupportedError: Failed to execute 'generateRequest' on 'MediaKeySession': No supported PSSH box found.,)

    Failed: Shaka Error DRM.FAILED_TO_GENERATE_LICENSE_REQUEST (Failed to execute 'generateRequest' on 'MediaKeySession': No supported PSSH box found.,NotSupportedError: Failed to execute 'generateRequest' on 'MediaKeySession': No supported PSSH box found.,)

Failed 1 tests, passed 2168, skipped 39

docs/tutorials/fairplay.md Outdated Show resolved Hide resolved
lib/media/drm_engine.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
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
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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

lib/media/drm_engine.js Show resolved Hide resolved
lib/util/platform.js Outdated Show resolved Hide resolved
@valotvince
Copy link
Contributor Author

@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 ?

@joeyparrish
Copy link
Member

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:

Safari version Browser usage share
15.x 1.78%
14.x 1.51%
13.x 0.57%

externs/shaka/player.js Outdated Show resolved Hide resolved
lib/media/drm_engine.js Outdated Show resolved Hide resolved
@@ -140,6 +126,26 @@ shaka.polyfill.PatchedMediaKeysApple = class {
return Promise.resolve();
}

/**
*
* @param {!MediaKeyEvent} sourceEvent
Copy link
Member

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?

Copy link
Contributor Author

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);
Copy link
Member

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) {
Copy link
Member

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()) {
Copy link
Member

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.

Copy link
Contributor Author

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

@joeyparrish
Copy link
Member

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!

@valotvince
Copy link
Contributor Author

@joeyparrish I've added a window.shakaMediaKeysPolyfill property to detect wether polyfill have been installed or not. I don't really like the window thing, but changing MediaKeys static properties didn't feel right either. Let me know what you prefer !

@valotvince
Copy link
Contributor Author

valotvince commented Jan 25, 2022

@joeyparrish

Maybe we should add a run on of the tests on Safari < 14 if it's possible ?

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.

@valotvince valotvince changed the title feat: add modern EME support for HLS FairPlay feat: add modern EME support for FairPlay Jan 26, 2022
@valotvince
Copy link
Contributor Author

👋 @joeyparrish @theodab Ready to be merged (rebased today) :)

@avelad
Copy link
Collaborator

avelad commented Feb 3, 2022

@joeyparrish can you review it? Thanks!

@joeyparrish
Copy link
Member

Will do. I'm out of time for today, but I'll try to look at it tomorrow if I can.

@joeyparrish
Copy link
Member

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.

README.md Outdated Show resolved Hide resolved
docs/tutorials/fairplay.md Show resolved Hide resolved
lib/polyfill/patchedmediakeys_apple.js Show resolved Hide resolved
lib/hls/hls_parser.js Show resolved Hide resolved
lib/media/drm_engine.js Outdated Show resolved Hide resolved
@joeyparrish
Copy link
Member

The Safari test process hung for some reason and had to be cancelled, but the logs show all tests ran and passed first.

@joeyparrish joeyparrish dismissed theodab’s stale review February 7, 2022 19:15

Re-reviewed in full by me

@joeyparrish joeyparrish merged commit 6d76a13 into shaka-project:master Feb 7, 2022
@valotvince valotvince deleted the feat-apple-fps branch February 8, 2022 07:45
joeyparrish pushed a commit that referenced this pull request Apr 28, 2022
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
@avelad avelad added this to the v4.0 milestone May 4, 2022
joeyparrish pushed a commit that referenced this pull request May 17, 2022
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
joeyparrish pushed a commit that referenced this pull request May 17, 2022
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
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MSE support for FairPlay Prefer unprefixed EME for Safari
5 participants