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 Change-Id: I0db87f4107c94f68e846f06f365b8eefd0076599 --- base/android/android_image_reader_compat.cc | 8 +++++++- base/android/android_image_reader_compat.h | 4 ++++ chrome/browser/flag-metadata.json | 6 +++--- 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 | 5 +++++ gpu/ipc/service/stream_texture_android.cc | 11 ++++++++++- media/base/media_switches.cc | 5 +++++ media/base/media_switches.h | 1 + 12 files changed, 66 insertions(+), 5 deletions(-) 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 @@ -23,6 +23,8 @@ namespace base { namespace android { +bool AndroidImageReader::disable_support_ = false; + AndroidImageReader& AndroidImageReader::GetInstance() { // C++11 static local variable initialization is // thread-safe. @@ -30,8 +32,12 @@ AndroidImageReader& AndroidImageReader::GetInstance() { return instance; } +void AndroidImageReader::DisableSupport() { + disable_support_ = true; +} + bool AndroidImageReader::IsSupported() { - return is_supported_; + return !disable_support_ && is_supported_; } AndroidImageReader::AndroidImageReader() : is_supported_(LoadFunctions()) {} 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 @@ -24,6 +24,9 @@ class BASE_EXPORT AndroidImageReader { AndroidImageReader(const AndroidImageReader&) = delete; AndroidImageReader& operator=(const AndroidImageReader&) = delete; + // Disable image reader support. + static void DisableSupport(); + // Check if the image reader usage is supported. This function returns TRUE // if android version is >=OREO, image reader support is not disabled and all // the required functions are loaded. @@ -61,6 +64,7 @@ class BASE_EXPORT AndroidImageReader { jobject ANativeWindow_toSurface(JNIEnv* env, ANativeWindow* window); private: + static bool disable_support_; friend class base::NoDestructor; AndroidImageReader(); 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 @@ -2604,9 +2604,9 @@ "expiry_milestone": 115 }, { - "name": "enable-image-reader", - "owners": [ "vikassoni", "liberato" ], - "expiry_milestone": 115 + "name": "enable-image-reader", // Bromite: do not expire + "owners": [ "vikassoni", "liberato" ], // flag + "expiry_milestone": -1 }, { "name": "enable-immersive-fullscreen-toolbar", 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 @@ -3142,6 +3142,22 @@ "dont_delete_source_texture_for_egl_image" ] }, + { + "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": 336, "cr_bugs": [625785], 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, #endif ); + +// Use android AImageReader when playing videos with MediaPlayer. +const base::Feature kAImageReaderMediaPlayer{"AImageReaderMediaPlayer", + base::FEATURE_ENABLED_BY_DEFAULT}; + #if BUILDFLAG(IS_ANDROID) // Use android SurfaceControl API for managing display compositor's buffer queue // and using overlays on Android. Also used by webview to disable surface 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 @@ -17,6 +17,7 @@ namespace features { GPU_EXPORT BASE_DECLARE_FEATURE(kUseGles2ForOopR); // All features in alphabetical order. The features should be documented +GPU_EXPORT extern const base::Feature kAImageReaderMediaPlayer; // alongside the definition of their values in the .cc file. #if BUILDFLAG(IS_ANDROID) GPU_EXPORT BASE_DECLARE_FEATURE(kAndroidSurfaceControl); 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 @@ -121,6 +121,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; @@ -342,6 +345,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 @@ -15,6 +15,7 @@ decode_encode_srgb_for_generatemipmap depth_stencil_renderbuffer_resize_emulation disable_2d_canvas_auto_flush disable_accelerated_av1_decode +disable_aimagereader disable_accelerated_av1_encode disable_accelerated_h264_encode disable_accelerated_hevc_decode 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 @@ -616,6 +616,11 @@ bool GpuInit::InitializeAndStartSandbox(base::CommandLine* command_line, } #endif // BUILDFLAG(IS_WIN) + // Disable AImageReader if the workaround is enabled. + if (gpu_feature_info_.IsWorkaroundEnabled(DISABLE_AIMAGEREADER)) { + base::android::AndroidImageReader::DisableSupport(); + } + 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; } diff --git a/media/base/media_switches.cc b/media/base/media_switches.cc --- a/media/base/media_switches.cc +++ b/media/base/media_switches.cc @@ -885,6 +885,11 @@ BASE_FEATURE(kHardwareSecureDecryptionExperiment, // Allows automatically disabling hardware secure Content Decryption Module // (CDM) after failures or crashes to fallback to software secure CDMs. If this // feature is disabled, the fallback will never happen and users could be stuck +// Enables the Android Image Reader path for Video decoding(for AVDA and MCVD) +BASE_FEATURE(kAImageReaderVideoOutput, + "AImageReaderVideoOutput", + base::FEATURE_ENABLED_BY_DEFAULT); + // in playback failures. BASE_FEATURE(kHardwareSecureDecryptionFallback, "HardwareSecureDecryptionFallback", diff --git a/media/base/media_switches.h b/media/base/media_switches.h --- a/media/base/media_switches.h +++ b/media/base/media_switches.h @@ -297,6 +297,7 @@ MEDIA_EXPORT BASE_DECLARE_FEATURE(kVaapiVp8TemporalLayerHWEncoding); MEDIA_EXPORT BASE_DECLARE_FEATURE(kVaapiVp9kSVCHWEncoding); #endif // defined(ARCH_CPU_X86_FAMILY) && BUILDFLAG(IS_CHROMEOS) MEDIA_EXPORT BASE_DECLARE_FEATURE(kVideoBlitColorAccuracy); +MEDIA_EXPORT BASE_DECLARE_FEATURE(kAImageReaderVideoOutput); MEDIA_EXPORT BASE_DECLARE_FEATURE(kVp9kSVCHWDecoding); MEDIA_EXPORT BASE_DECLARE_FEATURE(kWebContentsCaptureHiDpi); MEDIA_EXPORT BASE_DECLARE_FEATURE(kWebrtcMediaCapabilitiesParameters); -- 2.40.1