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: silently try com.microsoft.playready.recommendation before com.microsoft.playready #2750
feat: silently try com.microsoft.playready.recommendation before com.microsoft.playready #2750
Conversation
Just came across to check what is the status with #2662 and saw this. Please note that dash.js discussion came to conclusion that there is no good way to discover if recommendation is supported or not as it is async. BTW - @valotvince did you verify persistent license works on Edge? is offline possible with Shaka on Edge with this? |
Hi @OrenMe I came across Dash-Industry-Forum/dash.js#2714 while trying to hardware decrypt content with Playready and added that check into the drm_engine without testing it (we don't have the use case here for now on) It's unclear to me which config the drm_engine internally chooses after I hardcode the keySystem (will it take com.microsoft.playready or the recommendation one I am testing it against my own DRM provider, querying them with that keySytem results into an internal server error... I am still investigating on that part so I didn't make it work all the way to playback :) |
|
@valotvince there's this section https://github.com/google/shaka-player/blob/master/docs/tutorials/drm-config.md |
5275dff
to
b71cbb0
Compare
7f23e5b
to
c1c0c43
Compare
Hi @joeyparrish Any chances of getting that PR reviewed ? Thank you ! :) |
Sorry for the delay in review, and thank you for contributing! |
7ef028e
to
7901731
Compare
7901731
to
ad4ea7b
Compare
@joeyparrish Any thoughts about my comment ? |
fe24f09
to
d139221
Compare
@valotvince I have a question, is there a way to ban this behavior on platforms that do not support this? For example, in SmartTV it does not exist and always doing this check adds an extra time that I would like to save myself. |
@avelad Sure, if you can provide a listing like |
@valotvince I think it should be the other way around, this change should only apply if the browser is Edge |
@avelad Why not, but this would exclude platforms adding the support for that keySystem (without adding some code). As long as this is known, no worries :) |
@@ -173,6 +173,11 @@ it fails, silently fallback to `com.microsoft.playready`. | |||
|
|||
NB: Audio Hardware DRM is not supported (PlayReady limitation) | |||
|
|||
NB: If you are using **purchase persistent licenses**, you will need to set the |
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.
Thanks for putting this note 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.
Just one minor nit. Looks great! I'll start a test run.
In addition to the tests being run by the build bot, since this has so many platform-specific changes, I am also running a full test pass in our test lab across all devices and operating systems. |
All tests passed! |
I am seeing a bunch of failures in my test run, on multiple platforms. I'll need a little time to dig into the specifics and understand what is wrong. Thanks for your patience! |
@joeyparrish Thank you ! I'd love to know more about the failures, so I might be of some help to understand them :) |
It turns out that there are already some test failures on the master branch, which appear to be related to the MediaCapabilities work destined for v3.2. So it's possible that there's actually nothing wrong with this PR. To be sure, I'm going to try cherry-picking this PR into the v3.1.x branch to see if there are any test failures with the PR in that context. If not, I'll go ahead and merge it to master, then cherry-pick it to v3.1.x. Thanks! |
@valotvince We've found the root cause and will have it fixed very soon! It's my bad, not relevant with your PR :D |
@joeyparrish @michellezhuogg Thanks for the heads up :) |
All tests passed! |
With Michelle's recent fix, I'm still getting failures:
I'm going to double-check by cherry-picking your change onto v3.0.x, since there are no MediaCapabilities changes there. I suspect the Firefox failures are unrelated to your work. But the Chromecast failure looks likely to be related, since it is the test case you added which is failing there. |
A few more errors that look like they could be related:
|
Okay, the Chromecast failure, and the offline test failures are reproducible even when I cherry-pick your work onto v3.0.x. (The Firefox failures went away, and are probably related to MediaCapabilities in master.) I'm going to recommend that you split this work up into multiple PRs. For example, a smaller PR that just adds the Maybe next you could create a PR for the "tears down & removes active persistent sessions" test & feature. And finally the preference for com.microsoft.playready.recommendation. How does that sound? |
@joeyparrish About the offline tests, you are playing the same tests in the repo, but on a Windows 10 Edge Chromium, right ? If so, I am able to reproduce those cases. About the Chromecast, I think this is what I did here #2750 (comment) that causes the issue, I'll revert that change and rename the function |
Okay, sounds good. Yes, we're running tests on Chromium-based Edge. I can certainly run the tests again when you've made revisions. Thanks! |
After further investigations for the Chromecast part, I think this is more related to the way I did the test, rather than the code itself (fix in 70a5652). Edit: turns out i am going to ignore the test when not running on Edge, new commit incoming I am still investigating on the offline tests |
@joeyparrish Offline tests should be fixed in fe65b22. I've added a check for storage initialisation, to prevent removing sessions specifically created for storage and used for playback afterwards. |
Do we have to follow a specific setup to make the tests work on Windows 10 / Edge ? The script doesn't seem to be able to start MicrosoftEdge (to check the last
Edit: i've installed a Chromium Edge launcher on my local setup, I am able to launch the test and see the failure |
Yes, it appears that karma-edge-launcher is outdated and only works for legacy Edge. Perhaps we should switch to this instead: https://github.com/ChiragRupani/karma-chromiumedge-launcher |
@joeyparrish To fix the last test Edge, I had to heavily tweak offline / drm and I think it will be a pain to maintain in the future |
Following #818 and #1495, firstly and silently try to request a media key access with the
com.microsoft.playready.recommendation
keySytem, and fallback to the legacy onecom.microsoft.playready
if it fails.See PlayReady playback tests #1495 (comment)
Also added a new field in the DRM AdvancedConfig
sessionType
allowing to force a specific MediaKey session to be created with that sessionType. Particularly useful when we issue purchase licenses, and the whole decryption chain checks for the configuration to be nice & pretty (unlikecom.microsoft.playready
).We need this whenever content right-holders request hardware decryption for HD content.
Closes #1495