fix(motion): cap motion area so a whole-frame lighting shift isn't flagged as a subject - #618
Merged
Merged
Conversation
…agged as a subject FrontDoor's fast motion engine (added to catch brief events the slower SOD sampling could miss) was producing a clip every few minutes around the clock: ~40% of all 'motion' events over 24h were the exact ceiling reading (area=100%, single cluster spanning the whole frame), spread evenly across every hour rather than clustered at dawn/dusk. A real subject never fills the entire monitored area at once; this pattern matches a camera's IR-cut filter flipping between day/night mode, which can be triggered any time of day (headlights, a porch light, clouds), not just at twilight. The detector had a minimum area to trigger but no maximum, so a whole-frame brightness/colour step reads identically to a real, localized object -- and since it always saturates the score to its maximum, raising the confidence threshold can't distinguish the two; a real fix needs an upper bound on area instead. Adds max_motion_area (default 0.90, matching the existing per-stream min_motion_area convention) and applies it in both the grid-based and non-grid detection paths. Not exposed via configure_motion_detection() or any API, consistent with several other tunables in this struct (blur_radius, noise_threshold, grid_size) that are also internal-only. tests/unit/test_motion_max_area.c (new): a whole-frame uniform brightness step (no real subject could ever produce this) must not be flagged, while a localized change confined to part of the frame (what a real subject looks like) must still be detected normally. Both verified against a genuine RED/GREEN cycle reproducing the exact production log signature (score=1.000, area=100.00%, clusters=1), and a clean mutation check (removing the cap fails only the whole-frame test, not the regression test). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Non-grid detection lacks regression coverage, and the new test is omitted from the integration workflow allowlists.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Adds a 90% maximum motion-area threshold to suppress whole-frame lighting-change false positives while preserving localized motion detection.
Changes:
- Applies the cap to grid and non-grid detection paths.
- Adds regression tests for whole-frame and localized changes.
- Registers the new unit test.
| File | Summary | Review notes |
|---|---|---|
tests/unit/test_motion_max_area.c |
Adds motion-area regression tests. | Coverage currently exercises only grid detection; add non-grid cases. |
tests/unit/CMakeLists.txt |
Registers the new test target. | The integration workflow allowlists omit this target; update the allowlists or run all registered tests. |
src/video/motion_detection.c |
Enforces the maximum area threshold in both detection paths. | Non-grid behavior requires dedicated regression coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1289
to
+1290
| motion_detected = (motion_area >= stream->min_motion_area) && | ||
| (motion_area <= stream->max_motion_area); |
| add_layer3_test(test_api_handlers_system) | ||
| add_layer1_test(test_external_motion_trigger) # Layer 1: external_motion_trigger state-machine (PR #356) | ||
| add_layer2_test(test_motion_trigger_parse) # Layer 2: motion trigger body parsing (#466) | ||
| add_layer2_test(test_motion_max_area) # Layer 2: motion detector max-area sanity cap |
Addresses two findings from Copilot's review of PR opensensor#618 — both real: - The new tests only exercised the default grid-based detection path; the non-grid (simple frame-differencing) path got the identical cap but had no coverage of its own, so a regression that removed or misapplied it there would have passed every existing test in the file. Adds the same whole-frame/localized pair configured with use_grid_detection=false, verified with a genuine mutation check (reverting just the non-grid cap fails only that one test). - test_motion_max_area was registered in CMake but missing from both places integration-test.yml lists tests by name (the build-target list and the ctest -R filter), so it never actually ran in that CI job despite building successfully everywhere else. No change to src/video/motion_detection.c -- confirmed identical to the already-deployed fix commit; this round is test/CI coverage only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
Pushed 92373a9 addressing both findings — both real:
No change to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The built-in frame-differencing
motiondetection engine had a minimum area to trigger detection but no maximum. On a reference install, one camera's fast motion engine (added to catch brief events between the slower object-detector's sampling interval) was producing a clip every few minutes around the clock instead of only on real activity:area=100.00%, a single connected-component cluster spanning the entire grid — spread evenly across every hour of the day and night (2–31 per hour), not clustered around dawn/dusk twilight.sqrt(1.0) = 1.0), so it passes almost any threshold, while a higher threshold risks missing real, smaller-magnitude events too.Change
Adds
max_motion_area(default0.90) to the per-stream motion config, alongside the existingmin_motion_area, and applies it in both the grid-based and non-grid (simple frame-differencing) detection paths insrc/video/motion_detection.c. Not exposed viaconfigure_motion_detection()or any API — consistent with several other tunables on the same struct (blur_radius,noise_threshold,grid_size) that are also internal-only today.Testing
tests/unit/test_motion_max_area.c(new, registered intests/unit/CMakeLists.txt):Both were verified against a genuine RED/GREEN cycle: the whole-frame test reproduces the exact production log signature (
score=1.000, area=100.00%, clusters=1) before the fix, passes after it, and a mutation check (temporarily removing the cap) fails only that test, not the localized-detection regression test — confirming the two are correctly isolated.Existing suites unaffected:
test_detection_model_motion(16/16) andtest_motion_trigger_parse(27/27) still pass.Production verification
Deployed on the reference install with the affected camera's engine configuration unchanged otherwise:
area=100%)Zero ceiling-value spikes across the full 4-hour observation window post-deploy, versus a rate that had been roughly 10–20 per hour before. The remaining recordings all show localized, plausible area percentages (16.67%–83.33%), with nothing at the impossible full-frame maximum.
🤖 Generated with Claude Code