add rounding-using-doubles methods on SkScalar and SkRect
Inspired by the excellent repro case for https://crbug.com/364224
patch from issue 265933010
BUG=skia:
R=bungeman@google.com
Author: reed@google.com
Review URL: https://codereview.chromium.org/267003002
git-svn-id: http://skia.googlecode.com/svn/trunk@14566 2bbb7eff-a529-9590-31e7-b0007b416f81
diff --git a/include/core/SkRect.h b/include/core/SkRect.h
index 397e4a0..fd8cb16 100644
--- a/include/core/SkRect.h
+++ b/include/core/SkRect.h
@@ -732,6 +732,24 @@
}
/**
+ * Variant of round() that explicitly performs the rounding step (i.e. floor(x + 0.5)) using
+ * double instead of SkScalar (float). It does this by calling SkDScalarRoundToInt(), which
+ * may be slower than calling SkScalarRountToInt(), but gives slightly more accurate results.
+ *
+ * e.g.
+ * SkScalar x = 0.49999997f;
+ * int ix = SkScalarRoundToInt(x);
+ * SkASSERT(0 == ix); // <--- fails
+ * ix = SkDScalarRoundToInt(x);
+ * SkASSERT(0 == ix); // <--- succeeds
+ */
+ void dround(SkIRect* dst) const {
+ SkASSERT(dst);
+ dst->set(SkDScalarRoundToInt(fLeft), SkDScalarRoundToInt(fTop),
+ SkDScalarRoundToInt(fRight), SkDScalarRoundToInt(fBottom));
+ }
+
+ /**
* Set the dst rectangle by rounding "out" this rectangle, choosing the
* SkScalarFloor of top and left, and the SkScalarCeil of right and bottom.
*/
diff --git a/include/core/SkScalar.h b/include/core/SkScalar.h
index b9256ba..b37cf5c 100644
--- a/include/core/SkScalar.h
+++ b/include/core/SkScalar.h
@@ -83,6 +83,26 @@
#define SkScalarRoundToInt(x) sk_float_round2int(x)
#define SkScalarTruncToInt(x) static_cast<int>(x)
+/**
+ * Variant of SkScalarRoundToInt, that performs the rounding step (adding 0.5) explicitly using
+ * double, to avoid possibly losing the low bit(s) of the answer before calling floor().
+ *
+ * This routine will likely be slower than SkScalarRoundToInt(), and should only be used when the
+ * extra precision is known to be valuable.
+ *
+ * In particular, this catches the following case:
+ * SkScalar x = 0.49999997;
+ * int ix = SkScalarRoundToInt(x);
+ * SkASSERT(0 == ix); // <--- fails
+ * ix = SkDScalarRoundToInt(x);
+ * SkASSERT(0 == ix); // <--- succeeds
+ */
+static inline int SkDScalarRoundToInt(SkScalar x) {
+ double xx = x;
+ xx += 0.5;
+ return (int)floor(xx);
+}
+
/** Returns the absolute value of the specified SkScalar
*/
#define SkScalarAbs(x) sk_float_abs(x)
diff --git a/src/core/SkScan_Path.cpp b/src/core/SkScan_Path.cpp
index 66e9507..b32d68e 100644
--- a/src/core/SkScan_Path.cpp
+++ b/src/core/SkScan_Path.cpp
@@ -602,7 +602,11 @@
// don't reference "origClip" any more, just use clipPtr
SkIRect ir;
- path.getBounds().round(&ir);
+ // We deliberately call dround() 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 dround() here,
+ // we could generate the wrong ir for values like 0.4999997.
+ path.getBounds().dround(&ir);
if (ir.isEmpty()) {
if (path.isInverseFillType()) {
blitter->blitRegion(*clipPtr);
diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index e1fbff2..3de10cb 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -20,6 +20,49 @@
#include "SkWriter32.h"
#include "Test.h"
+static void make_path_crbug364224(SkPath* path) {
+ path->reset();
+ path->moveTo(3.747501373f, 2.724499941f);
+ path->lineTo(3.747501373f, 3.75f);
+ path->cubicTo(3.747501373f, 3.88774991f, 3.635501385f, 4.0f, 3.497501373f, 4.0f);
+ path->lineTo(0.7475013733f, 4.0f);
+ path->cubicTo(0.6095013618f, 4.0f, 0.4975013733f, 3.88774991f, 0.4975013733f, 3.75f);
+ path->lineTo(0.4975013733f, 1.0f);
+ path->cubicTo(0.4975013733f, 0.8622499704f, 0.6095013618f, 0.75f, 0.7475013733f,0.75f);
+ path->lineTo(3.497501373f, 0.75f);
+ path->cubicTo(3.50275135f, 0.75f, 3.5070014f, 0.7527500391f, 3.513001442f, 0.753000021f);
+ path->lineTo(3.715001345f, 0.5512499809f);
+ path->cubicTo(3.648251295f, 0.5194999576f, 3.575501442f, 0.4999999702f, 3.497501373f, 0.4999999702f);
+ path->lineTo(0.7475013733f, 0.4999999702f);
+ path->cubicTo(0.4715013802f, 0.4999999702f, 0.2475013733f, 0.7239999771f, 0.2475013733f, 1.0f);
+ path->lineTo(0.2475013733f, 3.75f);
+ path->cubicTo(0.2475013733f, 4.026000023f, 0.4715013504f, 4.25f, 0.7475013733f, 4.25f);
+ path->lineTo(3.497501373f, 4.25f);
+ path->cubicTo(3.773501396f, 4.25f, 3.997501373f, 4.026000023f, 3.997501373f, 3.75f);
+ path->lineTo(3.997501373f, 2.474750042f);
+ path->lineTo(3.747501373f, 2.724499941f);
+ path->close();
+}
+
+static void make_path_crbug364224_simplified(SkPath* path) {
+ path->moveTo(3.747501373f, 2.724499941f);
+ path->cubicTo(3.648251295f, 0.5194999576f, 3.575501442f, 0.4999999702f, 3.497501373f, 0.4999999702f);
+ path->close();
+}
+
+static void test_path_crbug364224() {
+ SkPath path;
+ SkPaint paint;
+ SkAutoTUnref<SkSurface> surface(SkSurface::NewRasterPMColor(84, 88));
+ SkCanvas* canvas = surface->getCanvas();
+
+ make_path_crbug364224_simplified(&path);
+ canvas->drawPath(path, paint);
+
+ make_path_crbug364224(&path);
+ canvas->drawPath(path, paint);
+}
+
static void make_path0(SkPath* path) {
// from * https://code.google.com/p/skia/issues/detail?id=1706
@@ -3326,6 +3369,8 @@
};
DEF_TEST(Paths, reporter) {
+ test_path_crbug364224();
+
SkTSize<SkScalar>::Make(3,4);
SkPath p, empty;
diff --git a/tests/ScalarTest.cpp b/tests/ScalarTest.cpp
index 82f61ec..6b7fe6b 100644
--- a/tests/ScalarTest.cpp
+++ b/tests/ScalarTest.cpp
@@ -12,6 +12,19 @@
#include "SkRect.h"
#include "Test.h"
+static void test_roundtoint(skiatest::Reporter* reporter) {
+ SkScalar x = 0.49999997;
+ int ix = SkScalarRoundToInt(x);
+ // We "should" get 0, since x < 0.5, but we don't due to float addition rounding up the low
+ // bit after adding 0.5.
+ REPORTER_ASSERT(reporter, 1 == ix);
+
+ // This version explicitly performs the +0.5 step using double, which should avoid losing the
+ // low bits.
+ ix = SkDScalarRoundToInt(x);
+ REPORTER_ASSERT(reporter, 0 == ix);
+}
+
struct PointSet {
const SkPoint* fPts;
size_t fCount;
@@ -187,4 +200,5 @@
DEF_TEST(Scalar, reporter) {
test_isfinite(reporter);
+ test_roundtoint(reporter);
}