Limit AImageReader max size to 1.

1. Limit AImageReader max size to 1 based on the model name of the device.
Model name of the device is sent from server side using the finch
experiment.

2. There will be an equivalent critique CL to add a list of known
devices which can only support max size of 1. This CL will be checked
in first.

3. Move the finch experiment from media/ to gpu/ to avoid dependency
issues.

4. Add error logs to print device model name for the devices which fails
creating image reader with max size > 1. This is required so that the
model name can then be added to the finch experiment.

Bug: 1051705
Change-Id: Ie4809da3591b0535ca129642f248bf9534e37874
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2578385
Commit-Queue: vikas soni <vikassoni@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836751}

base/android/android_image_reader_compat.cc

@@ -34,14 +34,6 @@ bool AndroidImageReader::IsSupported() {
   return is_supported_;
 }
 
-// static
-bool AndroidImageReader::LimitAImageReaderMaxSizeToOne() {
-  // Using MIBOX for both MiBox 4k and MiBox S 4k devices.
-  constexpr char kDisabledModel[] = "MIBOX";
-  return StartsWith(BuildInfo::GetInstance()->model(), kDisabledModel,
-                    CompareCase::INSENSITIVE_ASCII);
-}
-
 AndroidImageReader::AndroidImageReader() : is_supported_(LoadFunctions()) {}
 
 bool AndroidImageReader::LoadFunctions() {

base/android/android_image_reader_compat.h

@@ -27,11 +27,6 @@ class BASE_EXPORT AndroidImageReader {
   // the required functions are loaded.
   bool IsSupported();
 
-  // Some devices do not support more than 1 image to be acquired from the
-  // AImageReader.(crbug.com/1051705). This method returns true for those
-  // devices.
-  static bool LimitAImageReaderMaxSizeToOne();
-
   // Naming convention of all the below functions are chosen to exactly match
   // the function names in the NDK.
   void AImage_delete(AImage* image);

gpu/command_buffer/service/image_reader_gl_owner.cc

@@ -10,8 +10,10 @@
 
 #include "base/android/android_hardware_buffer_compat.h"
 #include "base/android/android_image_reader_compat.h"
+#include "base/android/build_info.h"
 #include "base/android/jni_android.h"
 #include "base/android/scoped_hardware_buffer_fence_sync.h"
+#include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_functions.h"
@@ -20,6 +22,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "gpu/command_buffer/service/abstract_texture.h"
+#include "gpu/config/gpu_finch_features.h"
 #include "gpu/ipc/common/android/android_image_reader_utils.h"
 #include "ui/gfx/android/android_surface_control_compat.h"
 #include "ui/gl/gl_fence_android_native_fence_sync.h"
@@ -62,11 +65,10 @@ bool IsSurfaceControl(TextureOwner::Mode mode) {
 uint32_t NumRequiredMaxImages(TextureOwner::Mode mode) {
   if (IsSurfaceControl(mode) ||
       mode == TextureOwner::Mode::kAImageReaderInsecureMultithreaded) {
-    DCHECK(!base::android::AndroidImageReader::LimitAImageReaderMaxSizeToOne());
+    DCHECK(!features::LimitAImageReaderMaxSizeToOne());
     return 3;
   }
-  return base::android::AndroidImageReader::LimitAImageReaderMaxSizeToOne() ? 1
-                                                                            : 2;
+  return features::LimitAImageReaderMaxSizeToOne() ? 1 : 2;
 }
 
 }  // namespace
@@ -152,12 +154,16 @@ ImageReaderGLOwner::ImageReaderGLOwner(
   media_status_t return_code = loader_.AImageReader_newWithUsage(
       width, height, format, usage, max_images_, &reader);
   if (return_code != AMEDIA_OK) {
-    LOG(ERROR) << " Image reader creation failed.";
-    if (return_code == AMEDIA_ERROR_INVALID_PARAMETER)
+    LOG(ERROR) << " Image reader creation failed on device model : "
+               << base::android::BuildInfo::GetInstance()->model()
+               << ". maxImages used is : " << max_images_;
+    base::debug::DumpWithoutCrashing();
+    if (return_code == AMEDIA_ERROR_INVALID_PARAMETER) {
       LOG(ERROR) << "Either reader is null, or one or more of width, height, "
                     "format, maxImages arguments is not supported";
-    else
+    } else {
       LOG(ERROR) << "unknown error";
+    }
     return;
   }
   DCHECK(reader);

gpu/command_buffer/service/image_reader_gl_owner_unittest.cc

@@ -13,6 +13,7 @@
 #include "gpu/command_buffer/service/abstract_texture.h"
 #include "gpu/command_buffer/service/image_reader_gl_owner.h"
 #include "gpu/command_buffer/service/mock_abstract_texture.h"
+#include "gpu/config/gpu_finch_features.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_context_egl.h"
@@ -147,11 +148,10 @@ TEST_F(ImageReaderGLOwnerTest, DestructionWorksWithWrongContext) {
 TEST_F(ImageReaderGLOwnerTest, MaxImageExpectation) {
   if (!IsImageReaderSupported())
     return;
+
   EXPECT_EQ(static_cast<ImageReaderGLOwner*>(image_reader_.get())
                 ->max_images_for_testing(),
-            base::android::AndroidImageReader::LimitAImageReaderMaxSizeToOne()
-                ? 1
-                : 2);
+            features::LimitAImageReaderMaxSizeToOne() ? 1 : 2);
 }
 
 class ImageReaderGLOwnerSecureSurfaceControlTest

gpu/config/gpu_finch_features.cc

@@ -13,10 +13,28 @@
 #include "base/android/build_info.h"
 #include "base/metrics/field_trial_params.h"
 #include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
 #include "ui/gfx/android/android_surface_control_compat.h"
 #endif
 
 namespace features {
+namespace {
+
+#if defined(OS_ANDROID)
+bool FieldIsInBlocklist(const char* current_value, std::string blocklist_str) {
+  std::vector<std::string> blocklist = base::SplitString(
+      blocklist_str, ",", base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+  for (const std::string& blocklisted_value : blocklist) {
+    if (base::StartsWith(current_value, blocklisted_value,
+                         base::CompareCase::INSENSITIVE_ASCII)) {
+      return true;
+    }
+  }
+  return false;
+}
+#endif
+
+}  // namespace
 
 #if defined(OS_ANDROID)
 // Used to limit GL version to 2.0 for skia raster on Android.
@@ -37,6 +55,24 @@ const base::Feature kAImageReader{"AImageReader",
 // raster.
 const base::Feature kWebViewVulkan{"WebViewVulkan",
                                    base::FEATURE_ENABLED_BY_DEFAULT};
+
+// Used to enable/disable zero copy video path on webview for MCVD.
+const base::Feature kWebViewZeroCopyVideo{"WebViewZeroCopyVideo",
+                                          base::FEATURE_DISABLED_BY_DEFAULT};
+
+// List of devices on which WebViewZeroCopyVideo should be disabled.
+const base::FeatureParam<std::string> kWebViewZeroCopyVideoBlocklist{
+    &kWebViewZeroCopyVideo, "WebViewZeroCopyVideoBlocklist", ""};
+
+// Used to limit AImageReader max queue size to 1 since many devices especially
+// android Tv devices do not support more than 1 images.
+const base::Feature kLimitAImageReaderMaxSizeToOne{
+    "LimitAImageReaderMaxSizeToOne", base::FEATURE_ENABLED_BY_DEFAULT};
+
+// List of devices on which to limit AImageReader max queue size to 1.
+const base::FeatureParam<std::string> kLimitAImageReaderMaxSizeToOneBlocklist{
+    &kLimitAImageReaderMaxSizeToOne, "LimitAImageReaderMaxSizeToOneBlocklist",
+    "MIBOX"};
 #endif
 
 // Enable GPU Rasterization by default. This can still be overridden by
@@ -148,6 +184,31 @@ bool IsAndroidSurfaceControlEnabled() {
          base::FeatureList::IsEnabled(kAndroidSurfaceControl) &&
          gfx::SurfaceControl::IsSupported();
 }
+
+// Many devices do not support more than 1 image to be acquired from the
+// AImageReader.(crbug.com/1051705). This method returns true for those
+// devices. Currently the list of device model names are sent from server side
+// via a finch config file. There is a known device MIBOX for which max size
+// should be 1 irrespecticve of the feature LimitAImageReaderMaxSizeToOne
+// enabled or not. Get() returns default value even if the feature is disabled.
+bool LimitAImageReaderMaxSizeToOne() {
+  return (FieldIsInBlocklist(base::android::BuildInfo::GetInstance()->model(),
+                             kLimitAImageReaderMaxSizeToOneBlocklist.Get()));
+}
+
+// Zero copy is disabled if device can not support 3 max images.
+bool IsWebViewZeroCopyVideoEnabled() {
+  const bool limit_max_size_to_one = LimitAImageReaderMaxSizeToOne();
+  if (!IsAImageReaderEnabled() || limit_max_size_to_one)
+    return false;
+
+  if (!base::FeatureList::IsEnabled(kWebViewZeroCopyVideo))
+    return false;
+
+  return !(FieldIsInBlocklist(base::android::BuildInfo::GetInstance()->model(),
+                              kWebViewZeroCopyVideoBlocklist.Get()));
+}
+
 #endif
 
 }  // namespace features

gpu/config/gpu_finch_features.h

@@ -21,6 +21,8 @@ GPU_EXPORT extern const base::Feature kUseGles2ForOopR;
 GPU_EXPORT extern const base::Feature kAndroidSurfaceControl;
 GPU_EXPORT extern const base::Feature kAImageReader;
 GPU_EXPORT extern const base::Feature kWebViewVulkan;
+GPU_EXPORT extern const base::Feature kLimitAImageReaderMaxSizeToOne;
+GPU_EXPORT extern const base::Feature kWebViewZeroCopyVideo;
 #endif  // defined(OS_ANDROID)
 
 GPU_EXPORT extern const base::Feature kDefaultEnableGpuRasterization;
@@ -59,6 +61,8 @@ GPU_EXPORT bool IsUsingVulkan();
 #if defined(OS_ANDROID)
 GPU_EXPORT bool IsAImageReaderEnabled();
 GPU_EXPORT bool IsAndroidSurfaceControlEnabled();
+GPU_EXPORT bool LimitAImageReaderMaxSizeToOne();
+GPU_EXPORT bool IsWebViewZeroCopyVideoEnabled();
 #endif
 
 }  // namespace features

media/base/media_switches.cc

@@ -638,10 +638,6 @@ const base::Feature kUseAudioLatencyFromHAL{"UseAudioLatencyFromHAL",
 // the GPU main thread during VideoFrame construction.
 const base::Feature kUsePooledSharedImageVideoProvider{
     "UsePooledSharedImageVideoProvider", base::FEATURE_ENABLED_BY_DEFAULT};
-
-// Used to enable/disable zero copy video path on webview for MCVD.
-const base::Feature kWebViewZeroCopyVideo{"WebViewZeroCopyVideo",
-                                          base::FEATURE_DISABLED_BY_DEFAULT};
 #endif  // defined(OS_ANDROID)
 
 #if BUILDFLAG(IS_ASH) && BUILDFLAG(USE_CHROMEOS_MEDIA_ACCELERATION)

media/base/media_switches.h

@@ -212,7 +212,6 @@ MEDIA_EXPORT extern const base::Feature kHlsPlayer;
 MEDIA_EXPORT extern const base::Feature kRequestSystemAudioFocus;
 MEDIA_EXPORT extern const base::Feature kUseAudioLatencyFromHAL;
 MEDIA_EXPORT extern const base::Feature kUsePooledSharedImageVideoProvider;
-MEDIA_EXPORT extern const base::Feature kWebViewZeroCopyVideo;
 #endif  // defined(OS_ANDROID)
 
 #if BUILDFLAG(IS_ASH) && BUILDFLAG(USE_CHROMEOS_MEDIA_ACCELERATION)

media/gpu/android/video_frame_factory_impl.cc

@@ -43,22 +43,16 @@ base::Optional<VideoFrameMetadata::CopyMode> GetVideoFrameCopyMode(
   if (!enable_threaded_texture_mailboxes)
     return base::nullopt;
 
-  if (features::IsAImageReaderEnabled() &&
-      base::FeatureList::IsEnabled(media::kWebViewZeroCopyVideo) &&
-      !base::android::AndroidImageReader::LimitAImageReaderMaxSizeToOne()) {
-    return VideoFrameMetadata::CopyMode::kCopyMailboxesOnly;
-  } else {
-    return VideoFrameMetadata::CopyMode::kCopyToNewTexture;
-  }
+  return features::IsWebViewZeroCopyVideoEnabled()
+             ? VideoFrameMetadata::CopyMode::kCopyMailboxesOnly
+             : VideoFrameMetadata::CopyMode::kCopyToNewTexture;
 }
 
 gpu::TextureOwner::Mode GetTextureOwnerMode(
     VideoFrameFactory::OverlayMode overlay_mode,
     const base::Optional<VideoFrameMetadata::CopyMode>& copy_mode) {
   if (copy_mode == VideoFrameMetadata::kCopyMailboxesOnly) {
-    DCHECK(features::IsAImageReaderEnabled() &&
-           base::FeatureList::IsEnabled(media::kWebViewZeroCopyVideo) &&
-           !base::android::AndroidImageReader::LimitAImageReaderMaxSizeToOne());
+    DCHECK(features::IsWebViewZeroCopyVideoEnabled());
     return gpu::TextureOwner::Mode::kAImageReaderInsecureMultithreaded;
   }