Bug 1302331 (EME_CryptonInfo)

Add CryptonInfo into Sample.java for further queueSecureInputBuffer usage

RESOLVED FIXED in Firefox 52

Status

()

Firefox for Android
Audio/Video
P2
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: JamesCheng, Assigned: kikuo)

Tracking

unspecified
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
For the further EME support on Fennec,

We should pass CryptoInfo via IPC to child side process for [1].

So We should parcel CryptoInfo into the structure of Sample.

[1]
https://developer.android.com/reference/android/media/MediaCodec.html#queueSecureInputBuffer(int, int, android.media.MediaCodec.CryptoInfo, long, int)
Priority: -- → P2
(Assignee)

Updated

2 years ago
Assignee: nobody → kikuo
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
mozreview-review
Comment on attachment 8792256 [details]
Bug 1302331 - [Part1] Support MediaCodec.CryptoInfo in Sample class

https://reviewboard.mozilla.org/r/79394/#review77934

James Cheng and I have integrated Gecko EME with android MediaDrm API for both non-oop-decode case and oop-decode case.
We're gonna separate these work into different parts for review.
(Assignee)

Updated

a year ago
Attachment #8792256 - Flags: review?(nchen)
Attachment #8792256 - Flags: review?(jolin)
Attachment #8792257 - Flags: review?(nchen)
Attachment #8792257 - Flags: review?(jolin)
Attachment #8792258 - Flags: review?(nchen)
Attachment #8792258 - Flags: review?(jolin)

Comment 6

a year ago
mozreview-review
Comment on attachment 8792258 [details]
Bug 1302331 - [Part3] Create CryptoInfo from MediaRawData and deliver it to MediaCodecDataDecoder or remote codec decoder.

https://reviewboard.mozilla.org/r/79400/#review78044

Overall looks good to me. Please address the comments if it's okay. Thanks a lot.

