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

Commit 5c35213

Browse files
flarflar
authored
fix bounds of inverted rendered rectangles (#42556)
Fixes: flutter/flutter#128159 In diagnosing test failures for #42330 we discovered that the DisplayList code was not always backwards compatible with computing the bounds of inverted rectangles (where left > right or top > bottom). Historically such rectangles were always rendered as if they were sorted (i.e. `SkRect::makeSorted()`), but we computed bounds as if the bounds only mattered if the supplied rectangle was ordered. So, we would sometimes render a rectangle for which we mis-computed the bounds. This would rarely surface in the current code as most rendered rectangles would pass through `SkMatrix::mapRect()` which implicitly orders the rectangle as it transforms it, but any attributes applied to the bounds before that method may have been applied "in the wrong direction" - such as: - stroke width padding - mask blur padding - image filter padding
1 parent df7ab67 commit 5c35213

File tree

2 files changed

+144
-2
lines changed

2 files changed

+144
-2
lines changed

‎display_list/display_list_unittests.cc

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,95 @@ class DisplayListTestBase : public BaseT {
124124
EXPECT_EQ(builder.GetSaveCount(), 1);
125125
}
126126

127+
typedef const std::function<void(DlCanvas&)> DlSetup;
128+
typedef const std::function<void(DlCanvas&, DlPaint&, SkRect& rect)>
129+
DlRenderer;
130+
131+
static void verify_inverted_bounds(DlSetup& setup,
132+
DlRenderer& renderer,
133+
DlPaint paint,
134+
SkRect render_rect,
135+
SkRect expected_bounds,
136+
const std::string& desc) {
137+
DisplayListBuilder builder;
138+
setup(builder);
139+
renderer(builder, paint, render_rect);
140+
auto dl = builder.Build();
141+
EXPECT_EQ(dl->op_count(), 1u) << desc;
142+
EXPECT_EQ(dl->bounds(), expected_bounds) << desc;
143+
}
144+
145+
static void check_inverted_bounds(DlRenderer& renderer,
146+
const std::string& desc) {
147+
SkRect rect = SkRect::MakeLTRB(0.0f, 0.0f, 10.0f, 10.0f);
148+
SkRect invertedLR = SkRect::MakeLTRB(rect.fRight, rect.fTop, //
149+
rect.fLeft, rect.fBottom);
150+
SkRect invertedTB = SkRect::MakeLTRB(rect.fLeft, rect.fBottom, //
151+
rect.fRight, rect.fTop);
152+
SkRect invertedLTRB = SkRect::MakeLTRB(rect.fRight, rect.fBottom, //
153+
rect.fLeft, rect.fTop);
154+
auto empty_setup = [](DlCanvas&) {};
155+
156+
ASSERT_TRUE(rect.fLeft < rect.fRight);
157+
ASSERT_TRUE(rect.fTop < rect.fBottom);
158+
ASSERT_FALSE(rect.isEmpty());
159+
ASSERT_TRUE(invertedLR.fLeft > invertedLR.fRight);
160+
ASSERT_TRUE(invertedLR.isEmpty());
161+
ASSERT_TRUE(invertedTB.fTop > invertedTB.fBottom);
162+
ASSERT_TRUE(invertedTB.isEmpty());
163+
ASSERT_TRUE(invertedLTRB.fLeft > invertedLTRB.fRight);
164+
ASSERT_TRUE(invertedLTRB.fTop > invertedLTRB.fBottom);
165+
ASSERT_TRUE(invertedLTRB.isEmpty());
166+
167+
DlPaint ref_paint = DlPaint();
168+
SkRect ref_bounds = rect;
169+
verify_inverted_bounds(empty_setup, renderer, ref_paint, invertedLR,
170+
ref_bounds, desc + " LR swapped");
171+
verify_inverted_bounds(empty_setup, renderer, ref_paint, invertedTB,
172+
ref_bounds, desc + " TB swapped");
173+
verify_inverted_bounds(empty_setup, renderer, ref_paint, invertedLTRB,
174+
ref_bounds, desc + " LR&TB swapped");
175+
176+
// Round joins are used because miter joins greatly pad the bounds,
177+
// but only on paths. So we use round joins for consistency there.
178+
// We aren't fully testing all stroke-related bounds computations here,
179+
// those are more fully tested in the render tests. We are simply
180+
// checking that they are applied to the ordered bounds.
181+
DlPaint stroke_paint = DlPaint() //
182+
.setDrawStyle(DlDrawStyle::kStroke) //
183+
.setStrokeJoin(DlStrokeJoin::kRound) //
184+
.setStrokeWidth(2.0f);
185+
SkRect stroke_bounds = rect.makeOutset(1.0f, 1.0f);
186+
verify_inverted_bounds(empty_setup, renderer, stroke_paint, invertedLR,
187+
stroke_bounds, desc + " LR swapped, sw 2");
188+
verify_inverted_bounds(empty_setup, renderer, stroke_paint, invertedTB,
189+
stroke_bounds, desc + " TB swapped, sw 2");
190+
verify_inverted_bounds(empty_setup, renderer, stroke_paint, invertedLTRB,
191+
stroke_bounds, desc + " LR&TB swapped, sw 2");
192+
193+
DlBlurMaskFilter mask_filter(DlBlurStyle::kNormal, 2.0f);
194+
DlPaint maskblur_paint = DlPaint() //
195+
.setMaskFilter(&mask_filter);
196+
SkRect maskblur_bounds = rect.makeOutset(6.0f, 6.0f);
197+
verify_inverted_bounds(empty_setup, renderer, maskblur_paint, invertedLR,
198+
maskblur_bounds, desc + " LR swapped, mask 2");
199+
verify_inverted_bounds(empty_setup, renderer, maskblur_paint, invertedTB,
200+
maskblur_bounds, desc + " TB swapped, mask 2");
201+
verify_inverted_bounds(empty_setup, renderer, maskblur_paint, invertedLTRB,
202+
maskblur_bounds, desc + " LR&TB swapped, mask 2");
203+
204+
DlErodeImageFilter erode_filter(2.0f, 2.0f);
205+
DlPaint erode_paint = DlPaint() //
206+
.setImageFilter(&erode_filter);
207+
SkRect erode_bounds = rect.makeInset(2.0f, 2.0f);
208+
verify_inverted_bounds(empty_setup, renderer, erode_paint, invertedLR,
209+
erode_bounds, desc + " LR swapped, erode 2");
210+
verify_inverted_bounds(empty_setup, renderer, erode_paint, invertedTB,
211+
erode_bounds, desc + " TB swapped, erode 2");
212+
verify_inverted_bounds(empty_setup, renderer, erode_paint, invertedLTRB,
213+
erode_bounds, desc + " LR&TB swapped, erode 2");
214+
}
215+
127216
private:
128217
FML_DISALLOW_COPY_AND_ASSIGN(DisplayListTestBase);
129218
};
@@ -2874,5 +2963,58 @@ TEST_F(DisplayListTest, DrawSaveDrawCannotInheritOpacity) {
28742963
ASSERT_FALSE(display_list->can_apply_group_opacity());
28752964
}
28762965

