Closed Bug 1302331 (EME_CryptonInfo) Opened 8 years ago Closed 8 years ago

Add CryptonInfo into Sample.java for further queueSecureInputBuffer usage

Categories

(Firefox for Android Graveyard :: Audio/Video, defect, P2)

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: JamesCheng, Assigned: kikuo)

References

Details

Attachments

(3 files)

Bug 1302331 - [Part1] Support MediaCodec.CryptoInfo in Sample class
58 bytes, text/x-review-board-request
jhlin
: review+
jchen
: review+
Details
Bug 1302331 - [Part2] Make CryptoInfo as an argument for method CodecProxy.input.
58 bytes, text/x-review-board-request
jhlin
: review+
jchen
: review+
Details
Bug 1302331 - [Part3] Create CryptoInfo from MediaRawData and deliver it to MediaCodecDataDecoder or remote codec decoder.
58 bytes, text/x-review-board-request
jhlin
: review+
jchen
: review+
Details 
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: nobody → kikuo
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.
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 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 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 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 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 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 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)
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)
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 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?
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 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)
Thank you.

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

Thanks.
Blocks: 1304258
(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 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}
(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
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: