From cca82ed93b2b27979acafebb26f72bc72ca40e9f Mon Sep 17 00:00:00 2001 From: "auto-submit[bot]" <98614782+auto-submit[bot]@users.noreply.github.com> Date: Wed, 26 Feb 2025 04:32:26 +0000 Subject: [PATCH] Reverts "[Impeller] move AHB check to Vulkan, use Vulkan surface on 29. (#164109)" (#164166) Reverts: flutter/flutter#164109 Initiated by: jonahwilliams Reason for reverting: brain not work too good. Original PR Author: jonahwilliams Reviewed By: {matanlurey} This change reverts the following previous change: Rather than conditionally disabling AHBs, just disable Vulkan on devices where AHB imports don't work. https://github.com/flutter/flutter/issues/163473 https://github.com/flutter/flutter/issues/160854 Co-authored-by: auto-submit[bot] --- .../ci/licenses_golden/licenses_flutter | 4 ++ .../backend/vulkan/swapchain/swapchain_vk.cc | 1 + .../flutter/impeller/toolkit/android/BUILD.gn | 2 + .../impeller/toolkit/android/shadow_realm.cc | 45 +++++++++++++++++++ .../impeller/toolkit/android/shadow_realm.h | 27 +++++++++++ .../android/toolkit_android_unittests.cc | 15 +++++++ .../shell/platform/android/flutter_main.cc | 9 ---- .../flutter/embedding/engine/FlutterJNI.java | 10 +++++ .../engine/renderer/FlutterRenderer.java | 4 +- .../android/platform_view_android_jni_impl.cc | 7 +++ 10 files changed, 114 insertions(+), 10 deletions(-) create mode 100644 engine/src/flutter/impeller/toolkit/android/shadow_realm.cc create mode 100644 engine/src/flutter/impeller/toolkit/android/shadow_realm.h diff --git a/engine/src/flutter/ci/licenses_golden/licenses_flutter b/engine/src/flutter/ci/licenses_golden/licenses_flutter index 348bd12248..7c45a24340 100644 --- a/engine/src/flutter/ci/licenses_golden/licenses_flutter +++ b/engine/src/flutter/ci/licenses_golden/licenses_flutter @@ -42254,6 +42254,8 @@ ORIGIN: ../../../flutter/impeller/toolkit/android/native_window.cc + ../../../fl ORIGIN: ../../../flutter/impeller/toolkit/android/native_window.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/toolkit/android/proc_table.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/toolkit/android/proc_table.h + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/impeller/toolkit/android/shadow_realm.cc + ../../../flutter/LICENSE +ORIGIN: ../../../flutter/impeller/toolkit/android/shadow_realm.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/toolkit/android/surface_control.cc + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/toolkit/android/surface_control.h + ../../../flutter/LICENSE ORIGIN: ../../../flutter/impeller/toolkit/android/surface_transaction.cc + ../../../flutter/LICENSE @@ -45167,6 +45169,8 @@ FILE: ../../../flutter/impeller/toolkit/android/native_window.cc FILE: ../../../flutter/impeller/toolkit/android/native_window.h FILE: ../../../flutter/impeller/toolkit/android/proc_table.cc FILE: ../../../flutter/impeller/toolkit/android/proc_table.h +FILE: ../../../flutter/impeller/toolkit/android/shadow_realm.cc +FILE: ../../../flutter/impeller/toolkit/android/shadow_realm.h FILE: ../../../flutter/impeller/toolkit/android/surface_control.cc FILE: ../../../flutter/impeller/toolkit/android/surface_control.h FILE: ../../../flutter/impeller/toolkit/android/surface_transaction.cc diff --git a/engine/src/flutter/impeller/renderer/backend/vulkan/swapchain/swapchain_vk.cc b/engine/src/flutter/impeller/renderer/backend/vulkan/swapchain/swapchain_vk.cc index 07e94042de..64a85733ca 100644 --- a/engine/src/flutter/impeller/renderer/backend/vulkan/swapchain/swapchain_vk.cc +++ b/engine/src/flutter/impeller/renderer/backend/vulkan/swapchain/swapchain_vk.cc @@ -11,6 +11,7 @@ #if FML_OS_ANDROID #include "impeller/renderer/backend/vulkan/swapchain/ahb/ahb_swapchain_vk.h" +#include "impeller/toolkit/android/shadow_realm.h" #endif // FML_OS_ANDROID namespace impeller { diff --git a/engine/src/flutter/impeller/toolkit/android/BUILD.gn b/engine/src/flutter/impeller/toolkit/android/BUILD.gn index 0e0b30337f..c4a95f7681 100644 --- a/engine/src/flutter/impeller/toolkit/android/BUILD.gn +++ b/engine/src/flutter/impeller/toolkit/android/BUILD.gn @@ -19,6 +19,8 @@ impeller_component("android") { "native_window.h", "proc_table.cc", "proc_table.h", + "shadow_realm.cc", + "shadow_realm.h", "surface_control.cc", "surface_control.h", "surface_transaction.cc", diff --git a/engine/src/flutter/impeller/toolkit/android/shadow_realm.cc b/engine/src/flutter/impeller/toolkit/android/shadow_realm.cc new file mode 100644 index 0000000000..7aeb4ac074 --- /dev/null +++ b/engine/src/flutter/impeller/toolkit/android/shadow_realm.cc @@ -0,0 +1,45 @@ +// Copyright 2013 The Flutter 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 "impeller/toolkit/android/shadow_realm.h" + +#include + +namespace impeller::android { + +constexpr std::string_view kAndroidHuawei = "android-huawei"; + +bool ShadowRealm::ShouldDisableAHB() { + char clientidbase[PROP_VALUE_MAX]; + __system_property_get("ro.com.google.clientidbase", clientidbase); + + auto api_level = android_get_device_api_level(); + char first_api_level[PROP_VALUE_MAX]; + __system_property_get("ro.product.first_api_level", first_api_level); + + return ShouldDisableAHBInternal(clientidbase, first_api_level, api_level); +} + +// static +bool ShadowRealm::ShouldDisableAHBInternal(std::string_view clientidbase, + std::string_view first_api_level, + uint32_t api_level) { + // Most devices that have updated to API 29 don't seem to correctly + // support AHBs: https://github.com/flutter/flutter/issues/157113 + if (first_api_level.compare("28") == 0 || + first_api_level.compare("27") == 0 || + first_api_level.compare("26") == 0 || + first_api_level.compare("25") == 0 || + first_api_level.compare("24") == 0) { + return true; + } + // From local testing, neither the swapchain nor AHB import works, see also: + // https://github.com/flutter/flutter/issues/154068 + if (clientidbase == kAndroidHuawei && api_level <= 29) { + return true; + } + return false; +} + +} // namespace impeller::android diff --git a/engine/src/flutter/impeller/toolkit/android/shadow_realm.h b/engine/src/flutter/impeller/toolkit/android/shadow_realm.h new file mode 100644 index 0000000000..90f265ec61 --- /dev/null +++ b/engine/src/flutter/impeller/toolkit/android/shadow_realm.h @@ -0,0 +1,27 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef FLUTTER_IMPELLER_TOOLKIT_ANDROID_SHADOW_REALM_H_ +#define FLUTTER_IMPELLER_TOOLKIT_ANDROID_SHADOW_REALM_H_ + +#include + +namespace impeller::android { + +// Looks like you're going to the Shadow Realm, Jimbo. +class ShadowRealm { + public: + /// @brief Whether the device should disable any usage of Android Hardware + /// Buffers regardless of stated support. + static bool ShouldDisableAHB(); + + // For testing. + static bool ShouldDisableAHBInternal(std::string_view clientidbase, + std::string_view first_api_level, + uint32_t api_level); +}; + +} // namespace impeller::android + +#endif // FLUTTER_IMPELLER_TOOLKIT_ANDROID_SHADOW_REALM_H_ diff --git a/engine/src/flutter/impeller/toolkit/android/toolkit_android_unittests.cc b/engine/src/flutter/impeller/toolkit/android/toolkit_android_unittests.cc index 9c6218fa87..ca8837d732 100644 --- a/engine/src/flutter/impeller/toolkit/android/toolkit_android_unittests.cc +++ b/engine/src/flutter/impeller/toolkit/android/toolkit_android_unittests.cc @@ -7,6 +7,7 @@ #include "impeller/toolkit/android/choreographer.h" #include "impeller/toolkit/android/hardware_buffer.h" #include "impeller/toolkit/android/proc_table.h" +#include "impeller/toolkit/android/shadow_realm.h" #include "impeller/toolkit/android/surface_control.h" #include "impeller/toolkit/android/surface_transaction.h" @@ -134,4 +135,18 @@ TEST(ToolkitAndroidTest, CanPostAndWaitForFrameCallbacks) { event.Wait(); } +TEST(ToolkitAndroidTest, ShouldDisableAHB) { + EXPECT_FALSE( + ShadowRealm::ShouldDisableAHBInternal("android-huawei", "30", 30)); + EXPECT_FALSE( + ShadowRealm::ShouldDisableAHBInternal("something made up", "29", 29)); + + EXPECT_TRUE( + ShadowRealm::ShouldDisableAHBInternal("android-huawei", "29", 29)); + EXPECT_TRUE( + ShadowRealm::ShouldDisableAHBInternal("something made up", "27", 29)); + EXPECT_TRUE( + ShadowRealm::ShouldDisableAHBInternal("android-huawei", "garbage", 29)); +} + } // namespace impeller::android::testing diff --git a/engine/src/flutter/shell/platform/android/flutter_main.cc b/engine/src/flutter/shell/platform/android/flutter_main.cc index b0147f9ccf..d255f4a766 100644 --- a/engine/src/flutter/shell/platform/android/flutter_main.cc +++ b/engine/src/flutter/shell/platform/android/flutter_main.cc @@ -45,8 +45,6 @@ namespace { fml::jni::ScopedJavaGlobalRef* g_flutter_jni_class = nullptr; -static const constexpr char* kAndroidHuawei = "android-huawei"; - /// These are SoCs that crash when using AHB imports. static constexpr const char* kBLC[] = { // Most Exynos Series SoC @@ -312,13 +310,6 @@ AndroidRenderingAPI FlutterMain::SelectedRenderingAPI( return kVulkanUnsupportedFallback; } - __system_property_get("ro.com.google.clientidbase", product_model); - if (strcmp(product_model, kAndroidHuawei)) { - // Avoid using Vulkan on Huawei as AHB imports do not - // consistently work. - return kVulkanUnsupportedFallback; - } - __system_property_get("ro.product.board", product_model); if (IsKnownBadSOC(product_model)) { // Avoid using Vulkan on known bad SoCs. diff --git a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java index d67f5daa65..dac6d25149 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java @@ -1637,6 +1637,16 @@ public class FlutterJNI { void asyncWaitForVsync(final long cookie); } + /** + * Whether Android Hardware Buffer import is known to not work on this particular vendor + API + * level and should be disabled. + */ + public boolean ShouldDisableAHB() { + return nativeShouldDisableAHB(); + } + + private native boolean nativeShouldDisableAHB(); + /** Whether the SurfaceControl swapchain required for hcpp is enabled and active. */ public boolean IsSurfaceControlEnabled() { return nativeIsSurfaceControlEnabled(nativeShellHolderId); diff --git a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java index 33c8a81954..e3095ff7bb 100644 --- a/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java +++ b/engine/src/flutter/shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java @@ -212,7 +212,9 @@ public class FlutterRenderer implements TextureRegistry { // version that is // running Vulkan, so we don't have to worry about it not being supported. final SurfaceProducer entry; - if (!debugForceSurfaceProducerGlTextures && Build.VERSION.SDK_INT >= API_LEVELS.API_29) { + if (!debugForceSurfaceProducerGlTextures + && Build.VERSION.SDK_INT >= API_LEVELS.API_29 + && !flutterJNI.ShouldDisableAHB()) { final long id = nextTextureId.getAndIncrement(); final ImageReaderSurfaceProducer producer = new ImageReaderSurfaceProducer(id); registerImageTexture(id, producer); diff --git a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc index b8e806fef1..06d2b3467b 100644 --- a/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc +++ b/engine/src/flutter/shell/platform/android/platform_view_android_jni_impl.cc @@ -11,6 +11,7 @@ #include #include +#include "impeller/toolkit/android/shadow_realm.h" #include "unicode/uchar.h" #include "flutter/common/constants.h" @@ -870,6 +871,12 @@ bool RegisterApi(JNIEnv* env) { .signature = "(J)V", .fnPtr = reinterpret_cast(&UpdateDisplayMetrics), }, + { + .name = "nativeShouldDisableAHB", + .signature = "()Z", + .fnPtr = reinterpret_cast( + &impeller::android::ShadowRealm::ShouldDisableAHB), + }, { .name = "nativeIsSurfaceControlEnabled", .signature = "(J)Z",