[Impeller] moved to bgra10_xr (#52019)

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 · 092710a · 092710a
1 parent 78e5d71
commit 092710a
Show file tree

Hide file tree

Showing 10 changed files with 76 additions and 34 deletions.
diff --git a/ci/builders/mac_host_engine.json b/ci/builders/mac_host_engine.json
@@ -215,7 +215,6 @@
                     "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"
@@ -249,7 +248,7 @@
                         "--variant",
                         "ci/host_release",
                         "--type",
-                        "dart,dart-host,engine,impeller-golden"
+                        "dart,dart-host,engine"
                     ]
                 }
             ]
@@ -405,7 +404,7 @@
             "drone_dimensions": [
                 "device_type=none",
                 "os=Mac-13",
-                "cpu=x86"
+                "cpu=arm64"
             ],
             "gclient_variables": {
                 "download_android_deps": false,
@@ -423,16 +422,21 @@
                 "--prebuilt-dart-sdk",
                 "--rbe",
                 "--no-goma",
-                "--xcode-symlinks"
+                "--xcode-symlinks",
+                "--use-glfw-swiftshader"
             ],
             "name": "ci/mac_release_arm64",
             "description": "Produces release mode arm64 macOS host-side tooling.",
             "ninja": {
                 "config": "ci/mac_release_arm64",
                 "targets": [
-                    "flutter/tools/font_subset",
+                    "flutter:unittests",
+                    "flutter/build/archives:archive_gen_snapshot",
                     "flutter/build/archives:artifacts",
-                    "flutter/shell/platform/darwin/macos:zip_macos_flutter_framework"
+                    "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"
                 ]
             },
            "postsubmit_overrides": {
@@ -454,7 +458,20 @@
                 "$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,11 +132,8 @@ 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::kR16G16B16A16Float);
+            PixelFormat::kB10G10R10A10XR);
   auto texture = CreateTextureForFixture("airplane.jpg",
                                          /*enable_mipmapping=*/true);
 
@@ -158,11 +155,8 @@ 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::kR16G16B16A16Float);
+            PixelFormat::kB10G10R10A10XR);
   auto texture = CreateTextureForFixture("airplane.jpg",
                                          /*enable_mipmapping=*/true);
 

diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc
@@ -938,17 +938,14 @@ TEST_P(AiksTest, CanDrawPaintMultipleTimes) {
 }
 
 // This makes sure the WideGamut named tests use 16bit float pixel format.
-TEST_P(AiksTest, F16WideGamut) {
-  if (GetParam() != PlaygroundBackend::kMetal) {
-    GTEST_SKIP_("This backend doesn't yet support wide gamut.");
-  }
+TEST_P(AiksTest, FormatWideGamut) {
   EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(),
-            PixelFormat::kR16G16B16A16Float);
-  EXPECT_FALSE(IsAlphaClampedToOne(
+            PixelFormat::kB10G10R10A10XR);
+  EXPECT_TRUE(IsAlphaClampedToOne(
       GetContext()->GetCapabilities()->GetDefaultColorFormat()));
 }
 
-TEST_P(AiksTest, NotF16) {
+TEST_P(AiksTest, FormatSRGB) {
   EXPECT_TRUE(IsAlphaClampedToOne(
       GetContext()->GetCapabilities()->GetDefaultColorFormat()));
 }
@@ -3107,12 +3104,8 @@ 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::kR16G16B16A16Float);
+            PixelFormat::kB10G10R10A10XR);
 
   // 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,6 +128,16 @@ 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();
@@ -142,17 +152,27 @@ 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,11 +73,12 @@
   if (!window) {
     return;
   }
+
   auto context = ContextMTL::Create(
       ShaderLibraryMappingsForPlayground(), is_gpu_disabled_sync_switch_,
       "Playground Library",
       switches.enable_wide_gamut
-          ? std::optional<PixelFormat>(PixelFormat::kR16G16B16A16Float)
+          ? std::optional<PixelFormat>(PixelFormat::kB10G10R10A10XR)
           : std::nullopt);
   if (!context) {
     return;

diff --git a/impeller/playground/playground_test.cc b/impeller/playground/playground_test.cc
@@ -15,6 +15,16 @@ 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.");
@@ -34,6 +44,12 @@ 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,6 +25,8 @@ 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,6 +259,9 @@ - (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) {
+    if (pixelFormat == MTLPixelFormatRGBA16Float || pixelFormat == MTLPixelFormatBGRA10_XR) {
       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,11 +122,7 @@ - (void)layoutSubviews {
       CGColorSpaceRef srgb = CGColorSpaceCreateWithName(kCGColorSpaceExtendedSRGB);
       layer.colorspace = srgb;
       CFRelease(srgb);
-      // 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;
+      layer.pixelFormat = MTLPixelFormatBGRA10_XR;
     } else if (_isWideGamutEnabled && !isWideGamutSupported) {
       PrintWideGamutWarningOnce();
     }