fix(fast_corners): index the per-row candidate buffer consistently (#202)

[kalwalt]

The defect

fast_corners.detect wrote each image row's corner columns into the cpbuf scratch buffer 1-based and read them back 0-based one row later:

++ncorners; cpbuf[cornerpos + ncorners] = j;        // writes [1 .. n]
for (k = 0; k < ncorners; ++k) j = cpbuf[cornerpos + k];  // reads  [0 .. n-1]

Two consequences, on every image:

1. Each row's last candidate was never read, so genuine corners were dropped.
2. cpbuf[cornerpos + 0] was read without ever being written. The buffer comes from the shared pool, and get_buffer only ever grows a node — it never zeroes one. That cell therefore held whatever an earlier, unrelated call left behind, so detect's output depended on process history rather than on the image.

(2) is why the same image with the same options could return different corner sets across calls. It is only observable when the recycled cell happens to hold a small non-negative integer — any other value indexes buf out of bounds, yields undefined, and fails every comparison — which is why the defect looked intermittent rather than constant.

The fix

Write 0-based, matching the read — the two lines swapped, in both the darker (d & 1) and brighter (d & 2) branches. detect is now a pure function of (image, border, threshold) and recovers the dropped corners. No API change.

The row's count cell cpbuf[cornerpos + w] is also safer than before: the highest candidate index now written is w - 7, strictly below it.

Why this is an intentional divergence, not a parity regression

The vendored jsfeat oracle carries the identical defect, so the correct behaviour cannot be bit-parity with jsfeat. Following the pattern already used for #102, #114, #119, #139 and #186, the cross-implementation assertion moves out of tests/parity/ into tests/divergences.test.ts.

What replaces equality is containment: jsfeatNext's corner set always contains jsfeat's. That is structural, not an artefact of the chosen scenes. jsfeat emits a corner only if score = buf[prev+j] is strictly greater than all eight neighbours. For a phantom column j that was not a real candidate of that row, buf[prev+j] === 0 and 0 > 0 is false; for a j out of range, every comparison against undefined is false. So a phantom can pass suppression only if it is already a real candidate of that row — i.e. already in jsfeatNext's list. cpbuf only enumerates candidates; every suppression decision reads scores out of buf, which the defect never touched.

Measured counts: 33 corners vs jsfeat's 27 on the divergence test's scene (threshold 20, border 3). The containment relation is asserted across seven scene seeds, so jsfeat's score arithmetic is still cross-checked bit-for-bit on the common subset — on seven scenes now, where the old parity test used one.

Coverage

Net stronger than what was removed. Three invariants are new, and each of them fails on the unfixed code (verified by reverting the swap):

• detect is independent of what the shared buffer pool contains (poisons all 30 pool nodes with a plausible column index — note that poisoning with 0, or with a byte fill, is invisible).
• detect does not drop the last corner candidate of a row.
• The superset relation against jsfeat, across seven seeds.

Plus a purity check across repeated calls with an unrelated detect in between. Full suite: 356 tests, 28 files, green.

Audit answers (the two questions in #202)

Do any fixtures carry expected values produced by the current detect? No. Every fast_corners comparison in this repository runs live against the vendored oracle; nothing static pins detect output. This change alters no committed fixture — only the six files in this diff.

Does any other module share the pattern? No. All 44 get_buffer call sites across 10 files in src/ were walked: every borrowed buffer is fully written before it is read, and every get_buffer is balanced by a put_buffer on every exit path.

Notes for the reviewer

• dist/ is deliberately not rebuilt here. Whether a fix PR rebuilds dist is a release-convention decision; it needs a rebuild at release time.
• Follow-up worth its own issue: detect writes into the caller's corners pool without checking corners.length. Recovering the dropped corners raises the count, which makes that pre-existing hazard marginally more reachable.
• Open question from fast_corners.detect reads uninitialised memory and drops each row's last corner #202, not answered here: whether PureCV's port shares the off-by-one. PureCV is a separate repository and out of this branch's scope.

Closes #202
Unblocks step 4 of webarkit/webarkit#22, tracked in webarkit/webarkit#27.

🤖 Generated with Claude Code

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[kalwalt]

Follow-ups from the review of this PR, now tracked:

• fast_corners.detect writes past the end of the caller's corners array without checking its length #205 — detect writes into the caller's corners pool without checking corners.length. Pre-existing, not introduced here, but this PR raises corner counts on every image so a pool sized empirically against the old behaviour is marginally more likely to overflow.
• When porting FAST, do not inherit jsfeat's per-row candidate-buffer off-by-one purecv#136 — answers the "does any port share the off-by-one?" question from fast_corners.detect reads uninitialised memory and drops each row's last corner #202. PureCV has no FAST port yet, so there is nothing to fix there; the issue is preventive, so the defect is not carried over when FAST is ported. Consequence (2) (the uninitialised read) is largely a JS-pool artefact that idiomatic Rust would not reproduce, but consequence (1) (the dropped last candidate per row) ports cleanly and silently if the index convention is transcribed as-is.

Also note the CI fix in 3a08720: the bench/ smoke check has its own cross-implementation equality guards that npm test never runs, and they failed for the same reason the parity assertion did. bench/optical_flow_lk.bench.ts now seeds both track() calls from one corner set (the seeds are an input to what it measures), and bench/detectors.bench.ts keeps both sides detecting independently but asserts containment within a tolerance instead of equality — detect is the measured workload there, so the workloads cannot be equalised. That tolerance (5%) was chosen with headroom over the largest measured excess rather than derived from the per-row bound; tightening it is fair review feedback if you want it.