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>
 
@@ skipping 23 matching lines @@
   if (codec == "mp4a")
     return "audio/mp4a-latm";
   if (codec == "vp8" || codec == "vp8.0")
     return "video/x-vnd.on2.vp8";
   if (codec == "vp9" || codec == "vp9.0")
     return "video/x-vnd.on2.vp9";
   if (codec == "vorbis")
     return "audio/vorbis";
   if (codec == "opus")
     return "audio/opus";
   return std::string();

[ddorwin, 2016/04/18 20:30:44]

Should there be a NOTREACHED() here or at least a DLOG(WARNING)?

[qinmin, 2016/04/18 21:32:53]

Done.

 }
 
 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) {

[ddorwin, 2016/04/18 20:30:44]

Would it be safer to have this function (or even the Java code) handle such
conversion? I guess we want devs to use |mime_type| consistently and know what
they are doing, though.
Perhaps this and similar functions should DCHECK() on a funciton that checks
that the value is one of those known in CodecTypeToAndroidMimeType().
It might also help to rename the variable to |android_mime_type| and perhaps
reinforce this in a comment.

[qinmin, 2016/04/18 21:32:54]

Done.

   DCHECK(MediaCodecUtil::IsMediaCodecAvailable());
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jstring> j_mime = ConvertUTF8ToJavaString(env, 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)
@@ skipping 41 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,

[ddorwin, 2016/04/18 20:30:44]

|android_mime_type|?

[qinmin, 2016/04/18 21:32:54]

Done.

                                           MediaCodecDirection direction) {
   if (!IsMediaCodecAvailable())
     return true;
 
   std::string codec_name = GetDefaultCodecName(mime_type, direction);
   DVLOG(1) << __FUNCTION__ << "Default codec for " << 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")

[ddorwin, 2016/04/18 20:30:44]

Should we make these constants. This one appears twice (and they could appear
more if my suggestion above is followed). Also, it might be nice to have all
such strings in one place.

[qinmin, 2016/04/18 21:32:54]

Done.

       return true;
 
     if (mime_type == "video/x-vnd.on2.vp9")
       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
@@ skipping 26 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() &&

[Tima Vaisburd, 2016/04/19 01:19:18]

I would have just replaced "vp8" and "vp9" down there with proper constants
"video/x-vnd.on2.vp8" and "video/x-vnd.on2.vp9".

lgtm

+         IsDecoderSupportedByDevice(CodecTypeToAndroidMimeType("vp8"));
 }
 
 // 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(CodecTypeToAndroidMimeType("vp9"));
 }
 
 // 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