From: csagan5 <32685696+csagan5@users.noreply.github.com> Date: Tue, 5 May 2020 07:22:20 +0200 Subject: AImageReader CFI crash mitigations Revert "gpu/android: Remove setup for disabling AImageReader." This reverts commit dcd5a39518246eb999f1cc63bf1ec95d93fd5b2f. Revert "Remove flags to enable/disable AImageReader." This reverts commit 463fa0f2e3b9e418bc26e2c8954463f0b0f76634. Restore GPU bug blacklist for AImageReader on ARM and Qualcomm CPUs Restore the AImageReader blacklist for ARM/Qualcomm chipsets which causes crashes on Android 9 and 10 (at different code locations). See discussions at: * https://github.com/bromite/bromite/issues/445 * https://github.com/bromite/bromite/issues/814 * https://github.com/bromite/bromite/issues/1005 License: GPL-3.0-only - https://spdx.org/licenses/GPL-3.0-only.html --- base/android/android_image_reader_compat.cc | 16 ++++++++++++++++ base/android/android_image_reader_compat.h | 14 ++++++++++++++ chrome/browser/flag-metadata.json | 6 +++--- .../AImageReader-CFI-crash-mitigations.inc | 4 ++++ .../AImageReader-CFI-crash-mitigations.inc | 1 + gpu/config/gpu_driver_bug_list.json | 16 ++++++++++++++++ gpu/config/gpu_finch_features.cc | 5 +++++ gpu/config/gpu_finch_features.h | 1 + gpu/config/gpu_util.cc | 8 ++++++++ gpu/config/gpu_workaround_list.txt | 1 + gpu/ipc/service/gpu_init.cc | 8 ++++++++ gpu/ipc/service/stream_texture_android.cc | 11 ++++++++++- 12 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 cromite_flags/media/base/media_switches_cc/AImageReader-CFI-crash-mitigations.inc create mode 100644 cromite_flags/media/base/media_switches_h/AImageReader-CFI-crash-mitigations.inc diff --git a/base/android/android_image_reader_compat.cc b/base/android/android_image_reader_compat.cc --- a/base/android/android_image_reader_compat.cc +++ b/base/android/android_image_reader_compat.cc @@ -7,7 +7,23 @@ namespace base { namespace android { +AndroidImageReader& AndroidImageReader::GetInstance() { + // C++11 static local variable initialization is + // thread-safe. + static AndroidImageReader instance; + return instance; +} + +void AndroidImageReader::DisableSupport() { + disable_support_ = true; +} + +bool AndroidImageReader::IsSupported() { + return !disable_support_; +} + bool EnableAndroidImageReader() { + if (!AndroidImageReader::GetInstance().IsSupported()) return false; // Currently we want to enable AImageReader only for android P+ devices. if (__builtin_available(android 28, *)) { return true; diff --git a/base/android/android_image_reader_compat.h b/base/android/android_image_reader_compat.h --- a/base/android/android_image_reader_compat.h +++ b/base/android/android_image_reader_compat.h @@ -10,6 +10,20 @@ namespace base { namespace android { +class BASE_EXPORT AndroidImageReader { + public: + // Thread safe GetInstance. + static AndroidImageReader& GetInstance(); + + bool IsSupported(); + + // Disable image reader support. + void DisableSupport(); + + private: + bool disable_support_ = false; +}; + // Check if the image reader usage is supported. BASE_EXPORT bool EnableAndroidImageReader(); diff --git a/chrome/browser/flag-metadata.json b/chrome/browser/flag-metadata.json --- a/chrome/browser/flag-metadata.json +++ b/chrome/browser/flag-metadata.json @@ -3046,9 +3046,9 @@ "expiry_milestone": 125 }, { - "name": "enable-image-reader", - "owners": [ "vikassoni@chromium.org", "liberato@chromium.org" ], - "expiry_milestone": 125 + "name": "enable-image-reader", // Bromite: do not expire + "owners": [ "vikassoni@chromium.org", "liberato@chromium.org" ], // flag + "expiry_milestone": -1 }, { "name": "enable-immersive-fullscreen-toolbar", diff --git a/cromite_flags/media/base/media_switches_cc/AImageReader-CFI-crash-mitigations.inc b/cromite_flags/media/base/media_switches_cc/AImageReader-CFI-crash-mitigations.inc new file mode 100644 --- /dev/null +++ b/cromite_flags/media/base/media_switches_cc/AImageReader-CFI-crash-mitigations.inc @@ -0,0 +1,4 @@ +// Enables the Android Image Reader path for Video decoding(for AVDA and MCVD) +CROMITE_FEATURE(kAImageReaderVideoOutput, + "AImageReaderVideoOutput", + base::FEATURE_ENABLED_BY_DEFAULT); diff --git a/cromite_flags/media/base/media_switches_h/AImageReader-CFI-crash-mitigations.inc b/cromite_flags/media/base/media_switches_h/AImageReader-CFI-crash-mitigations.inc new file mode 100644 --- /dev/null +++ b/cromite_flags/media/base/media_switches_h/AImageReader-CFI-crash-mitigations.inc @@ -0,0 +1 @@ +MEDIA_EXPORT BASE_DECLARE_FEATURE(kAImageReaderVideoOutput); diff --git a/gpu/config/gpu_driver_bug_list.json b/gpu/config/gpu_driver_bug_list.json --- a/gpu/config/gpu_driver_bug_list.json +++ b/gpu/config/gpu_driver_bug_list.json @@ -2689,6 +2689,22 @@ "no_downscaled_overlay_promotion" ] }, + { + "id":335, + "cr_bugs": [1051705], + "description": "Disable AImageReader on ARM GPUs", + "os": { + "type": "android", + "version": { + "op": "<", + "value": "10" + } + }, + "gl_vendor": "ARM.*|Qualcomm.*", + "features": [ + "disable_aimagereader" + ] + }, { "id": 381, "cr_bugs": [ diff --git a/gpu/config/gpu_finch_features.cc b/gpu/config/gpu_finch_features.cc --- a/gpu/config/gpu_finch_features.cc +++ b/gpu/config/gpu_finch_features.cc @@ -65,6 +65,11 @@ BASE_FEATURE(kUseGles2ForOopR, base::FEATURE_DISABLED_BY_DEFAULT); #if BUILDFLAG(IS_ANDROID) +// Use android AImageReader when playing videos with MediaPlayer. +CROMITE_FEATURE(kAImageReaderMediaPlayer, + "AImageReaderMediaPlayer", + base::FEATURE_ENABLED_BY_DEFAULT); + // Use android SurfaceControl API for managing display compositor's buffer queue // and using overlays on Android. Also used by webview to disable surface // SurfaceControl. diff --git a/gpu/config/gpu_finch_features.h b/gpu/config/gpu_finch_features.h --- a/gpu/config/gpu_finch_features.h +++ b/gpu/config/gpu_finch_features.h @@ -24,6 +24,7 @@ GPU_EXPORT BASE_DECLARE_FEATURE(kUseGles2ForOopR); // All features in alphabetical order. The features should be documented // alongside the definition of their values in the .cc file. #if BUILDFLAG(IS_ANDROID) +GPU_EXPORT BASE_DECLARE_FEATURE(kAImageReaderMediaPlayer); GPU_EXPORT BASE_DECLARE_FEATURE(kAndroidSurfaceControl); GPU_EXPORT BASE_DECLARE_FEATURE(kWebViewSurfaceControl); GPU_EXPORT BASE_DECLARE_FEATURE(kAImageReader); diff --git a/gpu/config/gpu_util.cc b/gpu/config/gpu_util.cc --- a/gpu/config/gpu_util.cc +++ b/gpu/config/gpu_util.cc @@ -122,6 +122,9 @@ GpuFeatureStatus GetAndroidSurfaceControlFeatureStatus( #if !BUILDFLAG(IS_ANDROID) return kGpuFeatureStatusDisabled; #else + if (blocklisted_features.count(GPU_FEATURE_TYPE_ANDROID_SURFACE_CONTROL)) + return kGpuFeatureStatusBlocklisted; + if (!gpu_preferences.enable_android_surface_control) return kGpuFeatureStatusDisabled; @@ -368,6 +371,11 @@ void AdjustGpuFeatureStatusToWorkarounds(GpuFeatureInfo* gpu_feature_info) { gpu_feature_info->status_values[GPU_FEATURE_TYPE_CANVAS_OOP_RASTERIZATION] = kGpuFeatureStatusBlocklisted; } + + if (gpu_feature_info->IsWorkaroundEnabled(DISABLE_AIMAGEREADER)) { + gpu_feature_info->status_values[GPU_FEATURE_TYPE_ANDROID_SURFACE_CONTROL] = + kGpuFeatureStatusBlocklisted; + } } // Estimates roughly user total disk space by counting in the drives where diff --git a/gpu/config/gpu_workaround_list.txt b/gpu/config/gpu_workaround_list.txt --- a/gpu/config/gpu_workaround_list.txt +++ b/gpu/config/gpu_workaround_list.txt @@ -11,6 +11,7 @@ clamp_texture_base_level_and_max_level clear_pixel_unpack_buffer_before_copyteximage disable_2d_canvas_auto_flush disable_accelerated_av1_decode +disable_aimagereader disable_accelerated_av1_encode disable_accelerated_h264_decode disable_accelerated_h264_encode diff --git a/gpu/ipc/service/gpu_init.cc b/gpu/ipc/service/gpu_init.cc --- a/gpu/ipc/service/gpu_init.cc +++ b/gpu/ipc/service/gpu_init.cc @@ -65,6 +65,7 @@ #if BUILDFLAG(IS_ANDROID) #include "ui/gfx/android/android_surface_control_compat.h" +#include "base/android/android_image_reader_compat.h" #endif #if BUILDFLAG(ENABLE_VULKAN) @@ -662,6 +663,13 @@ bool GpuInit::InitializeAndStartSandbox(base::CommandLine* command_line, } #endif // BUILDFLAG(IS_WIN) +#if BUILDFLAG(IS_ANDROID) + // Disable AImageReader if the workaround is enabled. + if (gpu_feature_info_.IsWorkaroundEnabled(DISABLE_AIMAGEREADER)) { + base::android::AndroidImageReader::GetInstance().DisableSupport(); + } +#endif + if (gpu_feature_info_.status_values[GPU_FEATURE_TYPE_VULKAN] != kGpuFeatureStatusEnabled || !InitializeVulkan()) { diff --git a/gpu/ipc/service/stream_texture_android.cc b/gpu/ipc/service/stream_texture_android.cc --- a/gpu/ipc/service/stream_texture_android.cc +++ b/gpu/ipc/service/stream_texture_android.cc @@ -6,6 +6,7 @@ #include +#include "base/android/android_image_reader_compat.h" #include "base/android/scoped_hardware_buffer_fence_sync.h" #include "base/feature_list.h" #include "base/functional/bind.h" @@ -50,7 +51,15 @@ std::unique_ptr MakeCurrent( } TextureOwner::Mode GetTextureOwnerMode() { - return features::IsAImageReaderEnabled() + const bool a_image_reader_supported = + base::android::AndroidImageReader::GetInstance().IsSupported(); + + // TODO(vikassoni) : Currently we have 2 different flags to enable/disable + // AImageReader - one for MCVD and other for MediaPlayer here. Merge those 2 + // flags into a single flag. Keeping the 2 flags separate for now since finch + // experiment using this flag is in progress. + return a_image_reader_supported && features::IsAImageReaderEnabled() && + base::FeatureList::IsEnabled(features::kAImageReaderMediaPlayer) ? TextureOwner::Mode::kAImageReaderInsecure : TextureOwner::Mode::kSurfaceTextureInsecure; } --