-
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
fix: Fix key ID byteswapping for PlayReady on PS4 #4377
fix: Fix key ID byteswapping for PlayReady on PS4 #4377
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Thanks for your contribution! It may seem like silly bureaucracy, but you'll need to sign a CLA before I can accept your contribution. Please review the message above from the |
The code looks fine, FWIW, so as soon as the CLA is signed, I will be happy to merge this. Thanks! |
@Vasanthavanan-Devarajan, please visit https://cla.developers.google.com/ so that I can accept your PR. Thank you! |
@joeyparrish I have submitted the CLA form. But we are not yet getting the digital form to sign. Once we received the form, We will sign and proceed further. |
@joeyparrish we have signed the CLA document. Though we are updating the checks as suggested in the CLA guideline document, The validation is still failing. can you help with this further? |
The CLA bot says that it can't find CLAs signed by either of the two email addresses it's checking for you. One is at firstlight, the other at quickplay. Please look here for more details: https://github.com/shaka-project/shaka-player/pull/4377/checks?check_run_id=7697333801 The internal diagnostics additionally say this: "If the user has signed a Corporate CLA, the email address on the commit must match the email address added to the CLA group by their company's CLA manager." Did you sign an individual or corporate CLA? |
c6e94d1
to
6432661
Compare
We have updated the discrepancies on the email address used in the last commit and PR. We have signed the CLA as the corporate. We don't know if the signed CLA's status has been approved. We suspect that was the one failing the checks. |
Hello, sorry that I have just seen the notification for this issue via the linked issue #4376. For a number of reasons the PlayStation 4 WebMAF framework had decided to make a breaking change by correcting the UUID endianness. Versions of the framework post 3.x use MSE/EME and prior to that a custom player and API was available. Additionally, the framework is not evergreen and requires the service to explicitly update. Therefore, for these reasons the number of services the endianness actually impacts right now is very few and the decision was made to correct the behaviour without introducing a new key system. @Vasanthavanan-Devarajan I requested contact via the dedicated support forums to make you aware of this change and hopefully to avoid the need for this PR as it will now be the wrong behaviour in future versions of WebMAF. I'm not sure what the Shaka team would like to do to resolve this but we would like to discuss it further. Thanks |
I have merged this before I knew about this information. @joeyparrish , what do you want to do about this? |
@ArktekniK Is there any difference between the user agents of both versions? Is there any way to differentiate the version of WebMAF to apply the hack? |
Hi Álvaro, The useragent contains the version. Our expectation is to make the fix at 3.1.0. However, we are conducting further testing on the impact of the change not just for Shaka but other major players so my suggestion is to wait until that's complete and we can revisit this. FYI the useragent format can be checked for, amongst other tokens, |
To clarify; checking for |
My suggestion is that @ArktekniK make a PR with a version check when the WebMAF fix is confirmed. You can follow the example of other checks in lib/util/platform.js, such as safariVersion(), then add a call in DrmEngine where this PR added isPS4(). If the isPS4() check added by this PR is not specific enough, you may want to replace isPS4() with isWebMAF() or similar. @ArktekniK, @Vasanthavanan-Devarajan, @avelad, does that sound good to everyone? |
Oh, also, @ArktekniK, what is the timeline for a fix in WebMAF? I'm thinking about release timing for Shaka Player, and how we might want to coordinate the official release of this change and a possible follow-up. |
We don't know how long it would take to fix the WebMAF. Shall we introduce the |
Our expectation (if the breaking change is confirmed) is to release this around the week of October 3rd.
Understood - we'll check and get back to you with a PR if necessary.
I think this is a good approach to take, and allows us some wiggle room with approach and release date. We're in the process of of testing other players and it seems Shaka is the only one that has a problem with the UUIDs (I'm not sure why just yet). As an example, Dash.js works fine as-is but breaks after changing the endianness. So the decision is still up in the air, and making this opt in would be helpful Will these changes be back-ported to earlier versions of shaka? (I see some other issues which have been, EG #4320). Thanks |
Based on the title "fix", it's eligible for cherry-picking to existing release branches. Since it fixed interpretation of key IDs, that seemed appropriate at the time. But it hasn't been cherry-picked yet. I'd like to push out new releases in the near future, though. I could exclude it for now, or someone could follow-up with a useragent check or opt-in config. I would prefer a useragent check to an opt-in config, since it seems unreasonable to ask applications to know in advance which specific WebMAF versions need the config. But I'm happy to discuss it further, and to take the advice of people with WebMAF experience. (I have none.) |
I'm going to push out v3.2.11 soon, specifically for the sake of the Cast Application Framework. I'll omit this change from the v3.2.x branch for now, since it's not clear if we should release this fix without a UA check, and since it won't impact Cast anyway. @ArktekniK, could we develop the UA check now, in advance of WebMAF/v3.1.0? Or would that be too risky without the actual release? |
Hi @joeyparrish We completed our assessment today and due to the impact our change would have on other players, we've ultimately decided to avoid making any changes within the existing key system as the situation is a bit complicated and we also dont want to affect your release plans. So this PR as authored is fine to go ahead with from our perspective. If there's a change in in future we will raise an issue and PR directly to keep your team in the loop
|
Thanks, @ArktekniK. @joeyparrish can you please consider this PR for the 3.2.11 release? |
Alright, no problem. Thanks, @ArktekniK, for the update. @Vasanthavanan-Devarajan, I'll cherry-pick the change to all active branches, starting with v3.2.x. |
This change was just released in v3.2.11, v3.3.9, v4.0.5, v4.1.3, and v4.2.0. |
Close #4376