Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] fixes behavior for blurred rounded rect clear #46167

Merged
gaaclarke merged 8 commits into from
Sep 26, 2023
Merged

[Impeller] fixes behavior for blurred rounded rect clear #46167

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
53a0d03
added test
gaaclarke Sep 25, 2023
12872e1
removed hack from test
gaaclarke Sep 25, 2023
e5b52cf
added other test
gaaclarke Sep 25, 2023
dbfe2e1
added image test
gaaclarke Sep 25, 2023
b87acb6
format
gaaclarke Sep 25, 2023
98cec73
[Impeller] fixed the behavior for clear
gaaclarke Sep 21, 2023
2a31a30
removed clearblendimage
gaaclarke Sep 26, 2023
fc1adcb
switched tests to use a blue square to account for clear color in ski…
gaaclarke Sep 26, 2023
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
46 changes: 46 additions & 0 deletions impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3581,5 +3581,51 @@ TEST_P(AiksTest, VerticesGeometryUVPositionData) {
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, ClearBlendWithBlur) {
Canvas canvas;
Paint white;
white.color = Color::White();
canvas.DrawRect(Rect::MakeXYWH(0, 0, 600.0, 600.0), white);

Paint clear;
clear.blend_mode = BlendMode::kClear;
clear.mask_blur_descriptor = Paint::MaskBlurDescriptor{
.style = FilterContents::BlurStyle::kNormal,
.sigma = Sigma(20),
};

canvas.DrawCircle(Point::MakeXY(300.0, 300.0), 200.0, clear);

ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, ClearBlend) {
Canvas canvas;
Paint white;
white.color = Color::White();
canvas.DrawRect(Rect::MakeXYWH(0, 0, 600.0, 600.0), white);

Paint clear;
clear.blend_mode = BlendMode::kClear;

canvas.DrawCircle(Point::MakeXY(300.0, 300.0), 200.0, clear);

ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

TEST_P(AiksTest, ClearBlendImage) {
Copy link
Member

@bdero bdero Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, there's actually already a golden for this, but it's kind of sneaky because the correct behavior is for it to render nothing, unlike all the other blend variants. :) AiksTest.BlendModeClear

It looks like both the existing golden and this new golden aren't quite rendering correctly with this change: https://api.flutter.dev/flutter/dart-ui/BlendMode.html#clear

Copy link
Member Author

@gaaclarke gaaclarke Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, the golden results are surprising, there is a lot to unpack:

  • The new clear test impeller_Play_AiksTest_ClearBlend_* renders nothing correctly
  • impeller_Play_AiksTest_ClearBlendWithBlur_Vulkan gives me different results locally with moltenvk
  • The existing clear test impeller_Play_AiksTest_BlendModeClear_Metal rendered nothing incorrectly, but this PR seems to have broken that (I would have expected my change to only effect rrect blur). This is also surprising considering impeller_Play_AiksTest_ClearBlend_*
  • impeller_Play_AiksTest_ClearBlendImage_Vulkan is also giving me different results locally

I'll see if I can get this sorted out.

local impeller_Play_AiksTest_ClearBlendImage_Vulkan

Screenshot 2023-09-26 at 8 50 25 AM

local impeller_Play_AiksTest_ClearBlendWithBlur_Vulkan

Screenshot 2023-09-26 at 8 50 09 AM

Copy link
Member Author

@gaaclarke gaaclarke Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had been working with the vulkan tests, just verified that locally the metal tests give different/correct results. I wonder if skiagold got messed up. It is reporting 98cec73 though.

Copy link
Member Author

@gaaclarke gaaclarke Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed an issue about the discrepancy between skiagold and local results: flutter/flutter#135507

I thought maybe it was a moltenvk thing, but metal and vulkan are giving the same correct results locally which differ from skiagolds results.

Copy link
Member Author

@gaaclarke gaaclarke Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed ClearBlendImage since it's a duplicate. We'll also see if pushing a new commit flushes out the skiagold issue.

Copy link
Member

@bdero bdero Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, weird. Yeah I couldn't spot anything that should cause the existing golden to fail yesterday. Thanks for digging in!

Canvas canvas;
Paint paint;
auto dst =
std::make_shared<Image>(CreateTextureForFixture("blend_mode_dst.png"));
auto src =
std::make_shared<Image>(CreateTextureForFixture("blend_mode_src.png"));
canvas.DrawImage(dst, Point::MakeXY(100.0, 100.0), paint);
Paint clear;
clear.blend_mode = BlendMode::kClear;
canvas.DrawImage(src, Point::MakeXY(100.0, 100.0), clear);
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

} // namespace testing
} // namespace impeller
17 changes: 13 additions & 4 deletions impeller/entity/contents/content_context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,19 @@ void ContentContextOptions::ApplyToPipelineDescriptor(

switch (pipeline_blend) {
case BlendMode::kClear:
color0.dst_alpha_blend_factor = BlendFactor::kZero;
color0.dst_color_blend_factor = BlendFactor::kZero;
color0.src_alpha_blend_factor = BlendFactor::kZero;
color0.src_color_blend_factor = BlendFactor::kZero;
if (is_for_rrect_blur_clear) {
color0.alpha_blend_op = BlendOperation::kReverseSubtract;
color0.color_blend_op = BlendOperation::kReverseSubtract;
color0.dst_alpha_blend_factor = BlendFactor::kOne;
color0.dst_color_blend_factor = BlendFactor::kOne;
color0.src_alpha_blend_factor = BlendFactor::kDestinationColor;
color0.src_color_blend_factor = BlendFactor::kDestinationColor;
Copy link
Member

@bdero bdero Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will cause the clear to weaken as the paint color becomes more transparent. Is that behavior correct?
I wonder if Skia is only employing this trick when the color source is entirely opaque.

Copy link
Member Author

@gaaclarke gaaclarke Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, thats how you get the blurred results (that work locally):
Screenshot 2023-09-26 at 8 50 09 AM

Copy link
Member

@bdero bdero Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried this out with wacky transparent colors in your repro and this seems to work great!

} else {
color0.dst_alpha_blend_factor = BlendFactor::kZero;
color0.dst_color_blend_factor = BlendFactor::kZero;
color0.src_alpha_blend_factor = BlendFactor::kZero;
color0.src_color_blend_factor = BlendFactor::kZero;
}
break;
case BlendMode::kSource:
color0.blending_enabled = false;
Expand Down
12 changes: 7 additions & 5 deletions impeller/entity/contents/content_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,13 +308,14 @@ struct ContentContextOptions {
PixelFormat color_attachment_pixel_format = PixelFormat::kUnknown;
bool has_stencil_attachment = true;
bool wireframe = false;
bool is_for_rrect_blur_clear = false;

struct Hash {
constexpr std::size_t operator()(const ContentContextOptions& o) const {
return fml::HashCombine(o.sample_count, o.blend_mode, o.stencil_compare,
o.stencil_operation, o.primitive_type,
o.color_attachment_pixel_format,
o.has_stencil_attachment, o.wireframe);
return fml::HashCombine(
o.sample_count, o.blend_mode, o.stencil_compare, o.stencil_operation,
o.primitive_type, o.color_attachment_pixel_format,
o.has_stencil_attachment, o.wireframe, o.is_for_rrect_blur_clear);
}
};

Expand All @@ -329,7 +330,8 @@ struct ContentContextOptions {
lhs.color_attachment_pixel_format ==
rhs.color_attachment_pixel_format &&
lhs.has_stencil_attachment == rhs.has_stencil_attachment &&
lhs.wireframe == rhs.wireframe;
lhs.wireframe == rhs.wireframe &&
lhs.is_for_rrect_blur_clear == rhs.is_for_rrect_blur_clear;
}
};

Expand Down
9 changes: 7 additions & 2 deletions impeller/entity/contents/solid_rrect_blur_contents.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,13 @@ bool SolidRRectBlurContents::Render(const ContentContext& renderer,

Command cmd;
DEBUG_COMMAND_INFO(cmd, "RRect Shadow");
auto opts = OptionsFromPassAndEntity(pass, entity);
ContentContextOptions opts = OptionsFromPassAndEntity(pass, entity);
opts.primitive_type = PrimitiveType::kTriangle;
Color color = color_;
if (entity.GetBlendMode() == BlendMode::kClear) {
opts.is_for_rrect_blur_clear = true;
color = Color::White();
}
cmd.pipeline = renderer.GetRRectBlurPipeline(opts);
cmd.stencil_reference = entity.GetStencilDepth();

Expand All @@ -102,7 +107,7 @@ bool SolidRRectBlurContents::Render(const ContentContext& renderer,
VS::BindFrameInfo(cmd, pass.GetTransientsBuffer().EmplaceUniform(frame_info));

FS::FragInfo frag_info;
frag_info.color = color_;
frag_info.color = color;
frag_info.blur_sigma = blur_sigma;
frag_info.rect_size = Point(positive_rect.size);
frag_info.corner_radius =
Expand Down