Add 'cbcs' encryption scheme support in Android media.

Add support for 'cbcs' encryption scheme in Android media.

BUG=568326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/e9081981f2fc7cff6a7eb0c01883dc724d5b02b2
Cr-Commit-Position: refs/heads/master@{#436432}

[dougsteed, 2016-06-10 16:47:58]

Description was changed from

==========
WIP on adding encryption scheme for Android media.

BUG=568326
==========

to

==========
WIP on adding encryption scheme for Android media.

BUG=568326
==========

[dougsteed, 2016-06-10 16:47:58]

dougsteed@chromium.org changed reviewers:
+ xhwang@chromium.org

[dougsteed, 2016-06-10 16:53:48]

Xiaohan: we talked a few weeks ago of the changes needed to add Cast support for
HLS SAMPLE_AES to current Android media pipeline. 

Can you give a preliminary look, and check if this CL hits the correct source
files based on the current (Spitzer) code ?

Will not be merging this right away, as the code is not yet tested, going to
start working on that next.

[xhwang, 2016-06-13 21:48:45]

Sorry for the delay. I'll take a look at this shortly.

[xhwang, 2016-06-15 05:29:05]

I didn't look into details but the general approach looks pretty good. I left
some comments as I saw them. Thanks!

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/android/j...
File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right):

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:379:
cryptoInfoPatternClazz.getConstructor(int.class, int.class);
Can you just check the API level instead of using reflections? You may want to
add an Android owner for this kind of questions.

Here's how we check the API level in MediaDrmBridge:
https://cs.chromium.org/chromium/src/media/base/android/java/src/org/chromium...

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:381:
Log.e(TAG, "Cannot find MediaCodec.CryptInfo.Pattern constructor", e);
s/CryptInfo/CryptoInfo

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:388:
Log.e(TAG, "Cannot access MediaCodec.CryptInfo.setPattern method", e);
ditto

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/android/s...
File media/base/android/sdk_media_codec_bridge.h (right):

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/android/s...
media/base/android/sdk_media_codec_bridge.h:52: const EncryptionScheme&
encryption_scheme,
OOC, can we have Pattern and Subsamples at the same time? It seems we shouldn't.
Do we have code the enforce this somewhere?

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/encryptio...
File media/base/encryption_scheme.h (right):

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/encryptio...
media/base/encryption_scheme.h:36: // disabled.
Could please add some comment about the relation between subsamples and pattern
mode? 

I found the doc in Android N MediaCodec.CryptoInfo helpful:

... A buffer's data is considered to be partitioned into "subSamples", each
subSample starts with a (potentially empty) run of plain, unencrypted bytes
followed by a (also potentially empty) run of encrypted bytes. If pattern
encryption applies, each of the latter runs is encrypted only partly, according
to a repeating pattern of "encrypt" and "skip" blocks. ...

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/encryptio...
media/base/encryption_scheme.h:52: uint8_t skip_blocks_ = 0;
OOC why do we make this change?

Also, the constructor still takes uint32_t...

[dougsteed, 2016-06-15 17:57:38]

Xiaohan: thanks for the initial look. I will be trying to get this working
(almost certainly there will be bugs) and then I'll update again.

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/android/j...
File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right):

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/android/j...
media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:379:
cryptoInfoPatternClazz.getConstructor(int.class, int.class);
On 2016/06/15 05:29:05, xhwang wrote:
> Can you just check the API level instead of using reflections? You may want to
> add an Android owner for this kind of questions.
> 
> Here's how we check the API level in MediaDrmBridge:
>
https://cs.chromium.org/chromium/src/media/base/android/java/src/org/chromium...

