Make ContourMeasure more robust Make assertions and operations inside ContourMeasure more robust with degenerate data and NaN. Add a rive::math::clamp() method with better behavior surrounding NaN. Change the ContourMeasureIter constructor to accept a "const RawPath*". (The former definition with "const RawPath&" accepted r-values, which was unsafe since the iterator stored raw pointers directly to objects owned by the RawPath.) Delete RawPath operator*(). The way this operator was defined, the matrix had to go on the wrong side of the path relative to how it operated on the data. Diffs= cd6210f42 Make ContourMeasure more robust (#7294) Co-authored-by: Chris Dalton <99840794+csmartdalton@users.noreply.github.com>
diff --git a/.rive_head b/.rive_head index 618f380..29b2afd 100644 --- a/.rive_head +++ b/.rive_head
@@ -1 +1 @@ -405b8ef907d29cf480422b94d4717fdcdfad0824 +cd6210f42d644de3cc1c83fa65b3d4e1f790cece
diff --git a/include/rive/math/contour_measure.hpp b/include/rive/math/contour_measure.hpp index 8077ee6..93eea52 100644 --- a/include/rive/math/contour_measure.hpp +++ b/include/rive/math/contour_measure.hpp
@@ -97,12 +97,12 @@ // approximation for the curves actual length. static constexpr float kDefaultTolerance = 0.5f; - ContourMeasureIter(const RawPath& path, float tol = kDefaultTolerance) + ContourMeasureIter(const RawPath* path, float tol = kDefaultTolerance) { this->rewind(path, tol); } - void rewind(const RawPath&, float = kDefaultTolerance); + void rewind(const RawPath*, float = kDefaultTolerance); // Returns a measure object for each contour in the path // (contours with zero-length are skipped over)
diff --git a/include/rive/math/math_types.hpp b/include/rive/math/math_types.hpp index 481c9b5..17dfaea 100644 --- a/include/rive/math/math_types.hpp +++ b/include/rive/math/math_types.hpp
@@ -112,6 +112,17 @@ "math::round_up_to_multiple_of<> only supports powers of 2."); return (x + (N - 1)) & ~(N - 1); } + +// Behaves better with NaN than std::clamp(). (Matching simd::clamp().) +// +// Returns lo if x == NaN (but std::clamp() returns NaN). +// Returns hi if hi <= lo. +// Ignores hi and/or lo if they are NaN. +// +RIVE_ALWAYS_INLINE static float clamp(float x, float lo, float hi) +{ + return fminf(fmaxf(lo, x), hi); +} } // namespace math template <typename T> T lerp(const T& a, const T& b, float t) { return a + (b - a) * t; }
diff --git a/include/rive/math/raw_path.hpp b/include/rive/math/raw_path.hpp index 7f277f8..df50245 100644 --- a/include/rive/math/raw_path.hpp +++ b/include/rive/math/raw_path.hpp
@@ -49,7 +49,6 @@ RawPath transform(const Mat2D&) const; void transformInPlace(const Mat2D&); - RawPath operator*(const Mat2D& mat) const { return this->transform(mat); } Span<const Vec2D> points() const { return m_Points; } Span<Vec2D> points() { return m_Points; }
diff --git a/src/constraints/follow_path_constraint.cpp b/src/constraints/follow_path_constraint.cpp index fa3bb21..0b8096f 100644 --- a/src/constraints/follow_path_constraint.cpp +++ b/src/constraints/follow_path_constraint.cpp
@@ -154,7 +154,7 @@ commandPath->addToRawPath(m_rawPath, path->pathTransform()); } - auto measure = ContourMeasureIter(m_rawPath); + auto measure = ContourMeasureIter(&m_rawPath); for (auto contour = measure.next(); contour != nullptr; contour = measure.next()) { m_contours.push_back(contour);
diff --git a/src/math/contour_measure.cpp b/src/math/contour_measure.cpp index 247f908..89c1e42 100644 --- a/src/math/contour_measure.cpp +++ b/src/math/contour_measure.cpp
@@ -247,9 +247,12 @@ } } - assert(prevDist < seg.m_distance); + assert(prevDist <= seg.m_distance); const auto ratio = (distance - prevDist) / (seg.m_distance - prevDist); - return lerp(prevT, seg.getT(), ratio); + float t = lerp(prevT, seg.getT(), ratio); + t = math::clamp(t, prevT, seg.getT()); + assert(prevT <= t && t <= seg.getT()); + return t; } void ContourMeasure::getSegment(float startDist, @@ -366,16 +369,16 @@ return distance; } -void ContourMeasureIter::rewind(const RawPath& path, float tolerance) +void ContourMeasureIter::rewind(const RawPath* path, float tolerance) { - m_iter = path.begin(); - m_end = path.end(); - m_srcPoints = path.points().data(); + m_iter = path->begin(); + m_end = path->end(); + m_srcPoints = path->points().data(); constexpr float kMinTolerance = 1.0f / 16; m_invTolerance = 1.0f / std::max(tolerance, kMinTolerance); - m_segmentCounts.resize(path.verbs().count()); + m_segmentCounts.resize(path->verbs().count()); } // Can return null if either it encountered an empty contour (length == 0) @@ -515,12 +518,15 @@ m_iter = endOfContour; - if (distance == 0 || pts.size() < 2) + if (distance > 0 && pts.size() >= 2) { - return nullptr; + assert(!std::isnan(distance)); + return rcp<ContourMeasure>( + new ContourMeasure(std::move(segs), std::move(pts), distance, isClosed)); } - return rcp<ContourMeasure>( - new ContourMeasure(std::move(segs), std::move(pts), distance, isClosed)); + + assert(distance == 0 || std::isnan(distance)); + return nullptr; } rcp<ContourMeasure> ContourMeasureIter::next() @@ -537,5 +543,6 @@ break; } } + assert(!cm || !std::isnan(cm->length())); return cm; }
diff --git a/src/shapes/metrics_path.cpp b/src/shapes/metrics_path.cpp index 46fea41..f66aa5e 100644 --- a/src/shapes/metrics_path.cpp +++ b/src/shapes/metrics_path.cpp
@@ -63,7 +63,8 @@ if (!m_Contour || transform != m_ComputedLengthTransform) { m_ComputedLengthTransform = transform; - m_Contour = ContourMeasureIter(m_RawPath * transform).next(); + RawPath transformedPath = m_RawPath.transform(transform); + m_Contour = ContourMeasureIter(&transformedPath).next(); m_ComputedLength = m_Contour ? m_Contour->length() : 0; } return m_ComputedLength;
diff --git a/test/contour_measure_test.cpp b/test/contour_measure_test.cpp index a52f1b6..c9164d6 100644 --- a/test/contour_measure_test.cpp +++ b/test/contour_measure_test.cpp
@@ -39,15 +39,15 @@ const float tol = 0.000001f; RawPath path; - ContourMeasureIter iter(path, false); + ContourMeasureIter iter(&path); REQUIRE(iter.next() == nullptr); path.moveTo(1, 2); - iter.rewind(path, false); + iter.rewind(&path); REQUIRE(iter.next() == nullptr); path.lineTo(4, 6); - iter.rewind(path, false); + iter.rewind(&path); auto cm = iter.next(); REQUIRE(cm); REQUIRE(nearly_eq(cm->length(), 5, tol)); @@ -58,7 +58,7 @@ path = RawPath(); const float w = 4, h = 6; path.addRect({0, 0, w, h}, PathDirection::cw); - iter.rewind(path, false); + iter.rewind(&path); cm = iter.next(); REQUIRE(cm); REQUIRE(nearly_eq(cm->length(), 2 * (w + h), tol)); @@ -118,7 +118,7 @@ path.addPoly(span, false); // len == 7 - ContourMeasureIter iter(path, false); + ContourMeasureIter iter(&path); auto cm = iter.next(); REQUIRE(cm->length() == 7); cm = iter.next(); @@ -136,7 +136,7 @@ const float r = 10; RawPath path; path.addOval({-r, -r, r, r}, PathDirection::cw); - ContourMeasureIter iter(path, false); + ContourMeasureIter iter(&path, tol); auto cm = iter.next(); REQUIRE(nearly_eq(cm->length(), 2 * r * math::PI, tol)); @@ -152,3 +152,28 @@ auto machine = artboard->defaultStateMachine(); machine->advanceAndApply(0.0f); } + +// NaN paths don't return contours. +TEST_CASE("nan-path", "[contourmeasure]") +{ + RawPath path; + path.lineTo(1, 2); + path.cubicTo(3, 4, 5, 6, 7, 8); + path.cubicTo(9, 10, 11, 12, 13, 14); + path.cubicTo(15, 16, 17, 18, 19, 20); + + { + ContourMeasureIter iter(&path); + auto cm = iter.next(); + CHECK(cm != nullptr); + CHECK(std::isfinite(cm->length())); + CHECK(iter.next() == nullptr); + } + + { + auto nan = std::numeric_limits<float>::quiet_NaN(); + RawPath path_ = path.transform(Mat2D(nan, nan, nan, nan, nan, nan)); + ContourMeasureIter iter(&path_); + CHECK(iter.next() == nullptr); + } +}
diff --git a/test/metrics_path_test.cpp b/test/metrics_path_test.cpp index 44efab7..cde4727 100644 --- a/test/metrics_path_test.cpp +++ b/test/metrics_path_test.cpp
@@ -1,5 +1,11 @@ #include <catch.hpp> +#include <rive/math/math_types.hpp> #include <rive/shapes/metrics_path.hpp> +#include <rive/shapes/paint/stroke.hpp> +#include <rive/shapes/paint/trim_path.hpp> +#include <utils/no_op_factory.hpp> + +using namespace rive; TEST_CASE("path metrics compute correctly", "[bezier]") { @@ -25,4 +31,24 @@ // float cubicLength = cubicPath.computeLength(identity); // REQUIRE(cubicLength == 238.38698f); -} \ No newline at end of file +} + +// Regression test for a crash found by fuzzing. +TEST_CASE("fuzz_issue_7295", "[MetricsPath]") +{ + NoOpFactory factory; + + OnlyMetricsPath innerPath; + innerPath.moveTo(.0f, -20.5f); + innerPath.cubicTo(11.3218384f, -20.5f, 20.5f, -11.3218384f, 20.5f, .0f); + innerPath.cubicTo(20.5f, 11.3218384f, 11.3218384f, 20.5f, .0f, 20.5f); + innerPath.cubicTo(-11.3218384f, 20.5f, -20.5f, 11.3218384f, -20.5f, .0f); + innerPath.cubicTo(-20.5f, -11.3218384f, -11.3218384f, -20.5f, .0f, -20.5f); + + OnlyMetricsPath outerPath; + outerPath.addPath(&innerPath, Mat2D(1.f, .0f, .0f, 1.f, -134217728.f, -134217728.f)); + + RawPath result; + outerPath.paths()[0]->trim(.0f, 168.389008f, true, &result); + CHECK(math::nearly_equal(outerPath.paths()[0]->length(), 168.389008f)); +}
diff --git a/test/simd_test.cpp b/test/simd_test.cpp index 7838c36..2192f12 100644 --- a/test/simd_test.cpp +++ b/test/simd_test.cpp
@@ -369,11 +369,17 @@ // Returns lo if x == NaN, but std::clamp() returns NaN. CHECK(simd::clamp<float, 1>(kNaN, 1, 2).x == 1); + // Matches math::clamp(). + CHECK(simd::clamp<float, 1>(kNaN, 1, 2).x == math::clamp(kNaN, 1, 2)); // Returns hi if hi <= lo. CHECK(simd::clamp<float, 1>(3, 2, 1).x == 1); CHECK(simd::clamp<float, 1>(kNaN, 2, 1).x == 1); CHECK(simd::clamp<float, 1>(kNaN, kNaN, 1).x == 1); + // Matches math::clamp(). + CHECK(simd::clamp<float, 1>(3, 2, 1).x == math::clamp(3, 2, 1)); + CHECK(simd::clamp<float, 1>(kNaN, 2, 1).x == math::clamp(kNaN, 2, 1)); + CHECK(simd::clamp<float, 1>(kNaN, kNaN, 1).x == math::clamp(kNaN, kNaN, 1)); // Ignores hi and/or lo if they are NaN. CHECK(simd::clamp<float, 1>(3, 4, kNaN).x == 4); @@ -381,6 +387,12 @@ CHECK(simd::clamp<float, 1>(3, kNaN, 2).x == 2); CHECK(simd::clamp<float, 1>(3, kNaN, 4).x == 3); CHECK(simd::clamp<float, 1>(3, kNaN, kNaN).x == 3); + // Matches math::clamp(). + CHECK(simd::clamp<float, 1>(3, 4, kNaN).x == math::clamp(3, 4, kNaN)); + CHECK(simd::clamp<float, 1>(3, 2, kNaN).x == math::clamp(3, 2, kNaN)); + CHECK(simd::clamp<float, 1>(3, kNaN, 2).x == math::clamp(3, kNaN, 2)); + CHECK(simd::clamp<float, 1>(3, kNaN, 4).x == math::clamp(3, kNaN, 4)); + CHECK(simd::clamp<float, 1>(3, kNaN, kNaN).x == math::clamp(3, kNaN, kNaN)); } // Check simd::abs.
diff --git a/viewer/src/viewer_content/textpath_content.cpp b/viewer/src/viewer_content/textpath_content.cpp index 69fe3fd..9b05375 100644 --- a/viewer/src/viewer_content/textpath_content.cpp +++ b/viewer/src/viewer_content/textpath_content.cpp
@@ -241,7 +241,7 @@ RawPath warp = make_quad_path(m_pathpts); this->draw_warp(renderer, warp); - auto meas = ContourMeasureIter(warp).next(); + auto meas = ContourMeasureIter(&warp).next(); const float warpLength = meas->length(); const float textLength = gruns.back().xpos.back();
diff --git a/viewer/src/viewer_content/trimpath_content.cpp b/viewer/src/viewer_content/trimpath_content.cpp index af28c42..d4ffb3d 100644 --- a/viewer/src/viewer_content/trimpath_content.cpp +++ b/viewer/src/viewer_content/trimpath_content.cpp
@@ -114,7 +114,7 @@ { renderer->save(); - auto cm = ContourMeasureIter(*p, false).next(); + auto cm = ContourMeasureIter(p, false).next(); auto p1 = trim(cm.get(), m_trimFrom, m_trimTo); stroke_path(renderer, p1, 20, 0xFFFF0000);