From 549dd82f2ff1ab43d78be4285e0bc110e5bccb7a Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Fri, 21 Apr 2023 08:58:05 -0700 Subject: [PATCH] [Impeller] Fix division by zero for transparent shadows (flutter/engine#41391) A division by zero happens if the shadow color is fully transparent, and the NaNs are getting assigned to the RGB values of the paint color being handed to the rrect shadow draw. This doesn't cause a problem when wide gamut is off because NaN output ends up being interpreted as zero (thus making the shadow output fully transparent black, which happens to be the expected and correct result). However, when a wide gamut attachment is present, the NaN output ends up being interpreted as a negative value. Reproduction app: ```dart import 'package:flutter/material.dart'; void main() => runApp(const GeneralDialogApp()); class EvilPainter extends CustomPainter { @override void paint(Canvas canvas, Size size) { final Rect rect = Offset.zero & size; canvas.drawPaint(Paint()..color = Colors.white); canvas.saveLayer(null, Paint()..blendMode = BlendMode.srcOver); canvas.drawShadow(Path()..addRect(Rect.fromLTRB(100, 100, 300, 300)), Colors.black54, 15, false); canvas.drawShadow(Path()..addRect(Rect.fromLTRB(100, 100, 300, 300)), Colors.black54, 15, false); canvas.drawShadow(Path()..addRect(Rect.fromLTRB(100, 100, 300, 300)), Colors.transparent, 15, false); canvas.restore(); } @override bool shouldRepaint(EvilPainter oldDelegate) => false; @override bool shouldRebuildSemantics(EvilPainter oldDelegate) => false; } class GeneralDialogApp extends StatelessWidget { const GeneralDialogApp({super.key}); @override Widget build(BuildContext context) { return MaterialApp( restorationScopeId: 'app', home: CustomPaint(painter: EvilPainter()), ); } } ``` Before: ![image](https://user-images.githubusercontent.com/919017/233558512-0bfdcdc0-017d-4df3-870f-bd8808ef73b7.png) After: ![image](https://user-images.githubusercontent.com/919017/233558076-b56c7f43-c81e-4ca6-897d-246251dae930.png) --- .../impeller/display_list/dl_dispatcher.cc | 2 +- .../impeller/display_list/dl_unittests.cc | 36 +++++++++++++++++++ .../entity/contents/rrect_shadow_contents.cc | 5 +++ .../entity/contents/rrect_shadow_contents.h | 2 ++ 4 files changed, 44 insertions(+), 1 deletion(-) diff --git a/engine/src/flutter/impeller/display_list/dl_dispatcher.cc b/engine/src/flutter/impeller/display_list/dl_dispatcher.cc index 925d6515b8..d3eb8f54ac 100644 --- a/engine/src/flutter/impeller/display_list/dl_dispatcher.cc +++ b/engine/src/flutter/impeller/display_list/dl_dispatcher.cc @@ -1103,7 +1103,7 @@ void DlDispatcher::drawShadow(const SkPath& path, Scalar color_scale = color_alpha * (1 - greyscale_alpha); Scalar tonal_alpha = color_scale + greyscale_alpha; - Scalar unpremul_scale = color_scale / tonal_alpha; + Scalar unpremul_scale = tonal_alpha != 0 ? color_scale / tonal_alpha : 0; spot_color = Color(unpremul_scale * spot_color.red, unpremul_scale * spot_color.green, unpremul_scale * spot_color.blue, tonal_alpha); diff --git a/engine/src/flutter/impeller/display_list/dl_unittests.cc b/engine/src/flutter/impeller/display_list/dl_unittests.cc index e2668dd043..9029d3ea7f 100644 --- a/engine/src/flutter/impeller/display_list/dl_unittests.cc +++ b/engine/src/flutter/impeller/display_list/dl_unittests.cc @@ -17,10 +17,14 @@ #include "flutter/display_list/effects/dl_image_filter.h" #include "flutter/display_list/effects/dl_mask_filter.h" #include "flutter/testing/testing.h" +#include "gtest/gtest.h" +#include "impeller/display_list/dl_dispatcher.h" #include "impeller/display_list/dl_image_impeller.h" #include "impeller/display_list/dl_playground.h" +#include "impeller/entity/contents/rrect_shadow_contents.h" #include "impeller/geometry/constants.h" #include "impeller/geometry/point.h" +#include "impeller/geometry/scalar.h" #include "impeller/playground/widgets.h" #include "impeller/scene/node.h" #include "third_party/imgui/imgui.h" @@ -825,6 +829,38 @@ TEST_P(DisplayListTest, CanDrawShadow) { ASSERT_TRUE(OpenPlaygroundHere(builder.Build())); } +TEST_P(DisplayListTest, TransparentShadowProducesCorrectColor) { + flutter::DisplayListBuilder builder; + { + builder.Save(); + builder.Scale(1.618, 1.618); + builder.DrawShadow(SkPath{}.addRect(SkRect::MakeXYWH(0, 0, 200, 100)), + SK_ColorTRANSPARENT, 15, false, 1); + builder.Restore(); + } + auto dl = builder.Build(); + + DlDispatcher dispatcher; + dispatcher.drawDisplayList(dl, 1); + auto picture = dispatcher.EndRecordingAsPicture(); + + std::shared_ptr rrect_shadow; + picture.pass->IterateAllEntities([&rrect_shadow](Entity& entity) { + if (ScalarNearlyEqual(entity.GetTransformation().GetScale().x, 1.618f)) { + rrect_shadow = + std::static_pointer_cast(entity.GetContents()); + return false; + } + return true; + }); + + ASSERT_NE(rrect_shadow, nullptr); + ASSERT_EQ(rrect_shadow->GetColor().red, 0); + ASSERT_EQ(rrect_shadow->GetColor().green, 0); + ASSERT_EQ(rrect_shadow->GetColor().blue, 0); + ASSERT_EQ(rrect_shadow->GetColor().alpha, 0); +} + // Draw a hexagon using triangle fan TEST_P(DisplayListTest, CanConvertTriangleFanToTriangles) { constexpr Scalar hexagon_radius = 125; diff --git a/engine/src/flutter/impeller/entity/contents/rrect_shadow_contents.cc b/engine/src/flutter/impeller/entity/contents/rrect_shadow_contents.cc index 228d7c9994..4812ebea63 100644 --- a/engine/src/flutter/impeller/entity/contents/rrect_shadow_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/rrect_shadow_contents.cc @@ -7,6 +7,7 @@ #include "impeller/entity/contents/content_context.h" #include "impeller/entity/entity.h" +#include "impeller/geometry/color.h" #include "impeller/geometry/path.h" #include "impeller/geometry/path_builder.h" #include "impeller/renderer/render_pass.h" @@ -33,6 +34,10 @@ void RRectShadowContents::SetColor(Color color) { color_ = color.Premultiply(); } +Color RRectShadowContents::GetColor() const { + return color_; +} + std::optional RRectShadowContents::GetCoverage( const Entity& entity) const { if (!rect_.has_value()) { diff --git a/engine/src/flutter/impeller/entity/contents/rrect_shadow_contents.h b/engine/src/flutter/impeller/entity/contents/rrect_shadow_contents.h index d6d46b7f52..3c459d9f95 100644 --- a/engine/src/flutter/impeller/entity/contents/rrect_shadow_contents.h +++ b/engine/src/flutter/impeller/entity/contents/rrect_shadow_contents.h @@ -30,6 +30,8 @@ class RRectShadowContents final : public Contents { void SetColor(Color color); + Color GetColor() const; + // |Contents| std::optional GetCoverage(const Entity& entity) const override;