Replace SkIsFinite with a parameter-pack implementation.
This should be more ergonomic, and will generate slightly better
code than `SkIsFinite(a, b) && SkIsFinite(c, d)`.
The algorithm has been slightly tweaked; it's now a hybrid of the
previous approaches. The first element of the list is subtracted
from itself (yielding zero or NAN) and multiplied against all the
subsequent elements. Finally the value is compared against itself.
This gives us one instruction per value, plus one comparison op,
and does not require us to synthesize a constant zero.
Change-Id: I7f87c09a74bc847f0afc10346b51211b17811c1c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/842484
Commit-Queue: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
diff --git a/include/core/SkPoint3.h b/include/core/SkPoint3.h
index af2443e..abf8dfd 100644
--- a/include/core/SkPoint3.h
+++ b/include/core/SkPoint3.h
@@ -114,7 +114,7 @@
@return true for values other than infinities and NaN
*/
bool isFinite() const {
- return SkIsFinite(fX, fY) && SkIsFinite(fZ);
+ return SkIsFinite(fX, fY, fZ);
}
/** Returns the dot product of a and b, treating them as 3D vectors
diff --git a/include/core/SkRect.h b/include/core/SkRect.h
index a07bc50..65c053a 100644
--- a/include/core/SkRect.h
+++ b/include/core/SkRect.h
@@ -709,7 +709,7 @@
@return true if no member is infinite or NaN
*/
bool isFinite() const {
- return SkIsFinite(fLeft, fTop) && SkIsFinite(fRight, fBottom);
+ return SkIsFinite(fLeft, fTop, fRight, fBottom);
}
/** Returns left edge of SkRect, if sorted. Call isSorted() to see if SkRect is valid.
diff --git a/include/private/base/SkFloatingPoint.h b/include/private/base/SkFloatingPoint.h
index 2cdf75c..7d2547a 100644
--- a/include/private/base/SkFloatingPoint.h
+++ b/include/private/base/SkFloatingPoint.h
@@ -14,6 +14,7 @@
#include <cmath>
#include <cstdint>
#include <limits>
+#include <type_traits>
inline constexpr float SK_FloatSqrt2 = 1.41421356f;
inline constexpr float SK_FloatPI = 3.14159265f;
@@ -36,54 +37,32 @@
// as floatf(x + .5f), they would be 1 higher than expected.
#define sk_float_round(x) (float)sk_double_round((double)(x))
-static inline bool SkIsNaN(float x) {
- return x != x;
-}
-
-static inline bool SkIsNaN(double x) {
+template <typename T, std::enable_if_t<std::is_floating_point_v<T>, bool> = true>
+static inline bool SkIsNaN(T x) {
return x != x;
}
// Subtracting a value from itself will result in zero, except for NAN or ±Inf, which make NAN.
+// Multiplying a group of values against zero will result in zero for each product, except for
+// NAN or ±Inf, which will result in NAN and continue resulting in NAN for the rest of the elements.
// This generates better code than `std::isfinite` when building with clang-cl (April 2024).
-static inline bool SkIsFinite(float x) {
- return (x - x) == 0;
+template <typename T, typename... Pack, std::enable_if_t<std::is_floating_point_v<T>, bool> = true>
+static inline bool SkIsFinite(T x, Pack... values) {
+ T prod = x - x;
+ prod = (prod * ... * values);
+ // At this point, `prod` will either be NaN or 0.
+ return prod == prod;
}
-static inline bool SkIsFinite(double x) {
- return (x - x) == 0;
-}
-
-// Subtracting a value from itself will result in zero, except for NAN or ±Inf, which make NAN.
-// A NAN is not equal to any value, so a NAN or ±Inf in either `a` or `b` will cause the
-// comparison to evaluate as false.
-// If both `a` and `b` are finite, the comparison will reduce to `0 == 0`, which is true.
-static inline bool SkIsFinite(float a, float b) {
- return (a - a) == (b - b);
-}
-
-static inline bool SkIsFinite(double a, double b) {
- return (a - a) == (b - b);
-}
-
-// Multiplying a group of values against zero will result in zero at each iteration, except for
-// NAN or ±Inf, which will result in NAN and continue resulting in NAN for the rest of the loop.
-static inline bool SkIsFinite(const float array[], int count) {
- float prod = 0.0f;
- for (int i = 0; i < count; ++i) {
+template <typename T, std::enable_if_t<std::is_floating_point_v<T>, bool> = true>
+static inline bool SkIsFinite(const T array[], int count) {
+ T x = array[0];
+ T prod = x - x;
+ for (int i = 1; i < count; ++i) {
prod *= array[i];
}
- // At this point, prod will either be NaN or 0.
- return prod == 0.0f;
-}
-
-static inline bool SkIsFinite(const double array[], int count) {
- double prod = 0.0;
- for (int i = 0; i < count; ++i) {
- prod *= array[i];
- }
- // At this point, prod will either be NaN or 0.
- return prod == 0.0;
+ // At this point, `prod` will either be NaN or 0.
+ return prod == prod;
}
inline constexpr int SK_MaxS32FitsInFloat = 2147483520;
diff --git a/modules/skottie/src/Skottie.cpp b/modules/skottie/src/Skottie.cpp
index a33ff42..c0ae9ed 100644
--- a/modules/skottie/src/Skottie.cpp
+++ b/modules/skottie/src/Skottie.cpp
@@ -403,7 +403,7 @@
duration = sk_ieee_float_divide(outPoint - inPoint, fps);
if (size.isEmpty() || version.isEmpty() || fps <= 0 ||
- !SkIsFinite(inPoint, outPoint) || !SkIsFinite(duration)) {
+ !SkIsFinite(inPoint, outPoint, duration)) {
if (fLogger) {
const auto msg = SkStringPrintf(
"Invalid animation params (version: %s, size: [%f %f], frame rate: %f, "
diff --git a/src/base/SkCubics.cpp b/src/base/SkCubics.cpp
index 4fba299..90de88e 100644
--- a/src/base/SkCubics.cpp
+++ b/src/base/SkCubics.cpp
@@ -207,7 +207,7 @@
int SkCubics::BinarySearchRootsValidT(double A, double B, double C, double D,
double solution[3]) {
- if (!SkIsFinite(A, B) || !SkIsFinite(C, D)) {
+ if (!SkIsFinite(A, B, C, D)) {
return 0;
}
double regions[4] = {0, 0, 0, 1};
diff --git a/src/core/SkFontPriv.h b/src/core/SkFontPriv.h
index 7c2808f..49b6e94 100644
--- a/src/core/SkFontPriv.h
+++ b/src/core/SkFontPriv.h
@@ -76,9 +76,7 @@
const SkPoint& textLocation);
static bool IsFinite(const SkFont& font) {
- return SkIsFinite(font.getSize()) &&
- SkIsFinite(font.getScaleX()) &&
- SkIsFinite(font.getSkewX());
+ return SkIsFinite(font.getSize(), font.getScaleX(), font.getSkewX());
}
// Returns the number of elements (characters or glyphs) in the array.
diff --git a/src/core/SkMatrix.cpp b/src/core/SkMatrix.cpp
index bfaf915..c8766f3 100644
--- a/src/core/SkMatrix.cpp
+++ b/src/core/SkMatrix.cpp
@@ -1563,7 +1563,7 @@
const SkScalar sx = SkVector::Length(this->getScaleX(), this->getSkewY());
const SkScalar sy = SkVector::Length(this->getSkewX(), this->getScaleY());
- if (!SkIsFinite(sx) || !SkIsFinite(sy) ||
+ if (!SkIsFinite(sx, sy) ||
SkScalarNearlyZero(sx) || SkScalarNearlyZero(sy)) {
return false;
}
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index bd21fc8..2625cf3 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -2226,7 +2226,7 @@
DirChange directionChange(const SkVector& curVec) {
SkScalar cross = SkPoint::CrossProduct(fLastVec, curVec);
if (!SkIsFinite(cross)) {
- return kUnknown_DirChange;
+ return kUnknown_DirChange;
}
if (cross == 0) {
return fLastVec.dot(curVec) < 0 ? kBackwards_DirChange : kStraight_DirChange;
@@ -3760,7 +3760,7 @@
b *= dscale;
c *= dscale;
// check if we're not finite, or normal is zero-length
- if (!SkIsFinite(a, b) || !SkIsFinite(c) ||
+ if (!SkIsFinite(a, b, c) ||
(a == 0 && b == 0)) {
fA = fB = 0;
fC = SK_Scalar1;
diff --git a/src/core/SkPoint3.cpp b/src/core/SkPoint3.cpp
index bb573a5..7ae83eb 100644
--- a/src/core/SkPoint3.cpp
+++ b/src/core/SkPoint3.cpp
@@ -71,7 +71,7 @@
fX *= scale;
fY *= scale;
fZ *= scale;
- if (!SkIsFinite(fX, fY) || !SkIsFinite(fZ)) {
+ if (!SkIsFinite(fX, fY, fZ)) {
this->set(0, 0, 0);
return false;
}
diff --git a/src/core/SkRRect.cpp b/src/core/SkRRect.cpp
index 2b4d76f..c5f90b9 100644
--- a/src/core/SkRRect.cpp
+++ b/src/core/SkRRect.cpp
@@ -118,7 +118,7 @@
return;
}
- if (!SkIsFinite(leftRad, topRad) || !SkIsFinite(rightRad, bottomRad)) {
+ if (!SkIsFinite(leftRad, topRad, rightRad, bottomRad)) {
this->setRect(rect); // devolve into a simple rect
return;
}
diff --git a/src/core/SkStroke.cpp b/src/core/SkStroke.cpp
index b442ca3..2d623ed 100644
--- a/src/core/SkStroke.cpp
+++ b/src/core/SkStroke.cpp
@@ -1183,7 +1183,7 @@
}
}
}
- if (!SkIsFinite(quadPts->fQuad[2].fX) || !SkIsFinite(quadPts->fQuad[2].fY)) {
+ if (!SkIsFinite(quadPts->fQuad[2].fX, quadPts->fQuad[2].fY)) {
DEBUG_CUBIC_RECURSION_TRACK_DEPTH(fRecursionDepth);
return false; // just abort if projected quad isn't representable
}
diff --git a/src/effects/SkBlenders.cpp b/src/effects/SkBlenders.cpp
index d82da2b..86b3439 100644
--- a/src/effects/SkBlenders.cpp
+++ b/src/effects/SkBlenders.cpp
@@ -21,7 +21,7 @@
bool enforcePremul) {
using namespace SkKnownRuntimeEffects;
- if (!SkIsFinite(k1, k2) || !SkIsFinite(k3, k4)) {
+ if (!SkIsFinite(k1, k2, k3, k4)) {
return nullptr;
}
diff --git a/src/effects/imagefilters/SkLightingImageFilter.cpp b/src/effects/imagefilters/SkLightingImageFilter.cpp
index 38fde08..268cf49 100644
--- a/src/effects/imagefilters/SkLightingImageFilter.cpp
+++ b/src/effects/imagefilters/SkLightingImageFilter.cpp
@@ -282,16 +282,15 @@
const SkImageFilters::CropRect& cropRect) {
// According to the spec, ks and kd can be any non-negative number:
// http://www.w3.org/TR/SVG/filters.html#feSpecularLightingElement
- if (!SkIsFinite(material.fK) || material.fK < 0.f ||
- !SkIsFinite(material.fShininess, ZValue(material.fSurfaceDepth))) {
+ if (!SkIsFinite(material.fK, material.fShininess, ZValue(material.fSurfaceDepth)) ||
+ material.fK < 0.f) {
return nullptr;
}
// Ensure light values are finite, and the cosine should be between -1 and 1
- if (!SkPoint(light.fLocationXY).isFinite() ||
- !skif::Vector(light.fDirectionXY).isFinite() ||
- !SkIsFinite(ZValue(light.fLocationZ), ZValue(light.fDirectionZ)) ||
- !SkIsFinite(light.fFalloffExponent, light.fCosCutoffAngle) ||
+ if (!SkPoint(light.fLocationXY).isFinite() || !skif::Vector(light.fDirectionXY).isFinite() ||
+ !SkIsFinite(light.fFalloffExponent, light.fCosCutoffAngle,
+ ZValue(light.fLocationZ), ZValue(light.fDirectionZ)) ||
light.fCosCutoffAngle < -1.f || light.fCosCutoffAngle > 1.f) {
return nullptr;
}
diff --git a/tests/MathTest.cpp b/tests/MathTest.cpp
index 809e9f5..30a3a03 100644
--- a/tests/MathTest.cpp
+++ b/tests/MathTest.cpp
@@ -182,10 +182,11 @@
template <typename T>
static void unittest_isfinite(skiatest::Reporter* reporter) {
+ const T zero = T(0);
+ const T plain = T(123);
const T inf = std::numeric_limits<T>::infinity();
const T big = std::numeric_limits<T>::max();
- const T nan = inf * 0;
- const T zero = 0;
+ const T nan = inf * zero;
REPORTER_ASSERT(reporter, !SkIsNaN(inf));
REPORTER_ASSERT(reporter, !SkIsNaN(-inf));
@@ -201,6 +202,21 @@
REPORTER_ASSERT(reporter, SkIsFinite(big));
REPORTER_ASSERT(reporter, SkIsFinite(-big));
REPORTER_ASSERT(reporter, SkIsFinite(zero));
+
+ // SkIsFinite supports testing multiple values at once.
+ REPORTER_ASSERT(reporter, !SkIsFinite(inf, plain));
+ REPORTER_ASSERT(reporter, !SkIsFinite(plain, -inf));
+ REPORTER_ASSERT(reporter, !SkIsFinite(nan, plain));
+ REPORTER_ASSERT(reporter, SkIsFinite(plain, big));
+ REPORTER_ASSERT(reporter, SkIsFinite(-big, plain));
+ REPORTER_ASSERT(reporter, SkIsFinite(plain, zero));
+
+ REPORTER_ASSERT(reporter, !SkIsFinite(inf, plain, plain));
+ REPORTER_ASSERT(reporter, !SkIsFinite(plain, -inf, plain));
+ REPORTER_ASSERT(reporter, !SkIsFinite(plain, plain, nan));
+ REPORTER_ASSERT(reporter, SkIsFinite(big, plain, plain));
+ REPORTER_ASSERT(reporter, SkIsFinite(plain, -big, plain));
+ REPORTER_ASSERT(reporter, SkIsFinite(plain, plain, zero));
}
static void unittest_half(skiatest::Reporter* reporter) {
