Skip to content

Commit

Permalink
Reverts "[Impeller] move AHB check to Vulkan, use Vulkan surface on 2…
Browse files Browse the repository at this point in the history
…9. (flutter#164109)" (flutter#164166)

<!-- start_original_pr_link -->
Reverts: flutter#164109
<!-- end_original_pr_link -->
<!-- start_initiating_author -->
Initiated by: jonahwilliams
<!-- end_initiating_author -->
<!-- start_revert_reason -->
Reason for reverting: brain not work too good.
<!-- end_revert_reason -->
<!-- start_original_pr_author -->
Original PR Author: jonahwilliams
<!-- end_original_pr_author -->

<!-- start_reviewers -->
Reviewed By: {matanlurey}
<!-- end_reviewers -->

<!-- start_revert_body -->
This change reverts the following previous change:
Rather than conditionally disabling AHBs, just disable Vulkan on devices
where AHB imports don't work.

flutter#163473
flutter#160854
<!-- end_revert_body -->

Co-authored-by: auto-submit[bot] <[email protected]>
  • Loading branch information
auto-submit[bot] and auto-submit[bot] authored Feb 26, 2025
1 parent 092be76 commit cca82ed
Show file tree
Hide file tree
Showing 10 changed files with 114 additions and 10 deletions.
4 changes: 4 additions & 0 deletions engine/src/flutter/ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 2 additions & 0 deletions engine/src/flutter/impeller/toolkit/android/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
45 changes: 45 additions & 0 deletions engine/src/flutter/impeller/toolkit/android/shadow_realm.cc
Original file line number Diff line number Diff line change
@@ -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 <sys/system_properties.h>

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
27 changes: 27 additions & 0 deletions engine/src/flutter/impeller/toolkit/android/shadow_realm.h
Original file line number Diff line number Diff line change
@@ -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 <string_view>

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_
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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
9 changes: 0 additions & 9 deletions engine/src/flutter/shell/platform/android/flutter_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@ namespace {

fml::jni::ScopedJavaGlobalRef<jclass>* 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
Expand Down Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1637,6 +1637,16 @@ public interface AsyncWaitForVsyncDelegate {
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,9 @@ public SurfaceProducer createSurfaceProducer() {
// 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <memory>
#include <utility>

#include "impeller/toolkit/android/shadow_realm.h"
#include "unicode/uchar.h"

#include "flutter/common/constants.h"
Expand Down Expand Up @@ -870,6 +871,12 @@ bool RegisterApi(JNIEnv* env) {
.signature = "(J)V",
.fnPtr = reinterpret_cast<void*>(&UpdateDisplayMetrics),
},
{
.name = "nativeShouldDisableAHB",
.signature = "()Z",
.fnPtr = reinterpret_cast<void*>(
&impeller::android::ShadowRealm::ShouldDisableAHB),
},
{
.name = "nativeIsSurfaceControlEnabled",
.signature = "(J)Z",
Expand Down

0 comments on commit cca82ed

Please sign in to comment.