Repository navigation
Conversation
- ISO 8601 durations are parsed in 64 bits - A failed PullMessages allocation is answered with an error - RelatesTo has room for the longest message ID
Measured on SSC323: port 2 gives 320x180 NV12 frames with a 320 stride. Port 3 returns buffers with fresh timestamps whose memory is never written, even through an uncached mapping.
- Encoders no longer take the port the motion detector reads - Frames smaller than the detector input are skipped - More headroom for MI_SYS_BufInfo_t
PR Summary by QodoAdd infinity6 software motion detection and ONVIF PullPoint events
AI Description
Diagram
High-Level Assessment
Files changed (40)
|
Code Review by Qodo
1. Motion detection never starts when snapshots are on
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+77
to
+80
| for (int i = 0; i < ONVIF_EVENT_MAX_SUBS && sub->id; i++) | ||
| if (!subs[i].id || (!subs[i].waiting && sub->waiting) || | ||
| (!subs[i].waiting == !sub->waiting && subs[i].lastSeen < sub->lastSeen)) | ||
| sub = &subs[i]; |
There was a problem hiding this comment.
2. A new subscriber evicts an active client 🐞 Bug ☼ Reliability
onvif_event_subscribe selects an occupied slot even when every subscription has a waiting pull, then clears that slot for the newcomer. Once four clients are long-polling, a fifth subscription invalidates one active client's subscription and its pending pull returns an unknown-subscription fault.
Agent Prompt
## Issue description
Creating a fifth subscription replaces a client whose pull is actively waiting when all four slots are in use.
## Fix Focus Areas
- src/onvif_event.c[70-88]
- src/onvif.c[399-414]
## Recommended Fix
Select only free or non-waiting slots for replacement. If none exists, return a failure from the store and send an appropriate SOAP fault instead of invalidating an active subscription.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+95
to
+98
| onvif_event_sub *sub = find_sub(id); | ||
| if (sub) { | ||
| sub->expires = *expires = now + seconds; | ||
| sub->lastSeen = now; |
There was a problem hiding this comment.
3. Expired subscriptions can be renewed 🐞 Bug ≡ Correctness
onvif_event_renew checks only whether the ID remains in the store and replaces its expiration without checking the previous deadline. Between the termination time and the main loop's next once-per-second expiry sweep, a Renew request revives the subscription; PullMessages can also use it during that interval.
Agent Prompt
## Issue description
Subscription operations can accept an expired ID until a separate periodic sweep removes it.
## Fix Focus Areas
- src/onvif_event.c[93-102]
- src/onvif_event.c[129-163]
- src/onvif_event.c[169-176]
## Recommended Fix
Compare the stored expiration with the supplied monotonic time while holding the event mutex, and treat expired subscriptions as unknown before renewing or returning messages. Keep the periodic sweep for idle subscriptions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+99
to
+100
| if (app_config.motion_detect_enable) | ||
| motion_start(); |
There was a problem hiding this comment.
1. Motion detection never starts when snapshots are on 🐞 Bug ≡ Correctness
rawChannel is only set inside raw_create, called from motion_start() after sdk_start(), while sdk_start() already ran media_mp4_enable (channel 0), media_mjpeg_enable (channel 1) and jpeg_init (channel 2 via take_next_free_channel). With mp4, mjpeg and jpeg all enabled, i6_raw_create sees i6_state[I6_RAW_PORT].enable set, prints "VPE port 2 is already used by an encoder", and detection is disabled; the new skip in take_next_free_channel only protects port 2 from channels created later.
Agent Prompt
## Issue description
The raw VPE port (I6_RAW_PORT = 2) is only reserved when motion_start() runs, which is after sdk_start() has already handed channel 2 to the JPEG encoder. With mp4, mjpeg and jpeg all enabled, motion detection then fails to start.
## Fix Focus Areas
- src/main.c[93-100]
- src/media.c[374-393]
- src/media.c[983-990]
## Recommended Fix
Reserve the raw channel before any encoder channel is taken. For example, when `app_config.motion_detect_enable` is set on HAL_PLATFORM_I6, set `rawChannel = I6_RAW_PORT` at the top of sdk_start(), before media_mp4_enable/media_mjpeg_enable/jpeg_init run, and clear it again if raw_create fails. Another option is to call raw_create inside sdk_start right after the pipeline is created and before the encoders, with motion_start() only starting the thread.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+13
to
+17
| void motion_pause(int ms) { | ||
| pthread_mutex_lock(&pauseMtx); | ||
| pauseFrames = ms * MOTION_FPS / 1000; | ||
| pthread_mutex_unlock(&pauseMtx); | ||
| } |
There was a problem hiding this comment.
5. Motion pause function is never called 🐞 Bug ⚙ Maintainability
motion_pause is exported in motion.h and writes pauseFrames, which motion_thread reads under pauseMtx, but nothing in the tree calls it (night mode / IR cut switching does not use it). The mutex, the pauseFrames plumbing and motion_core_pause in production are dead code, and a later reader may assume IR-cut switches are already suppressed through this path.
Agent Prompt
## Issue description
motion_pause() is declared and implemented but never called, so the pause path is dead code.
## Fix Focus Areas
- src/motion.c[8-17]
- src/motion.h[16-16]
## Recommended Fix
Either call `motion_pause(<ms>)` from the night-mode code wherever the IR cut filter or IR LEDs are switched, or remove motion_pause, pauseMtx, pauseFrames and the matching block in motion_thread.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
|
Pushed 7109f2c for the frame stall finding (details in its thread). The other findings are not addressed in this PR:
|
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
1 participant
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.
Depends on #53 (ONVIF events); this branch is on top of it, so the first commits here are #53's.
For "Motion detection" on the roadmap. infinity6 has no hardware motion detector available here
(no IVE, no vendor MD library in OpenIPC's SDK for this family), so this adds a small one in C:
a 320x180 NV12 copy from a free VPE port (port 3 hands out buffers that are never written on
SSC323, so port 2), read at 5 fps; a fixed-point running background, 16x16 blocks, a
whole-frame change (lights, IR cut) ignored and relearnt.
motion_detect: enable, sensitivity (1-10), hold_s. Encoders no longer take the port the detector reads. State changes feed theONVIF event service from #53 as
tns1:VideoSource/MotionAlarm.Under 1 % CPU on SSC323. Host tests for the detector core (
make -C test). Running for days ona camera watched by an NVR that records on these events: people and a small dog detected, day
and IR night.
The HAL changes are kept inside
src/hal/star/i6_*and conditioned on the series where theyare specific to SSC32x; glad to rebase once the HAL naming refactor you mentioned in #43 lands.