media: Clean up MediaCodecBridge and remove subclasses

media: Clean up MediaCodecBridge and remove subclasses

Previously we had two subclasses of MediaCodecBridgeImpl,
VideoCodecBridge and AudioCodecBridge, which have become unnecessary.
For simplicity and ease of mocking, now we just have
MediaCodecBridgeImpl.

This CL also includes various cleanups, including:
* MediaCodecBridge factories return unique_ptrs.
* Remove some unused MCB functions.
* Use the java enum generator where possible to sync native and java
  enums.
* Collapsing DEQUEUE_OUTPUT_TRY_AGAIN and DEQUEUE_INPUT_TRY_AGAIN into
  a single TRY_AGAIN (like MediaCodec).
* Remove size params from isAdaptivePlaybackSupported() because they
  weren't used.
* Localizing codec to mime type conversions to MediaCodecUtil.
* Passing mime strings to MediaCodecUtil::CanDecode() instead of an
  ad-hoc codec name string.

BUG=691828
TEST=existing tests
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

Review-Url: https://codereview.chromium.org/2697643003
Cr-Commit-Position: refs/heads/master@{#452334}
Committed: https://chromium.googlesource.com/chromium/src/+/c71ef8d720f0aea74a3ac4efe15df87670fa3875

[watk, 2017-02-14 00:48:16]

Description was changed from

==========
media: Clean up MediaCodecBridge and remove subclasses

Previously we had two subclasses of MediaCodecBridgeImpl,
VideoCodecBridge and AudioCodecBridge, which have become unnecessary.
For simplicity and ease of mocking, now we just have
MediaCodecBridgeImpl.

This CL also includes various cleanups, including:
* MediaCodecBridge factories return unique_ptrs.
* Remove some unused MCB functions.
* Use the java enum generator where possible to sync native and java
  enums.
* Collapsing DEQUEUE_OUTPUT_TRY_AGAIN and DEQUEUE_INPUT_TRY_AGAIN into
  a single TRY_AGAIN (like MediaCodec).
* Remove size params from isAdaptivePlaybackSupported() because they
  weren't used.
* Localizing codec to mime type conversions to MediaCodecUtil.
* Passing mime strings to MediaCodecUtil::CanDecode() instead of an
  ad-hoc codec name string.

BUG=691828
TEST=existing tests
==========

to

==========
media: Clean up MediaCodecBridge and remove subclasses

Previously we had two subclasses of MediaCodecBridgeImpl,
VideoCodecBridge and AudioCodecBridge, which have become unnecessary.
For simplicity and ease of mocking, now we just have
MediaCodecBridgeImpl.

This CL also includes various cleanups, including:
* MediaCodecBridge factories return unique_ptrs.
* Remove some unused MCB functions.
* Use the java enum generator where possible to sync native and java
  enums.
* Collapsing DEQUEUE_OUTPUT_TRY_AGAIN and DEQUEUE_INPUT_TRY_AGAIN into
  a single TRY_AGAIN (like MediaCodec).
* Remove size params from isAdaptivePlaybackSupported() because they
  weren't used.
* Localizing codec to mime type conversions to MediaCodecUtil.
* Passing mime strings to MediaCodecUtil::CanDecode() instead of an
  ad-hoc codec name string.

BUG=691828
TEST=existing tests
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
==========

[watk, 2017-02-14 00:48:22]

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

[commit-bot: I haz the power, 2017-02-14 00:49:12]

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, 2017-02-14 00:52:25]

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

[commit-bot: I haz the power, 2017-02-14 00:52:26]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[watk, 2017-02-14 01:40:50]

watk@chromium.org changed reviewers:
+ liberato@chromium.org

[watk, 2017-02-14 01:40:51]

PTAL. This follows on from https://codereview.chromium.org/2672313006/.

https://codereview.chromium.org/2697643003/diff/1/components/cdm/browser/cdm_...
File components/cdm/browser/cdm_message_filter_android.cc (left):

https://codereview.chromium.org/2697643003/diff/1/components/cdm/browser/cdm_...
components/cdm/browser/cdm_message_filter_android.cc:40:
{media::EME_CODEC_WEBM_OPUS, CODEC_AUDIO, "opus", "video/webm"},
By passing the mime type instead of names we can remove the name->mime
conversion function from MediaCodecUtil.

