Fix a bug that mime type isn't passed when checking Codec capabilities

media/base/android/media_codec_util.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/base/android/media_codec_util.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 
 #include "base/android/build_info.h"
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "jni/MediaCodecUtil_jni.h"
 #include "url/gurl.h"
 
 using base::android::AttachCurrentThread;
 using base::android::ConvertJavaStringToUTF8;
 using base::android::ConvertUTF8ToJavaString;
 using base::android::JavaIntArrayToIntVector;
 using base::android::ScopedJavaLocalRef;
 
+namespace {

[ddorwin, 2016/04/18 22:09:09]

nit: empty line at beginning and end of namespaces.

[qinmin, 2016/04/19 23:27:11]

Done.

+const char kMP4AMimeType[] = "audio/mp4a-latm";

[ddorwin, 2016/04/18 22:09:09]

I think we generally do kMp4a... (The MP4 examples don't support this, but I
believe that would be correct. Vp9 is definitely done more that way.

ditto for Avc, Vp8, Vp9 and the others I commented below.

[qinmin, 2016/04/19 23:27:10]

Done.

+const char kOPUSMimeType[] = "audio/opus";

[ddorwin, 2016/04/18 22:09:09]

Opus

[qinmin, 2016/04/19 23:27:10]

Done.

+const char kVORBISMimeType[] = "audio/vorbis";

[ddorwin, 2016/04/18 22:09:08]

Vorbis

[qinmin, 2016/04/19 23:27:10]

Done.

+const char kAVCMimeType[] = "video/avc";
+const char kHVCMimeType[] = "video/hevc";

[ddorwin, 2016/04/18 22:09:09]

kHevc... I haven't seen HVC used as an acronym.

[qinmin, 2016/04/19 23:27:11]

Done.

+const char kVP8MimeType[] = "video/x-vnd.on2.vp8";
+const char kVP9MimeType[] = "video/x-vnd.on2.vp9";
+}
+
 namespace media {

[ddorwin, 2016/04/18 22:09:09]

nit: Put the anonymous namespace in this one. No impact here, but it's generally
the preferred pattern I believe.

[qinmin, 2016/04/19 23:27:10]

Done.

 
 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";
+    return kAVCMimeType;
   if (codec == "hvc1")
-    return "video/hevc";
+    return kHVCMimeType;
   if (codec == "mp4a")
-    return "audio/mp4a-latm";
+    return kMP4AMimeType;
   if (codec == "vp8" || codec == "vp8.0")
-    return "video/x-vnd.on2.vp8";
+    return kVP8MimeType;
   if (codec == "vp9" || codec == "vp9.0")
-    return "video/x-vnd.on2.vp9";
+    return kVP9MimeType;
   if (codec == "vorbis")
-    return "audio/vorbis";
+    return kVORBISMimeType;
   if (codec == "opus")
-    return "audio/opus";
+    return kOPUSMimeType;
+  DLOG(WARNING) << "Cannot convert codec to Android MIME type: " << codec;
   return std::string();
 }
 
+static bool IsMimeTypeSupported(const std::string& mime_type) {

[ddorwin, 2016/04/18 22:09:09]

|android_mime_type|

[qinmin, 2016/04/19 23:27:10]

Done. However, this function should checks whether the mime_type is an android
supported mime_type, so using mime_type is more proper.

+  std::vector<std::string> supported{

[ddorwin, 2016/04/18 22:09:09]

nit: space before '{' ?

[qinmin, 2016/04/19 23:27:10]

I added the space initially, but "git cl media format" removed that space on
purpose.

+      kMP4AMimeType, kOPUSMimeType, kVORBISMimeType, kAVCMimeType,
+      kHVCMimeType,  kVP8MimeType,  kVP9MimeType};

[ddorwin, 2016/04/18 22:09:09]

Note: Initializer lists are in the process of being allowed [1]. Since this is
Android only, this seems fine.

[1]
https://groups.google.com/a/chromium.org/forum/#!searchin/cxx/Initialization$...

[qinmin, 2016/04/19 23:27:10]

good to know, thanks

+  return std::find(supported.begin(), supported.end(), mime_type) !=

[ddorwin, 2016/04/18 22:09:09]

This is a bit simpler if you use a base::hash_set above, though that might be
more size and complexity in implementation. Up to you.

[qinmin, 2016/04/19 23:27:11]

Since this is only used in DCHECK, i will leave this as it is now.

+         supported.end();
+}
+
 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());
 }
 
