Skip to content

Commit

Permalink
[Impeller] Fix division by zero for transparent shadows (flutter#41391)
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
bdero committed Apr 21, 2023
1 parent 3f7e12b commit dd67063
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 1 deletion.
2 changes: 1 addition & 1 deletion impeller/display_list/dl_dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
36 changes: 36 additions & 0 deletions impeller/display_list/dl_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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<RRectShadowContents> rrect_shadow;
picture.pass->IterateAllEntities([&rrect_shadow](Entity& entity) {
if (ScalarNearlyEqual(entity.GetTransformation().GetScale().x, 1.618f)) {
rrect_shadow =
std::static_pointer_cast<RRectShadowContents>(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;
Expand Down
5 changes: 5 additions & 0 deletions impeller/entity/contents/rrect_shadow_contents.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -33,6 +34,10 @@ void RRectShadowContents::SetColor(Color color) {
color_ = color.Premultiply();
}

Color RRectShadowContents::GetColor() const {
return color_;
}

std::optional<Rect> RRectShadowContents::GetCoverage(
const Entity& entity) const {
if (!rect_.has_value()) {
Expand Down
2 changes: 2 additions & 0 deletions impeller/entity/contents/rrect_shadow_contents.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ class RRectShadowContents final : public Contents {

void SetColor(Color color);

Color GetColor() const;

// |Contents|
std::optional<Rect> GetCoverage(const Entity& entity) const override;

Expand Down

0 comments on commit dd67063

Please sign in to comment.