Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Apr 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 7 additions & 24 deletions ci/builders/mac_host_engine.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -248,7 +249,7 @@
"--variant",
"ci/host_release",
"--type",
"dart,dart-host,engine"
"dart,dart-host,engine,impeller-golden"
]
}
]
Expand Down Expand Up @@ -404,7 +405,7 @@
"drone_dimensions": [
"device_type=none",
"os=Mac-13",
"cpu=arm64"
"cpu=x86"
],
"gclient_variables": {
"download_android_deps": false,
Expand All @@ -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": {
Expand All @@ -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": {
Expand Down
10 changes: 8 additions & 2 deletions impeller/aiks/aiks_blend_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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);

Expand Down
17 changes: 12 additions & 5 deletions impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
Expand Down Expand Up @@ -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 =
Expand Down
20 changes: 0 additions & 20 deletions impeller/golden_tests/golden_playground_test_mac.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions impeller/playground/backend/metal/playground_impl_mtl.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
16 changes: 0 additions & 16 deletions impeller/playground/playground_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Expand All @@ -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();
}
Expand Down
2 changes: 0 additions & 2 deletions lib/ui/painting/image_encoding_impeller.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
3 changes: 0 additions & 3 deletions shell/platform/darwin/ios/framework/Source/FlutterMetalLayer.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion shell/platform/darwin/ios/framework/Source/FlutterOverlayView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
6 changes: 5 additions & 1 deletion shell/platform/darwin/ios/framework/Source/FlutterView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down