As I am accessing a new method only added in N, I can't do that until the N SDK
is added to chromium. Also I need the Build.VERSION enumeration to be updated.
In the meantime, I can code this up and hopefully get this working using
reflection, which makes it is extremely convenient.
Once the SDK is in chromium, I will be happy to change this. Might possibly wait
until that happens before merging this.
(I am not the right person and I don't think it is yet the time to actually make
those updates).

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/android/s...
File media/base/android/sdk_media_codec_bridge.h (right):

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/android/s...
media/base/android/sdk_media_codec_bridge.h:52: const EncryptionScheme&
encryption_scheme,
On 2016/06/15 05:29:05, xhwang wrote:
> OOC, can we have Pattern and Subsamples at the same time? It seems we
shouldn't.
> Do we have code the enforce this somewhere?

We definitely need both. Pattern applies within the "encrypted" part of the
subsample.

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/encryptio...
File media/base/encryption_scheme.h (right):

https://chromiumcodereview.appspot.com/2058113002/diff/1/media/base/encryptio...
media/base/encryption_scheme.h:52: uint8_t skip_blocks_ = 0;
On 2016/06/15 05:29:05, xhwang wrote:
> OOC why do we make this change?
> 
> Also, the constructor still takes uint32_t...

Actually, I need to clean this up. I think now that my original use of uint32_t
was a mistake based on a misreading of the spec. I want to clean it up, but I
should probably do it in a separate CL.

[dougsteed, 2016-10-20 17:00:28]

Description was changed from

==========
WIP on adding encryption scheme for Android media.

BUG=568326
==========

to

==========
Add support for 'cbcs' encryption scheme in Android media.

BUG=568326
==========

[dougsteed, 2016-10-20 17:07:36]

xhwang@. PTAL. This now works with the test content, and, since Android N SDK is
now in chromium, I have been able to eliminate the reflection that was used in
previous patch sets to determine whether the CryptoInfo.setPattern is supported.


Owing to a DRM related bug in N, this code actually will not work correctly
end-to-end until post-N, so the version probing is code accordingly.

https://codereview.chromium.org/2058113002/diff/1/media/base/android/java/src...
File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right):

https://codereview.chromium.org/2058113002/diff/1/media/base/android/java/src...
media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:379:
cryptoInfoPatternClazz.getConstructor(int.class, int.class);
On 2016/06/15 17:57:38, dougsteed wrote:
> On 2016/06/15 05:29:05, xhwang wrote:
> > Can you just check the API level instead of using reflections? You may want
to
> > add an Android owner for this kind of questions.
> > 
> > Here's how we check the API level in MediaDrmBridge:
> >
>
https://cs.chromium.org/chromium/src/media/base/android/java/src/org/chromium...
> 
> As I am accessing a new method only added in N, I can't do that until the N
SDK
> is added to chromium. Also I need the Build.VERSION enumeration to be updated.
> In the meantime, I can code this up and hopefully get this working using
> reflection, which makes it is extremely convenient.
> Once the SDK is in chromium, I will be happy to change this. Might possibly
wait
> until that happens before merging this.
> (I am not the right person and I don't think it is yet the time to actually
make
> those updates).

Android N SDK has now been added to chromium, so I now fixed this.

[xhwang, 2016-10-20 17:33:57]

Thanks for the headsup! It's been a while since I looked at it last time so I
need refresh myself a bit :) I'll take a look.

https://codereview.chromium.org/2058113002/diff/100001/media/base/ipc/media_p...
File media/base/ipc/media_param_traits_macros.h (right):

https://codereview.chromium.org/2058113002/diff/100001/media/base/ipc/media_p...
media/base/ipc/media_param_traits_macros.h:51:
media::EncryptionScheme::CIPHER_MODE_MAX);
You need to rebase since I recently changed this part as well.

[dougsteed, 2016-10-20 20:48:05]

Yes, another rebase was necessary - done. The roll to Android N SDK was reverted
so this CL doesn't currently build and can't be merged until after the SDK roll
relands.

[xhwang, 2016-10-20 23:52:07]

Mostly looking good, just a few questions for discussion.

