Feature/onvif event stream#273
Open
Bazze wants to merge 7 commits into
Open
Conversation
Contributor
Author
|
Currently working on sharedjourney#1 as a next step on re-using the same stream for the other things as well. Untested so far, will update once confirmed working. Let me know if any feedback |
Member
|
many thanks for the update still have to review this one. The Onvif contribution has already been merged, many thanks 🫶 |
Contributor
Author
|
Yes I saw, thanks! I've been testing the onvif changes on an AXIS device today and I'm working on a follow-up PR to address a few things that wasn't working fully. Coming soon! |
Member
|
Love the spirit! Feel free to get some free coffees at our offices and a city guide when you would come to Ghent |
ttradesman
force-pushed
the
feature/onvif-event-stream
branch
from
June 10, 2026 07:16
36c03f6 to
ebd4599
Compare
Member
|
@ttradesman any help needed? |
|
None at all! Would be great to get both kerberos-io/onvif#6 merged and then this one. We are by now relying on this code to control motion recording via ONVIF; super if we soon could go back to use official build instead of our fork. We are as well running these changes on the agent: sharedjourney#1 . We were thinking once this branch is merge, to open a next PR with those changes 🙏 |
Member
|
Looks like this is building, can you provide more information about the current brands currently tested (ONVIF specs vary from brand to brand). I see Axis, mentioned is this the brand you developed against? |
|
Yes, correct! AXIS when it comes to ONVIF (and kerberos-io/onvif#6 was needed for AXIS motion part to work) |
Bazze
force-pushed
the
feature/onvif-event-stream
branch
2 times, most recently
from
June 22, 2026 12:58
753737c to
82e847a
Compare
ttradesman
force-pushed
the
feature/onvif-event-stream
branch
2 times, most recently
from
June 29, 2026 06:29
0e36b7b to
5d000ff
Compare
Member
|
We have a few conflicts now @Bazze |
Bazze
force-pushed
the
feature/onvif-event-stream
branch
from
July 9, 2026 05:38
5d000ff to
e7b880a
Compare
Adds Capture.ONVIFMotion and the matching AGENT_CAPTURE_ONVIF_MOTION environment override. When set to 'true', the agent will open an ONVIF event stream against the configured camera and route Motion events into the existing HandleMotion channel (wired up in the next commit). Defaults to empty (disabled), so existing deployments using the pixel-diff motion detector see no behaviour change.
Resolves kerberos-io#173. When Capture.ONVIFMotion='true' the agent opens a stream.NewStream against the configured ONVIF endpoint and forwards Motion+Active events to HandleMotion, so AXIS cameras (and any other ONVIF-conformant device) can drive motion-triggered recording without relying on the pixel-diff detector. Why this shape -------------- Maintainer @cedricve's direction on kerberos-io#194 was 'extend the ONVIF library first to expose a go channel and hide the protocol complexity'. That work landed in kerberos-io/onvif (event/stream sub-package); the agent integration is now a thin consumer: connect to the device, wrap it in NewStream, range over Events, push Motion events at the existing HandleMotion channel. Design choices -------------- * New file machinery/src/onvif/events.go keeps the new code separate from the existing PTZ/IO-focused onvif/main.go so reviewers can read it without scrolling. * Opt-in via Capture.ONVIFMotion. Defaults preserve the current pixel-diff behaviour so nothing changes for existing users. * DispatchEvent only fires on StateActive — the leading edge. Motion STOP requires the recorder state machine to accept an explicit stop signal which today does not exist; tracked as a follow-up so this first PR stays small. * Non-blocking send to HandleMotion: drop ONVIF motion events when the channel is full rather than block the stream goroutine and starve subscription renewal. * Error logging routes by typed-error category from the library: ErrRecreateFailed is loud (camera may be offline), ErrPullFailed and ErrRenewFailed are debug-level (auto-recovers). * Goroutine lifetime tied to *communication.Context, the same cancellable context the agent uses to restart on config change. Not in this commit (intentionally deferred to follow-ups) --------------------------------------------------------- * Motion STOP wiring into capture/main.go's recorder state machine. * Replacing the ad-hoc CreatePullPointSubscription / GetEventMessages polling in cloud/Cloud.go with a stream consumer for DigitalInput and DigitalOutput events. The current code keeps working; the stream is purely additive. * Removing the temporary 'replace' directive on github.com/kerberos-io/onvif once the event/stream changes are tagged upstream.
Addresses the critical and important findings from the second review of
the agent integration. TDD followed locally: tests were written first
and confirmed RED against the previous implementation before the fix
turned them GREEN.
Critical fixes
--------------
* Shutdown-race panic (concurrency P0): the 3s gap between the agent's
ctx cancel and close(HandleMotion) was reachable by a buffered event
delivered after cancel, where dispatchEvent's send-with-default
select would panic on the closed channel. dispatchEvent now takes
ctx, has a pre-check after the kind/state/recording filters, and
the send select includes a <-ctx.Done() arm. Pinned by
TestDispatchEvent_CtxCancelledAndHandleMotionClosed_DoesNotPanic
(asserts NotPanics; current code without the fix panics).
* No retry on initial connect (Go P0 + ops P1): previously the
goroutine exited permanently if ConnectToOnvifDevice or
stream.NewStream failed at agent start — a brief boot-time DNS or
network blip silently disabled ONVIF until restart. Construction is
now wrapped in a retry loop with exponential backoff (1s -> 5min),
matching what cloud.HandleHeartBeat does for its ONVIF connection
attempts. The library handles in-stream recovery already; this
covers the gap the library cannot see.
* Strict 'true' match (Go P0): isONVIFMotionEnabled now normalises
case and trims whitespace, so 'True', 'TRUE', ' true ' all enable
the feature. Pinned by TestIsONVIFMotionEnabled_CaseAndWhitespace.
Important fixes
---------------
* Empty DeviceID fallback (Go P1): resolveDeviceID falls back from
configuration.Name to camera.ONVIFXAddr to 'unknown' so log lines
and metrics always have a useful identifier. Pinned by
TestResolveDeviceID_FallbackChain.
* Recovery log (ops P1): the run loop tracks a 'recovering' flag set
when an ErrPullFailed/ErrRecreateFailed lands on Errors and cleared
on the first successful Event. Logs an Info 'event stream recovered'
line so on-call operators can see error streaks clear, instead of
waking up to ERROR with no closure.
* Misconfig log bumped Info -> Warning so the
'ONVIFXAddr is empty' line stands out from the heartbeat noise.
Tests
-----
events_test.go covers the dispatch contract end-to-end:
* Motion+Active -> HandleMotion (happy path).
* Motion+Inactive ignored (motion-stop is a documented follow-up).
* Non-motion kinds ignored.
* Recording='false' gates the send.
* Full HandleMotion drops rather than blocks.
* Ctx-cancelled + closed HandleMotion does not panic.
* isONVIFMotionEnabled handles case and whitespace.
* resolveDeviceID fallback chain.
go.mod / go.sum: testify moved from indirect to direct dependency.
Deferred (out of scope for this commit, tracked as follow-ups):
* Heartbeat surface for ONVIF state ('disabled|running|failed') —
requires a Cloud.go change beyond this integration's scope.
* OTel span/metric for stream lifecycle.
* Runtime toggle without restart (config-reload).
* Replace-directive layout documentation — separate docs commit.
Audit against CLAUDE.md's 'default to no comments; only when WHY is
non-obvious'. Net ~30 lines removed.
Dropped (rot-prone or redundant)
--------------------------------
* 'matching what the pixel-diff detector emits' — references a
sibling file's behaviour.
* 'Timestamp in seconds matches what computervision/main.go emits;
downstream consumers (capture/main.go) tolerate...' — both
cross-file references; classic 'will rot when the sibling
changes'.
* 'Motion-stop wiring into the recorder state machine is tracked as
a follow-up; today the recorder uses a fixed PostRecording
timeout' — PR-description content masquerading as a code comment.
* 'happens only on ctx cancel today (library handles its own
reconnect)' — 'today' is a red flag; either drop or assert via
test, not narrate.
* dispatchEvent's first paragraph restating what the function does.
* runStreamOnce's first sentence (WHAT).
* isONVIFMotionEnabled's reference to sibling Capture fields
('unlike Recording / Motion / Snapshots which default to
enabled').
Kept (real WHYs)
----------------
* The shutdown-race rationale on dispatchEvent's ctx guards.
* logStreamError's severity-mapping rationale.
* The library-handles-reconnect-but-not-initial-connect rationale
for the backoff constants.
* The recovering-flag rationale (on-call ops use case).
* The flag-read-once invariant on HandleONVIFEventStream.
ttradesman
force-pushed
the
feature/onvif-event-stream
branch
from
July 23, 2026 09:29
e7b880a to
357cc71
Compare
dispatchEvent logged only rejected events, so the topic that actually started a recording was invisible — the only way to identify it was to enumerate every rejected topic and reason about what was left. On a camera emitting 18 distinct topics that is not a diagnosis. Log the Kind and topic on the dispatch path too, at debug, matching the reject line's shape so both sides of the decision grep the same way.
A camera replays the current state of every property topic as PropertyOperation=Initialized whenever a pull-point subscription is created. dispatchEvent looked only at Kind and State, so any motion property that happened to be active at that moment counted as a fresh trigger — meaning every reconnect restarts a recording, and a flapping subscription manufactures motion with no motion. Observed on a camera whose pull-point was being recreated every ~18s: each recreate replayed ~90 property events, and once real motion made the VMD property active the replays kept re-triggering it. Rejects Initialized specifically rather than accepting only Changed. PropertyOperation is optional per WS-Notification and absent on many non-property events, which decode reports as PropertyUnknown; those are real events and must still trigger.
Three defects in the dispatch path. Deleted was still a trigger. The Initialized guard was a denylist of one value, so a property removal with an active-looking payload passed straight through. Both Initialized and Deleted are announcements about a property, not motion starting, so accept the transitions instead: Changed, and Unknown for the events that omit the optional attribute. ev.Topic reached the log unmodified. It is camera-controlled, unbounded and unfiltered, and logrus's coloured text formatter — the default — writes the message without quoting, so an embedded newline forges whole log entries. A compromised camera could fabricate ERROR lines or spoof another device's id in the logs an operator is reading to diagnose that camera. Escape control characters and bound the length; the reject path logs every event received, so an oversized topic was also a cheap way to evict a container's retained history. The trigger line was logged before the send, so an event dropped on a full channel or at shutdown left a line claiming a recording that never started. Log it in the send case.
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
3 participants
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.
Add ONVIF event-stream motion source
Disclaimer
I am currently testing these changes. We are opening this PR now to get early feedback and pointers on direction (in case we have gone too bananas) - perhaps this is not the path you were intending.
Motivation
Closes #173. Today the only motion-triggered recording source is the pixel-diff detector in
computervision/. ONVIF cameras (AXIS in particular) emit their own native motion events that are more accurate, lower-CPU, and configurable in the camera's own analytics UI. This PR lets the agent consume those events.The earlier attempt at this — #194 — was set aside on @cedricve's direction:
That library work lives in kerberos-io/onvif#5 — new
event/streamsub-package. This PR depends on that one landing first so thereplacedirective inmachinery/go.modcan be removed and a tagged release pinned. Until then the directive points at a pinned commit on the fork; the official Dockerfile builds unchanged.What ships
The user-visible surface is a single new flag:
Capture.ONVIFMotion(string"true"/"false", defaults disabled)AGENT_CAPTURE_ONVIF_MOTIONcapture.onvif_motionWhen enabled, the agent opens an event stream against the camera's ONVIF endpoint and forwards
KindMotion+StateActiveevents to the existingcommunication.HandleMotionchannel — the same channel the pixel-diff detector and MQTT-motion source feed. The recorder pipeline downstream sees a familiarMotionDataPartialand triggers recording as today.Architecture
HandleONVIFEventStreamis a long-running goroutine launched inKerberos.goalongside the existingHandleONVIFActions(PTZ handler):The heavy lifting (pull-point subscription, renew loop, reconnect-with-backoff, vendor topic classification, SOAP fault extraction) lives in the
event/streamlibrary and is the same code the lib PR adds.Three concurrency concerns are explicitly handled in
events.go:stream.NewStreamfailures (camera not yet ready at agent boot, brief network blip, credentials reloaded) are recoverable. The library handles in-stream reconnect; the agent layer wraps construction in a retry loop covering the gap the library cannot see.communication.HandleMotion~3s after cancelling the goroutine's context.dispatchEventpre-checksctx.Err()and usesselect { case <-ctx.Done(); case communication.HandleMotion <- ...; default: }so a buffered event arriving during shutdown cannot panic on a closed channel.recoveringflag set on the firstErrorsarrival logsInfo "event stream recovered for <id>"on the next successful event, so on-call operators see the clear-of-condition for anERRORthey were paged on.Design rationale
Opt-in flag, default disabled
Capture.ONVIFMotiondefaults to empty/disabled. Existing deployments using the pixel-diff detector see no behavioural change. Activating ONVIF motion is a one-line compose / env change. No data migration, no config bump.Routed into
HandleMotion, not a new channelTwo reasons: (a) the recorder state machine is the same regardless of motion source — adding a parallel channel would mean duplicating downstream wiring; (b) MQTT motion already follows this convention, so ONVIF as a third source is the same shape. Trade-off acknowledged: motion-STOP signalling is not wired through
HandleMotiontoday, so this PR only triggers on motion-START (the leading edge). The recorder's existingPostRecordingtimeout decides when to stop. Motion-STOP wiring is tracked as a follow-up — it needs a recorder state-machine change beyond this PR's scope.Case-insensitive flag parsing (
isONVIFMotionEnabled)The rest of
Captureis strict-string equality, but a user settingAGENT_CAPTURE_ONVIF_MOTION=True(capital T) silently doing nothing is the kind of footgun reviewers always flag. The new flag normalises case and whitespace viastrings.EqualFold(strings.TrimSpace(v), "true"). Unit-tested across 11 inputs.DeviceID fallback chain (
resolveDeviceID)Logs and downstream metrics need a non-empty identifier. Falls back from
configuration.Nameto the camera'sONVIFXAddrto a constant"unknown". Without this an unconfiguredNameproduces empty grep targets in the operator's logs.Typed-error routing in
logStreamErrorThe library exposes
stream.ErrPullFailed,ErrRenewFailed,ErrRecreateFailed.logStreamErrormatches viaerrors.Asand logs at severity matching the failure mode: recreate is loud (camera may be offline), pull and renew are debug (library recovers automatically). Operators do not get paged for transient pulls.Default-arm drop on full
HandleMotionThe pixel-diff and MQTT sources do blocking sends. Adding a third source that drops on full preserves the existing back-pressure semantics rather than starving the stream goroutine and the renewal it depends on. A
Debugline records each drop.Verification
events_test.go: 8 test functions covering dispatch contract (motion-active sends, inactive ignored, non-motion ignored,Recording=falsegates, full-channel drops, closed-channel does not panic), case-insensitive flag, DeviceID fallback chain.go test -race ./src/onvif/...clean.go build ./...for the affected packages (onvif,config,models,components) clean. The full module build requireslibavcodec/libavutil/libswscaledev headers (capture package); not impacted by this PR.roleoroleo/onvif_simple_server: agent goroutine launches under the flag,consuming events for <name>log line appears, subscription is created and pull-loop iterates against the simulator.What's NOT in this PR
capture/main.go's recorder. Needs a new explicit-stop signal in the recorder state machine. Something to consider to add as an alternative to pre/post recording config.cloud/Cloud.goheartbeat. That code currently maintainsinputOutputDeviceMapfor digital I/O. Migrating it to astream.Streamconsumer is a separate PR — keeps this one small and reversible.Capture.ONVIFMotion. Activation is via env var orconfig.jsonfor this first release; UI exposure can follow once real-camera verification is in.Temporary go.mod note
machinery/go.modcarries areplace github.com/kerberos-io/onvif => github.com/sharedjourney/kerberos-onvif <pseudo-version>directive pinned to the commit on the lib PR's branch. This must be removed before merge — once the lib PR lands andkerberos-io/onvifships a tagged release with theevent/streampackage, drop thereplaceand bump the version inrequire. The directive is documented inline ingo.modwith the removal recipe.Migration notes for users
AGENT_CAPTURE_ONVIF_MOTION=trueand ensureAGENT_CAPTURE_IPCAMERA_ONVIF_XADDR/_USERNAME/_PASSWORDare correct. AXIS motion source is auto-detected — no topic-filter configuration needed.