-static bool IsDecoderSupportedByDevice(const std::string& mime_type) {
+static bool IsDecoderSupportedByDevice(const std::string& android_mime_type) {
   DCHECK(MediaCodecUtil::IsMediaCodecAvailable());
+  DCHECK(IsMimeTypeSupported(android_mime_type));
   JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jstring> j_mime = ConvertUTF8ToJavaString(env, mime_type);
+  ScopedJavaLocalRef<jstring> j_mime =
+      ConvertUTF8ToJavaString(env, android_mime_type);
   return Java_MediaCodecUtil_isDecoderSupportedForDevice(env, j_mime.obj());
 }
 
 // static
 bool MediaCodecUtil::IsMediaCodecAvailable() {
   // MediaCodec is only available on JB and greater.
   if (base::android::BuildInfo::GetInstance()->sdk_int() < 16)
     return false;
   // Blacklist some devices on Jellybean as for MediaCodec support is buggy.
   // http://crbug.com/365494.
@@ skipping 38 matching lines @@
 
   JNIEnv* env = AttachCurrentThread();
   std::string mime = CodecTypeToAndroidMimeType(codec);
   if (mime.empty())
     return false;
   ScopedJavaLocalRef<jstring> j_mime = ConvertUTF8ToJavaString(env, mime);
   return Java_MediaCodecUtil_canDecode(env, j_mime.obj(), is_secure);
 }
 
 // static
-bool MediaCodecUtil::IsKnownUnaccelerated(const std::string& mime_type,
+bool MediaCodecUtil::IsKnownUnaccelerated(const std::string& android_mime_type,
                                           MediaCodecDirection direction) {
   if (!IsMediaCodecAvailable())
     return true;
 
-  std::string codec_name = GetDefaultCodecName(mime_type, direction);
-  DVLOG(1) << __FUNCTION__ << "Default codec for " << mime_type << " : "
+  DCHECK(IsMimeTypeSupported(android_mime_type));

[ddorwin, 2016/04/18 22:09:09]

We can still do debug checks of input even regardless of 142. IOW, make this the
first line.

[qinmin, 2016/04/19 23:27:11]

Done.

+  std::string codec_name = GetDefaultCodecName(android_mime_type, direction);
+  DVLOG(1) << __FUNCTION__ << "Default codec for " << android_mime_type << " : "
            << codec_name << ", direction: " << direction;
   if (!codec_name.size())
     return true;
 
   // MediaTek hardware vp8 is known slower than the software implementation.
   // MediaTek hardware vp9 is known crashy, see http://crbug.com/446974 and
   // http://crbug.com/597836.
   if (base::StartsWith(codec_name, "OMX.MTK.", base::CompareCase::SENSITIVE)) {
-    if (mime_type == "video/x-vnd.on2.vp8")
+    if (android_mime_type.compare(kVP8MimeType) == 0)

[ddorwin, 2016/04/18 22:09:09]

Is there a reason you changed from just using ==?

[qinmin, 2016/04/19 23:27:11]

They are essentially the same, personal preference thing. Anyway, reverted to
use ==.

       return true;
 
-    if (mime_type == "video/x-vnd.on2.vp9")
+    if (android_mime_type.compare(kVP9MimeType) == 0)
       return base::android::BuildInfo::GetInstance()->sdk_int() < 21;
 
     return false;
   }
 
   // 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.
@@ skipping 23 matching lines @@
   return (spec.find("m3u8") != std::string::npos);
 }
 
 // static
 bool MediaCodecUtil::RegisterMediaCodecUtil(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 // static
 bool MediaCodecUtil::IsVp8DecoderAvailable() {
-  return IsMediaCodecAvailable() && IsDecoderSupportedByDevice("vp8");
+  return IsMediaCodecAvailable() && IsDecoderSupportedByDevice(kVP8MimeType);
 }
 
 // static
 bool MediaCodecUtil::IsVp8EncoderAvailable() {
   // Currently the vp8 encoder and decoder blacklists cover the same devices,
   // but we have a second method for clarity in future issues.
   return IsVp8DecoderAvailable();
 }
 
 // static
 bool MediaCodecUtil::IsVp9DecoderAvailable() {
-  return IsMediaCodecAvailable() && IsDecoderSupportedByDevice("vp9");
+  return IsMediaCodecAvailable() && IsDecoderSupportedByDevice(kVP9MimeType);
 }
 
 // static
 bool MediaCodecUtil::IsSurfaceViewOutputSupported() {
   // Disable SurfaceView output for the Samsung Galaxy S3; it does not work
   // well enough for even 360p24 H264 playback.  http://crbug.com/602870.
   //
   // Notably this is codec agnostic at present, so any devices added to
   // the blacklist will avoid trying to play any codecs on SurfaceView.  If
   // needed in the future this can be expanded to be codec specific.
   return !base::StartsWith(base::android::BuildInfo::GetInstance()->model(),
                            "GT-I9300", base::CompareCase::INSENSITIVE_ASCII);
 }
 
 }  // namespace media