::: dom/media/platforms/android/AndroidDecoderModule.cpp:34
(Diff revision 1)
>  using namespace mozilla::java::sdk;
>  using media::TimeUnit;
>  
> +namespace {
> +  template<class T>
> +  static jbyteArray CreateAndInitJByteArray(const T& data, jsize length)

Nit: passing ```nsTArray&``` rather than its elements and length separately seems simpler to me.

::: dom/media/platforms/android/AndroidDecoderModule.cpp:84
(Diff revision 1)
>  
> +CryptoInfo::LocalRef
> +GetCryptoInfoFromSample(const MediaRawData* aSample)
> +{
> +  auto& cryptoObj = aSample->mCrypto;
> +  if (cryptoObj.mValid) {

Nit: How about returning early when no crypto info?
```
  if (!crypto.mValid) {
    return nullptr;
  }

  CryptoInfo::LocalRef cryptoInfo;
  ...
```

::: dom/media/platforms/android/AndroidDecoderModule.cpp:98
(Diff revision 1)
> +    for (auto& size : cryptoObj.mEncryptedSizes) {
> +      totalSubSamplesSize += size;
> +    }
> +
> +    // mPlainSizes is uint16_t, need to transform to uint32_t first.
> +    nsTArray<uint32_t> plainSizes;

Make this ```nsTArray<jint>```?

::: dom/media/platforms/android/AndroidDecoderModule.cpp:109
(Diff revision 1)
> +    uint32_t codecSpecificDataSize = aSample->Size() - totalSubSamplesSize;
> +    // Size of codec specific data("CSD") for Andriod MediaCodec usage should be
> +    // included in the 1st plain size.
> +    plainSizes[0] += codecSpecificDataSize;
> +
> +    // IV length is 16 bytes for Andriod API.

s/Andriod/Android/

::: dom/media/platforms/android/AndroidDecoderModule.cpp:112
(Diff revision 1)
> +    plainSizes[0] += codecSpecificDataSize;
> +
> +    // IV length is 16 bytes for Andriod API.
> +    static const int kExpectedIVLength = 16;
> +    auto tempIV(cryptoObj.mIV);
> +    auto tempIVlength = tempIV.Length();

s/tempIVlength/tempIVLength/

::: dom/media/platforms/android/AndroidDecoderModule.cpp:115
(Diff revision 1)
> +    static const int kExpectedIVLength = 16;
> +    auto tempIV(cryptoObj.mIV);
> +    auto tempIVlength = tempIV.Length();
> +    if (tempIVlength < kExpectedIVLength) {
> +      // Padding with 0
> +      for (size_t i = 0; i < (kExpectedIVLength - tempIVlength); i++) {

Nit: How about
```
for (size_t i = tempIVLength; i < kExpectedIVLength; i++) {
  tempIV[i] = 0;
}
```
Also ```if``` is not necessary (the condition expression in ```for``` checks the same thing in the 1st iteration).

::: dom/media/platforms/android/AndroidDecoderModule.cpp:132
(Diff revision 1)
> +                    mozilla::jni::IntArray::Ref::From(numBytesOfEncryptedData),
> +                    mozilla::jni::ByteArray::Ref::From(keyId),
> +                    mozilla::jni::ByteArray::Ref::From(iv),
> +                    MediaCodec::CRYPTO_MODE_AES_CTR);
> +
> +    // Avoid local reference table overflow.

Could you elaborate? Local ref will be deleted automatically when out of scope, won't they?

::: dom/media/platforms/android/AndroidDecoderModule.cpp:139
(Diff revision 1)
> +    jenv->DeleteLocalRef(numBytesOfPlainData);
> +    jenv->DeleteLocalRef(numBytesOfEncryptedData);
> +    jenv->DeleteLocalRef(iv);
> +    jenv->DeleteLocalRef(keyId);
> +
> +    return jni::Object::LocalRef::Adopt(cryptoInfo.Env(), cryptoInfo.Forget());

Dumb question: why ```Adopt()``` here?

::: dom/media/platforms/android/RemoteDataDecoder.cpp:484
(Diff revision 1)
>      mCallback->Error(MediaDataDecoderError::FATAL_ERROR);
>      return;
>    }
>    bufferInfo->Set(0, aSample->Size(), aSample->mTime, 0);
>  
> -  mJavaDecoder->Input(bytes, bufferInfo);
> +  CryptoInfo::LocalRef cryptoInfo = GetCryptoInfoFromSample(aSample);

Nit: ```mJavaDecoder->Input(..., GetCryptoInfoFromSample(aSample));```
Attachment #8792258 - Flags: review?(jolin) → review+

Comment 7

a year ago
mozreview-review
Comment on attachment 8792256 [details]
Bug 1302331 - [Part1] Support MediaCodec.CryptoInfo in Sample class

https://reviewboard.mozilla.org/r/79396/#review78076

LGTM.
Attachment #8792256 - Flags: review?(jolin) → review+

Comment 8

a year ago
mozreview-review
Comment on attachment 8792257 [details]
Bug 1302331 - [Part2] Make CryptoInfo as an argument for method CodecProxy.input.

https://reviewboard.mozilla.org/r/79398/#review78078

LGTM.
Attachment #8792257 - Flags: review?(jolin) → review+

Comment 9

a year ago
mozreview-review
Comment on attachment 8792256 [details]
Bug 1302331 - [Part1] Support MediaCodec.CryptoInfo in Sample class

https://reviewboard.mozilla.org/r/79396/#review78214
Attachment #8792256 - Flags: review?(nchen) → review+

Comment 10

a year ago
mozreview-review
Comment on attachment 8792257 [details]
Bug 1302331 - [Part2] Make CryptoInfo as an argument for method CodecProxy.input.

https://reviewboard.mozilla.org/r/79398/#review78216
Attachment #8792257 - Flags: review?(nchen) → review+

Comment 11

a year ago
mozreview-review
Comment on attachment 8792258 [details]
Bug 1302331 - [Part3] Create CryptoInfo from MediaRawData and deliver it to MediaCodecDataDecoder or remote codec decoder.

https://reviewboard.mozilla.org/r/79400/#review78204

Looks good! Just a few simple fixes.

::: dom/media/platforms/android/AndroidDecoderModule.cpp:34
(Diff revision 1)
>  using namespace mozilla::java::sdk;
>  using media::TimeUnit;
>  
> +namespace {
> +  template<class T>
> +  static jbyteArray CreateAndInitJByteArray(const T& data, jsize length)

You should return `jni::ByteArray::LocalRef`, and use `jni::ByteArray::LocalRef::Adopt` when returning.

::: dom/media/platforms/android/AndroidDecoderModule.cpp:37
(Diff revision 1)
> +namespace {
> +  template<class T>
> +  static jbyteArray CreateAndInitJByteArray(const T& data, jsize length)
> +  {
> +    JNIEnv* const jenv = mozilla::jni::GetEnvForThread();
> +    jbyteArray result = jenv->NewByteArray(length);

Add `MOZ_CATCH_JNI_EXCEPTION(jenv);` after `NewByteArray` call.

::: dom/media/platforms/android/AndroidDecoderModule.cpp:38
(Diff revision 1)
> +  template<class T>
> +  static jbyteArray CreateAndInitJByteArray(const T& data, jsize length)
> +  {
> +    JNIEnv* const jenv = mozilla::jni::GetEnvForThread();
> +    jbyteArray result = jenv->NewByteArray(length);
> +    jenv->SetByteArrayRegion(result, 0, length, reinterpret_cast<const jbyte*>(const_cast<T>(data)));

Add `MOZ_CATCH_JNI_EXCEPTION(jenv);` after `SetByteArrayRegion` call.

::: dom/media/platforms/android/AndroidDecoderModule.cpp:139
(Diff revision 1)
> +    jenv->DeleteLocalRef(numBytesOfPlainData);
> +    jenv->DeleteLocalRef(numBytesOfEncryptedData);
> +    jenv->DeleteLocalRef(iv);
> +    jenv->DeleteLocalRef(keyId);
> +
> +    return jni::Object::LocalRef::Adopt(cryptoInfo.Env(), cryptoInfo.Forget());

You can use `return cryptoInfo;` here.
Attachment #8792258 - Flags: review?(nchen)
(Reporter)

Comment 12

a year ago
Hi Jim,

Do you think it is worth moving CreateAndInitJIntArray and CreateAndInitJByteArray into widget/android/jni/Utils.h ?

I want to use it in other cpps as well but I don't want to write it again or extern the declaration.

Thank you.
Flags: needinfo?(nchen)
(Assignee)

Comment 13

a year ago
mozreview-review-reply
Comment on attachment 8792258 [details]
Bug 1302331 - [Part3] Create CryptoInfo from MediaRawData and deliver it to MediaCodecDataDecoder or remote codec decoder.

https://reviewboard.mozilla.org/r/79400/#review78044

> Nit: passing ```nsTArray&``` rather than its elements and length separately seems simpler to me.

I'd like to keep the current sigatures because of considering it as an utility-function which is able to create array by using not only nsTArray& .

> Nit: How about returning early when no crypto info?
> ```
>   if (!crypto.mValid) {
>     return nullptr;
>   }
> 
>   CryptoInfo::LocalRef cryptoInfo;
>   ...
> ```

Good point ! Simpler to read.

> Make this ```nsTArray<jint>```?

jint is signed 32 bits, leaving it unsinged here is more appropriate.

> s/Andriod/Android/

Will be fixed.

> s/tempIVlength/tempIVLength/

Will be fixed.

> Nit: How about
> ```
> for (size_t i = tempIVLength; i < kExpectedIVLength; i++) {
>   tempIV[i] = 0;
> }
> ```
> Also ```if``` is not necessary (the condition expression in ```for``` checks the same thing in the 1st iteration).

Good finding !
In fact, the max IV size for AES is 16 bytes, so I'd like to remove the comment
```
// IV length is 16 bytes for Andriod API.
```

and modify as follows,
```
MOZ_ASSERT(tempIVLength <= 16);
for (size_t i = tempIVLength; i < kExpectedIVLength; i++) {
  tempIV[i].AppendElement(0);
}
```

What do you think ?

> Could you elaborate? Local ref will be deleted automatically when out of scope, won't they?

In ```cryptoInfo->Set()```, we're calling mozilla::jni::IntArray::Ref::From / mozilla::jni::BtyeArray::Ref::From, and that created extra references. So application crashed because of local reference table overflow if we do not delete it explicitly.
But by introduceing Jim's suggestions below, this will also be fixed. So I'll remove the comment & codes here.

> Dumb question: why ```Adopt()``` here?

Hmm, actaully no special reason, returning ```cryptoInfo``` directly is ok too, then I saw this [1], so just try different ways :p   I'll return ```cryptoIfno``` directly in the next patch.

[1] https://dxr.mozilla.org/mozilla-central/source/widget/android/NativeJSContainer.cpp#262,271,342
Comment hidden (mozreview-request)

Comment 15

a year ago
mozreview-review-reply
Comment on attachment 8792258 [details]
Bug 1302331 - [Part3] Create CryptoInfo from MediaRawData and deliver it to MediaCodecDataDecoder or remote codec decoder.

https://reviewboard.mozilla.org/r/79400/#review78044

> Good finding !
> In fact, the max IV size for AES is 16 bytes, so I'd like to remove the comment
> ```
> // IV length is 16 bytes for Andriod API.
> ```
> 
> and modify as follows,
> ```
> MOZ_ASSERT(tempIVLength <= 16);
> for (size_t i = tempIVLength; i < kExpectedIVLength; i++) {
>   tempIV[i].AppendElement(0);
> }
> ```
> 
> What do you think ?

Great! 
`[i]` in the loop is not needed, right?
(Assignee)

Comment 16

a year ago
mozreview-review-reply
Comment on attachment 8792258 [details]
Bug 1302331 - [Part3] Create CryptoInfo from MediaRawData and deliver it to MediaCodecDataDecoder or remote codec decoder.

https://reviewboard.mozilla.org/r/79400/#review78044

> Great! 
> `[i]` in the loop is not needed, right?

Oops, sorry, that's a typo. ^^||

Comment 17

a year ago
mozreview-review
Comment on attachment 8792258 [details]
Bug 1302331 - [Part3] Create CryptoInfo from MediaRawData and deliver it to MediaCodecDataDecoder or remote codec decoder.

https://reviewboard.mozilla.org/r/79400/#review78518
Attachment #8792258 - Flags: review?(nchen) → review+
(In reply to James Cheng[:JamesCheng] from comment #12)
> Hi Jim,
> 
> Do you think it is worth moving CreateAndInitJIntArray and
> CreateAndInitJByteArray into widget/android/jni/Utils.h ?
> 
> I want to use it in other cpps as well but I don't want to write it again or
> extern the declaration.
> 
> Thank you.

If you want to put it under jni/, instead of CreateAndInitJIntArray or CreateAndInitJByteArray, you should add jni::ByteArray::New and jni::IntArray::New, for example,

> jni::ByteArray::LocalRef jni::ByteArray::New(const int32_t* aArray, size_t aLength);

or,

> jni::ByteArray::LocalRef jni::ByteArray::New(const nsTArray<int32_t>& aArray);
Flags: needinfo?(nchen)
(Reporter)

Comment 19

a year ago
Thank you.

I will create a follow up bug and work on it once it is landed.

Thanks.
(Reporter)

Updated

a year ago
Blocks: 1304258
(Assignee)

Comment 20

a year ago
(In reply to Jim Chen [:jchen] [:darchons] from comment #17)
> Comment on attachment 8792258 [details]
> Bug 1302331 - [Part3] Create CryptoInfo from MediaRawData and deliver it to
> MediaCodecDataDecoder or remote codec decoder.
> 
> https://reviewboard.mozilla.org/r/79400/#review78518

Thanks for the review : )
Looks like autoland couldn't rebase this patch for landing.
Flags: needinfo?(kikuo)
Keywords: checkin-needed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 26

a year ago
mozreview-review
Comment on attachment 8792256 [details]
Bug 1302331 - [Part1] Support MediaCodec.CryptoInfo in Sample class

https://reviewboard.mozilla.org/r/79394/#review78994

Patches rebased, now fennec-specific JNI will be generated in FennecJNIWrapper.{h,cpp}
(Assignee)

Comment 27

a year ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #22)
> Looks like autoland couldn't rebase this patch for landing.

Thanks for the notification, Ryan, patches were rebased in Comment 26. Need your favor again, thanks :)
Flags: needinfo?(kikuo)
Keywords: checkin-needed

Comment 28

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/37a3221d68e4
[Part1] Support MediaCodec.CryptoInfo in Sample class r=jchen,jolin
https://hg.mozilla.org/integration/autoland/rev/6c9170dd13cf
[Part2] Make CryptoInfo as an argument for method CodecProxy.input. r=jchen,jolin
https://hg.mozilla.org/integration/autoland/rev/e0db3bcd71d9
[Part3] Create CryptoInfo from MediaRawData and deliver it to MediaCodecDataDecoder or remote codec decoder. r=jchen,jolin
Keywords: checkin-needed

Comment 29

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/37a3221d68e4
https://hg.mozilla.org/mozilla-central/rev/6c9170dd13cf
https://hg.mozilla.org/mozilla-central/rev/e0db3bcd71d9
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.