https://codereview.chromium.org/2697643003/diff/1/media/base/android/media_co...
File media/base/android/media_codec_bridge_impl.cc (left):

https://codereview.chromium.org/2697643003/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge_impl.cc:47: const std::string
AudioCodecToAndroidMimeType(const AudioCodec& codec) {
Moved to MediaCodecUtil

https://codereview.chromium.org/2697643003/diff/1/media/base/android/media_co...
File media/base/android/media_codec_bridge_impl.cc (right):

https://codereview.chromium.org/2697643003/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge_impl.cc:58: bool
ConfigureMediaFormatForAudio(jobject j_format,
Unchanged; moved  only

https://codereview.chromium.org/2697643003/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge_impl.cc:191:
std::unique_ptr<MediaCodecBridge> MediaCodecBridgeImpl::CreateAudioDecoder(
This used to be two separate calls: Create() and ConfigureAndStart(). I
collapsed them into one like the video versions.

https://codereview.chromium.org/2697643003/diff/1/media/base/android/media_co...
File media/base/android/media_codec_bridge_impl.h (left):

https://codereview.chromium.org/2697643003/diff/1/media/base/android/media_co...
media/base/android/media_codec_bridge_impl.h:30: // Calls start() against the
media codec instance. Returns whether media
These comments were duplicating the interface comments.

https://codereview.chromium.org/2697643003/diff/1/media/base/android/media_co...
File media/base/android/media_codec_util.cc (right):

https://codereview.chromium.org/2697643003/diff/1/media/base/android/media_co...
media/base/android/media_codec_util.cc:74: kMp3MimeType, kAacMimeType, 
kOpusMimeType, kVorbisMimeType,
I added mp3 here. Seems like it was missing before?

[watk, 2017-02-14 02:32:14]

Btw, I moved the factory functions back to line up with their previous versions
so you can review the changes. After you review this I'll move them up to the
top of the file.

[watk, 2017-02-14 02:32:45]

https://codereview.chromium.org/2697643003/diff/60001/components/cdm/browser/...
File components/cdm/browser/cdm_message_filter_android.cc (right):

https://codereview.chromium.org/2697643003/diff/60001/components/cdm/browser/...
components/cdm/browser/cdm_message_filter_android.cc:18: #include
"media/base/video_codecs.h"
Note to me: delete these now.

[liberato (no reviews please), 2017-02-14 17:44:11]

very nice.

lgtm.

-fl

https://codereview.chromium.org/2697643003/diff/60001/media/base/android/medi...
File media/base/android/media_codec_util.cc (right):

https://codereview.chromium.org/2697643003/diff/60001/media/base/android/medi...
media/base/android/media_codec_util.cc:32: namespace mime_type {
these seem to be alternately named "android mime type" and "mime type" in the
various places it's used.  if these types have android-specific things, then
probably this namespace should be android_mime_type.

i really like that you've put these into a namespace.

if you want to go really crazy, 'using AndroidMimeType = std::string'.  however,
i don't know if there are cases where we actually preserve the resulting string
separately from the function that generates it.  if not, then it would add no
clarity.

https://codereview.chromium.org/2697643003/diff/60001/media/base/encryption_s...
File media/base/encryption_scheme.h (right):

https://codereview.chromium.org/2697643003/diff/60001/media/base/encryption_s...
media/base/encryption_scheme.h:19: // GENERATED_JAVA_ENUM_PACKAGE:
org.chromium.media
shiny!

[DaleCurtis, 2017-02-14 18:10:31]

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

[DaleCurtis, 2017-02-14 18:10:32]

https://codereview.chromium.org/2697643003/diff/60001/media/base/android/medi...
File media/base/android/media_codec_util.cc (right):

https://codereview.chromium.org/2697643003/diff/60001/media/base/android/medi...
media/base/android/media_codec_util.cc:32: namespace mime_type {
On 2017/02/14 at 17:44:11, liberato wrote:
> these seem to be alternately named "android mime type" and "mime type" in the
various places it's used.  if these types have android-specific things, then
probably this namespace should be android_mime_type.
> 
> i really like that you've put these into a namespace.
> 
> if you want to go really crazy, 'using AndroidMimeType = std::string'. 
however, i don't know if there are cases where we actually preserve the
resulting string separately from the function that generates it.  if not, then
it would add no clarity.

I feel like these should share the values we have in
media/base/mime_util_internal.

https://codereview.chromium.org/2697643003/diff/60001/media/base/encryption_s...
File media/base/encryption_scheme.h (right):

https://codereview.chromium.org/2697643003/diff/60001/media/base/encryption_s...
media/base/encryption_scheme.h:19: // GENERATED_JAVA_ENUM_PACKAGE:
org.chromium.media
On 2017/02/14 at 17:44:11, liberato wrote:
> shiny!

+1, didn't know about this!

[watk, 2017-02-14 23:28:58]

In the latest PS I moved the static functions in MCBridgeImpl back to the top of
the file and made a minor change. I deleted ToJavaIntArray in favor of the
base::android version with the same name.

https://codereview.chromium.org/2697643003/diff/60001/media/base/android/medi...
File media/base/android/media_codec_util.cc (right):

https://codereview.chromium.org/2697643003/diff/60001/media/base/android/medi...
media/base/android/media_codec_util.cc:32: namespace mime_type {
On 2017/02/14 18:10:32, DaleCurtis wrote:
> On 2017/02/14 at 17:44:11, liberato wrote:
> > these seem to be alternately named "android mime type" and "mime type" in
the
> various places it's used.  if these types have android-specific things, then
> probably this namespace should be android_mime_type.
> > 
> > i really like that you've put these into a namespace.
> > 
> > if you want to go really crazy, 'using AndroidMimeType = std::string'. 
> however, i don't know if there are cases where we actually preserve the
> resulting string separately from the function that generates it.  if not, then
> it would add no clarity.
> 
> I feel like these should share the values we have in
> media/base/mime_util_internal.

I agree, but didn't see an obvious way to share, so leaving it for now.

Renamed to android_mime_type because the vp8 and vp9 ones are android specific.
Thanks for noticing.

https://codereview.chromium.org/2697643003/diff/60001/media/base/encryption_s...
File media/base/encryption_scheme.h (right):

https://codereview.chromium.org/2697643003/diff/60001/media/base/encryption_s...
media/base/encryption_scheme.h:19: // GENERATED_JAVA_ENUM_PACKAGE:
org.chromium.media
On 2017/02/14 18:10:32, DaleCurtis wrote:
> On 2017/02/14 at 17:44:11, liberato wrote:
> > shiny!
> 
> +1, didn't know about this!

Yeah pretty cool! It automatically strips the common prefix if it's the same as
the enum name. And you can use annotations in the java code to verify that you
only assign values from the enum set, but didn't seem worth it. e.g., you'd
declare it as: @CipherModeEnum int cipherMode;

[watk, 2017-02-14 23:35:56]

https://codereview.chromium.org/2697643003/diff/60001/media/base/android/medi...
File media/base/android/media_codec_util.cc (right):

https://codereview.chromium.org/2697643003/diff/60001/media/base/android/medi...
media/base/android/media_codec_util.cc:59: mime_type::kMp3, mime_type::kAac, 
mime_type::kOpus, mime_type::kVorbis,
I added mp3 here, because I didn't know why it would be missing.. It's only used
in DCHECKs anyway. Dale?

[DaleCurtis, 2017-02-14 23:44:43]

https://codereview.chromium.org/2697643003/diff/60001/media/base/android/medi...
File media/base/android/media_codec_util.cc (right):

https://codereview.chromium.org/2697643003/diff/60001/media/base/android/medi...
media/base/android/media_codec_util.cc:59: mime_type::kMp3, mime_type::kAac, 
mime_type::kOpus, mime_type::kVorbis,
On 2017/02/14 at 23:35:56, watk wrote:
> I added mp3 here, because I didn't know why it would be missing.. It's only
used in DCHECKs anyway. Dale?

Probably because there are no protected content mp3s :)

[watk, 2017-02-15 00:03:55]

The CQ bit was checked by watk@chromium.org

[watk, 2017-02-15 00:03:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2697643003/#ps80001
(title: "move functions; remove ToIntArray")

[watk, 2017-02-15 00:03:59]

The CQ bit was unchecked by watk@chromium.org

[watk, 2017-02-15 00:04:06]

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

[commit-bot: I haz the power, 2017-02-15 00:05:04]

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, 2017-02-15 00:36:49]

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

[commit-bot: I haz the power, 2017-02-15 00:36:50]

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...)

[watk, 2017-02-15 00:56:43]

The CQ bit was checked by watk@chromium.org

[watk, 2017-02-15 00:56:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2697643003/#ps100001
(title: "fix clang warnings")

[commit-bot: I haz the power, 2017-02-15 00:57:52]

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, 2017-02-15 01:24:13]

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

[commit-bot: I haz the power, 2017-02-15 01:24:14]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[watk, 2017-02-15 01:31:38]

The CQ bit was checked by watk@chromium.org

[watk, 2017-02-15 01:31:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2697643003/#ps120001
(title: "missed renaming some instances")

[commit-bot: I haz the power, 2017-02-15 01:33:26]

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, 2017-02-15 02:01:23]

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

[commit-bot: I haz the power, 2017-02-15 02:01:24]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[watk, 2017-02-15 21:30:22]

Patchset #8 (id:140001) has been deleted

[watk, 2017-02-15 21:46:28]

Patchset #8 (id:160001) has been deleted

[watk, 2017-02-15 21:54:27]

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

[commit-bot: I haz the power, 2017-02-15 21:55:41]

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, 2017-02-15 22:46:04]

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

[commit-bot: I haz the power, 2017-02-15 22:46:06]

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...)

[watk, 2017-02-16 00:16:34]

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

[commit-bot: I haz the power, 2017-02-16 00:17: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, 2017-02-16 01:06:15]

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

[commit-bot: I haz the power, 2017-02-16 01:06:16]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[watk, 2017-02-17 01:58:53]

I'm dumb. I reordered the codec delay calculation to be more intuitive and
completely broke it because the intermediate result was being truncated. I
commented that the order is important.

[watk, 2017-02-17 01:59:09]

The CQ bit was checked by watk@chromium.org

[watk, 2017-02-17 01:59:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2697643003/#ps220001
(title: "Revert codec delay in ns calculation")

[commit-bot: I haz the power, 2017-02-17 02:01:32]

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, 2017-02-17 03:56:59]

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

[commit-bot: I haz the power, 2017-02-17 03:57:01]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[watk, 2017-02-17 20:12:21]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2017-02-17 20:13:20]

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

[watk, 2017-02-17 22:46:43]

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

[watk, 2017-02-17 22:46:44]

+xhwang@: ptal at components/cdm

[commit-bot: I haz the power, 2017-02-17 22:59:53]

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

[commit-bot: I haz the power, 2017-02-17 22:59:54]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xhwang, 2017-02-21 18:48:42]

