Skip to content

fix(imgproc): compute_integral_image never writes the first column #131

Description

@kalwalt

Summary

imgproc.compute_integral_image zeroes only the first row of its destination
tables. The first column is never written, so it keeps whatever the caller's
buffer already held. Fix it to zero both, matching cv2.integral and the
function's own JSDoc.

The defect is latent today — no module under src/ calls the function — and
becomes live the moment a descriptor built on integral images lands.

Why

src/imgproc/imgproc.ts (the compute_integral_image body) fills the first row
and then starts writing at p = w1 + 1. Each row advances p by exactly w1,
so index i * w1 for i >= 1 is never assigned. The same index arithmetic is
used by all three branches, so sum, sqsum and tilted are equally affected.

The JSDoc immediately above the function already states:

Each destination must be sized (src.cols + 1) × (src.rows + 1); the first
row/column are zero.

So this is not a deliberate convention that differs from OpenCV — the code does
not do what its own contract says.

Reproduction

import jsfeat from "@webarkit/jsfeat-next";

const w = 4, h = 3;
const src = new jsfeat.matrix_t(w, h, jsfeat.U8_t | jsfeat.C1_t);
src.data.fill(1);

const sum = new Int32Array((w + 1) * (h + 1)).fill(999); // a reused buffer
jsfeat.imgproc.compute_integral_image(src, sum, null, null);
reused buffer (pre-filled with 999)     freshly allocated
0   0   0   0   0                       0   0   0   0   0
999 1   2   3   4                       0   1   2   3   4
999 2   4   6   8                       0   2   4   6   8
999 3   6   9  12                       0   3   6   9  12

Why this is worse than it looks

The existing ground-truth test passes by accident of allocation: it hands in
a new Int32Array(...), which is already zero-filled, so column 0 reads as zero
because the caller made it so — not because the function wrote it. The current
test suite therefore cannot detect this defect at all.

Meanwhile AGENTS.md instructs every module to borrow scratch buffers from the
single shared pool (shared_cache.get_buffer, balanced with put_buffer), and
the pool hands out reused buffers. A module doing exactly what the repo's
conventions require would read stale data in column 0 — that is, for every box
whose left edge sits at x = 0, which is precisely the case for keypoints near
the left border.

That is the same failure family as #110 (orb.describe silently producing
contaminated descriptors near the image edge): wrong output, no error, visible
only as degraded tracking.

Proposed change

  1. Zero dst_sum[i * w1] for every row i in the sum, sum + sqsum, sqsum
    and tilted branches (four sites, one line each).
  2. Strengthen the tests: fill the destination buffer with a sentinel value
    before calling
    , so a regression is visible instead of being masked by
    zero-initialised allocation. Add the same sentinel treatment to the existing
    edge-case and ground-truth tests that exercise this function.

Risk assessment: parity with jsfeat is unaffected

jsfeat carries the same code, so this looks like a parity divergence — it is
not, and the reason matters:

  • The parity test allocates fresh buffers on both sides. Where jsfeat leaves
    0 by allocation, jsfeatNext will now write 0 explicitly. The compared
    values are identical.
  • Behaviour changes only for buffers that are not zero-filled, which no
    current test exercises and no src/ caller produces.

No tests/divergences.test.ts entry is required (see #102).

Acceptance criteria

  • The first column is zeroed for sum, sqsum and tilted.
  • Calling with a sentinel-filled destination produces a table whose first row
    and first column are zero, for every branch combination
    (sum only, sqsum only, sum + sqsum, tilted).
  • The existing parity test against the vendored jsfeat oracle stays green,
    unchanged.
  • No entry added to tests/divergences.test.ts — if one turns out to be
    needed, the analysis above is wrong and should be revisited before merging.

Out of scope

  • Changing the destination parameter type from number[] to a typed array or
    matrix_t. The signature stays as-is; that is a separate API question.
  • Any new consumer of the integral image.

Related

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Typescriptall about TypescriptbugSomething isn't workingtests

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions