[vangel76]

Summary

null_vector used the full SVD output even though it only needs one right singular vector. Share the existing Jacobi factorization core with svd, then select the required column directly for null-space queries. This avoids normalizing U and allocating sorted U/V copies on that path.

Jacobi sweep order, arithmetic and stopping criterion are unchanged. Equal singular values select the same column as the original stable descending sort. Full svd output keeps its previous normalization and ordering.

Fixes #2329

Measurements

Release builds based on upstream/main 99190488, Ryzen 7 7800X3D. RAYON_NUM_THREADS=1, core 12, perf stat -e instructions:u,cycles:u; five alternating original/changed pairs, medians. Each process performs 500 RANSAC calls on 500 correspondences, with 10% outliers, small observation noise and up to 400 iterations per call.

Metric	Before	After
User instructions	13,820,949,970	13,688,113,977
User cycles	4,105,152,876	4,014,192,894
Timed RANSAC loop	845.202 ms	826.184 ms

0.96% fewer instructions: a small measured gain from removing unnecessary output work. The benchmark's aggregate inlier count is identical in all ten runs. These are RANSAC-kernel measurements; counters include setup/input generation. No speedup for the full camera tracker is claimed.

Verification

• cargo fmt --all --check
• cargo clippy -p effectcraft-track --lib --tests -- -D warnings
• RAYON_NUM_THREADS=1 cargo test --release -p effectcraft-track
• Committed bit comparisons between null_vector and the full SVD's last V column for square/tall/wide/empty inputs, zero/rank-deficient matrices, scales from 1e-100 to 1e100 and tied singular values.
• Uncommitted comparison against the original implementations: full U/s/V outputs match by to_bits() across matrix shapes. RANSAC essential matrices, every Sampson error and all inlier masks match bitwise for noisy 3D, planar and near-degenerate scenes (three seeds each).
• Three alternating original/changed full camera reconstructions (150 points, 40 frames, observation noise, specified focal length) produce identical serialized output hashes: 7cd84d48f874c076 in all six runs.
• No public signature change. Throwaway benchmarks and copied reference modules are excluded from the commit.

Overlap

Only camtrack/linalg.rs changes, around SVD/null-space queries and their test. The Jacobi sweep body and umeyama are byte-for-byte unchanged; #1876's umeyama work is separate. #2342 also edits linalg.rs in the Cholesky function; this branch starts independently from fresh upstream/main. No other agents' files change.

Generated with Codex

Summary by CodeRabbit

• Bug Fixes
  • Improved null-vector consistency across matrix shapes, scales, and tied singular values.
  • Preserved existing normalization and sorting behavior for singular value decomposition results.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: storytold/effectcraft
• Branch: main
• File: .coderabbit.yaml
• Commit: 9919048
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: e9e0f6cd-40f9-4977-abfb-5b3bcf06f9d6

Commits

Reviewing files that changed from the base of the PR and between 9919048 and 8dbb0f5.

Files selected for processing: 1

• crates/track/src/camtrack/linalg.rs

Limit details: You’ve used all 10 included reviews currently available.

Walkthrough

svd now uses a shared Jacobi factorization helper and retains its normalization and sorting steps. null_vector selects the smallest singular value directly from the raw factors, with ties resolved in favor of the highest index. Tests compare its output with the corresponding SVD column.

Changes: Jacobi factor sharing

Layer / File(s)	Summary
Shared factors and consumers crates/track/src/camtrack/linalg.rs	svd and null_vector use the shared Jacobi factorization. null_vector selects the smallest singular value and uses the highest index for ties. Tests compare its output bit-for-bit with the last right-singular-vector column from svd across matrix shapes, scales, and an identity-matrix tie case.

Priority: Low

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Refactor

Suggested reviewers: echelon

Merge Risk: Minimal · up to 8dbb0

No actionable merge-blocking risk is established; the change is mergeable after normal checks.

Architecture risk: Medium · up to 8dbb0

The change affects 1 system.

Changed systems: crates

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