Sorry for the delay.

For android_mime_type, typically we don't use inner namespaces for constants, so
when I first saw media::android_mime_type::kOpus I got confused. I wonder
whether we can just use a class with static consts instead of the namespace, so
that we can use media::AndroidMimeType::kOpus which seems more readable to me.

But I don't have strong opinion on this. +dalecurtis for comments.

[watk, 2017-02-21 20:00:56]

On 2017/02/21 18:48:42, xhwang_slow wrote:
> Sorry for the delay.
> 
> For android_mime_type, typically we don't use inner namespaces for constants,
so
> when I first saw media::android_mime_type::kOpus I got confused. I wonder
> whether we can just use a class with static consts instead of the namespace,
so
> that we can use media::AndroidMimeType::kOpus which seems more readable to me.
> 
> But I don't have strong opinion on this. +dalecurtis for comments.

Looks like the style guide has no guidance on constants specifically. For
nonmember functions it says to prefer namespaces over classes
(https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...)

I also don't have a strong opinion so you have the power Dale!

[watk, 2017-02-21 20:02:08]

On 2017/02/21 20:00:56, watk wrote:
> On 2017/02/21 18:48:42, xhwang_slow wrote:
> > Sorry for the delay.
> > 
> > For android_mime_type, typically we don't use inner namespaces for
constants,
> so
> > when I first saw media::android_mime_type::kOpus I got confused. I wonder
> > whether we can just use a class with static consts instead of the namespace,
> so
> > that we can use media::AndroidMimeType::kOpus which seems more readable to
me.
> > 
> > But I don't have strong opinion on this. +dalecurtis for comments.
> 
> Looks like the style guide has no guidance on constants specifically. For
> nonmember functions it says to prefer namespaces over classes
>
(https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...)
> 
> I also don't have a strong opinion so you have the power Dale!

