From 07628a205960bd699360c9febdc44304a3066d67 Mon Sep 17 00:00:00 2001 From: daniel-c-harvey Date: Sat, 1 Aug 2026 13:59:09 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20close=20=CE=98-W7-T1=20round-2=20leftove?= =?UTF-8?q?rs=20=E2=80=94=20NaN=20guard=20placement,=20comment=20attributi?= =?UTF-8?q?on,=20CLAUDE.md=20export?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Moves the NaN/Inf finiteness check to addSegment where the UB-causing cast actually happens, makes strokeBounds reject an interior non-finite point instead of swallowing it, corrects a LICE_GetPixel misattribution, and documents rasterRowOffset. --- src/core/ui/CLAUDE.md | 4 +++- src/core/ui/stroke_aa.cpp | 32 ++++++++++++++++++-------------- src/core/ui/stroke_aa.h | 2 +- tests/test_stroke_aa.cpp | 31 +++++++++++++++++++++++++++++-- 4 files changed, 51 insertions(+), 18 deletions(-) diff --git a/src/core/ui/CLAUDE.md b/src/core/ui/CLAUDE.md index 6e110d7..f6209db 100644 --- a/src/core/ui/CLAUDE.md +++ b/src/core/ui/CLAUDE.md @@ -101,7 +101,9 @@ L7 sub-pass, 2026-07-27): - `tooltip` — pure tooltip placement + prefix-strip: strips the `ReaSampler:` display prefix from the registered action phrase; width clamped to the client rect. - `card_drag` — pure drag-gesture precedence + slot hit-test: leave-client → OS drag-out; other-bank → move/copy; same-bank → reorder / Alt-over-occupied → replace. - `card_meta` — pure card-metadata formatters: bars.beats.subdivisions and seconds.milliseconds; blank when the sample is unstamped. -- `stroke_aa` — analytic antialiased thick-stroke COVERAGE (the shell blends it): `StrokeCanvas`, a reusable mask holding distance-to-polyline coverage MAX-accumulated across segments, plus `strokePolyline` / `strokeBounds` / `appendArc`. An arc is just a flattened polyline, so ONE path serves the knob arcs, the inner dial, the envelope polyline and both spline traces. Coverage is `clamp(halfWidth + 0.5 - distance, 0, 1)`, which makes perpendicular weight exactly `2·halfWidth` at every angle. **The guaranteed-opaque-core threshold is width ≥ 2 px, not any width above 1 px**: opacity needs `distance <= halfWidth - 0.5`, and the worst-case distance from a pixel centre to the centreline is 0.5, so a 1 px stroke (`halfWidth = 0.5`) has zero slack — its peak alpha modulates with the stroke's exact alignment to the pixel grid instead of pinning to 255 (the knob track arc and the mini curve-thumbnail trace are both 1 px and both live with this). Long segments are subdivided before rasterizing — EXACT, not an approximation (min-distance to a partition of a segment is min-distance to the whole), purely to keep each piece's bounding box tight, since one long diagonal's box has area O(len²). +- `stroke_aa` — analytic antialiased thick-stroke COVERAGE (the shell blends it): `StrokeCanvas`, a reusable mask holding distance-to-polyline coverage MAX-accumulated across segments, plus `strokePolyline` / `strokeBounds` / `appendArc` / `rasterRowOffset` (the row-major offset + math for a possibly bottom-up raster, pulled out of the shell's LICE blend so its flipped + branch is pinned by a host-free test). An arc is just a flattened polyline, so ONE path serves the knob arcs, the inner dial, the envelope polyline and both spline traces. Coverage is `clamp(halfWidth + 0.5 - distance, 0, 1)`, which makes perpendicular weight exactly `2·halfWidth` at every angle. **The guaranteed-opaque-core threshold is width ≥ 2 px, not any width above 1 px**: opacity needs `distance <= halfWidth - 0.5`, and the worst-case distance from a pixel centre to the centreline is 0.5, so a 1 px stroke (`halfWidth = 0.5`) has zero slack — its peak alpha modulates with the stroke's exact alignment to the pixel grid instead of pinning to 255 (the knob track arc and the mini curve-thumbnail trace are both 1 px and both live with this). Long segments are subdivided before rasterizing — EXACT, not an approximation (min-distance to a partition of a segment is min-distance to the whole), purely to keep each piece's bounding box tight, since one long diagonal's box has area O(len²). ## Gotchas diff --git a/src/core/ui/stroke_aa.cpp b/src/core/ui/stroke_aa.cpp index f8fc8f0..e9f372d 100644 --- a/src/core/ui/stroke_aa.cpp +++ b/src/core/ui/stroke_aa.cpp @@ -56,13 +56,7 @@ void StrokeCanvas::extendRow(int y, int x0, int x1) { } void StrokeCanvas::addPiece(float ax, float ay, float bx, float by, float halfWidth) { - // Every current caller feeds bounded geometry, but this is a pure module: a NaN/Inf - // coordinate would otherwise reach static_cast below, which is UB rather than a - // clipped no-op. - if (!(std::isfinite(ax) && std::isfinite(ay) && std::isfinite(bx) && std::isfinite(by) && - std::isfinite(halfWidth))) { - return; - } + // addSegment (the only caller) guarantees finite inputs before this point. const float reach = halfWidth + 0.5f; // beyond this the coverage is 0 const float dx = bx - ax; const float dy = by - ay; @@ -104,6 +98,14 @@ void StrokeCanvas::addPiece(float ax, float ay, float bx, float by, float halfWi void StrokeCanvas::addSegment(float ax, float ay, float bx, float by, float halfWidth) { if (bounds_.empty() || halfWidth <= 0.0f) return; + // Must guard here, not in addPiece: len2 below goes NaN/Inf on a bad input too, so the + // `len2 <= kMaxPieceLen^2` comparison is false either way (NaN compares false against + // anything) and the pieces-count cast a few lines down is reached as UB regardless of which + // branch is taken. + if (!(std::isfinite(ax) && std::isfinite(ay) && std::isfinite(bx) && std::isfinite(by) && + std::isfinite(halfWidth))) { + return; + } const float dx = bx - ax; const float dy = by - ay; const float len2 = dx * dx + dy * dy; @@ -125,20 +127,22 @@ void StrokeCanvas::addSegment(float ax, float ay, float bx, float by, float half } Rect strokeBounds(const StrokePoint* pts, std::size_t count, float halfWidth, const Rect& clip) { - if (pts == nullptr || count == 0 || halfWidth <= 0.0f || clip.empty()) return Rect{}; + if (pts == nullptr || count == 0 || halfWidth <= 0.0f || clip.empty() || + !std::isfinite(halfWidth)) { + return Rect{}; + } + if (!(std::isfinite(pts[0].x) && std::isfinite(pts[0].y))) return Rect{}; float minX = pts[0].x, maxX = pts[0].x, minY = pts[0].y, maxY = pts[0].y; for (std::size_t i = 1; i < count; ++i) { + // Checked per-point, not via isfinite(minX/maxX) after the reduction: std::min/max + // against NaN silently returns the OTHER (finite) operand, so a NaN anywhere but pts[0] + // would otherwise vanish from the reduction instead of rejecting the stroke. + if (!(std::isfinite(pts[i].x) && std::isfinite(pts[i].y))) return Rect{}; minX = (std::min)(minX, pts[i].x); maxX = (std::max)(maxX, pts[i].x); minY = (std::min)(minY, pts[i].y); maxY = (std::max)(maxY, pts[i].y); } - // Same NaN/Inf guard as addPiece: an unbounded coordinate must clip to nothing, not reach - // the static_cast below as UB. - if (!(std::isfinite(minX) && std::isfinite(maxX) && std::isfinite(minY) && - std::isfinite(maxY) && std::isfinite(halfWidth))) { - return Rect{}; - } const float reach = halfWidth + 0.5f; // Same tightened box as addPiece (see its comment): a pixel only takes ink when its centre // is within `reach`, so this is [ceil(min-reach-0.5), floor(max+reach+0.5)) rather than the diff --git a/src/core/ui/stroke_aa.h b/src/core/ui/stroke_aa.h index d93b09e..d716427 100644 --- a/src/core/ui/stroke_aa.h +++ b/src/core/ui/stroke_aa.h @@ -88,7 +88,7 @@ void appendArc(std::vector& out, float cx, float cy, float radius, // accounting for a possibly bottom-up (`flipped`) layout. Pulled out of the shell's LICE blend // so its flipped branch — dead for every bitmap type the shell actually constructs, and // otherwise unverifiable without a live LICE surface — is pinned by a host-free test. Matches -// LICE's own row math (`lice.cpp`'s `LICE_SysBitmap` pixel accessor: `(h-1-y)*rowspan + x`). +// LICE's own row math (`lice.cpp`'s free-function `LICE_GetPixel`: `(h-1-y)*rowspan + x`). inline std::size_t rasterRowOffset(int y, int height, int rowSpan, bool flipped) { const int row = flipped ? height - 1 - y : y; return static_cast(row) * static_cast(rowSpan); diff --git a/tests/test_stroke_aa.cpp b/tests/test_stroke_aa.cpp index 42d4d23..1594fce 100644 --- a/tests/test_stroke_aa.cpp +++ b/tests/test_stroke_aa.cpp @@ -289,6 +289,31 @@ static void testCoverageOutsideTheValidSpanReadsZero() { CHECK(c.coverageAt(1000, 1000) == 0.0f); } +// --- non-finite input rejection ------------------------------------------------ + +static void testInteriorNonFiniteCoordinateRejectsTheWholeStroke() { + // Only pts[0] going non-finite used to propagate through strokeBounds's min/max reduction: + // std::min/max against NaN silently returns the OTHER, finite, operand, so a NaN anywhere + // else in the polyline vanished from the reduction instead of rejecting it. pts[1] here is + // the case that check missed. + const StrokePoint pts[3] = {{10.0f, 10.0f}, {NAN, 50.0f}, {90.0f, 10.0f}}; + CHECK(strokeBounds(pts, 3, 1.5f, kBig).empty()); + + StrokeCanvas c; + strokePolyline(c, pts, 3, 1.5f, kBig); + CHECK(c.bounds().empty()); +} + +static void testAddSegmentRejectsNonFiniteInputsDirectly() { + // addSegment casts a pieces-count derived from len2 to int; a NaN/Inf endpoint must be + // caught before that cast, not one call layer down in addPiece where it's already too late. + StrokeCanvas c; + c.reset(kBig); + c.addSegment(10.0f, 10.0f, NAN, 50.0f, 1.5f); + c.addSegment(10.0f, 10.0f, INFINITY, 50.0f, 1.5f); + CHECK(peakCoverage(c) == 0.0f); +} + // --- raster row addressing ---------------------------------------------------- static void testRasterRowOffsetMatchesUnflippedAndFlippedLayouts() { @@ -296,8 +321,8 @@ static void testRasterRowOffsetMatchesUnflippedAndFlippedLayouts() { CHECK(rasterRowOffset(0, 100, 240, false) == 0u); CHECK(rasterRowOffset(5, 100, 240, false) == 5u * 240u); CHECK(rasterRowOffset(99, 100, 240, false) == 99u * 240u); - // Flipped (bottom-up DIBs): row y is (height-1-y)*rowSpan — LICE_SysBitmap's own pixel - // accessor, `(h-1-y)*rowspan + x` (lice.cpp:2262). + // Flipped (bottom-up DIBs): row y is (height-1-y)*rowSpan — matches the free function + // LICE_GetPixel's own row math, `(h-1-y)*rowspan + x` (lice.cpp:2262). CHECK(rasterRowOffset(0, 100, 240, true) == 99u * 240u); CHECK(rasterRowOffset(99, 100, 240, true) == 0u); CHECK(rasterRowOffset(40, 100, 240, true) == 59u * 240u); @@ -441,6 +466,8 @@ int main() { testBoundsClipToTheClipRectAndCoverTheReach(); testStrokeEntirelyOutsideTheClipDrawsNothing(); testCoverageOutsideTheValidSpanReadsZero(); + testInteriorNonFiniteCoordinateRejectsTheWholeStroke(); + testAddSegmentRejectsNonFiniteInputsDirectly(); testRasterRowOffsetMatchesUnflippedAndFlippedLayouts(); testSubdivisionDoesNotChangeTheRenderedStroke(); testArcPointsLieOnTheCircleAndRespectTheFlatness();