• observed — crates (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

• observed — Modified behavior in crates/track/src/camtrack/linalg.rs: svd now calls jacobi_factors for unsorted factors, then performs its existing normalization and descending sort. A private shared helper is introduced for the Jacobi factorization.
• observed — Modified behavior in crates/track/src/camtrack/linalg.rs: jacobi_factors returns the rotated columns and their norms without normalizing or sorting them; those steps now occur in svd.
• observed — Modified behavior in crates/track/src/camtrack/linalg.rs: null_vector now selects the smallest singular value directly from the shared raw factors, breaking equal-value ties in favor of the highest index, instead of taking the final column of sorted svd output.
• observed — Modified behavior in crates/track/src/camtrack/linalg.rs: Adds a test comparing null_vector bit-for-bit with the last right-singular-vector column from svd across empty, rectangular, and square shapes and scales from zero to 1e100, plus an identity-matrix tie case.

Reliability and maintainability

• inferred — Risk-relevant change factors for crates: blast_radius_4; direct_dependents_2

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: avoiding unnecessary SVD output work in null-vector queries.
Linked Issues check		The changes satisfy the coding objectives in #2329. jacobi_factors preserves the existing Jacobi sweep body and stopping criterion. null_vector uses the raw factors, selects the smallest singular-…
Out of Scope Changes check		The diff changes only crates/track/src/camtrack/linalg.rs. The refactoring, null_vector implementation, and equivalence test directly support #2329. No unrelated production behavior or public API …

Full details: Linked Issues check

Explanation

The changes satisfy the coding objectives in #2329. jacobi_factors preserves the existing Jacobi sweep body and stopping criterion. null_vector uses the raw factors, selects the smallest singular-value column, and avoids U normalization and sorted copies. svd retains normalization and descending ordering. The added test checks bitwise null-vector equivalence across shapes, scales, zero input, and ties. The PR also reports the requested singular-vector, essential-matrix, Sampson-error, inlier-mask, and reconstruction comparisons.

Full details: Out of Scope Changes check

Explanation

The diff changes only crates/track/src/camtrack/linalg.rs. The refactoring, null_vector implementation, and equivalence test directly support #2329. No unrelated production behavior or public API change is shown.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Usage-based review receipt

• Mode: Continue automatically
• Reviewed files: 1
• Waived: $0.25 (charged $0.00)
• View usage details

Note

This review exceeded your plan’s limits and used usage-based reviews—free during trial, billed after paid activation unless disabled. Manage usage-based reviews.

_{Comment @coderabbitai help to get the list of available commands.}

[vangel76]

This PR is one of 17 from a speed audit of crates/track and its callers (issues #2317-#2330, #2322, #2349). Overview of what each one measured, so the effect is visible in one place:

PR	Area	Measured (single thread, before -> after)	Output
#2332	Subspace Warp plan()	22.3 G -> 4.8 G instructions (4.7x, Automatic); 33.7 G -> 5.2 G (6.5x, Enhanced); 24x on clean low-rank trajectories	rounding level, at most 2.7e-6 px
#2331	KLT lk_point (Warp Stabilizer analysis, mask tracking)	analysis 3.84 s -> 1.44 s (2.6x), mask tracking 1.57 s -> 0.95 s	bit-identical
#2339	Point tracker (NCC taps, gradient window)	54.6 G -> 31.0 G instructions (1.76x)	bit-identical
#2334	Face tracker polygon test	12.5 G -> 2.2 G instructions (5.7x)	bit-identical
#2337	Mask tracker frame clone	4K: 3.42 s -> 2.19 s (-36 %); 1080p -5 %	identical outline
#2335	Roto Brush propagate band crop	208.5 G -> 172.0 G instructions per 20 frames (-17.5 %)	bit-identical
#2333	Roto Brush distance transform	3.04 G -> 2.80 G instructions (-8 %)	bit-identical
#2346	Camera solve, long tracks (matrix-free Schur)	44.6 G -> 4.2 G instructions (10.6x), timed loop 1966 ms -> 216 ms	practically identical (reprojection < 1e-9 px), direct fallback
#2342	Camera solve, short tracks (band Cholesky)	8.34 G -> 3.40 G instructions (2.46x)	bit-identical
#2338	triangulate allocations	2.38 G -> 1.32 G instructions (1.81x)	bit-identical
#2340	BA Jacobian on the stack	13.4 G -> 12.6 G instructions (-6.2 %)	bit-identical
#2345	null_vector without unused SVD output	-0.96 % instructions	bit-identical
#2351	Point tracker, 3x3 refine when the motion was small (stacked on #2339)	another 31.0 G -> 20.2 G instructions (1.54x) on top of #2339	not bit-identical by design, accuracy unchanged in all 8 ground-truth cases
#2348	Warp Stabilizer / Camera Tracker analysis lookup per render	0.73 ms -> 0.014 ms per call for 300 frames (about 50x)	identical
#2347	Warp Stabilizer default plan computed twice	first render 2313 ms -> 1156 ms (-50 % instructions)	identical pixels
#2350	Warp analysis shares one luma pyramid	87.7 G -> 81.8 G instructions (-6.8 %)	identical analysis JSON
#2341	Input validation (panics, NaN, allocation budget)	correctness fix, +6.1 % instructions on valid input	valid input unchanged

Notes:

• All numbers are single-thread (RAYON_NUM_THREADS=1), perf stat -e instructions:u (the PRs that quote time use the timed kernel), base and branch alternated, medians. They are kernel/stage measurements from each PR's own benchmark, not one end-to-end number for the whole tracker; the stages mostly act on different features (point tracking, Warp Stabilizer, mask tracking, Roto Brush, camera solve, effect caches), so they do not simply multiply.
• Not every PR is a speedup: Camera tracking: skip unused SVD output work in null-vector queries #2345 is a small gain and Camera tracking: validate solver input and bound workspaces #2341 costs a little on valid input in exchange for fixing the panics and NaN results from camtrack: invalid or non-finite input panics or yields NaN (bundle, select_pose, Track2D) #2330.
• The PRs touch disjoint files except where noted in their bodies (Camera tracking: avoid per-view triangulation allocations #2338/Camera tracking: keep bundle-adjustment Jacobians on the stack #2340/Camera tracking: validate solver input and bound workspaces #2341 share files and merge independently; Reuse the validated Warp Stabilizer plan on first render #2347/Reuse Warp Stabilizer and Camera Tracker analysis keys #2348 share the effects analysis cache and may need adjacent tests combined; Point tracker: refine in a 3x3 neighbourhood when the motion was small #2351 is stacked on Point tracker: candidates share one tap table, gradients only where lk_refine reads them (#2318) #2339; Camera tracking: factor narrow camera systems within their band #2342/Camera tracking: skip unused SVD output work in null-vector queries #2345/Avoid quadratic Schur assembly for long camera tracks #2346 touch camtrack/linalg.rs, umeyama from 3D rotation order, keyed Orientation, linear-to-eased keyframes and Timewarp behave as in After Effects #1876 is untouched).

[bflatastic]

Merged, thank you @vangel76! It landed on main with a batch of community PRs, after a full workspace build and test run on Windows against the refactored main (fmt, strict clippy, about 4,100 tests, layers, assets and wasm).