Oh, I checked, and there is precedent for this too, e.g.,
cc::switches::kSwitchName.

[DaleCurtis, 2017-02-21 20:25:49]

https://codereview.chromium.org/2697643003/diff/220001/components/cdm/browser...
File components/cdm/browser/cdm_message_filter_android.cc (right):

https://codereview.chromium.org/2697643003/diff/220001/components/cdm/browser...
components/cdm/browser/cdm_message_filter_android.cc:14: #include
"ipc/ipc_message_macros.h"
Hmm, can't we delete this class now? I.e. we're using mojo cdm everywhere. If so
it seems we wouldn't need to expose the android mime types externally at all?

[DaleCurtis, 2017-02-21 20:27:42]

https://codereview.chromium.org/2697643003/diff/220001/components/cdm/browser...
File components/cdm/browser/cdm_message_filter_android.cc (right):

https://codereview.chromium.org/2697643003/diff/220001/components/cdm/browser...
components/cdm/browser/cdm_message_filter_android.cc:14: #include
"ipc/ipc_message_macros.h"
On 2017/02/21 at 20:25:49, DaleCurtis wrote:
> Hmm, can't we delete this class now? I.e. we're using mojo cdm everywhere. If
so it seems we wouldn't need to expose the android mime types externally at all?

Even if not, I would just use the media:: variants of the codec values here and
translate internally in MediaCodecUtil. So we don't need to expose them outside
of MediaCodecUtil.

