Fixed-budget tiled detection to recover small-object resolution - #8
Conversation
Frames reaching the model were downscaled twice — once to MAX_IMAGE_SIZE=1024 in the endpoint, then letterboxed to 640 by the backend. At 1080p that is a net linear scale of 0.333, so a 60px person arrives 20px tall, right at the floor where YOLO stops firing. Two changes, together: 1. Detection keeps full resolution. The endpoint now passes MAX_DETECTION_IMAGE_SIZE (8192, a sanity ceiling rather than a working limit) to preprocess_image. The backend letterboxes to model input anyway, so the intermediate resample bought nothing and cost small objects. 2. Optional tiling, on a fixed inference budget. With tiles=T>1 the frame is split into overlapping crops sized so a min_object_px object survives the scale down to model input. Tile 0 is always the whole frame, so behaviour never regresses below the untiled path; the remaining T-1 rotate over the grid on a clock-derived cursor. Cost is exactly T inferences whether the scene is empty or a tree is thrashing — content changes which regions are looked at, never how many. Cross-tile merge is class-aware NMS ordered by (truncated, -confidence), so an object seen whole in one tile beats the copy clipped at a seam in another. A containment test catches fragments that fall below the IoU threshold against the full box and would otherwise survive as phantoms. utils/tiling.py has no third-party imports, so the geometry is testable without a model, numpy or pydantic. 60 new tests; existing suites still pass. Backward compatible at the interface: tiles defaults to 1, which takes the untiled path, and DetectionResponse is unchanged so lightNVR's parser keeps working. Detection *values* do change on every path, because change 1 removes the downscale — that is the intent. Note change 1 also affects the tflite and edgetpu backends, which share the endpoint and are otherwise out of scope here. They will now receive full-resolution images and squash them harder in their bare resize(). Their output geometry is self-normalizing so this is a quality shift, not a correctness break, but it is untested on those backends. Known limitation, pinned by test: tile_period must match the rate the caller actually fires at. The cursor is linear in the clock, so a caller whose period is an exact multiple of tile_period samples the same residues forever and the tiles in between never get visited. On lightNVR's keyframe-gated path the effective rate is the GOP length, not the configured detection_interval. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This should work on an edgetpu at about the same performance as the host I used for testing.
I've only briefly tested it, I was planning a larger test on my in-service box but haven't gotten around to it |
There was a problem hiding this comment.
🟡 Not ready to approve
The new tile sizing math can undershoot the intended MIN_MODEL_PX guarantee due to rounding, and a newly added overflow test uses a large min_object_px that won’t exercise the intended overflow path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR improves small-object detection fidelity by preventing an unnecessary pre-model downscale on /v1/detect, and introduces an optional fixed-budget tiled-detection mode (constant inference count per request) with cross-tile merge logic and accompanying tests.
Changes:
- Add
utils/tiling.pywith tile planning, stateless time-based tile selection, box mapping, truncation detection, and cross-tile merge (NMS + containment). - Extend
preprocess_image()to accept an optionalmax_size, and use a higher ceiling for/v1/detectwhile keeping legacy behavior for/describeand/query. - Add stdlib-only unit tests and PIL-based integration tests for tiling geometry/merge behavior; add
pytestas an optional dev dependency; add tiling defaults to config.
File summaries
| File | Description |
|---|---|
| utils/tiling.py | New tiling/merge geometry module for fixed-budget tiled detection. |
| utils/image.py | Adds optional max_size parameter to avoid redundant downscale for detection. |
| api/v1/endpoints/detection.py | Wires per-request tiling controls into /detect and routes through tiled orchestration when enabled. |
| config.py | Adds detection max-size ceiling and default tiling parameters. |
| tests/test_tiling.py | New stdlib-only unit tests for tiling geometry/selection/merge helpers. |
| tests/test_tiling_integration.py | New PIL-based end-to-end geometry tests using an oracle “detector”. |
| Pipfile | Adds pytest as an optional dev dependency for running the new test suites. |
Review details
Suppressed comments (2)
utils/tiling.py:183
- tile_grid_overflowed() duplicates the region size math from plan_tiles(); if plan_tiles uses floor sizing to preserve the MIN_MODEL_PX guarantee, this helper should match it or it may disagree about whether the grid overflowed/truncated.
required_scale = MIN_MODEL_PX / float(min_object_px)
region_w = min(img_w, int(ceil(model_w / required_scale)))
region_h = min(img_h, int(ceil(model_h / required_scale)))
utils/tiling.py:17
- Unused import: Optional is imported from typing but not used (all signatures annotate min_object_px as int). Either remove Optional from the import list or update the relevant parameters to Optional[int] to match the existing None-handling.
from typing import Iterable, List, Optional, Sequence, Tuple
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| region_w = min(img_w, int(ceil(model_w / required_scale))) | ||
| region_h = min(img_h, int(ceil(model_h / required_scale))) |
| plan = plan_tiles(4000, 3000, 640, 640, 2000) | ||
| self.assertLessEqual(len(plan), MAX_TILES) | ||
| if len(plan) == MAX_TILES: | ||
| self.assertTrue( | ||
| tile_grid_overflowed(4000, 3000, 640, 640, 2000) | ||
| ) |
| Boxes are ``(x_min, y_min, x_max, y_max)`` normalized to whatever frame they belong to. | ||
| """ | ||
|
|
||
| from dataclasses import dataclass, replace |
From Copilot's review of opensensor#8. Three of its five points were correct. utils/tiling.py: the crop size was rounded up. A crop of `region` letterboxed to `model_dim` scales a source object by model_dim/region, so holding a min_object_px object at MIN_MODEL_PX needs region <= model_dim/required_scale. Rounding up overshoots that bound, landing the object fractionally under the floor — the opposite of the guarantee the module documents. It breaks the invariant in 129 of the 191 min_object_px values between 10 and 200. The shortfall is sub-pixel (worst 0.054px) and changes no detection in practice, but floor is correct and free. The two sites that computed this had the expression copy-pasted, which is how they would have drifted apart under exactly this fix. Extracted to _region_size() so plan_tiles and tile_grid_overflowed cannot disagree about tile size, and therefore cannot disagree about whether the grid overflowed. tests/test_tiling.py: test_grid_is_capped_and_overflow_is_reported passed min_object_px=2000 while claiming to test a "very small" value. Large values collapse the grid to the single full-frame tile, so assertLessEqual(1, 32) succeeded trivially and the real assertion sat behind an `if` that was never true. It tested nothing. Now uses 8, which does bind the cap, and asserts unconditionally. Added test_crop_scale_never_puts_the_target_below_the_model_floor, which asserts the MIN_MODEL_PX invariant directly across three resolutions and 28 object sizes. That invariant was documented in the module docstring and never checked, which is why the rounding bug survived 52 tests. Verified it fails on the old rounding (23.97 < 24) rather than passing vacuously like the test above. Also dropped the unused dataclasses.replace and typing.Optional imports.
|
Thanks for the review — three of the points were correct and are fixed in baa9273. Crop size now rounds downCorrect catch. A crop of
For calibration, the magnitude is sub-pixel — worst case a 0.054 px shortfall, e.g. a 40 px object arriving at 23.9925 px instead of 24. No detection changes in practice, so this isn't a "small objects were being lost" bug. But The two region-size sites are now oneFollowing the note about The overflow test was worse than wrongAlso correct, and this was the more valuable of the two findings. It now uses The finding behind the findingThe Added and it passes with AlsoDropped the unused Test suite is 61 now (53 unit + 8 integration), all passing, still stdlib-only and self-running under plain |
There was a problem hiding this comment.
🟡 Not ready to approve
A few new/updated docstrings promise “exactly T” inferences when the implementation can legitimately run fewer, and preprocess_image(max_size=...) needs basic validation to avoid misconfiguration causing misleading 400 errors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (4)
utils/tiling.py:216
- This docstring says it picks exactly
budgettiles, but the function clamps to the plan size (and can return only tile 0 when the plan collapses). Adjusting the wording avoids misleading callers/tests.
"""Pick exactly ``budget`` tiles: tile 0 plus a time-rotated slice of the rest.
utils/tiling.py:6
- The module docstring claims each request runs exactly T inferences, but the implementation can run fewer (e.g., when the tile plan collapses to only the full-frame tile, or when selection is clamped to the plan size). This mismatch can confuse users relying on a strict fixed-budget contract; update the docstring to describe the actual invariant (cost is independent of scene content and never exceeds T).
This issue also appears on line 216 of the same file.
The invariant this module exists to hold: **a request runs exactly T inferences**,
T being a caller-supplied constant. Scene content never changes T — it only changes
which T regions get looked at. Cost per request is therefore flat whether the frame
is empty or a tree is thrashing in the wind.
api/v1/endpoints/detection.py:190
- The endpoint docs say cost is "exactly T inferences" regardless of scene content, but
_detect_tiledcan run fewer whentile_deadline_sis hit (andselect_tilescan clamp to the plan size). Update the description to reflect "at most T" while keeping the scene-content invariance claim.
- **tiles**: Fixed inference budget T per request. With T>1 the frame is split into
overlapping crops sized so a `min_object_px` object survives the scale down to
model input; tile 0 is always the whole frame and the remaining T-1 rotate over
the grid on a clock-derived cursor. Cost is exactly T inferences regardless of
scene content.
utils/image.py:70
max_sizeis now caller-supplied but isn't validated. If it is misconfigured (e.g., 0 or negative via env), resize math can produce invalid dimensions and the request will fail with a misleading "Invalid image" 400. Validatemax_sizeis a positive integer before using it.
if max_size is None:
max_size = settings.MAX_IMAGE_SIZE
if image.width > max_size or image.height > max_size:
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
Thanks @johnchia -- if there are followups we can just do a new PR. |
|
Sounds good - if you have feedback from testing I'd love to have it |
|
I probably won't know for a while, but I'll deploy the latest code/build to detect.lightnvr.com/api/v1/detect at some point in the next week. |
Recovers small-object detection accuracy by removing a redundant downscale and adding an optional fixed-budget tiling mode. Independent of #6 — no overlapping files.
The problem
Frames are downscaled twice before reaching the model.
preprocess_imagecaps the longest side atMAX_IMAGE_SIZE = 1024, then the backend letterboxes that down to model input (640 for YOLO).For 1080p that is 1920 → 1024 → 640, a net linear scale of 0.333 across two resamples. At 5 MP it is 0.247. A person 60 px tall in the source arrives at the model as 20 px — right at the floor where YOLO stops firing. The 1024 cap buys nothing, because the backend letterboxes anyway; it only costs a resample.
Two changes
1.
preprocess_imagegains an optionalmax_size./v1/detectpasses a newMAX_DETECTION_IMAGE_SIZE(8192 — a sanity ceiling, not a working limit) so full resolution survives to the backend./describeand/querykeep the old 1024 default, since Moondream has its own sizing needs.This alone is most of the win, and it applies to every request whether or not tiling is used.
2. Optional tiled detection, off by default. Per request, run exactly T inferences. Content never changes T — it only changes which T regions get inspected, so cost is flat whether the scene is empty or a tree is thrashing in the wind.
The grid is derived from
min_object_pxrather than being a fixed layout. Crop size is whatever makes the smallest interesting object land at the model's ~24 px detection floor:Crops are laid on a 25%-overlap stride with the last row/column flushed to the frame edge. Tile 0 is always the whole frame, so large and near objects are caught every cycle and current behaviour never regresses. The remaining T−1 rotate over the pool on a clock-derived cursor — stateless, so there is no per-stream server state, nothing to evict, and no multi-worker constraint:
Grid sizes at
min_object_px=60, 640 model:Cross-tile merge is class-aware NMS ordered by
(truncated, -confidence), so an object seen whole beats a copy clipped at a seam, plus a containment test — a sliver of a large object has poor IoU against the full box, and plain NMS would keep it as a phantom.Usage
No client changes needed. A stream's
model_pathmay be a full URL with a query string, so this is configurable per camera:Query params:
tiles(T, default 1 = off),min_object_px,tile_overlap,tile_period,tile_deadline_s,stream.Cost
Measured on an i5-6500 / HD Graphics 530 with the OpenVINO provider, 5 MP frames: ~30 ms per inference, ~72 ms JPEG decode (paid once per request regardless of T). Six cameras at 1 Hz and T=4 is roughly 72% of that iGPU. Since T is a per-request parameter it is effectively per-camera — close-range cameras can run
tiles=1and still benefit from change 1.A
tile_deadline_slimiter (default 7 s) stops issuing tiles and returns what it has, so a slow or degraded host degrades to partial coverage rather than hitting LightNVR's hard-coded 10 s client timeout. Tile 0 always runs before the deadline is consulted, so the worst case is current behaviour rather than an empty response.Backward compatibility
tilesabsent or1takes the untiled path, andDetectionResponseis unchanged, so LightNVR's parser keeps working untouched.Detection values on that path do change, because change 1 removes the downscale ahead of it. That is the intended improvement — only the response contract is frozen.
This affects tflite and edgetpu too, since
MAX_IMAGE_SIZElives on the shared endpoint. They will now receive full-resolution images and squash them harder in their bareresize(). Output geometry is unchanged (both normalise to their own input), so it is a quality shift rather than a correctness break — but it is untested on those backends, and worth a second opinion from someone with Coral hardware.Known limitation
tile_periodmust match the rate the caller actually fires at. The cursor is linear in the clock, so a caller whose period is an exact multiple oftile_periodsamples the same residues forever — permanent blind spots, not merely a slower sweep. With a 4-tile pool and T=3, a caller at 2 s againsttile_period=1selects tiles 1 and 2 every cycle and never visits 3 or 4.This matters because LightNVR's keyframe-gated fallback path fires at the GOP length, not at the configured
detection_interval. A stride coprime to the pool size would absorb it, but that stretches a 24-tile sweep from 8 cycles to ~12, trading away the property the design rests on to paper over a misconfiguration that already has an exposed remedy. Documented inselect_tilesand pinned bytest_commensurate_period_mismatch_starves_tilesso it stays a known limitation rather than a field surprise.Testing
60 new tests, all stdlib-only and self-running under plain
python3as well as pytest, following the pattern intests/test_onnx_providers.py:tests/test_tiling.py— 52 tests. Grid planning at 1080p/5 MP/square/extreme aspect, the degenerate collapse to full frame, cursor sweep bounds and starvation, coordinate round-trips, truncation detection, merge behaviour, box maths.tests/test_tiling_integration.py— 8 tests. End-to-end geometry against real PIL crops with an oracle "detector", which is what would catch an off-by-one between a crop rectangle and the normalisation applied to boxes coming back from it. Needs PIL but no model, numpy or pydantic.utils/tiling.pydeliberately has zero third-party imports so the geometry is testable without a runtime installed.Verified in a live LightNVR deployment: persons, TVs and lights are picked up at noticeably greater distances than before.