Please also add a Clank owner who's more familiar with Java to review.

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:390: int
cipherMode, int patternEncrypt, int patternSkip, long presentationTimeUs) {
How can you make sure the Java cipherMode enums are in sync with the C++
cipherMode enums?

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/moc...
File media/base/android/mock_media_codec_bridge.cc (right):

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/moc...
media/base/android/mock_media_codec_bridge.cc:7: #include
"media/base/encryption_scheme.h"
OOC, why do you need this?

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/sdk...
File media/base/android/sdk_media_codec_bridge.cc (right):

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/sdk...
media/base/android/sdk_media_codec_bridge.cc:236:
static_cast<int>(encryption_scheme.pattern().skip_blocks()),
To be safe, does it make sense to use saturated_cast?

https://codereview.chromium.org/2058113002/diff/120001/media/base/encryption_...
File media/base/encryption_scheme.h (right):

https://codereview.chromium.org/2058113002/diff/120001/media/base/encryption_...
media/base/encryption_scheme.h:40: Pattern(uint32_t encrypt_blocks, uint32_t
skip_blocks);
the member variables are using uint8_t but the input here are uint32_t

https://codereview.chromium.org/2058113002/diff/120001/media/base/encryption_...
media/base/encryption_scheme.h:52: uint8_t skip_blocks_ = 0;
why do we make this change? Does it make sense to just use a simple type like
"int"?

Also, in general we don't use unsigned integers only because they can't be
negative:
https://google.github.io/styleguide/cppguide.html#Integer_Types

[dougsteed, 2016-10-21 17:15:05]

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
(right):

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:390: int
cipherMode, int patternEncrypt, int patternSkip, long presentationTimeUs) {
On 2016/10/20 23:52:07, xhwang wrote:
> How can you make sure the Java cipherMode enums are in sync with the C++
> cipherMode enums?

Hard to "make sure" but I will draw more attention to it.

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/moc...
File media/base/android/mock_media_codec_bridge.cc (right):

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/moc...
media/base/android/mock_media_codec_bridge.cc:7: #include
"media/base/encryption_scheme.h"
On 2016/10/20 23:52:07, xhwang wrote:
> OOC, why do you need this?

incomplete type problems otherwise.

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/sdk...
File media/base/android/sdk_media_codec_bridge.cc (right):

https://codereview.chromium.org/2058113002/diff/120001/media/base/android/sdk...
media/base/android/sdk_media_codec_bridge.cc:236:
static_cast<int>(encryption_scheme.pattern().skip_blocks()),
On 2016/10/20 23:52:07, xhwang wrote:
> To be safe, does it make sense to use saturated_cast?

saturated_cast doesn't work for enum, so I'd have to do static_cast inside
saturated_cast. I'm not quite sure I see the need for this here, I think passing
them as int seems adequate (all of these are small values that will easily fit
in the obvious ranges). But maybe I am missing something.

https://codereview.chromium.org/2058113002/diff/120001/media/base/encryption_...
File media/base/encryption_scheme.h (right):

https://codereview.chromium.org/2058113002/diff/120001/media/base/encryption_...
media/base/encryption_scheme.h:40: Pattern(uint32_t encrypt_blocks, uint32_t
skip_blocks);
On 2016/10/20 23:52:07, xhwang wrote:
> the member variables are using uint8_t but the input here are uint32_t

See my comment below.

https://codereview.chromium.org/2058113002/diff/120001/media/base/encryption_...
media/base/encryption_scheme.h:52: uint8_t skip_blocks_ = 0;
On 2016/10/20 23:52:07, xhwang wrote:
> why do we make this change? Does it make sense to just use a simple type like
> "int"?
> 
> Also, in general we don't use unsigned integers only because they can't be
> negative:
> https://google.github.io/styleguide/cppguide.html#Integer_Types

I am reverting this for this CL. The uint32_t originated from an error in my
reading of the spec. These numbers are allocated 4 bits (0-15) in the standard
whereas I had originally read that as 4 bytes. When I realized this error I
changed it to uint8_t (inconsistently). In any case I agree that there is
probably no reason not to just use int. But I think this probably for a separate
CL, so will leave it unchanged for now.

[xhwang, 2016-10-25 20:55:45]

LGTM, thanks!

[dougsteed, 2016-10-27 14:58:08]

The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 14:58:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 15:00:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 15:00:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dougsteed, 2016-10-27 16:31:13]

