media: Refactor MediaCodec creation requirements

Today we pass in both |requires_secure_codec| and
|require_software_codec| which may cotradict with each other given
secure codecs are all hardware codecs. Add an enum to avoid this issue.

Also fixes a bug where if both |requires_secure_codec| and
|require_software_codec| are true, we would still create a secure (and
hence hardware) codec which may hang the GPU process. In this case, we
should just fail to create the codec.

BUG=459414

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
Change-Id: Icd8a403fe5a63b68cfe4045da7057be64ea9de66
Reviewed-on: https://chromium-review.googlesource.com/530098Reviewed-by: Chris Watkins <watk@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478782}

media/base/android/BUILD.gn

@@ -136,6 +136,7 @@ if (is_android) {
   java_cpp_enum("java_enums") {
     sources = [
       "media_codec_bridge.h",
+      "media_codec_bridge_impl.h",
       "media_codec_direction.h",
     ]
   }

media/base/android/java/src/org/chromium/media/MediaCodecBridge.java

@@ -193,19 +193,18 @@ class MediaCodecBridge {
     }
 
     @CalledByNative
-    private static MediaCodecBridge create(
-            String mime, boolean isSecure, int direction, boolean requireSoftwareCodec) {
+    private static MediaCodecBridge create(String mime, int codecType, int direction) {
         MediaCodecUtil.CodecCreationInfo info = new MediaCodecUtil.CodecCreationInfo();
         try {
             if (direction == MediaCodecDirection.ENCODER) {
                 info = MediaCodecUtil.createEncoder(mime);
             } else {
-                // |isSecure| only applies to video decoders.
-                info = MediaCodecUtil.createDecoder(mime, isSecure, requireSoftwareCodec);
+                // |codecType| only applies to decoders not encoders.
+                info = MediaCodecUtil.createDecoder(mime, codecType);
             }
         } catch (Exception e) {
-            Log.e(TAG, "Failed to create MediaCodec: %s, isSecure: %s, direction: %d",
-                    mime, isSecure, direction, e);
+            Log.e(TAG, "Failed to create MediaCodec: %s, codecType: %d, direction: %d", mime,
+                    codecType, direction, e);
         }
 
         if (info.mediaCodec == null) return null;

media/base/android/java/src/org/chromium/media/MediaCodecUtil.java

@@ -35,7 +35,7 @@ class MediaCodecUtil {
     private static final String TAG = "cr_MediaCodecUtil";
 
     /**
-     * Class to pass parameters from createDecoder()
+     * Information returned by createDecoder()
      */
     public static class CodecCreationInfo {
         public MediaCodec mediaCodec;
@@ -204,7 +204,7 @@ class MediaCodecUtil {
     @CalledByNative
     private static boolean canDecode(String mime, boolean isSecure) {
         // TODO(liberato): Should we insist on software here?
-        CodecCreationInfo info = createDecoder(mime, isSecure, false);
+        CodecCreationInfo info = createDecoder(mime, isSecure ? CodecType.SECURE : CodecType.ANY);
         if (info.mediaCodec == null) return false;
 
         try {
@@ -270,12 +270,10 @@ class MediaCodecUtil {
     /**
      * Creates MediaCodec decoder.
      * @param mime MIME type of the media.
-     * @param secure Whether secure decoder is required.
-     * @param requireSoftwareCodec Whether a software decoder is required.
+     * @param codecType Type of codec to create.
      * @return CodecCreationInfo object
      */
-    static CodecCreationInfo createDecoder(
-            String mime, boolean isSecure, boolean requireSoftwareCodec) {
+    static CodecCreationInfo createDecoder(String mime, int codecType) {
         // Always return a valid CodecCreationInfo, its |mediaCodec| field will be null
         // if we cannot create the codec.
         CodecCreationInfo result = new CodecCreationInfo();
@@ -284,7 +282,10 @@ class MediaCodecUtil {
 
         // Creation of ".secure" codecs sometimes crash instead of throwing exceptions
         // on pre-JBMR2 devices.
-        if (isSecure && Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN_MR2) return result;
+        if (codecType == CodecType.SECURE
+                && Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN_MR2) {
+            return result;
+        }
 
         // Do not create codec for blacklisted devices.
         if (!isDecoderSupportedForDevice(mime)) {
@@ -293,17 +294,21 @@ class MediaCodecUtil {
         }
 
         try {
-            // |isSecure| only applies to video decoders.
-            if (mime.startsWith("video") && isSecure) {
-                String decoderName = getDefaultCodecName(
-                        mime, MediaCodecDirection.DECODER, requireSoftwareCodec);
+            // "SECURE" only applies to video decoders.
+            if (mime.startsWith("video") && codecType == CodecType.SECURE) {
+                // Creating secure codecs is not supported directly on older
+                // versions of Android. Therefore, always get the non-secure
+                // codec name and append ".secure" to get the secure codec name.
+                // TODO(xhwang): Now b/15587335 is fixed, we should have better
+                // API support.
+                String decoderName = getDefaultCodecName(mime, MediaCodecDirection.DECODER, false);
                 if (decoderName.equals("")) return null;
                 if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT) {
-                    // To work around an issue that we cannot get the codec info from the secure
-                    // decoder, create an insecure decoder first so that we can query its codec
-                    // info. http://b/15587335.
-                    // Futhermore, it is impossible to create an insecure decoder if the secure
-                    // one is already created.
+                    // To work around an issue that we cannot get the codec info
+                    // from the secure decoder, create an insecure decoder first
+                    // so that we can query its codec info. http://b/15587335.
+                    // Futhermore, it is impossible to create an insecure
+                    // decoder if the secure one is already created.
                     MediaCodec insecureCodec = MediaCodec.createByCodecName(decoderName);
                     result.supportsAdaptivePlayback =
                             codecSupportsAdaptivePlayback(insecureCodec, mime);
@@ -311,9 +316,9 @@ class MediaCodecUtil {
                 }
                 result.mediaCodec = MediaCodec.createByCodecName(decoderName + ".secure");
             } else {
-                if (requireSoftwareCodec) {
-                    String decoderName = getDefaultCodecName(
-                            mime, MediaCodecDirection.DECODER, requireSoftwareCodec);
+                if (codecType == CodecType.SOFTWARE) {
+                    String decoderName =
+                            getDefaultCodecName(mime, MediaCodecDirection.DECODER, true);
                     result.mediaCodec = MediaCodec.createByCodecName(decoderName);
                 } else if (mime.equals(MediaFormat.MIMETYPE_AUDIO_RAW)) {
                     result.mediaCodec = MediaCodec.createByCodecName("OMX.google.raw.decoder");
@@ -324,8 +329,7 @@ class MediaCodecUtil {
                         codecSupportsAdaptivePlayback(result.mediaCodec, mime);
             }
         } catch (Exception e) {
-            Log.e(TAG, "Failed to create MediaCodec: %s, isSecure: %s, requireSoftwareCodec: %s",
-                    mime, isSecure, requireSoftwareCodec ? "yes" : "no", e);
+            Log.e(TAG, "Failed to create MediaCodec: %s, codecType: %d", mime, codecType, e);
             result.mediaCodec = null;
         }
         return result;

media/base/android/media_codec_bridge_impl.cc

@@ -196,7 +196,7 @@ std::unique_ptr<MediaCodecBridge> MediaCodecBridgeImpl::CreateAudioDecoder(
     return nullptr;
 
   auto bridge = base::WrapUnique(new MediaCodecBridgeImpl(
-      mime, false, MediaCodecDirection::DECODER, false));
+      mime, CodecType::kAny, MediaCodecDirection::DECODER));
   if (bridge->j_bridge_.is_null())
     return nullptr;
 
@@ -233,14 +233,13 @@ std::unique_ptr<MediaCodecBridge> MediaCodecBridgeImpl::CreateAudioDecoder(
 // static
 std::unique_ptr<MediaCodecBridge> MediaCodecBridgeImpl::CreateVideoDecoder(
     VideoCodec codec,
-    bool is_secure,
+    CodecType codec_type,
     const gfx::Size& size,
     jobject surface,
     jobject media_crypto,
     const std::vector<uint8_t>& csd0,
     const std::vector<uint8_t>& csd1,
-    bool allow_adaptive_playback,
-    bool require_software_codec) {
+    bool allow_adaptive_playback) {
   if (!MediaCodecUtil::IsMediaCodecAvailable())
     return nullptr;
 
@@ -248,8 +247,8 @@ std::unique_ptr<MediaCodecBridge> MediaCodecBridgeImpl::CreateVideoDecoder(
   if (mime.empty())
     return nullptr;
 
-  std::unique_ptr<MediaCodecBridgeImpl> bridge(new MediaCodecBridgeImpl(
-      mime, is_secure, MediaCodecDirection::DECODER, require_software_codec));
+  std::unique_ptr<MediaCodecBridgeImpl> bridge(
+      new MediaCodecBridgeImpl(mime, codec_type, MediaCodecDirection::DECODER));
   if (bridge->j_bridge_.is_null())
     return nullptr;
 
@@ -297,7 +296,7 @@ std::unique_ptr<MediaCodecBridge> MediaCodecBridgeImpl::CreateVideoEncoder(
     return nullptr;
 
   std::unique_ptr<MediaCodecBridgeImpl> bridge(new MediaCodecBridgeImpl(
-      mime, false, MediaCodecDirection::ENCODER, false));
+      mime, CodecType::kAny, MediaCodecDirection::ENCODER));
   if (bridge->j_bridge_.is_null())
     return nullptr;
 
@@ -318,15 +317,13 @@ std::unique_ptr<MediaCodecBridge> MediaCodecBridgeImpl::CreateVideoEncoder(
 }
 
 MediaCodecBridgeImpl::MediaCodecBridgeImpl(const std::string& mime,
-                                           bool is_secure,
-                                           MediaCodecDirection direction,
-                                           bool require_software_codec) {
+                                           CodecType codec_type,
+                                           MediaCodecDirection direction) {
   JNIEnv* env = AttachCurrentThread();
   DCHECK(!mime.empty());
   ScopedJavaLocalRef<jstring> j_mime = ConvertUTF8ToJavaString(env, mime);
-  j_bridge_.Reset(Java_MediaCodecBridge_create(env, j_mime, is_secure,
-                                               static_cast<int>(direction),
-                                               require_software_codec));
+  j_bridge_.Reset(Java_MediaCodecBridge_create(
+      env, j_mime, static_cast<int>(codec_type), static_cast<int>(direction)));
 }
 
 MediaCodecBridgeImpl::~MediaCodecBridgeImpl() {

media/base/android/media_codec_bridge_impl.h

@@ -22,6 +22,13 @@
 
 namespace media {
 
+// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.media
+enum class CodecType {
+  kAny,
+  kSecure,    // Note that all secure codecs are HW codecs.
+  kSoftware,  // In some cases hardware codecs could hang the GPU process.
+};
+
 // A bridge to a Java MediaCodec.
 class MEDIA_EXPORT MediaCodecBridgeImpl : public MediaCodecBridge {
  public:
@@ -29,7 +36,7 @@ class MEDIA_EXPORT MediaCodecBridgeImpl : public MediaCodecBridge {
   // nullptr on failure.
   static std::unique_ptr<MediaCodecBridge> CreateVideoDecoder(
       VideoCodec codec,
-      bool is_secure,         // Will be used with encrypted content.
+      CodecType codec_type,
       const gfx::Size& size,  // Output frame size.
       jobject surface,        // Output surface, optional.
       jobject media_crypto,   // MediaCrypto object, optional.
@@ -37,8 +44,7 @@ class MEDIA_EXPORT MediaCodecBridgeImpl : public MediaCodecBridge {
       const std::vector<uint8_t>& csd0,
       const std::vector<uint8_t>& csd1,
       // Should adaptive playback be allowed if supported.
-      bool allow_adaptive_playback = true,
-      bool require_software_codec = false);
+      bool allow_adaptive_playback = true);
 
   // Creates and starts a new MediaCodec configured for encoding. Returns
   // nullptr on failure.
@@ -57,6 +63,8 @@ class MEDIA_EXPORT MediaCodecBridgeImpl : public MediaCodecBridge {
       jobject media_crypto);
 
   ~MediaCodecBridgeImpl() override;
+
+  // MediaCodecBridge implementation.
   void Stop() override;
   MediaCodecStatus Flush() override;
   MediaCodecStatus GetOutputSize(gfx::Size* size) override;
@@ -102,9 +110,8 @@ class MEDIA_EXPORT MediaCodecBridgeImpl : public MediaCodecBridge {
 
  private:
   MediaCodecBridgeImpl(const std::string& mime,
-                       bool is_secure,
-                       MediaCodecDirection direction,
-                       bool require_software_codec);
+                       CodecType codec_type,
+                       MediaCodecDirection direction);
 
   // Calls MediaCodec#start(). Returns whether it was successful.
   bool Start();

media/base/android/media_codec_bridge_impl_unittest.cc

@@ -166,7 +166,7 @@ TEST(MediaCodecBridgeTest, CreateH264Decoder) {
   SKIP_TEST_IF_MEDIA_CODEC_IS_NOT_AVAILABLE();
 
   MediaCodecBridgeImpl::CreateVideoDecoder(
-      kCodecH264, false, gfx::Size(640, 480), nullptr, nullptr,
+      kCodecH264, CodecType::kAny, gfx::Size(640, 480), nullptr, nullptr,
       std::vector<uint8_t>(), std::vector<uint8_t>());
 }
 
@@ -275,7 +275,7 @@ TEST(MediaCodecBridgeTest, PresentationTimestampsDoNotDecrease) {
 
   std::unique_ptr<MediaCodecBridge> media_codec(
       MediaCodecBridgeImpl::CreateVideoDecoder(
-          kCodecVP8, false, gfx::Size(320, 240), nullptr, nullptr,
+          kCodecVP8, CodecType::kAny, gfx::Size(320, 240), nullptr, nullptr,
           std::vector<uint8_t>(), std::vector<uint8_t>()));
   ASSERT_THAT(media_codec, NotNull());
   scoped_refptr<DecoderBuffer> buffer = ReadTestDataFile("vp8-I-frame-320x240");
@@ -303,10 +303,11 @@ TEST(MediaCodecBridgeTest, CreateUnsupportedCodec) {
   EXPECT_THAT(MediaCodecBridgeImpl::CreateAudioDecoder(
                   NewAudioConfig(kUnknownAudioCodec), nullptr),
               IsNull());
-  EXPECT_THAT(MediaCodecBridgeImpl::CreateVideoDecoder(
-                  kUnknownVideoCodec, false, gfx::Size(320, 240), nullptr,
-                  nullptr, std::vector<uint8_t>(), std::vector<uint8_t>()),
-              IsNull());
+  EXPECT_THAT(
+      MediaCodecBridgeImpl::CreateVideoDecoder(
+          kUnknownVideoCodec, CodecType::kAny, gfx::Size(320, 240), nullptr,
+          nullptr, std::vector<uint8_t>(), std::vector<uint8_t>()),
+      IsNull());
 }
 
 }  // namespace media

media/base/android/media_codec_util.cc

@@ -70,13 +70,13 @@ static bool IsSupportedAndroidMimeType(const std::string& mime_type) {
 
 static std::string GetDefaultCodecName(const std::string& mime_type,
                                        MediaCodecDirection direction,
-                                       bool require_software_codec) {
+                                       bool requires_software_codec) {
   DCHECK(MediaCodecUtil::IsMediaCodecAvailable());
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jstring> j_mime = ConvertUTF8ToJavaString(env, mime_type);
   ScopedJavaLocalRef<jstring> j_codec_name =
       Java_MediaCodecUtil_getDefaultCodecName(
-          env, j_mime, static_cast<int>(direction), require_software_codec);
+          env, j_mime, static_cast<int>(direction), requires_software_codec);
   return ConvertJavaStringToUTF8(env, j_codec_name.obj());
 }
 

media/gpu/avda_codec_allocator.cc

@@ -37,7 +37,7 @@ constexpr base::TimeDelta kHungTaskDetectionTimeout =
 // This must be safe to call on any thread. Returns nullptr on failure.
 std::unique_ptr<MediaCodecBridge> CreateMediaCodecInternal(
     scoped_refptr<CodecConfig> codec_config,
-    bool require_software_codec) {
+    bool requires_software_codec) {
   TRACE_EVENT0("media", "CreateMediaCodecInternal");
 
   jobject media_crypto =
@@ -46,17 +46,22 @@ std::unique_ptr<MediaCodecBridge> CreateMediaCodecInternal(
   // |requires_secure_codec| implies that it's an encrypted stream.
   DCHECK(!codec_config->requires_secure_codec || media_crypto);
 
-  // TODO(xhwang): Rename |is_secure| to |requires_secure_codec| in
-  // MediaCodec classes. Also, |requires_secure_codec| and
-  // |require_software_codec| contradicts each other. We should clarify and fix
-  // this.
+  CodecType codec_type = CodecType::kAny;
+  if (codec_config->requires_secure_codec && requires_software_codec) {
+    DVLOG(1) << "Secure software codec doesn't exist.";
+    return nullptr;
+  } else if (codec_config->requires_secure_codec) {
+    codec_type = CodecType::kSecure;
+  } else if (requires_software_codec) {
+    codec_type = CodecType::kSoftware;
+  }
+
   std::unique_ptr<MediaCodecBridge> codec(
       MediaCodecBridgeImpl::CreateVideoDecoder(
-          codec_config->codec, codec_config->requires_secure_codec,
+          codec_config->codec, codec_type,
           codec_config->initial_expected_coded_size,
           codec_config->surface_bundle->GetJavaSurface().obj(), media_crypto,
-          codec_config->csd0, codec_config->csd1, true,
-          require_software_codec));
+          codec_config->csd0, codec_config->csd1, true));
 
   return codec;
 }