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 ISP-gain night switching, motion detection, and ONVIF events
AI Description
Diagram
High-Level Assessment
Files changed (44)
|
Code Review by Qodo
1. Pull requests can exhaust server workers
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+389
to
+390
| if (i6_state[I6_RAW_PORT].enable) | ||
| HAL_ERROR("i6_raw", "VPE port %d is already used by an encoder!\n", I6_RAW_PORT); |
There was a problem hiding this comment.
2. Motion detection fails when jpeg uses port 2 🐞 Bug ≡ Correctness
i6_raw_create() fixes raw capture to port 2 and rejects that port when an encoder already uses it. With MP4 and MJPEG occupying the first two channels, enabling JPEG assigns it channel 2 before motion starts, so motion detection cannot start.
Agent Prompt
## Issue description
Raw capture conflicts with JPEG when the earlier encoders occupy channels 0 and 1.
## Fix Focus Areas
- src/hal/star/i6_hal.c[383-407]
- src/media.c[374-390]
- src/media.c[433-442]
## Recommended Fix
Reserve the raw port before encoder allocation when motion is enabled, or select an available supported raw port and carry that selection through capture and release.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+72
to
+75
| if (core->settle) { | ||
| core->settle--; | ||
| core->primed = false; | ||
| return motion_core_step(core, false); |
There was a problem hiding this comment.
4. Ir-cut pauses can emit false motion events 🐞 Bug ≡ Correctness
motion_core_feed() treats every paused frame as a no-motion frame and advances the normal motion-ending counter. If motion is active when the night thread requests a three-second pause and hold_s is set to 1, 2, or 3, the detector emits an end event during the pause and can emit a new start event afterward.
Agent Prompt
## Issue description
Paused frames count as quiet frames and can end an active motion event.
## Fix Focus Areas
- src/motion_core.c[48-75]
- src/night.c[103-107]
## Recommended Fix
While paused, suppress analysis without advancing the motion-ending counter; resume from a rebuilt background without manufacturing an end/start pair.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+487
to
+492
| if (pthread_create(&thread_id, &thread_attr, send_pullmessages_thread, (void *)task)) { | ||
| HAL_DANGER("onvif", "Can't create thread\n"); | ||
| send_http_error(client_fd, 500); | ||
| free(task); | ||
| } else | ||
| pthread_detach(thread_id); |
There was a problem hiding this comment.
1. Pull requests can exhaust server workers 🐞 Bug ⛨ Security
start_pullmessages() creates a detached thread for every PullMessages request without limiting outstanding workers. The four-wait limit applies only after thread creation, and dispatch releases the request's normal server slot, so a request flood can consume thread and socket resources outside that limit.
Agent Prompt
## Issue description
The server creates an unbounded number of detached PullMessages workers despite its limited subscription waits.
## Fix Focus Areas
- src/server.c[472-494]
- src/server.c[734-739]
- src/onvif_event.c[129-153]
## Recommended Fix
Reserve a bounded worker slot before allocating or starting each pull worker; reject excess requests and release the slot on every worker exit path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+77
to
+83
| 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]; | ||
| if (sub->id) pthread_cond_broadcast(&eventCond); | ||
|
|
||
| memset(sub, 0, sizeof(*sub)); |
There was a problem hiding this comment.
5. New subscribers can interrupt active pulls 🐞 Bug ☼ Reliability
onvif_event_subscribe() selects a waiting subscription for replacement when all four slots have waiting pulls. A fifth subscription then clears that active slot and wakes its client into an unknown-subscription response, even though the client's subscription has not reached its termination time.
Agent Prompt
## Issue description
Creating a fifth subscription evicts an existing client when every slot has a waiting pull.
## Fix Focus Areas
- src/onvif_event.c[70-90]
- src/onvif.c[399-415]
## Recommended Fix
If every slot is occupied by an active waiting subscription, return a capacity failure instead of replacing one, and turn that failure into an appropriate SOAP fault.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+366
to
+368
| if (*messageId) | ||
| snprintf(relates, sizeof(relates), "\n <wsa:RelatesTo>%s</wsa:RelatesTo>", messageId); | ||
| onvif_reply(response, respLen, "200 OK", eventxml, action, relates, body); |
There was a problem hiding this comment.
6. A message id can corrupt event replies 🐞 Bug ≡ Correctness
onvif_reply_event() inserts the request's MessageID directly into a RelatesTo XML element without escaping it. A request containing XML-significant characters in that field reaches the event response handlers and produces malformed or injected SOAP header content.
Agent Prompt
## Issue description
Request-controlled message IDs are copied into event-response XML without escaping.
## Fix Focus Areas
- src/onvif.c[357-368]
- res/onvif/event.xml[3-5]
## Recommended Fix
XML-escape the extracted message ID before formatting the RelatesTo element, and check the formatted output's bounds.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+90
to
+93
| if (get_isp_gain(&gain)) { | ||
| HAL_WARNING("night", "No ISP gain on this platform, automatic switching disabled!\n"); | ||
| break; | ||
| } |
There was a problem hiding this comment.
10. One failed gain read ends automatic switching 🐞 Bug ☼ Reliability
The ISP-gain loop breaks on the first non-zero return from get_isp_gain, and it logs "No ISP gain on this platform" even when MI_ISP_AE_QueryExposureInfo exists and only failed once. That single error stops day/night switching and the 10-minute IR cut re-pulse until the night thread is restarted.
Agent Prompt
## Issue description
A transient ISP query error permanently stops gain-based switching and the periodic IR cut pulse.
## Fix Focus Areas
- src/night.c[90-93]
- src/hal/star/i6_hal.c[559-571]
## Recommended Fix
On a gain read error, set `held = 0` and `continue` instead of breaking. Only stop with the warning when the platform has no gain support (missing symbol or unsupported platform) or after N consecutive failures.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Author
There was a problem hiding this comment.
Fixed in 4876b37. Whether the platform has an ISP gain is now checked once, before the loop. Inside the loop a failed read resets held and retries on the next second, so switching and the IR cut re-pulse keep running; it is logged once per run of failures.
Checked on an SSC323 with a test build that fails the gain read for 3 s every 30 s: the loop kept running through every failure.
On SSC323 a char left the picture purple with the IR cut filter off, the ISP reads a full word.
MI_BOOL is 4 bytes wide in MI_ISP_AE_QueryExposureInfo on SSC323. In the dark the ipc017 reaches 64x sensor and 3x ISP gain; with the IR LED on it settles around 6x, above the default day threshold.
- The night mode flag is set before its thread starts - Automatic switching stops with a warning when there is no ISP gain
…eft it out by day
…ad, keeping manual settings on restart
Contributor
Author
|
Pushed 4876b37 (on top of the updated #54, 7109f2c) for findings 3, 7, 8, 9 and 10; details are in each thread. Findings 1, 2, 4, 5 and 6 are not addressed in this PR: 2 is the same port 2 limitation discussed in #54, and 4 only applies with The image running here with these fixes has been streaming normally since the update (1080p at 20 fps, ONVIF events consumed by a PullPoint client). |
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 #54 (motion detection), which itself depends on #53; this branch is on top of both.
Cameras like the ipc017 have neither an ADC nor a light sensor pin. Without one the night
thread only slept; it now reads the ISP gain (
MI_ISP_AE_QueryExposureInfo, optional) andswitches to night at 8x held 15 s and back to day below 2x held 60 s (the IR LED brings the
gain down to ~6x, so the day threshold must stay under it), pausing motion detection while
the IR cut filter moves. The thresholds are
night_modesettings; automatic switching stopswith a warning when the ISP gain cannot be read.
Also:
night_disable(it looped onkeepRunningonly, and/api/night?enable=falsehung);was missed and the filter stayed out by day, the picture went pink and the lower gain meant
no crossing would ever correct it;
gpio_readread 0 bytes (only the read length here: thefd < 0checks are left tohal: add Fullhan FH8852/FH8856 (V100) support #43, which fixes them in every sysfs helper);
MI_ISP_IQ_SetColorToGraytakes a 4-byte boolean (a char left the picture purple on SSC323).Measured on SSC323: 64x sensor and 3x ISP gain in the dark, ~6x with the IR LED; one clean
transition each way per night, plus the expected ones when the room light goes on and off.
With the filter forced out by day, the next periodic pulse put it back.
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.