The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 16:32:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 16:44:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 16:44:27]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[dougsteed, 2016-10-27 18:04:58]

The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 18:05:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 18:37:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 18:37:56]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dougsteed, 2016-10-28 21:36:50]

The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 21:37:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 22:43:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 22:43:18]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dougsteed, 2016-10-28 22:53:20]

The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 22:53:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 23:18:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 23:18:27]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dougsteed, 2016-10-30 00:16:26]

The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-30 00:16:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-30 01:35:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-30 01:35:59]

Dry run: This issue passed the CQ dry run.

[dougsteed, 2016-11-10 18:10:58]

Description was changed from

==========
Add support for 'cbcs' encryption scheme in Android media.

BUG=568326
==========

to

==========
Add support for 'cbcs' encryption scheme in Android media.

BUG=568326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[dougsteed, 2016-11-10 18:34:56]

Description was changed from

==========
Add support for 'cbcs' encryption scheme in Android media.

BUG=568326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add support for 'cbcs' encryption scheme in Android media.

BUG=568326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[dougsteed, 2016-11-10 18:34:56]

dougsteed@chromium.org changed reviewers:
+ raymes@chromium.org, tsepez@chromium.org

[dougsteed, 2016-11-10 18:36:26]

tsepez@. Please look at the IPC changes in media/base/ipc/* and media/gpu/ipc/*
raymes@ please look at content/renderer/pepper/video_decode_shim.cc

[Tom Sepez, 2016-11-10 19:04:58]

This looks OK, but two things:
1.  Can you confirm that the target ipc_fuzzer_all builds locally?
    I think we omitted this on some of the bots to reduce build time.

2.  I suspect that encrypt_blocks_ and skip_blocks_ may be used as 
    indices into some data.  Could you point out where these are checked
    in the GPU process to guard against an untrusted renderer sending
    bad values?

[dougsteed, 2016-11-10 22:54:15]

On 2016/11/10 19:04:58, Tom Sepez wrote:
> This looks OK, but two things:
> 1.  Can you confirm that the target ipc_fuzzer_all builds locally?
>     I think we omitted this on some of the bots to reduce build time.
> 
> 2.  I suspect that encrypt_blocks_ and skip_blocks_ may be used as 
>     indices into some data.  Could you point out where these are checked
>     in the GPU process to guard against an untrusted renderer sending
>     bad values?

1. Yes, the target built OK.
2. Actually, the two values are not indices, but are counts that constitute
a pattern {M,N} where (repeatedly) M blocks are encrypted and N blocks skipped
(left in clear). 
The values in the pattern are not used directly in the GPU process, but by
the MediaCodec on Android (where the actual decryption occurs). 
The latter has responsibility for sanity checking the values.
In the chromium GPU process, the two values are just passed using JNI into
Java code which calls this API:
MediaCrypto.CryptoInfo.setPattern(MediaCodec.CryptoInfo.Pattern newPattern).

[Tom Sepez, 2016-11-11 18:22:09]

lgtm

[raymes, 2016-11-14 01:08:38]

pepper lgtm

[dougsteed, 2016-11-28 19:27:25]

The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 19:28:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 22:39:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 22:39:32]

Dry run: This issue passed the CQ dry run.

[dougsteed, 2016-11-29 18:22:49]

The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 18:23:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 18:34:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 18:34:55]

Dry run: This issue passed the CQ dry run.

[dougsteed, 2016-12-05 18:54:35]

The CQ bit was checked by dougsteed@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 18:55:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 21:48:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 21:48:32]

Dry run: This issue passed the CQ dry run.

[dougsteed, 2016-12-05 22:03:14]

The CQ bit was checked by dougsteed@chromium.org

[dougsteed, 2016-12-05 22:03:15]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
tsepez@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2058113002/#ps320001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-05 22:03:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 22:35:39]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1480975394645410,
"parent_rev": "0131faa07ae106d1e8facf8f374e686d64747231", "commit_rev":
"0c4550b44d407746f4c514e3a3e2d80a918e69a6"}

[commit-bot: I haz the power, 2016-12-05 22:36:05]

Description was changed from

==========
Add support for 'cbcs' encryption scheme in Android media.

BUG=568326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add support for 'cbcs' encryption scheme in Android media.

BUG=568326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-12-05 22:36:06]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-12-05 22:38:02]

Description was changed from

==========
Add support for 'cbcs' encryption scheme in Android media.

BUG=568326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add support for 'cbcs' encryption scheme in Android media.

BUG=568326
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/e9081981f2fc7cff6a7eb0c01883dc724d5b02b2
Cr-Commit-Position: refs/heads/master@{#436432}
==========

[commit-bot: I haz the power, 2016-12-05 22:38:03]

Patchset 17 (id:??) landed as
https://crrev.com/e9081981f2fc7cff6a7eb0c01883dc724d5b02b2
Cr-Commit-Position: refs/heads/master@{#436432}

[Torne, 2017-05-22 15:44:52]

torne@chromium.org changed reviewers:
+ torne@chromium.org

[Torne, 2017-05-22 15:44:56]

https://codereview.chromium.org/2058113002/diff/320001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java (right):

https://codereview.chromium.org/2058113002/diff/320001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:527: return
Build.VERSION.SDK_INT > Build.VERSION_CODES.N;
Is this check still correct? Was this bug fixed in N MR1, or does this need to
be updated to compare to N MR1?

I'm working on looking for suspicious SDK level checks and this showed up.

[dougsteed, 2017-05-24 17:44:34]

On 2017/05/22 15:44:56, Torne wrote:
>
https://codereview.chromium.org/2058113002/diff/320001/media/base/android/jav...
> File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java
(right):
> 
>
https://codereview.chromium.org/2058113002/diff/320001/media/base/android/jav...
> media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:527: return
> Build.VERSION.SDK_INT > Build.VERSION_CODES.N;
> Is this check still correct? Was this bug fixed in N MR1, or does this need to
> be updated to compare to N MR1?
> 
> I'm working on looking for suspicious SDK level checks and this showed up.

Hi. This feature required N MR1, because there was a bug in N (fixed in MR1)
that prevented it from working.
So the check is strictly correct, but would it be better style to change it to
>= Build.VERSION_CODES.N_MR1 ?

[Torne, 2017-05-25 17:00:40]

On 2017/05/24 17:44:34, dougsteed wrote:
> On 2017/05/22 15:44:56, Torne wrote:
> >
>
https://codereview.chromium.org/2058113002/diff/320001/media/base/android/jav...
> > File media/base/android/java/src/org/chromium/media/MediaCodecUtil.java
> (right):
> > 
> >
>
https://codereview.chromium.org/2058113002/diff/320001/media/base/android/jav...
> > media/base/android/java/src/org/chromium/media/MediaCodecUtil.java:527:
return
> > Build.VERSION.SDK_INT > Build.VERSION_CODES.N;
> > Is this check still correct? Was this bug fixed in N MR1, or does this need
to
> > be updated to compare to N MR1?
> > 
> > I'm working on looking for suspicious SDK level checks and this showed up.
> 
> Hi. This feature required N MR1, because there was a bug in N (fixed in MR1)
> that prevented it from working.
> So the check is strictly correct, but would it be better style to change it to
> >= Build.VERSION_CODES.N_MR1 ?

Thanks for confirming this. Yes, now that the N_MR1 constant is available it'd
be clearer that this is intentional if you used >=. I realise the required SDK
probably wasn't checked in at the time, though - it's tricky to deal with
unreleased API levels consistently.

I uploaded a CL to change this here: https://codereview.chromium.org/2896303006