Reverts "[Impeller] moved to bgra10_xr (#52019)" (#52140)

Reverts: #52019 Initiated by: jonahwilliams Reason for reverting: I believe the tree failures are due to changes in the build configuration this file introduced. Original PR Author: gaaclarke Reviewed By: {jonahwilliams} This change reverts the following previous change: fixes flutter/flutter#145933 This required that we moved the golden image tests to arm64 since the wide gamut tests would now require BGRA10_XR and that's only available to arm64. tests: in framework repo https://github.com/flutter/flutter/tree/master/dev/integration_tests/wide_gamut_test. There was a test added to that suite specifically for this case when we turned off BGRA10_XR the first time. This has a dependency on #51998 which includes the necessary skia change. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
flutter · Apr 15, 2024 · 503e7e8 · 503e7e8
1 parent 8916b88
commit 503e7e8
Show file tree

Hide file tree

Showing 10 changed files with 34 additions and 76 deletions.
diff --git a/ci/builders/mac_host_engine.json b/ci/builders/mac_host_engine.json
@@ -215,6 +215,7 @@
                     "flutter/build/archives:archive_gen_snapshot",
                     "flutter/build/archives:artifacts",
                     "flutter/build/dart:copy_dart_sdk",
+                    "flutter/impeller/golden_tests:impeller_golden_tests",
                     "flutter/shell/platform/darwin/macos:zip_macos_flutter_framework",
                     "flutter/tools/font_subset",
                     "flutter:unittests"
@@ -248,7 +249,7 @@
                         "--variant",
                         "ci/host_release",
                         "--type",
-                        "dart,dart-host,engine"
+                        "dart,dart-host,engine,impeller-golden"
                     ]
                 }
             ]
