webarkit / webarkit/jsfeatNext

fix(imgproc): compute_integral_image never writes the first column

Open
#131 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug tests Typescript
Dominant language
TypeScript
Stars
12
Forks
4
Avg merge
16h 24m
Merged PRs (30d)
34

Description

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

  • Same failure family: #110
  • Future consumers: #135 (TEBLID) and #80 (FREAK), plus haar (#43) and bbf (#44)
  • Divergence policy: #102
  • Plan: docs/features2d-expansion-plan.md

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/imgproc/imgproc.ts at the compute_integral_image body, then inspect the existing edge-case, ground-truth, and parity tests for this function. Use sentinel-filled destination buffers to verify that the first row and column are zero for each branch combination, while keeping the existing parity test unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
computer-vision
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
84/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.