1302331 - (EME_CryptonInfo) Add CryptonInfo into Sample.java for further queueSecureInputBuffer usage

Status: RESOLVED FIXED

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

Description

[James Cheng[:JamesCheng]]

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)

Comment 1

[Kilik Kuo [:kikuo] (inactive)]

Basically, I'll provide patches according to the modification [1][2][3][4]

[1] https://github.com/JamesWCCheng/gecko-dev/commit/dffc11d6816c82f32d52a5fa1645d23539c5bea4
[2] https://github.com/JamesWCCheng/gecko-dev/commit/197928c9091746f1f73d9ea9a07660d6a03957cc
[3] https://github.com/JamesWCCheng/gecko-dev/commit/ec32a6ab472fe91e81dc095a149918dd9f4c5e0b
[4] https://github.com/JamesWCCheng/gecko-dev/commit/9fa13f666273ca46b2de99ea02438d3648b89266

Comment 2

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1302331 - [Part1] Support MediaCodec.CryptoInfo in Sample class

Review commit: https://reviewboard.mozilla.org/r/79396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79396/

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1302331 - [Part2] Make CryptoInfo as an argument for method CodecProxy.input.

Review commit: https://reviewboard.mozilla.org/r/79398/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79398/

Comment 4

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1302331 - [Part3] Create CryptoInfo from MediaRawData and deliver it to MediaCodecDataDecoder or remote codec decoder.

Review commit: https://reviewboard.mozilla.org/r/79400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79400/

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

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.

Comment 6

[John Lin [:jhlin][: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));```

Comment 7

[John Lin [:jhlin][:jolin]]

Comment on attachment 8792256 [details]
Bug 1302331 - [Part1] Support MediaCodec.CryptoInfo in Sample class

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

LGTM.

Comment 8

[John Lin [:jhlin][:jolin]]

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.

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8792256 [details]
Bug 1302331 - [Part1] Support MediaCodec.CryptoInfo in Sample class

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

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

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

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

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 12

[James Cheng[:JamesCheng]]

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.

Comment 13

[Kilik Kuo [:kikuo] (inactive)]

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 14

[Kilik Kuo [:kikuo] (inactive)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79400/diff/1-2/

Comment 15

[John Lin [:jhlin][: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

> 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 16

[Kilik Kuo [:kikuo] (inactive)]

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

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

(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);

Comment 19

[James Cheng[:JamesCheng]]

Thank you.

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

Thanks.

Comment 20

[Kilik Kuo [:kikuo] (inactive)]

(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 : )

Comment 21

[Kilik Kuo [:kikuo] (inactive)]

Try run, https://treeherder.mozilla.org/#/jobs?repo=try&revision=51756bee9bec

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Looks like autoland couldn't rebase this patch for landing.

Comment 23

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8792256 [details]
Bug 1302331 - [Part1] Support MediaCodec.CryptoInfo in Sample class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79396/diff/1-2/

Comment 24

[Kilik Kuo [:kikuo] (inactive)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79398/diff/1-2/

Comment 25

[Kilik Kuo [:kikuo] (inactive)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/79400/diff/2-3/

Comment 26

[Kilik Kuo [:kikuo] (inactive)]

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}

Comment 27

[Kilik Kuo [:kikuo] (inactive)]

(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 :)

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/37a3221d68e4
https://hg.mozilla.org/mozilla-central/rev/6c9170dd13cf
https://hg.mozilla.org/mozilla-central/rev/e0db3bcd71d9