[watk, 2017-02-21 20:31:27]

On 2017/02/21 20:27:42, DaleCurtis wrote:
>
https://codereview.chromium.org/2697643003/diff/220001/components/cdm/browser...
> File components/cdm/browser/cdm_message_filter_android.cc (right):
> 
>
https://codereview.chromium.org/2697643003/diff/220001/components/cdm/browser...
> components/cdm/browser/cdm_message_filter_android.cc:14: #include
> "ipc/ipc_message_macros.h"
> On 2017/02/21 at 20:25:49, DaleCurtis wrote:
> > Hmm, can't we delete this class now? I.e. we're using mojo cdm everywhere.
If
> so it seems we wouldn't need to expose the android mime types externally at
all?
> 
> Even if not, I would just use the media:: variants of the codec values here
and
> translate internally in MediaCodecUtil. So we don't need to expose them
outside
> of MediaCodecUtil.

There is no other list of mime type constants. And we can't directly use the
codec enums because the audio and video enums are different types. I could make
two lists and overload CanDecode for audio/video if you'd prefer. 

In any case we need the mime type constants, so exporting them from
MediaCodecUtil doesn't cost us anything so I see no problem with this solution,
except for the namespace question :)

[xhwang, 2017-02-21 20:38:59]

https://codereview.chromium.org/2697643003/diff/220001/components/cdm/browser...
File components/cdm/browser/cdm_message_filter_android.cc (right):

https://codereview.chromium.org/2697643003/diff/220001/components/cdm/browser...
components/cdm/browser/cdm_message_filter_android.cc:14: #include
"ipc/ipc_message_macros.h"
On 2017/02/21 20:27:42, DaleCurtis wrote:
> On 2017/02/21 at 20:25:49, DaleCurtis wrote:
> > Hmm, can't we delete this class now? I.e. we're using mojo cdm everywhere.
If
> so it seems we wouldn't need to expose the android mime types externally at
all?
> 
> Even if not, I would just use the media:: variants of the codec values here
and
> translate internally in MediaCodecUtil. So we don't need to expose them
outside
> of MediaCodecUtil.

The key system capability query is still using old IPC. It's independent of the
mojo CDM work. We had discussions on switching to mojo but this is a low
priority.

[watk, 2017-02-21 21:42:54]

Patchset #11 (id:240001) has been deleted

[watk, 2017-02-21 21:43:55]

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

[commit-bot: I haz the power, 2017-02-21 21:45:05]

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

[watk, 2017-02-21 21:46:22]

Talked offline: Dale didn't want to export the mime types to prevent misuse of
them. So we've avoided the namespace issue. PTAL

[DaleCurtis, 2017-02-21 21:50:42]

lgtm, thanks!

https://codereview.chromium.org/2697643003/diff/260001/components/cdm/browser...
File components/cdm/browser/cdm_message_filter_android.cc (right):

https://codereview.chromium.org/2697643003/diff/260001/components/cdm/browser...
components/cdm/browser/cdm_message_filter_android.cc:64: bool is_secure =
!video_must_be_compositable;
Question for a followup: Do we still need this parameter?

[xhwang, 2017-02-21 21:53:23]

lgtm