2966+
TEST_F(DisplayListTest, DrawUnorderedRect) {
2967+
auto renderer = [](DlCanvas& canvas, DlPaint& paint, SkRect& rect) {
2968+
canvas.DrawRect(rect, paint);
2969+
};
2970+
check_inverted_bounds(renderer, "DrawRect");
2971+
}
2972+
2973+
TEST_F(DisplayListTest, DrawUnorderedRoundRect) {
2974+
auto renderer = [](DlCanvas& canvas, DlPaint& paint, SkRect& rect) {
2975+
canvas.DrawRRect(SkRRect::MakeRectXY(rect, 2.0f, 2.0f), paint);
2976+
};
2977+
check_inverted_bounds(renderer, "DrawRoundRect");
2978+
}
2979+
2980+
TEST_F(DisplayListTest, DrawUnorderedOval) {
2981+
auto renderer = [](DlCanvas& canvas, DlPaint& paint, SkRect& rect) {
2982+
canvas.DrawOval(rect, paint);
2983+
};
2984+
check_inverted_bounds(renderer, "DrawOval");
2985+
}
2986+
2987+
TEST_F(DisplayListTest, DrawUnorderedRectangularPath) {
2988+
auto renderer = [](DlCanvas& canvas, DlPaint& paint, SkRect& rect) {
2989+
canvas.DrawPath(SkPath().addRect(rect), paint);
2990+
};
2991+
check_inverted_bounds(renderer, "DrawRectangularPath");
2992+
}
2993+
2994+
TEST_F(DisplayListTest, DrawUnorderedOvalPath) {
2995+
auto renderer = [](DlCanvas& canvas, DlPaint& paint, SkRect& rect) {
2996+
canvas.DrawPath(SkPath().addOval(rect), paint);
2997+
};
2998+
check_inverted_bounds(renderer, "DrawOvalPath");
2999+
}
3000+
3001+
TEST_F(DisplayListTest, DrawUnorderedRoundRectPathCW) {
3002+
auto renderer = [](DlCanvas& canvas, DlPaint& paint, SkRect& rect) {
3003+
SkPath path = SkPath() //
3004+
.addRoundRect(rect, 2.0f, 2.0f, SkPathDirection::kCW);
3005+
canvas.DrawPath(path, paint);
3006+
};
3007+
check_inverted_bounds(renderer, "DrawRoundRectPath Clockwise");
3008+
}
3009+
3010+
TEST_F(DisplayListTest, DrawUnorderedRoundRectPathCCW) {
3011+
auto renderer = [](DlCanvas& canvas, DlPaint& paint, SkRect& rect) {
3012+
SkPath path = SkPath() //
3013+
.addRoundRect(rect, 2.0f, 2.0f, SkPathDirection::kCCW);
3014+
canvas.DrawPath(path, paint);
3015+
};
3016+
check_inverted_bounds(renderer, "DrawRoundRectPath Counter-Clockwise");
3017+
}
3018+
28773019
} // namespace testing
28783020
} // namespace flutter

