Remove Exynos from MediaCodec blacklist. Limit MediaTek blacklist.

media/base/android/media_codec_util.cc

Index: media/base/android/media_codec_util.cc
diff --git a/media/base/android/media_codec_util.cc b/media/base/android/media_codec_util.cc
index 27a2b5ab00c7077b6057269e298e8304f92c48b0..74375e5470c8926c7bd669b4334ff3ad9f51e448 100644
--- a/media/base/android/media_codec_util.cc
+++ b/media/base/android/media_codec_util.cc
@@ -25,8 +25,7 @@ using base::android::ScopedJavaLocalRef;
 
 namespace media {
 
-// static
-const std::string CodecTypeToAndroidMimeType(const std::string& codec) {
+static std::string CodecTypeToAndroidMimeType(const std::string& codec) {
   // TODO(xhwang): Shall we handle more detailed strings like "mp4a.40.2"?
   if (codec == "avc1")
     return "video/avc";
@@ -45,82 +44,46 @@ const std::string CodecTypeToAndroidMimeType(const std::string& codec) {
   return std::string();
 }
 
-// TODO(qinmin): using a map to help all the conversions in this class.
-const std::string AndroidMimeTypeToCodecType(const std::string& mime) {
-  if (mime == "video/mp4v-es")
-    return "mp4v";
-  if (mime == "video/avc")
-    return "avc1";
-  if (mime == "video/hevc")
-    return "hvc1";
-  if (mime == "video/x-vnd.on2.vp8")
-    return "vp8";
-  if (mime == "video/x-vnd.on2.vp9")
-    return "vp9";
-  if (mime == "audio/mp4a-latm")
-    return "mp4a";
-  if (mime == "audio/mpeg")
-    return "mp3";
-  if (mime == "audio/vorbis")
-    return "vorbis";
-  if (mime == "audio/opus")
-    return "opus";
-  return std::string();
-}
-
-std::string GetDefaultCodecName(const std::string& mime_type,
-                                MediaCodecDirection direction) {
-  if (!MediaCodecUtil::IsMediaCodecAvailable())
-    return std::string();
-
+static std::string GetDefaultCodecName(const std::string& mime_type,
+                                       MediaCodecDirection direction) {
+  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.obj(), direction);
-  return ConvertJavaStringToUTF8(env, j_codec_name.obj());
-}
-
-bool SupportsGetName() {
-  // MediaCodec.getName() is only available on JB MR2 and greater.
-  return base::android::BuildInfo::GetInstance()->sdk_int() >= 18;
-}
 
-// Represents supported codecs on android.
-// TODO(qinmin): Currently the codecs string only contains one codec. Do we
-// need to support codecs separated by comma. (e.g. "vp8" -> "vp8, vp8.0")?
-struct CodecsInfo {
-  std::string codecs;  // E.g. "vp8" or "avc1".
-  std::string name;    // E.g. "OMX.google.vp8.decoder".
-  MediaCodecDirection direction;
-};
-
-// Get a list of supported codecs.
-std::vector<CodecsInfo> GetCodecsInfo() {
-  std::vector<CodecsInfo> codecs_info;
-  if (!MediaCodecUtil::IsMediaCodecAvailable())
-    return codecs_info;
+  // MediaCodec::getName() is only supported on newer Android versions.

[Tima Vaisburd, 2016/03/15 03:26:42]

It seems the whole body of this function belongs in Java side.

[DaleCurtis, 2016/03/15 17:30:37]

You're right. I realize now my cleanup didn't go far enough :) I'll revert these
changes and leave only the functional change. I'll then send you a followup CL
to move this all into Java.

+  if (base::android::BuildInfo::GetInstance()->sdk_int() >= 18) {
+    ScopedJavaLocalRef<jstring> j_mime =
+        ConvertUTF8ToJavaString(env, mime_type);
+    ScopedJavaLocalRef<jstring> j_codec_name =
+        Java_MediaCodecUtil_getDefaultCodecName(env, j_mime.obj(), direction);
+    return ConvertJavaStringToUTF8(env, j_codec_name.obj());
+  }
 
-  JNIEnv* env = AttachCurrentThread();
-  std::string mime_type;
   ScopedJavaLocalRef<jobjectArray> j_codec_info_array =
       Java_MediaCodecUtil_getCodecsInfo(env);
-  jsize len = env->GetArrayLength(j_codec_info_array.obj());
+  const jsize len = env->GetArrayLength(j_codec_info_array.obj());
   for (jsize i = 0; i < len; ++i) {
     ScopedJavaLocalRef<jobject> j_info(
         env, env->GetObjectArrayElement(j_codec_info_array.obj(), i));
+    if (direction != static_cast<MediaCodecDirection>(
+                         Java_CodecInfo_direction(env, j_info.obj()))) {
+      continue;
+    }
+
+    std::string current_mime_type;
     ScopedJavaLocalRef<jstring> j_codec_type =
         Java_CodecInfo_codecType(env, j_info.obj());
-    ConvertJavaStringToUTF8(env, j_codec_type.obj(), &mime_type);
+    ConvertJavaStringToUTF8(env, j_codec_type.obj(), &current_mime_type);
+    if (mime_type != current_mime_type)
+      continue;
+
+    std::string current_codec_name;
     ScopedJavaLocalRef<jstring> j_codec_name =
         Java_CodecInfo_codecName(env, j_info.obj());
-    CodecsInfo info;
-    info.codecs = AndroidMimeTypeToCodecType(mime_type);
-    ConvertJavaStringToUTF8(env, j_codec_name.obj(), &info.name);
-    info.direction = static_cast<MediaCodecDirection>(
-        Java_CodecInfo_direction(env, j_info.obj()));
-    codecs_info.push_back(info);
+    ConvertJavaStringToUTF8(env, j_codec_name.obj(), &current_codec_name);
+    return current_codec_name;
   }
-  return codecs_info;
+
+  return std::string();
 }
 
 // static
@@ -183,40 +146,26 @@ bool MediaCodecUtil::IsKnownUnaccelerated(const std::string& mime_type,
   if (!IsMediaCodecAvailable())
     return true;
 
-  std::string codec_name;
-  if (SupportsGetName()) {
-    codec_name = GetDefaultCodecName(mime_type, direction);
-  } else {
-    std::string codec_type = AndroidMimeTypeToCodecType(mime_type);
-    std::vector<CodecsInfo> codecs_info = GetCodecsInfo();
-    for (size_t i = 0; i < codecs_info.size(); ++i) {
-      if (codecs_info[i].codecs == codec_type &&
-          codecs_info[i].direction == direction) {
-        codec_name = codecs_info[i].name;
-        break;
-      }
-    }
-  }
-  DVLOG(1) << __PRETTY_FUNCTION__ << "Default codec for " << mime_type << " : "
-           << codec_name;
+  std::string codec_name = GetDefaultCodecName(mime_type, direction);
+  DVLOG(1) << __FUNCTION__ << "Default codec for " << mime_type << " : "
+           << codec_name << ", " << direction;
+
+  if (!codec_name.size())
+    return true;
+
   // It would be nice if MediaCodecInfo externalized some notion of
   // HW-acceleration but it doesn't. Android Media guidance is that the
   // "OMX.google" prefix is always used for SW decoders, so that's what we
   // use. "OMX.SEC.*" codec is Samsung software implementation - report it
-  // as unaccelerated as well. Also temporary blacklist Exynos and MediaTek
-  // devices while HW decoder video freezes and distortions are
-  // investigated - http://crbug.com/446974.
-  if (codec_name.length() > 0) {
-    return (base::StartsWith(codec_name, "OMX.google.",
-                             base::CompareCase::SENSITIVE) ||
-            base::StartsWith(codec_name, "OMX.SEC.",
-                             base::CompareCase::SENSITIVE) ||
-            base::StartsWith(codec_name, "OMX.MTK.",
-                             base::CompareCase::SENSITIVE) ||
-            base::StartsWith(codec_name, "OMX.Exynos.",
-                             base::CompareCase::SENSITIVE));
-  }
-  return true;
+  // as unaccelerated as well. MediaTek hardware vp8 is known slower than
+  // the software implementation. http://crbug.com/446974.
+  return base::StartsWith(codec_name, "OMX.google.",
+                          base::CompareCase::SENSITIVE) ||
+         base::StartsWith(codec_name, "OMX.SEC.",
+                          base::CompareCase::SENSITIVE) ||
+         (base::StartsWith(codec_name, "OMX.MTK.",
+                           base::CompareCase::SENSITIVE) &&
+          mime_type == "video/x-vnd.on2.vp8");

[Tima Vaisburd, 2016/03/15 03:26:42]

Limiting the blacklisting to specific codec is great.
If I understand it correctly, this is the only change that alters the
functionality and is also the one that corresponds to the CL description. If
this is the case, indeed, can you keep here just this change and have the
refactoring part as a separate CL?

[DaleCurtis, 2016/03/15 17:30:37]

Kind of. This CL also needs the changes in the AndroidVideo_Encode_Accelerator
to preserve the existing VEA blacklist. I'm happy to pull just those changes out
(since they will likely need merging too).

 }
 
 // static
@@ -246,7 +195,7 @@ bool MediaCodecUtil::RegisterMediaCodecUtil(JNIEnv* env) {
 }
 
 // static
-bool MediaCodecUtil::IsVp8DecoderAvailable() {
+bool MediaCodecUtil::IsVp8CodecAvailable() {

[Tima Vaisburd, 2016/03/15 03:26:42]

How is this renaming justified?
We blacklist some decoders because they cannot work with a surface texture as
advertised by API, I'd expect that encoders won't have this problem / will have
different set of problems?

Do you think it would be reasonable to have separate blacklists for encoders and
decoders, at least at the interface level?

[Tima Vaisburd, 2016/03/15 03:26:42]

A side remark unrelated to this CL: I'd expect something like
IsCodecAvailable(codec) or IsAudio/VideoCodecAvailable(codec). What is so
special about Vp8?

[DaleCurtis, 2016/03/15 17:30:37]

This allows the code in AndroidVideo_Encode_Accelerator to continue to blacklist
VP8 on the devices it was previously blacklisted on. I didn't want to change the
limits of VEA without someone from WebRTC to verify.

When we have another codec which needs blacklisting I think we should change the
method, but prior to that it's generally preferred in Chromium to write the code
as simple as possible. E.g. if that maxim had been applied to
GetDefaultCodecName() we wouldn't have 3 different general purpose functions :)

   if (!IsMediaCodecAvailable())
     return false;