https://codereview.chromium.org/2697643003/diff/260001/components/cdm/browser...
File components/cdm/browser/cdm_message_filter_android.cc (right):

https://codereview.chromium.org/2697643003/diff/260001/components/cdm/browser...
components/cdm/browser/cdm_message_filter_android.cc:64: bool is_secure =
!video_must_be_compositable;
On 2017/02/21 21:50:42, DaleCurtis wrote:
> Question for a followup: Do we still need this parameter?

This whole stack need some rethought and refactoring. I plan to do this before
we enable Clank L1.

[watk, 2017-02-21 21:57:29]

The CQ bit was checked by watk@chromium.org

[watk, 2017-02-21 21:57:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/2697643003/#ps260001
(title: "Stop exporting mime types")

[watk, 2017-02-21 21:57:31]

The CQ bit was unchecked by watk@chromium.org

[watk, 2017-02-22 22:54:47]

The CQ bit was checked by watk@chromium.org

[commit-bot: I haz the power, 2017-02-22 22:55:32]

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, 2017-02-22 23:00:18]

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

[commit-bot: I haz the power, 2017-02-22 23:00:19]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[watk, 2017-02-22 23:12:47]

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

[commit-bot: I haz the power, 2017-02-22 23:13: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, 2017-02-22 23:20:00]

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

[commit-bot: I haz the power, 2017-02-22 23:20:02]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[watk, 2017-02-22 23:26:30]

Patchset #12 (id:280001) has been deleted

[watk, 2017-02-22 23:28:15]

The CQ bit was checked by watk@chromium.org

[watk, 2017-02-22 23:28:16]

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

[commit-bot: I haz the power, 2017-02-22 23:29:36]

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, 2017-02-23 01:40:47]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1487806095335810,
"parent_rev": "0e277884ee599a8a09b01c93c97934f3c8c285ac", "commit_rev":
"c71ef8d720f0aea74a3ac4efe15df87670fa3875"}

[commit-bot: I haz the power, 2017-02-23 01:41:24]

Description was changed from

==========
media: Clean up MediaCodecBridge and remove subclasses

Previously we had two subclasses of MediaCodecBridgeImpl,
VideoCodecBridge and AudioCodecBridge, which have become unnecessary.
For simplicity and ease of mocking, now we just have
MediaCodecBridgeImpl.

This CL also includes various cleanups, including:
* MediaCodecBridge factories return unique_ptrs.
* Remove some unused MCB functions.
* Use the java enum generator where possible to sync native and java
  enums.
* Collapsing DEQUEUE_OUTPUT_TRY_AGAIN and DEQUEUE_INPUT_TRY_AGAIN into
  a single TRY_AGAIN (like MediaCodec).
* Remove size params from isAdaptivePlaybackSupported() because they
  weren't used.
* Localizing codec to mime type conversions to MediaCodecUtil.
* Passing mime strings to MediaCodecUtil::CanDecode() instead of an
  ad-hoc codec name string.

BUG=691828
TEST=existing tests
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

==========
media: Clean up MediaCodecBridge and remove subclasses

Previously we had two subclasses of MediaCodecBridgeImpl,
VideoCodecBridge and AudioCodecBridge, which have become unnecessary.
For simplicity and ease of mocking, now we just have
MediaCodecBridgeImpl.

This CL also includes various cleanups, including:
* MediaCodecBridge factories return unique_ptrs.
* Remove some unused MCB functions.
* Use the java enum generator where possible to sync native and java
  enums.
* Collapsing DEQUEUE_OUTPUT_TRY_AGAIN and DEQUEUE_INPUT_TRY_AGAIN into
  a single TRY_AGAIN (like MediaCodec).
* Remove size params from isAdaptivePlaybackSupported() because they
  weren't used.
* Localizing codec to mime type conversions to MediaCodecUtil.
* Passing mime strings to MediaCodecUtil::CanDecode() instead of an
  ad-hoc codec name string.

BUG=691828
TEST=existing tests
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

Review-Url: https://codereview.chromium.org/2697643003
Cr-Commit-Position: refs/heads/master@{#452334}
Committed:
https://chromium.googlesource.com/chromium/src/+/c71ef8d720f0aea74a3ac4efe15d...
==========

[commit-bot: I haz the power, 2017-02-23 01:41:26]

Committed patchset #12 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/c71ef8d720f0aea74a3ac4efe15d...