‎display_list/dl_builder.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -751,7 +751,7 @@ void DisplayListBuilder::DrawLine(const SkPoint& p0,
751751
void DisplayListBuilder::drawRect(const SkRect& rect) {
752752
Push<DrawRectOp>(0, 1, rect);
753753
CheckLayerOpacityCompatibility();
754-
AccumulateOpBounds(rect, kDrawRectFlags);
754+
AccumulateOpBounds(rect.makeSorted(), kDrawRectFlags);
755755
}
756756
void DisplayListBuilder::DrawRect(const SkRect& rect, const DlPaint& paint) {
757757
SetAttributesFromPaint(paint, DisplayListOpFlags::kDrawRectFlags);
@@ -760,7 +760,7 @@ void DisplayListBuilder::DrawRect(const SkRect& rect, const DlPaint& paint) {
760760
void DisplayListBuilder::drawOval(const SkRect& bounds) {
761761
Push<DrawOvalOp>(0, 1, bounds);
762762
CheckLayerOpacityCompatibility();
763-
AccumulateOpBounds(bounds, kDrawOvalFlags);
763+
AccumulateOpBounds(bounds.makeSorted(), kDrawOvalFlags);
764764
}
765765
void DisplayListBuilder::DrawOval(const SkRect& bounds, const DlPaint& paint) {
766766
SetAttributesFromPaint(paint, DisplayListOpFlags::kDrawOvalFlags);

0 commit comments

Comments
 (0)