From 12477ffeb5d039d3b7fbddd738a08db8908092ed Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 29 Mar 2024 16:18:46 -0700 Subject: [PATCH 1/8] [Impeller] fix plus blend mode in porterduff shader. --- impeller/entity/entity_unittests.cc | 39 +++++++++++++++++++ .../shaders/blending/porter_duff_blend.frag | 5 +++ 2 files changed, 44 insertions(+) diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index ba3fccbc38e31..02a70fd47454d 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -1446,6 +1446,45 @@ TEST_P(EntityTest, DrawAtlasNoColor) { ASSERT_TRUE(OpenPlaygroundHere(std::move(e))); } +TEST_P(EntityTest, F16WideGamutDrawAtlasPlus) { + if (GetParam() != PlaygroundBackend::kMetal) { + GTEST_SKIP_("This backend doesn't yet support wide gamut."); + } + + // Draws the image as four squares stiched together. + auto atlas = CreateTextureForFixture("bay_bridge.jpg"); + auto size = atlas->GetSize(); + // Divide image into four quadrants. + Scalar half_width = size.width / 2; + Scalar half_height = size.height / 2; + std::vector texture_coordinates = { + Rect::MakeLTRB(0, 0, half_width, half_height), + Rect::MakeLTRB(half_width, 0, size.width, half_height), + Rect::MakeLTRB(0, half_height, half_width, size.height), + Rect::MakeLTRB(half_width, half_height, size.width, size.height)}; + // Position quadrants adjacent to eachother. + std::vector transforms = { + Matrix::MakeTranslation({0, 0, 0}), + Matrix::MakeTranslation({half_width, 0, 0}), + Matrix::MakeTranslation({0, half_height, 0}), + Matrix::MakeTranslation({half_width, half_height, 0})}; + std::vector colors = {Color::Red(), Color::Green(), Color::Blue(), + Color::Yellow()}; + std::shared_ptr contents = std::make_shared(); + + contents->SetTransforms(std::move(transforms)); + contents->SetTextureCoordinates(std::move(texture_coordinates)); + contents->SetTexture(atlas); + contents->SetColors(colors); + contents->SetBlendMode(BlendMode::kPlus); + + Entity e; + e.SetTransform(Matrix::MakeScale(GetContentScale())); + e.SetContents(contents); + + ASSERT_TRUE(OpenPlaygroundHere(std::move(e))); +} + TEST_P(EntityTest, DrawAtlasWithColorAdvanced) { // Draws the image as four squares stiched together. auto atlas = CreateTextureForFixture("bay_bridge.jpg"); diff --git a/impeller/entity/shaders/blending/porter_duff_blend.frag b/impeller/entity/shaders/blending/porter_duff_blend.frag index 2b6631f9d57f7..2c891b085cac4 100644 --- a/impeller/entity/shaders/blending/porter_duff_blend.frag +++ b/impeller/entity/shaders/blending/porter_duff_blend.frag @@ -45,4 +45,9 @@ void main() { dst * (frag_info.dst_coeff + src.a * frag_info.dst_coeff_src_alpha + src * frag_info.dst_coeff_src_color); frag_color *= frag_info.output_alpha; + // This currently needs a clamp so that floating point textures blend + // correctly in wide gamut. Remove if we switch to a fixed point extended + // range format. + float16_t clamped_alpha = clamp(frag_color.a, 0.0hf, 1.0hf); + frag_color.a = clamped_alpha; } From 8aa65ac15074d80f95560b46b810ca57fdb1a920 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 29 Mar 2024 16:21:28 -0700 Subject: [PATCH 2/8] add issue comment. --- impeller/entity/shaders/blending/porter_duff_blend.frag | 1 + 1 file changed, 1 insertion(+) diff --git a/impeller/entity/shaders/blending/porter_duff_blend.frag b/impeller/entity/shaders/blending/porter_duff_blend.frag index 2c891b085cac4..3da0051171c1b 100644 --- a/impeller/entity/shaders/blending/porter_duff_blend.frag +++ b/impeller/entity/shaders/blending/porter_duff_blend.frag @@ -48,6 +48,7 @@ void main() { // This currently needs a clamp so that floating point textures blend // correctly in wide gamut. Remove if we switch to a fixed point extended // range format. + // See https://github.com/flutter/flutter/issues/145933 . float16_t clamped_alpha = clamp(frag_color.a, 0.0hf, 1.0hf); frag_color.a = clamped_alpha; } From 49f9604b7c94a37d002efc253992ad269297b036 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 29 Mar 2024 16:26:38 -0700 Subject: [PATCH 3/8] ++ --- impeller/entity/entity_unittests.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 02a70fd47454d..6a09e3dd9f3ce 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -1446,11 +1446,14 @@ TEST_P(EntityTest, DrawAtlasNoColor) { ASSERT_TRUE(OpenPlaygroundHere(std::move(e))); } -TEST_P(EntityTest, F16WideGamutDrawAtlasPlus) { +TEST_P(EntityTest, DrawAtlasPlusWideGamut) { if (GetParam() != PlaygroundBackend::kMetal) { GTEST_SKIP_("This backend doesn't yet support wide gamut."); } + EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(), + PixelFormat::kR16G16B16A16Float); + // Draws the image as four squares stiched together. auto atlas = CreateTextureForFixture("bay_bridge.jpg"); auto size = atlas->GetSize(); From d0c8a3dbf51d95f21c61491a2398aa42db23e341 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 29 Mar 2024 16:27:52 -0700 Subject: [PATCH 4/8] ++ --- impeller/entity/shaders/blending/porter_duff_blend.frag | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/impeller/entity/shaders/blending/porter_duff_blend.frag b/impeller/entity/shaders/blending/porter_duff_blend.frag index 3da0051171c1b..f9f129fdcf781 100644 --- a/impeller/entity/shaders/blending/porter_duff_blend.frag +++ b/impeller/entity/shaders/blending/porter_duff_blend.frag @@ -36,6 +36,10 @@ f16vec4 Sample(f16sampler2D texture_sampler, vec2 texture_coords) { return IPHalfSampleDecal(texture_sampler, texture_coords); } +float16_t ClampAlpha(float16_t alpha) { + return clamp(alpha, 0.0hf, 1.0hf); +} + void main() { f16vec4 dst = texture(texture_sampler_dst, v_texture_coords) * frag_info.input_alpha; @@ -49,6 +53,5 @@ void main() { // correctly in wide gamut. Remove if we switch to a fixed point extended // range format. // See https://github.com/flutter/flutter/issues/145933 . - float16_t clamped_alpha = clamp(frag_color.a, 0.0hf, 1.0hf); - frag_color.a = clamped_alpha; + frag_color.a = ClampAlpha(frag_color.a); } From 02155946d3032b27d7d0b24f5a4f297bccfd5f20 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 29 Mar 2024 16:33:39 -0700 Subject: [PATCH 5/8] fix compilation. --- impeller/entity/shaders/blending/porter_duff_blend.frag | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/impeller/entity/shaders/blending/porter_duff_blend.frag b/impeller/entity/shaders/blending/porter_duff_blend.frag index f9f129fdcf781..5f2629bb6f6ab 100644 --- a/impeller/entity/shaders/blending/porter_duff_blend.frag +++ b/impeller/entity/shaders/blending/porter_duff_blend.frag @@ -37,7 +37,9 @@ f16vec4 Sample(f16sampler2D texture_sampler, vec2 texture_coords) { } float16_t ClampAlpha(float16_t alpha) { - return clamp(alpha, 0.0hf, 1.0hf); + float16_t min = 0.0hf; + float16_t max = 1.0hf; + return clamp(alpha, min, max); } void main() { From 4dc7507368ff05d732a1abb73ffce329954151af Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 29 Mar 2024 18:26:38 -0700 Subject: [PATCH 6/8] malioc diff. --- impeller/tools/malioc.json | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/impeller/tools/malioc.json b/impeller/tools/malioc.json index ae4d808c54a69..028e2f9443f03 100644 --- a/impeller/tools/malioc.json +++ b/impeller/tools/malioc.json @@ -8434,8 +8434,8 @@ "varying" ], "longest_path_cycles": [ - 0.21875, - 0.21875, + 0.234375, + 0.234375, 0.0, 0.0, 0.0, @@ -8455,8 +8455,8 @@ "varying" ], "shortest_path_cycles": [ - 0.21875, - 0.21875, + 0.234375, + 0.234375, 0.0, 0.0, 0.0, @@ -8467,8 +8467,8 @@ "varying" ], "total_cycles": [ - 0.21875, - 0.21875, + 0.234375, + 0.234375, 0.0, 0.0, 0.0, From ab0183035193ae9815b57a246e11653a0319f289 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 2 Apr 2024 09:15:10 -0700 Subject: [PATCH 7/8] move to aiks tests. --- impeller/aiks/aiks_unittests.cc | 36 +++++++++++++++++++++++++ impeller/entity/entity_unittests.cc | 42 ----------------------------- 2 files changed, 36 insertions(+), 42 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 96354f78d14c8..aef97c8855d18 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -3073,6 +3073,42 @@ TEST_P(AiksTest, MipmapGenerationWorksCorrectly) { ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); } +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); + + // Draws the image as four squares stiched together. + auto atlas = + std::make_shared(CreateTextureForFixture("bay_bridge.jpg")); + auto size = atlas->GetSize(); + // Divide image into four quadrants. + Scalar half_width = size.width / 2; + Scalar half_height = size.height / 2; + std::vector texture_coordinates = { + Rect::MakeLTRB(0, 0, half_width, half_height), + Rect::MakeLTRB(half_width, 0, size.width, half_height), + Rect::MakeLTRB(0, half_height, half_width, size.height), + Rect::MakeLTRB(half_width, half_height, size.width, size.height)}; + // Position quadrants adjacent to eachother. + std::vector transforms = { + Matrix::MakeTranslation({0, 0, 0}), + Matrix::MakeTranslation({half_width, 0, 0}), + Matrix::MakeTranslation({0, half_height, 0}), + Matrix::MakeTranslation({half_width, half_height, 0})}; + std::vector colors = {Color::Red(), Color::Green(), Color::Blue(), + Color::Yellow()}; + + Canvas canvas; + canvas.DrawAtlas(atlas, transforms, texture_coordinates, colors, + BlendMode::kPlus, {}, std::nullopt, {}); + + ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); +} + } // namespace testing } // namespace impeller diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 6a09e3dd9f3ce..ba3fccbc38e31 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -1446,48 +1446,6 @@ TEST_P(EntityTest, DrawAtlasNoColor) { ASSERT_TRUE(OpenPlaygroundHere(std::move(e))); } -TEST_P(EntityTest, DrawAtlasPlusWideGamut) { - if (GetParam() != PlaygroundBackend::kMetal) { - GTEST_SKIP_("This backend doesn't yet support wide gamut."); - } - - EXPECT_EQ(GetContext()->GetCapabilities()->GetDefaultColorFormat(), - PixelFormat::kR16G16B16A16Float); - - // Draws the image as four squares stiched together. - auto atlas = CreateTextureForFixture("bay_bridge.jpg"); - auto size = atlas->GetSize(); - // Divide image into four quadrants. - Scalar half_width = size.width / 2; - Scalar half_height = size.height / 2; - std::vector texture_coordinates = { - Rect::MakeLTRB(0, 0, half_width, half_height), - Rect::MakeLTRB(half_width, 0, size.width, half_height), - Rect::MakeLTRB(0, half_height, half_width, size.height), - Rect::MakeLTRB(half_width, half_height, size.width, size.height)}; - // Position quadrants adjacent to eachother. - std::vector transforms = { - Matrix::MakeTranslation({0, 0, 0}), - Matrix::MakeTranslation({half_width, 0, 0}), - Matrix::MakeTranslation({0, half_height, 0}), - Matrix::MakeTranslation({half_width, half_height, 0})}; - std::vector colors = {Color::Red(), Color::Green(), Color::Blue(), - Color::Yellow()}; - std::shared_ptr contents = std::make_shared(); - - contents->SetTransforms(std::move(transforms)); - contents->SetTextureCoordinates(std::move(texture_coordinates)); - contents->SetTexture(atlas); - contents->SetColors(colors); - contents->SetBlendMode(BlendMode::kPlus); - - Entity e; - e.SetTransform(Matrix::MakeScale(GetContentScale())); - e.SetContents(contents); - - ASSERT_TRUE(OpenPlaygroundHere(std::move(e))); -} - TEST_P(EntityTest, DrawAtlasWithColorAdvanced) { // Draws the image as four squares stiched together. auto atlas = CreateTextureForFixture("bay_bridge.jpg"); From 4485f597d193b0b9c1fdd270acfeaef77ea1d464 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 2 Apr 2024 10:04:18 -0700 Subject: [PATCH 8/8] fix impeller goldens diff. --- testing/impeller_golden_tests_output.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/testing/impeller_golden_tests_output.txt b/testing/impeller_golden_tests_output.txt index 47b290d6c968b..e6b25f7ae7151 100644 --- a/testing/impeller_golden_tests_output.txt +++ b/testing/impeller_golden_tests_output.txt @@ -436,6 +436,7 @@ impeller_Play_AiksTest_DrawAdvancedBlendPartlyOffscreen_Vulkan.png impeller_Play_AiksTest_DrawAtlasAdvancedAndTransform_Metal.png impeller_Play_AiksTest_DrawAtlasAdvancedAndTransform_OpenGLES.png impeller_Play_AiksTest_DrawAtlasAdvancedAndTransform_Vulkan.png +impeller_Play_AiksTest_DrawAtlasPlusWideGamut_Metal.png impeller_Play_AiksTest_DrawAtlasWithColorAdvancedAndTransform_Metal.png impeller_Play_AiksTest_DrawAtlasWithColorAdvancedAndTransform_OpenGLES.png impeller_Play_AiksTest_DrawAtlasWithColorAdvancedAndTransform_Vulkan.png