Skip to content

motion_estimator.get_subset uses the global Math.random; make the RNG injectable #189

Description

@kalwalt

Summary

get_subset draws minimal samples from Math.random() directly:

idx_i = indices[i] = Math.floor(Math.random() * max_cnt) | 0;

OpenCV's RANSACPointSetRegistrator::run constructs a fixed-seed RNG per call:

RNG rng((uint64)-1);

so cv::findHomography is deterministic run to run for the same input. jsfeatNext's is not.

Why this matters

Testing. tests/parity/motion_estimator.test.ts already works around this by monkeypatching the global:

function seededRandom(seed: number) {
    const r = mulberry32(seed);
    return vi.spyOn(Math, "random").mockImplementation(r);
}

It works, but it is a global mutation with vi.restoreAllMocks() cleanup, it can't run tests in parallel safely, and every downstream consumer that wants deterministic tests has to reinvent it. webarkit/webarkit's cv-backend-jsfeatnext adapter hit exactly this — a test that was flaky ~1 in 45 runs, worked around in webarkit/webarkit#8 by loosening the tolerance instead.

Production. Reproducibility matters for an AR tracking loop: same frame in, same pose out, is a property worth having when debugging jitter. Right now two identical frames can produce measurably different homographies (see #185 and the f64 issue).

To be clear about scope: this is not the root cause of the precision problem in #185. Even with a fixed seed, whichever 4-point subset gets drawn is still a worse estimate than a refit over all inliers. This issue is about determinism and ergonomics, not accuracy. Filing it separately so the two don't get conflated.

Proposed change

Add an optional RNG to ransac_params_t (or as a trailing argument on ransac/lmeds), defaulting to Math.random so nothing breaks:

export type RandomFn = () => number;

// ransac_params_t
constructor(size = 0, thresh = 0.5, eps = 0.5, prob = 0.99, rng: RandomFn = Math.random) { ... }

and thread it through get_subset's signature. A small seedable generator (mulberry32 or xorshift128) exported from src/math would let consumers get OpenCV-style determinism in one line without pulling in a dependency.

Open question for @ThorstenBux: do we match OpenCV and default to a fixed seed, or keep Math.random as the default and make seeding opt-in? OpenCV's choice surprises people who expect RANSAC to be stochastic, but it is what our parity oracle effectively does. Keeping Math.random as default is the non-breaking option; I lean that way, with the seeded generator exported and documented.

Acceptance criteria

  • ransac / lmeds / get_subset accept an injectable RNG, defaulting to current behaviour
  • A seedable PRNG exported from the public API and documented
  • tests/parity/motion_estimator.test.ts migrated off vi.spyOn(Math, "random") to the injected RNG
  • Note in the docs that the default is non-deterministic and how to make it deterministic
  • No behaviour change for existing callers that pass nothing

Related

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

Projects

No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions