Skip to content

tech-debt(kpm): expose deferred OrientationAssignment API surface when a consumer appears #138

Description

@kalwalt

Summary

Track the three API-surface items deferred during M8-3 (PR #137) under the Option C scope split. M8-4 (#129) does not absorb them because the FREAK descriptor samples the Gaussian pyramid directly and never reads gradient images — so there is no real consumer for these items today.

This issue exists so the items don't get forgotten. They should land when a real caller surfaces.

Background

Per the M8-3 status comment on #128, the original Option C plan said "deferred items land in M8-4 where they're actually needed." During M8-4 brainstorming we re-read freak.h / freak.cpp carefully and confirmed the FREAK descriptor algorithm:

  • Samples Gaussian-smoothed pyramid levels (pyramid.get(octave, scale)) via bilinear_interpolation<float>
  • Uses keypoint.angle (already computed in M8-3) as a similarity-transform parameter
  • Does not read gradient images, recompute orientations, or share state with OrientationAssignment

Therefore the M8-3-deferred items don't have a real consumer in M8-4 either. Adding them to M8-4 would be speculative API surface with no caller — pure YAGNI violation.

A correction comment on #128 and #129 walks reviewers through this finding.

Items to expose when a consumer appears

1. Public two-phase OrientationAssignment API

Today's API (orientation.rs):

pub fn compute_polar_gradient_image(level: &Matrix<f32>) -> Matrix<f32>;

impl OrientationAssignment {
    pub fn compute(&self, gradient: &Matrix<f32>, x, y, sigma) -> Vec<f32>;
}

Caller pattern (currently buried inside DoGScaleInvariantDetector::detect):

// Per detect() call (NOT shared across calls):
let gradients: Vec<Matrix<f32>> = (0..num_octaves)
    .flat_map(|o| (0..NUM_SCALES).map(move |s| compute_polar_gradient_image(pyramid.level(o, s))))
    .collect();
// Then per-keypoint: oa.compute(&gradients[oct * NUM_SCALES + scale], x, y, sigma)

Promote this to a real public two-phase API:

impl OrientationAssignment {
    /// Phase 1: precompute gradient images once per frame.
    pub fn compute_gradients(&mut self, pyramid: &GaussianScaleSpacePyramid);

    /// Phase 2: query orientations using the cached gradients.
    pub fn compute(&self, octave: usize, scale: usize, x: f32, y: f32, sigma: f32) -> Vec<f32>;
}

C equivalent: vision::OrientationAssignment::computeGradients + compute.

2. Full 7-parameter OrientationAssignment::new(...) constructor

Today's API hardcodes the FREAK pipeline defaults:

impl OrientationAssignment {
    pub fn new() -> Self {  // FREAK defaults: num_bins=36, gaussian_expansion=3.0,
        ...                  // support_region=1.5, smoothing_iters=5, peak_threshold=0.8
    }
}

Expose:

impl OrientationAssignment {
    pub fn with_params(
        num_bins: usize,
        gaussian_expansion_factor: f32,
        support_region_expansion_factor: f32,
        num_smoothing_iterations: usize,
        peak_threshold: f32,
    ) -> Self; }

C equivalent: vision::OrientationAssignment::alloc(...) (drops fine_width, fine_height, num_octaves, num_scales_per_octave — Rust gets those from the pyramid type).

3. Reusable gradient image cache across detect() / describe() calls

Today the gradient cache is rebuilt inside every DoGScaleInvariantDetector::detect() call. For multi-frame applications that share the same pyramid config, this is wasteful.

Either:

  • Move the cache ownership to OrientationAssignment itself (matches C++ mGradients)
  • Or expose a separate GradientCache type that callers manage

The first option is closer to C++.

When this matters

Any of these would justify landing this work:

  • Multi-frame KPM matcher that runs detect() on a stream of frames sharing pyramid config
  • FREAK descriptor extension that wants finer-grained control over OA parameters per call
  • Tuning experiments comparing different orientation-assignment configurations
  • External consumers of webarkitlib-rs who want to call OrientationAssignment directly without going through DoGScaleInvariantDetector::detect

What NOT to do here

  • Don't pre-emptively land this without a real consumer. The current internal API works fine for detect(); adding public surface now is speculative.
  • Don't change the internal API without versioning the public re-exports in freak/mod.rs.

References

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    Status
    Backlog

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions