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.

Sign up for GitHub

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

Jump to bottom

Added support for CENC ClearKey #2372

Merged
merged 1 commit into from
Feb 23, 2017
Merged

Added support for CENC ClearKey #2372

merged 1 commit into from
Feb 23, 2017

Conversation

wasabeef
Copy link
Contributor

ojw28
ojw28 reviewed Jan 25, 2017
View reviewed changes
Copy link
Contributor

@ojw28 ojw28 left a comment

Choose a reason for hiding this comment

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

Looks good aside from one comment (see inline). Thanks!

library/src/main/java/com/google/android/exoplayer2/extractor/mp4/FragmentedMp4Extractor.java Outdated
@@ -1192,7 +1192,7 @@ private static DrmInitData getDrmInitDataFromAtoms(List<Atom.LeafAtom> leafChild
if (uuid == null) {
Log.w(TAG, "Skipped pssh atom (failed to extract uuid)");
} else {
schemeDatas.add(new SchemeData(uuid, MimeTypes.VIDEO_MP4, psshData));
schemeDatas.add(new SchemeData(uuid, MimeTypes.CENC, psshData));
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't appear to be necessary? I have a hunch it might break Widevine/PlayReady on some earlier API levels also, although I didn't confirm this. If it works without, can we just leave it how it was and also remove the CENC constant from MimeTypes.java?

Copy link
Contributor Author

@wasabeef wasabeef Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Thank for your replay.

It accept "video/mp4" only on Android 7.1.1 or higher.
https://android.googlesource.com/platform/frameworks/av/+/01a977aa75c296c5467024a7beac50cbd4ed0335%5E%21/#F4
https://android.googlesource.com/platform/frameworks/av/+/android-5.0.0_r1/drm/mediadrm/plugins/clearkey/InitDataParser.cpp#48

Should I change the definition class?

Copy link

@abouda abouda Feb 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You should also add this for the case where the pssh is parsed from the manifest. That is in DashManifestParser#parseContentProtection.
Otherwise if you follow @ojw28 ´s tip about doing this in the DrmSessionManager, that would cover both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right.

@ojw28
Copy link
Contributor

ojw28 commented Jan 30, 2017 •
edited
Loading

Thanks for clarification in your previous response (although it seems to have disappeared somehow). I don't think you should modify MimeTypes as part of this change. I'm not convinced it's a good idea to change FragmentedMp4Extractor either.

It feels to me that you're trying to work around a bug in the underlying CDM implementation on older versions of Android. The correct place to do that is probably DefaultDrmSessionManager.acquireSession. There's already a workaround in that method for the Widevine CDM, so you can add a second workaround there that changes schemeMimeType if all of the following are true:

  1. Util.SDK_INT < 23
  2. C.CENC_UUID.equals(uuid)
  3. MimeTypes.VIDEO_MP4.equals(schemeMimeType)

That way the workaround will be applied correctly regardless of where the media is coming from (e.g. if another extractor implementation is used).

@wasabeef
Copy link
Contributor Author

wasabeef commented Feb 7, 2017 •
edited
Loading

Sorry for the late reply.

Oh, I see. Why is SDK_INT 23?
It seems the video/mp4 has been added since android-7.1.1_r1.

7.1.0_r7
https://android.googlesource.com/platform/frameworks/av/+/android-7.1.0_r7/drm/mediadrm/plugins/clearkey/InitDataParser.cpp#48

android-7.1.1_r1
https://android.googlesource.com/platform/frameworks/av/+/android-7.1.1_r1/drm/mediadrm/plugins/clearkey/InitDataParser.cpp#49

@ojw28
Copy link
Contributor

ojw28 commented Feb 7, 2017 •
edited
Loading

Util.SDK_INT < 26 then, since that would suggest not all devices with SDK_INT == 25 will include the change, but that all devices with SDK_INT == 26 or greater will?

@wasabeef
Copy link
Contributor Author

It's difficult to judge with SDK_INT == 25, In this case, I'd like to use MediaDrm#isCryptoSchemeSupported.

What do you think?

if (C.CENC_UUID.equals(uuid) && MimeTypes.VIDEO_MP4.equals(schemeMimeType)) {
  // If "video/mp4" is not supported as CENC schema, change it to "cenc".
  if (Util.SDK_INT < 19 || !MediaDrm.isCryptoSchemeSupported(uuid, MimeTypes.VIDEO_MP4)) {
    schemeMimeType = CENC_INIT_DATA_FORMAT;
  }
}

ojw28
ojw28 reviewed Feb 12, 2017
View reviewed changes
library/src/main/java/com/google/android/exoplayer2/drm/DefaultDrmSessionManager.java Outdated
@@ -337,6 +343,12 @@ public void setMode(@Mode int mode, byte[] offlineLicenseKeySetId) {
schemeInitData = psshData;
}
}
if (C.CENC_UUID.equals(uuid) && MimeTypes.VIDEO_MP4.equals(schemeMimeType)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should handle audio/mp4 as well for completeness, I think. I'd also rather there were a capping SDK level after which the workaround is not applied. We shouldn't be applying workarounds for platform issues to future platform releases; we should be making sure the problems are fixed in the platform (which it appears has been done).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ojw28

I'm sorry for the late reply. I updated again.

@ojw28 ojw28 merged commit 7e02e58 into google:dev-v2 Feb 23, 2017
@ojw28
Copy link
Contributor

ojw28 commented Feb 23, 2017 •
edited
Loading

Thanks! I simplified this somewhat in 3bb08e5. I think it's equivalent, except that it always replaces schemeMimeType to "cenc" on API level 25. I think this is fine - my main concern was just not applying the workaround indefinitely, which is achieved by capping to not apply on API levels above 25. Let me know if I accidentally broke something!

@ojw28
Copy link
Contributor

ojw28 commented Feb 23, 2017

Also corrected some naming in 5fe5076.

@wasabeef
Copy link
Contributor Author

No problem.
I'm sorry that I didn't understand your intentions correctly. 🙇

@wasabeef
Copy link
Contributor Author

wasabeef commented Feb 23, 2017 •
edited
Loading

Don't you like MediaDrm.isCryptoSchemeSupported ?

@ojw28
Copy link
Contributor

ojw28 commented Feb 23, 2017

I don't dislike it. It just seems unnecessarily complicated to use it just on one API level.

@wasabeef
Copy link
Contributor Author

Right, got it. Thanks!

ojw28 added a commit that referenced this pull request Feb 28, 2017
@ojw28
Merge remainder of #2372
563a397 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=148344124
@google google locked and limited conversation to collaborators Jun 28, 2017
@ojw28
Copy link
Contributor

ojw28 commented Jul 17, 2017

@wasabeef - The sample asset you added in this request appears to no longer work. Could you update or remove it? Thanks.

@google google unlocked this conversation Jul 17, 2017
@google google locked and limited conversation to collaborators Aug 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@abouda abouda abouda left review comments

@ojw28 ojw28 ojw28 left review comments

Assignees
No one assigned
Labels
cla: yes should merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wasabeef @ojw28 @abouda @googlebot