@@ -404,7 +405,7 @@
             "drone_dimensions": [
                 "device_type=none",
                 "os=Mac-13",
-                "cpu=arm64"
+                "cpu=x86"
             ],
             "gclient_variables": {
                 "download_android_deps": false,
@@ -422,21 +423,16 @@
                 "--prebuilt-dart-sdk",
                 "--rbe",
                 "--no-goma",
-                "--xcode-symlinks",
-                "--use-glfw-swiftshader"
+                "--xcode-symlinks"
             ],
             "name": "ci/mac_release_arm64",
             "description": "Produces release mode arm64 macOS host-side tooling.",
             "ninja": {
                 "config": "ci/mac_release_arm64",
                 "targets": [
-                    "flutter:unittests",
-                    "flutter/build/archives:archive_gen_snapshot",
+                    "flutter/tools/font_subset",
                     "flutter/build/archives:artifacts",
-                    "flutter/build/dart:copy_dart_sdk",
-                    "flutter/impeller/golden_tests:impeller_golden_tests",
-                    "flutter/shell/platform/darwin/macos:zip_macos_flutter_framework",
-                    "flutter/tools/font_subset"
+                    "flutter/shell/platform/darwin/macos:zip_macos_flutter_framework"
                 ]
             },
            "postsubmit_overrides": {
@@ -458,20 +454,7 @@
                 "$flutter/osx_sdk": {
                     "sdk_version": "15a240d"
                 }
-            },
-            "tests": [
-                {
-                    "language": "python3",
-                    "name": "Impeller-golden for host_release",
-                    "script": "flutter/testing/run_tests.py",
-                    "parameters": [
-                        "--variant",
-                        "ci/mac_release_arm64",
-                        "--type",
-                        "impeller-golden"
-                    ]
-                }
-            ]
+            }
         }
     ],
     "generators": {

diff --git a/impeller/aiks/aiks_blend_unittests.cc b/impeller/aiks/aiks_blend_unittests.cc
@@ -132,8 +132,11 @@ TEST_P(AiksTest, PaintBlendModeIsRespected) {
 
 // Bug: https://github.com/flutter/flutter/issues/142549
 TEST_P(AiksTest, BlendModePlusAlphaWideGamut) {
+  if (GetParam() != PlaygroundBackend::kMetal) {
+    GTEST_SKIP_("This backend doesn't yet support wide gamut.");
+  }
   EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(),
-            PixelFormat::kB10G10R10A10XR);
+            PixelFormat::kR16G16B16A16Float);
   auto texture = CreateTextureForFixture("airplane.jpg",
                                          /*enable_mipmapping=*/true);
 
@@ -155,8 +158,11 @@ TEST_P(AiksTest, BlendModePlusAlphaWideGamut) {
 
 // Bug: https://github.com/flutter/flutter/issues/142549
 TEST_P(AiksTest, BlendModePlusAlphaColorFilterWideGamut) {
+  if (GetParam() != PlaygroundBackend::kMetal) {
+    GTEST_SKIP_("This backend doesn't yet support wide gamut.");
+  }
   EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(),
-            PixelFormat::kB10G10R10A10XR);
+            PixelFormat::kR16G16B16A16Float);
   auto texture = CreateTextureForFixture("airplane.jpg",
                                          /*enable_mipmapping=*/true);
 

diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc
@@ -938,14 +938,17 @@ TEST_P(AiksTest, CanDrawPaintMultipleTimes) {
 }
 
 // This makes sure the WideGamut named tests use 16bit float pixel format.
-TEST_P(AiksTest, FormatWideGamut) {
+TEST_P(AiksTest, F16WideGamut) {
+  if (GetParam() != PlaygroundBackend::kMetal) {
+    GTEST_SKIP_("This backend doesn't yet support wide gamut.");
+  }
   EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(),
-            PixelFormat::kB10G10R10A10XR);
-  EXPECT_TRUE(IsAlphaClampedToOne(
+            PixelFormat::kR16G16B16A16Float);
+  EXPECT_FALSE(IsAlphaClampedToOne(
       GetContext()->GetCapabilities()->GetDefaultColorFormat()));
 }
 
-TEST_P(AiksTest, FormatSRGB) {
+TEST_P(AiksTest, NotF16) {
   EXPECT_TRUE(IsAlphaClampedToOne(
       GetContext()->GetCapabilities()->GetDefaultColorFormat()));
 }
@@ -3104,8 +3107,12 @@ TEST_P(AiksTest, MipmapGenerationWorksCorrectly) {
 }
 
 TEST_P(AiksTest, DrawAtlasPlusWideGamut) {
+  if (GetParam() != PlaygroundBackend::kMetal) {
+    GTEST_SKIP_("This backend doesn't yet support wide gamut.");
+  }
+
   EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(),
-            PixelFormat::kB10G10R10A10XR);
+            PixelFormat::kR16G16B16A16Float);
 
   // Draws the image as four squares stiched together.
   auto atlas =

diff --git a/impeller/golden_tests/golden_playground_test_mac.cc b/impeller/golden_tests/golden_playground_test_mac.cc
@@ -128,16 +128,6 @@ void GoldenPlaygroundTest::TearDown() {
   ASSERT_FALSE(dlopen("/usr/local/lib/libMoltenVK.dylib", RTLD_NOLOAD));
 }
 
-namespace {
-bool DoesSupportWideGamutTests() {
-#ifdef __arm64__
-  return true;
-#else
-  return false;
-#endif
-}
-}  // namespace
-
 void GoldenPlaygroundTest::SetUp() {
   std::filesystem::path testing_assets_path =
       flutter::testing::GetTestingAssetsPath();
@@ -152,27 +142,17 @@ void GoldenPlaygroundTest::SetUp() {
   bool enable_wide_gamut = test_name.find("WideGamut_") != std::string::npos;
   switch (GetParam()) {
     case PlaygroundBackend::kMetal:
-      if (!DoesSupportWideGamutTests()) {
-        GTEST_SKIP_(
-            "This metal device doesn't support wide gamut golden tests.");
-      }
       pimpl_->screenshotter =
           std::make_unique<testing::MetalScreenshotter>(enable_wide_gamut);
       break;
     case PlaygroundBackend::kVulkan: {
-      if (enable_wide_gamut) {
-        GTEST_SKIP_("Vulkan doesn't support wide gamut golden tests.");
-      }
       const std::unique_ptr<PlaygroundImpl>& playground =
           GetSharedVulkanPlayground(/*enable_validations=*/true);
       pimpl_->screenshotter =
           std::make_unique<testing::VulkanScreenshotter>(playground);
       break;
     }
     case PlaygroundBackend::kOpenGLES: {
-      if (enable_wide_gamut) {
-        GTEST_SKIP_("OpenGLES doesn't support wide gamut golden tests.");
-      }
       FML_CHECK(::glfwInit() == GLFW_TRUE);
       PlaygroundSwitches playground_switches;
       playground_switches.use_angle = true;

diff --git a/impeller/playground/backend/metal/playground_impl_mtl.mm b/impeller/playground/backend/metal/playground_impl_mtl.mm
@@ -73,12 +73,11 @@
   if (!window) {
     return;
   }
-
   auto context = ContextMTL::Create(
       ShaderLibraryMappingsForPlayground(), is_gpu_disabled_sync_switch_,
       "Playground Library",
       switches.enable_wide_gamut
-          ? std::optional<PixelFormat>(PixelFormat::kB10G10R10A10XR)
+          ? std::optional<PixelFormat>(PixelFormat::kR16G16B16A16Float)
           : std::nullopt);
   if (!context) {
     return;

diff --git a/impeller/playground/playground_test.cc b/impeller/playground/playground_test.cc
@@ -15,16 +15,6 @@ PlaygroundTest::PlaygroundTest()
 
 PlaygroundTest::~PlaygroundTest() = default;
 
-namespace {
-bool DoesSupportWideGamutTests() {
-#ifdef __arm64__
-  return true;
-#else
-  return false;
-#endif
-}
-}  // namespace
-
 void PlaygroundTest::SetUp() {
   if (!Playground::SupportsBackend(GetParam())) {
     GTEST_SKIP_("Playground doesn't support this backend type.");
@@ -44,12 +34,6 @@ void PlaygroundTest::SetUp() {
   switches.enable_wide_gamut =
       test_name.find("WideGamut/") != std::string::npos;
 
-  if (switches.enable_wide_gamut && (GetParam() != PlaygroundBackend::kMetal ||
-                                     !DoesSupportWideGamutTests())) {
-    GTEST_SKIP_("This backend doesn't yet support wide gamut.");
-    return;
-  }
-
   SetupContext(GetParam(), switches);
   SetupWindow();
 }

diff --git a/lib/ui/painting/image_encoding_impeller.cc b/lib/ui/painting/image_encoding_impeller.cc
@@ -25,8 +25,6 @@ std::optional<SkColorType> ToSkColorType(impeller::PixelFormat format) {
       return SkColorType::kBGRA_8888_SkColorType;
     case impeller::PixelFormat::kB10G10R10XR:
       return SkColorType::kBGR_101010x_XR_SkColorType;
-    case impeller::PixelFormat::kB10G10R10A10XR:
-      return SkColorType::kBGRA_10101010_XR_SkColorType;
     default:
       return std::nullopt;
   }

diff --git a/shell/platform/darwin/ios/framework/Source/FlutterMetalLayer.mm b/shell/platform/darwin/ios/framework/Source/FlutterMetalLayer.mm
@@ -259,9 +259,6 @@ - (IOSurface*)createIOSurface {
   } else if (self.pixelFormat == MTLPixelFormatBGRA8Unorm) {
     pixelFormat = kCVPixelFormatType_32BGRA;
     bytesPerElement = 4;
-  } else if (self.pixelFormat == MTLPixelFormatBGRA10_XR) {
-    pixelFormat = kCVPixelFormatType_40ARGBLEWideGamut;
-    bytesPerElement = 8;
   } else {
     FML_LOG(ERROR) << "Unsupported pixel format: " << self.pixelFormat;
     return nil;

diff --git a/shell/platform/darwin/ios/framework/Source/FlutterOverlayView.mm b/shell/platform/darwin/ios/framework/Source/FlutterOverlayView.mm
@@ -59,7 +59,7 @@ - (instancetype)initWithContentsScale:(CGFloat)contentsScale
     CAMetalLayer* layer = (CAMetalLayer*)self.layer;
 #pragma clang diagnostic pop
     layer.pixelFormat = pixelFormat;
-    if (pixelFormat == MTLPixelFormatRGBA16Float || pixelFormat == MTLPixelFormatBGRA10_XR) {
+    if (pixelFormat == MTLPixelFormatRGBA16Float) {
       self->_colorSpaceRef = fml::CFRef(CGColorSpaceCreateWithName(kCGColorSpaceExtendedSRGB));
       layer.colorspace = self->_colorSpaceRef;
     }

diff --git a/shell/platform/darwin/ios/framework/Source/FlutterView.mm b/shell/platform/darwin/ios/framework/Source/FlutterView.mm
@@ -122,7 +122,11 @@ - (void)layoutSubviews {
       CGColorSpaceRef srgb = CGColorSpaceCreateWithName(kCGColorSpaceExtendedSRGB);
       layer.colorspace = srgb;
       CFRelease(srgb);
-      layer.pixelFormat = MTLPixelFormatBGRA10_XR;
+      // MTLPixelFormatRGBA16Float was chosen since it is compatible with
+      // impeller's offscreen buffers which need to have transparency.  Also,
+      // F16 was chosen over BGRA10_XR since Skia does not support decoding
+      // BGRA10_XR.
+      layer.pixelFormat = MTLPixelFormatRGBA16Float;
     } else if (_isWideGamutEnabled && !isWideGamutSupported) {
       PrintWideGamutWarningOnce();
     }