Revert "Revert "Fix a couple float-cast-overflow in SkScan*.""
This reverts commit b6abb9b4e088abee8b8dfcce9c9f7eb759518198.
Reason for revert: It doesn't make sense that this CL would affect the tests implicated in the perf regression in skia:7143, and the revert had no effect on the perf of those tests. Seems like the perf alert was either noise or due to a different CL.
Original change's description:
> Revert "Fix a couple float-cast-overflow in SkScan*."
>
> This reverts commit 3cd0bef0fd9d062bbcc313c329b4f31925e8ded7.
>
> Reason for revert: https://bugs.chromium.org/p/skia/issues/detail?id=7143
>
> Original change's description:
> > Fix a couple float-cast-overflow in SkScan*.
> >
> > Bug: skia:5060
> > Change-Id: I60a48993c77631aaad9354bb86b13204dc618bf4
> > Reviewed-on: https://skia-review.googlesource.com/47422
> > Commit-Queue: Ben Wagner <benjaminwagner@google.com>
> > Reviewed-by: Mike Reed <reed@google.com>
>
> TBR=benjaminwagner@google.com,reed@google.com
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug: skia:7143
> Change-Id: I0f19720a7d8344789a375bbb6b9e28bf4f4e55ae
> Reviewed-on: https://skia-review.googlesource.com/57240
> Commit-Queue: Ben Wagner <benjaminwagner@google.com>
> Reviewed-by: Ben Wagner <benjaminwagner@google.com>
TBR=benjaminwagner@google.com,reed@google.com
Change-Id: I29ac47d6665e2e52ee2a6500488dc407c8d2af1c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:7143
Reviewed-on: https://skia-review.googlesource.com/57440
Reviewed-by: Ben Wagner <benjaminwagner@google.com>
Commit-Queue: Ben Wagner <benjaminwagner@google.com>
diff --git a/include/core/SkRect.h b/include/core/SkRect.h
index 2cc2604..12afddd 100644
--- a/include/core/SkRect.h
+++ b/include/core/SkRect.h
@@ -411,6 +411,14 @@
return r;
}
+ // The largest SkRect for which round() is well-defined.
+ static SkRect SK_WARN_UNUSED_RESULT MakeLargestS32() {
+ const SkRect r = MakeLTRB(SK_MinS32FitsInFloat, SK_MinS32FitsInFloat,
+ SK_MaxS32FitsInFloat, SK_MaxS32FitsInFloat);
+ SkASSERT(r == Make(r.roundOut()));
+ return r;
+ }
+
static constexpr SkRect SK_WARN_UNUSED_RESULT MakeWH(SkScalar w, SkScalar h) {
return SkRect{0, 0, w, h};
}
diff --git a/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-UBSAN_float_cast_overflow.json b/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-UBSAN_float_cast_overflow.json
index d5de1ce..473302b 100644
--- a/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-UBSAN_float_cast_overflow.json
+++ b/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-UBSAN_float_cast_overflow.json
@@ -457,54 +457,20 @@
"565",
"gm",
"_",
- "bigrect",
- "565",
- "gm",
- "_",
- "clippedcubic2",
- "565",
- "gm",
- "_",
- "conicpaths",
- "8888",
- "gm",
- "_",
- "bigrect",
- "8888",
- "gm",
- "_",
"clippedcubic2",
"8888",
"gm",
"_",
- "conicpaths",
- "f16",
- "gm",
- "_",
- "bigrect",
- "f16",
- "gm",
- "_",
"clippedcubic2",
"f16",
"gm",
"_",
- "conicpaths",
- "srgb",
- "gm",
- "_",
- "bigrect",
- "srgb",
- "gm",
- "_",
"clippedcubic2",
"srgb",
"gm",
"_",
- "conicpaths",
+ "clippedcubic2",
"--match",
- "~^DashPathEffectTest_asPoints_limit$",
- "~^PathBigCubic$",
"~^PathOpsCubicIntersection$",
"~^PathOpsCubicLineIntersection$",
"~^PathOpsOpCubicsThreaded$",
diff --git a/infra/bots/recipes/test.py b/infra/bots/recipes/test.py
index ffda2a1..bd68d92 100644
--- a/infra/bots/recipes/test.py
+++ b/infra/bots/recipes/test.py
@@ -532,11 +532,7 @@
if 'float_cast_overflow' in bot and 'CPU' in bot:
# skia:4632
for config in ['565', '8888', 'f16', 'srgb']:
- blacklist([config, 'gm', '_', 'bigrect'])
blacklist([config, 'gm', '_', 'clippedcubic2'])
- blacklist([config, 'gm', '_', 'conicpaths'])
- match.append('~^DashPathEffectTest_asPoints_limit$')
- match.append('~^PathBigCubic$')
match.append('~^PathOpsCubicIntersection$')
match.append('~^PathOpsCubicLineIntersection$')
match.append('~^PathOpsOpCubicsThreaded$')
diff --git a/src/core/SkDraw.cpp b/src/core/SkDraw.cpp
index bf91aa1..39b6843 100644
--- a/src/core/SkDraw.cpp
+++ b/src/core/SkDraw.cpp
@@ -722,6 +722,16 @@
return SkTCast<SkPoint*>(&r);
}
+static void draw_rect_as_path(const SkDraw& orig, const SkRect& prePaintRect,
+ const SkPaint& paint, const SkMatrix* matrix) {
+ SkDraw draw(orig);
+ draw.fMatrix = matrix;
+ SkPath tmp;
+ tmp.addRect(prePaintRect);
+ tmp.setFillType(SkPath::kWinding_FillType);
+ draw.drawPath(tmp, paint, nullptr, true);
+}
+
void SkDraw::drawRect(const SkRect& prePaintRect, const SkPaint& paint,
const SkMatrix* paintMatrix, const SkRect* postPaintRect) const {
SkDEBUGCODE(this->validate();)
@@ -746,14 +756,7 @@
RectType rtype = ComputeRectType(paint, *fMatrix, &strokeSize);
if (kPath_RectType == rtype) {
- SkDraw draw(*this);
- if (paintMatrix) {
- draw.fMatrix = matrix;
- }
- SkPath tmp;
- tmp.addRect(prePaintRect);
- tmp.setFillType(SkPath::kWinding_FillType);
- draw.drawPath(tmp, paint, nullptr, true);
+ draw_rect_as_path(*this, prePaintRect, paint, matrix);
return;
}
@@ -778,6 +781,12 @@
}
}
+ if (!SkRect::MakeLargestS32().contains(bbox)) {
+ // bbox.roundOut() is undefined; use slow path.
+ draw_rect_as_path(*this, prePaintRect, paint, matrix);
+ return;
+ }
+
SkIRect ir = bbox.roundOut();
if (fRC->quickReject(ir)) {
return;
diff --git a/src/core/SkScanPriv.h b/src/core/SkScanPriv.h
index 03b09ce..96ee695 100644
--- a/src/core/SkScanPriv.h
+++ b/src/core/SkScanPriv.h
@@ -16,7 +16,7 @@
class SkScanClipper {
public:
SkScanClipper(SkBlitter* blitter, const SkRegion* clip, const SkIRect& bounds,
- bool skipRejectTest = false);
+ bool skipRejectTest = false, bool boundsPreClipped = false);
SkBlitter* getBlitter() const { return fBlitter; }
const SkIRect* getClipRect() const { return fClipRect; }
diff --git a/src/core/SkScan_Hairline.cpp b/src/core/SkScan_Hairline.cpp
index ec6acb9..37be030 100644
--- a/src/core/SkScan_Hairline.cpp
+++ b/src/core/SkScan_Hairline.cpp
@@ -143,17 +143,14 @@
// we don't just draw 4 lines, 'cause that can leave a gap in the bottom-right
// and double-hit the top-left.
-// TODO: handle huge coordinates on rect (before calling SkScalarToFixed)
void SkScan::HairRect(const SkRect& rect, const SkRasterClip& clip,
SkBlitter* blitter) {
SkAAClipBlitterWrapper wrapper;
- SkBlitterClipper clipper;
- SkIRect r;
-
- r.set(SkScalarToFixed(rect.fLeft) >> 16,
- SkScalarToFixed(rect.fTop) >> 16,
- (SkScalarToFixed(rect.fRight) >> 16) + 1,
- (SkScalarToFixed(rect.fBottom) >> 16) + 1);
+ SkBlitterClipper clipper;
+ const SkIRect r = SkIRect::MakeLTRB(SkScalarFloorToInt(rect.fLeft),
+ SkScalarFloorToInt(rect.fTop),
+ SkScalarFloorToInt(rect.fRight) + 1,
+ SkScalarFloorToInt(rect.fBottom) + 1);
if (clip.quickReject(r)) {
return;
diff --git a/src/core/SkScan_Path.cpp b/src/core/SkScan_Path.cpp
index 8af0ea6..a8a4321 100644
--- a/src/core/SkScan_Path.cpp
+++ b/src/core/SkScan_Path.cpp
@@ -503,7 +503,7 @@
* is outside of the clip.
*/
SkScanClipper::SkScanClipper(SkBlitter* blitter, const SkRegion* clip,
- const SkIRect& ir, bool skipRejectTest) {
+ const SkIRect& ir, bool skipRejectTest, bool irPreClipped) {
fBlitter = nullptr; // null means blit nothing
fClipRect = nullptr;
@@ -514,7 +514,7 @@
}
if (clip->isRect()) {
- if (fClipRect->contains(ir)) {
+ if (!irPreClipped && fClipRect->contains(ir)) {
#ifdef SK_DEBUG
fRectClipCheckBlitter.init(blitter, *fClipRect);
blitter = &fRectClipCheckBlitter;
@@ -522,7 +522,8 @@
fClipRect = nullptr;
} else {
// only need a wrapper blitter if we're horizontally clipped
- if (fClipRect->fLeft > ir.fLeft || fClipRect->fRight < ir.fRight) {
+ if (irPreClipped ||
+ fClipRect->fLeft > ir.fLeft || fClipRect->fRight < ir.fRight) {
fRectBlitter.init(blitter, *fClipRect);
blitter = &fRectBlitter;
} else {
@@ -639,12 +640,21 @@
}
// don't reference "origClip" any more, just use clipPtr
+
+ SkRect bounds = path.getBounds();
+ bool irPreClipped = false;
+ if (!SkRect::MakeLargestS32().contains(bounds)) {
+ if (!bounds.intersect(SkRect::MakeLargestS32())) {
+ bounds.setEmpty();
+ }
+ irPreClipped = true;
+ }
SkIRect ir;
// We deliberately call round_asymmetric_to_int() instead of round(), since we can't afford
// to generate a bounds that is tighter than the corresponding SkEdges. The edge code basically
// converts the floats to fixed, and then "rounds". If we called round() instead of
// round_asymmetric_to_int() here, we could generate the wrong ir for values like 0.4999997.
- round_asymmetric_to_int(path.getBounds(), &ir);
+ round_asymmetric_to_int(bounds, &ir);
if (ir.isEmpty()) {
if (path.isInverseFillType()) {
blitter->blitRegion(*clipPtr);
@@ -652,7 +662,7 @@
return;
}
- SkScanClipper clipper(blitter, clipPtr, ir, path.isInverseFillType());
+ SkScanClipper clipper(blitter, clipPtr, ir, path.isInverseFillType(), irPreClipped);
blitter = clipper.getBlitter();
if (blitter) {