Fix rounding for values near 1/2 and > 2^24
Bug: chromium:1366345, oss-fuzz:51634, oss-fuzz:51629
Change-Id: I721d3512b28b8963ec6224cabf28bc647b6ad956
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/583583
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
diff --git a/include/core/SkScalar.h b/include/core/SkScalar.h
index e1203ef..2a6bb3e 100644
--- a/include/core/SkScalar.h
+++ b/include/core/SkScalar.h
@@ -22,13 +22,14 @@
#define SK_ScalarTanPIOver8 0.414213562f
#define SK_ScalarRoot2Over2 0.707106781f
#define SK_ScalarMax 3.402823466e+38f
+#define SK_ScalarMin (-SK_ScalarMax)
#define SK_ScalarInfinity SK_FloatInfinity
#define SK_ScalarNegativeInfinity SK_FloatNegativeInfinity
#define SK_ScalarNaN SK_FloatNaN
#define SkScalarFloorToScalar(x) sk_float_floor(x)
#define SkScalarCeilToScalar(x) sk_float_ceil(x)
-#define SkScalarRoundToScalar(x) sk_float_floor((x) + 0.5f)
+#define SkScalarRoundToScalar(x) sk_float_round(x)
#define SkScalarTruncToScalar(x) sk_float_trunc(x)
#define SkScalarFloorToInt(x) sk_float_floor2int(x)
@@ -62,8 +63,6 @@
#define SkScalarToDouble(x) static_cast<double>(x)
#define SkDoubleToScalar(x) sk_double_to_float(x)
-#define SK_ScalarMin (-SK_ScalarMax)
-
static inline bool SkScalarIsNaN(SkScalar x) { return x != x; }
/** Returns true if x is not NaN and not infinite
@@ -78,26 +77,6 @@
return sk_floats_are_finite(array, count);
}
-/**
- * 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 fractional part of the scalar. */
static inline SkScalar SkScalarFraction(SkScalar x) {
return x - SkScalarTruncToScalar(x);
diff --git a/include/private/SkFloatingPoint.h b/include/private/SkFloatingPoint.h
index 0801162..8135abb 100644
--- a/include/private/SkFloatingPoint.h
+++ b/include/private/SkFloatingPoint.h
@@ -57,7 +57,10 @@
return radians * (180 / SK_FloatPI);
}
-#define sk_float_round(x) sk_float_floor((x) + 0.5f)
+// floor(double+0.5) vs. floorf(float+0.5f) give comparable performance, but upcasting to double
+// means tricky values like 0.49999997 and 2^24 get rounded correctly. If these were rounded
+// as floatf(x + .5f), they would be 1 higher than expected.
+#define sk_float_round(x) (float)sk_double_round((double)(x))
// can't find log2f on android, but maybe that just a tool bug?
#ifdef SK_BUILD_FOR_ANDROID
@@ -130,19 +133,19 @@
}
#define sk_float_floor2int(x) sk_float_saturate2int(sk_float_floor(x))
-#define sk_float_round2int(x) sk_float_saturate2int(sk_float_floor((x) + 0.5f))
+#define sk_float_round2int(x) sk_float_saturate2int(sk_float_round(x))
#define sk_float_ceil2int(x) sk_float_saturate2int(sk_float_ceil(x))
#define sk_float_floor2int_no_saturate(x) (int)sk_float_floor(x)
-#define sk_float_round2int_no_saturate(x) (int)sk_float_floor((x) + 0.5f)
+#define sk_float_round2int_no_saturate(x) (int)sk_float_round(x)
#define sk_float_ceil2int_no_saturate(x) (int)sk_float_ceil(x)
#define sk_double_floor(x) floor(x)
#define sk_double_round(x) floor((x) + 0.5)
#define sk_double_ceil(x) ceil(x)
-#define sk_double_floor2int(x) (int)floor(x)
-#define sk_double_round2int(x) (int)floor((x) + 0.5)
-#define sk_double_ceil2int(x) (int)ceil(x)
+#define sk_double_floor2int(x) (int)sk_double_floor(x)
+#define sk_double_round2int(x) (int)sk_double_round(x)
+#define sk_double_ceil2int(x) (int)sk_double_ceil(x)
// Cast double to float, ignoring any warning about too-large finite values being cast to float.
// Clang thinks this is undefined, but it's actually implementation defined to return either
diff --git a/tests/ScalarTest.cpp b/tests/ScalarTest.cpp
index dea4548..044eb2a 100644
--- a/tests/ScalarTest.cpp
+++ b/tests/ScalarTest.cpp
@@ -15,14 +15,23 @@
static void test_roundtoint(skiatest::Reporter* reporter) {
SkScalar x = 0.49999997f;
int ix = SkScalarRoundToInt(x);
- // We "should" get 0, since x < 0.5, but we don't due to float addition rounding up the low
+ int badIx = (int) floorf(x + 0.5f);
+ // We should get 0, since x < 0.5, but we wouldn't if SkScalarRoundToInt uses the commonly
+ // recommended approach shown in 'badIx' 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);
+ REPORTER_ASSERT(reporter, 1 == badIx);
+
+ // Additionally, when the float value is between (2^23,2^24], it's precision is equal to
+ // 1 integral value. Adding 0.5f rounds up automatically *before* the floor, so naive
+ // rounding is also incorrect. Float values <= 2^23 and > 2^24 don't have this problem
+ // because either the sum can be represented sufficiently for floor() to do the right thing,
+ // or the sum will always round down to the integer multiple.
+ x = 8388609.f;
+ ix = SkScalarRoundToInt(x);
+ badIx = (int) floorf(x + 0.5f);
+ REPORTER_ASSERT(reporter, 8388609 == ix);
+ REPORTER_ASSERT(reporter, 8388610 == badIx);
}
struct PointSet {
