The previous 30-second cap meant a 1000-camera fleet recovering from
a switch reboot would generate a sustained 33 RPS of doomed
CreatePullPointSubscription traffic against still-booting cameras,
and the synchronised retries would arrive in phase. Two changes:
* Cap raised to 5 minutes. Single-camera recovery latency goes from
'<=30s after camera comes back' to '<=300s', which is fine because
by the time we are this deep in backoff the camera has already been
unreachable through 6+ attempts (1s, 2s, 4s, 8s, 16s, 30s under the
old cap) — the marginal recovery delay is acceptable to avoid the
network melt.
* Symmetric ±25% jitter on every recreate sleep so synchronised
drops (switch reboot, DHCP storm, NTP slew) do not cause
synchronised reconnect surges. Standard practice — same shape AWS,
Cloudflare and HA event_manager use.
Tests assert the jitter range, the documented cap value (so a future
maintainer flipping it back to 30s notices in CI), and that jitter
varies across calls (proves the rand source is wired).
API-shape changes flagged as 'hard to reverse after v1' by the
architect reviewer. Acceptable to do now while no external code
imports the package; would be breaking later.
Surface tightening
------------------
* Decode unexported to decode. The Stream is the only intended caller;
exposing the helper invited future API drift. Same-package tests
still reach it.
* TopicFilter renamed to RawTopicFilter to signal that the value is
fed verbatim into the SOAP envelope and is the 'advanced escape
hatch', not the supported routing surface. Callers should normally
leave it empty and rely on Classify.
Options zero-value policy clarified
-----------------------------------
* Field godoc on every numeric option now explicitly states 'zero
means default' so the policy is local, not buried in
withDefaults().
* New DisableReconnect bool — addresses the
ReconnectAfterFailures=0-as-disable footgun the API reviewer flagged.
Reader can no longer confuse 'unset, fallback to default' with 'opt
out of reconnect'.
* BufferSize semantics extended: zero -> default (16), negative ->
unbuffered (0), positive -> explicit size. Lets callers ask for
back-pressure-only channels.
Default tuning
--------------
* MessageLimit default raised from 10 to 32. Busy AXIS cameras with
several configured inputs / analytics rules can burst beyond 10
per pull; the lower cap meant up to one PullTimeout of added
latency for the queued overflow without saving anything
meaningful. 32 covers observed bursts with no real overhead on
quiet pulls.
Caller interface
----------------
* Doc comment now states the goroutine-safety contract Stream
depends on (pull loop and renew loop call from separate
goroutines). *onvif.Device satisfies it via http.Client.
Package documentation
---------------------
* doc.go rewritten as a real godoc landing page: usage snippet,
invariants (channel close, Close idempotency, NewStream does I/O,
buffer semantics), reconnect behaviour and AfterReconnect, and a
pointer to topics.go for the classifier table. Replaces the
earlier stub that referenced unimplemented identifiers.
Replaces the bare fmt.Errorf wrappers on the Errors channel with three
typed errors and adds an Event.AfterReconnect flag so consumers can
distinguish post-recreate replay events from live ones.
Typed errors
------------
ErrPullFailed, ErrRenewFailed, ErrRecreateFailed all implement
Unwrap() and Op() Op. Consumers can branch with errors.As without
parsing strings:
var pull ErrPullFailed
if errors.As(e, &pull) { /* transient; logged */ }
var recreate ErrRecreateFailed
if errors.As(e, &recreate) { /* alert: camera may be offline */ }
Op() returns OpPull / OpRenew / OpRecreate for cases where the caller
wants to log the operation name without unwrapping. Both addressed
the review's 'highest-leverage v1 change' concern about bare error on
the Errors channel.
AfterReconnect observability
----------------------------
ONVIF cameras replay each property's current value with
PropertyInitialized whenever a new pull-point subscription is
established (per the Event Service spec). A consumer doing edge
detection on motion = StateActive would otherwise see a phantom
'motion started' for every active property after every reconnect.
The pull loop now tracks an afterReconnect flag local to the
goroutine: set to true when attemptRecreate returns justRecreated,
applied to every emitted event, cleared on the first non-Initialized
event we see. This bounds the replay window naturally — once the
camera has finished sending current state, the next event tells us
we're live.
attemptRecreate now returns (justRecreated, cont) so the pull loop
knows whether the just-completed recreate succeeded vs. the call
returning due to ctx-cancel during backoff.
Test coverage
-------------
* errors_test.go: typed-error Unwrap/Op assertions plus
Stream-level proof that pull and recreate failures arrive on the
Errors channel wearing the right type.
* AfterReconnect flag: drives the stream through a failure, observes
the next event carries the flag and the one after does not.
Addresses the five ship-blocker findings from the second review:
1. Bounded body read (review F1 / R-HIGH)
readClose now wraps resp.Body with io.LimitReader(10 MiB). A
hostile or buggy camera streaming an unbounded body cannot OOM
the agent. Legitimate PullMessages payloads are <200KB even with
dense analytics.
2. SOAP Fault detection (review F3)
unmarshalNode now scans for SOAP 1.1 faultstring and SOAP 1.2
Reason/Text BEFORE the missing-element error path. Auth failures
('not authorized'), InvalidFilterFault and expired-subscription
faults now surface their reason text instead of collapsing to
the unhelpful 'response missing PullMessagesResponse element'.
This is the difference between a debuggable error and a hidden
one when a customer's credentials change.
3. Absolute Renew TerminationTime (review F1 wire-correctness)
renewPullPoint now sends an RFC3339 UTC datetime
('2026-05-21T10:30:00Z') instead of a relative xsd:duration
('PT60S'). WS-BaseNotification §6.1.1 accepts both, but older
Hikvision, some Dahua and Bosch firmwares only accept the
absolute form — the library's own type comment even flags this
('BUG(r) Bad AbsoluteOrRelativeTimeType type').
4. Bounded Close (review P0)
Close now wraps Unsubscribe in a 5s timeout. Previously a
TCP-accepted-but-never-replying camera would wedge Close
indefinitely; now Close returns with a timeout error and the
subscription expires on its own at InitialTermination.
5. Explicit channel-close ordering after wg.Wait
The run goroutine previously relied on defer-LIFO to guarantee
renew exits before close(errors). Future maintainers extending
run() could invert that order silently. Closes are now explicit
sequential statements after wg.Wait() so the invariant is
local, not order-of-defers magic.
Also expands wsnt:UtcTime parsing in decode.go to cover the four
formats observed across vendor firmwares: RFC3339 with sub-seconds,
compact offsets ('+0200', Geovision/Dahua), and naked timestamps
without timezone (older Hikvision; per spec UTC is implied).
Caller interface gains a doc comment noting it must be safe for
concurrent use, documenting the contract Stream depends on (*onvif.
Device satisfies it via http.Client).
Tests added: SOAP 1.1 and 1.2 fault extraction, fault surfacing
through unmarshalNode, Renew absolute-datetime assertion,
Close-with-blocked-Unsubscribe returning within the timeout. -race
clean.
Adds a small command at examples/event/stream that opens a real ONVIF
event stream against a camera and prints decoded events one per line.
Intended for verifying the classifier against actual hardware (AXIS in
particular) and as runnable documentation for new consumers of the
package — point it at a configured camera, trigger motion, watch the
events arrive.
Behaviour
---------
* Required flags: -xaddr, -username, -password (matches existing
examples/event/* commands so anyone running the older subscribe /
pullmessage demos already knows the shape).
* Optional -filter passes through to Options.TopicFilter; default
empty so AXIS works out of the box.
* -duration N stops after N (default 0 = run until Ctrl-C).
* Prints kind/state/op/topic on each event plus source and data maps
when present, so multi-item ONVIF payloads (AXIS AOA
active+classType+confidence, DigitalInput InputToken+LogicalState)
are visible without re-reading PullMessages SOAP.
* Errors channel surfaced to stderr via log; the stream auto-recovers
per the reconnect logic in stream.go so transient errors do not
terminate the demo.
Not part of any CI; not a production tool — this is a verification
harness.
gofmt -w pass on reconnect_test.go. The Options struct field names had
mismatched alignment; reformatted to match gofmt canonical layout. No
behaviour change.
Adds automatic CreatePullPointSubscription recreation when the pull
loop hits ReconnectAfterFailures (default 3) consecutive errors.
Mirrors what production ONVIF clients (Home Assistant event_manager,
Milestone integration) do because pull points die for many reasons
none of which surface as a clean SOAP fault: camera reboot, NAT
session timeout, subscription garbage-collected after a renew miss,
firmware bug. Recreating is the only reliable recovery; Renew alone
cannot save an already-dropped subscription.
Two new options
---------------
* ReconnectAfterFailures int (default 3) — how many consecutive pull
failures trigger recreate. Conservative default; tunable for
always-on cameras vs flaky NAT.
* RetryBackoff time.Duration (default 1s) — base sleep between pull
retries; recreate failures double this up to a 30s cap so a
permanently broken camera does not hammer the network.
Lifecycle changes
-----------------
* Stream.pullPoint is now mutex-protected — the renew goroutine reads
it concurrently with the pull loop installing a new address after
recreate. getPullPoint/setPullPoint accessors keep the locking
contained.
* On successful recreate, failure count and backoff reset to defaults
so the loop is back to its happy-path cadence.
* On recreate failure, the loop continues retrying (until ctx cancel)
with exponentially increasing sleep — never blocks Close.
Tests cover: post-failure recreate hits a different SubscriptionRef
Address and subsequent events come from the new endpoint; exponential
backoff drives multiple recreate attempts when the camera stays down;
defaults match production-sensible 3 failures / 1s backoff. -race
clean.
Adds a background renew loop alongside the pull loop. ONVIF pull-point
subscriptions expire at the InitialTerminationTime supplied to Create;
without periodic Renew calls the camera silently drops the
subscription and subsequent pulls start returning empty messages — the
shape the existing agent's heartbeat code in cloud/Cloud.go has been
papering over by occasionally recreating subscriptions.
Design
------
* New Options.RenewMargin (default 10s) — how far before
InitialTermination expiry the renew fires. Smaller margins mean
fewer SOAP round-trips; larger margins tolerate slow networks. With
default 60s termination + 10s margin we renew every 50s, which is in
line with what production NVRs (Milestone, Genetec) use.
* The renew loop runs in a separate goroutine sharing ctx with the
pull loop. WaitGroup synchronisation in run() ensures both have
exited before close()-of-channels happens, so a renew in flight
during Close() cannot send on a closed Errors channel.
* Pathological config (RenewMargin >= InitialTermination) falls back
to renewing at termination/2 rather than busy-looping or never
renewing.
* renewPullPoint sends a wsnt:Renew SOAP against the SubscriptionRef
Address with the same InitialTermination duration; renew errors
surface on Errors non-blockingly, identically to pull errors.
Tests use very short termination/margin (80-100ms / 10ms) so a single
test run observes multiple renews within ~500ms, and assert that
renew calls target the SubscriptionReference endpoint (not the device
endpoint). -race clean.
Introduces Stream, the typed event consumer the package will eventually
present to callers, plus the caller seam needed to test it without
hitting a real camera.
Stream owns one ONVIF pull-point subscription end-to-end:
* CreatePullPointSubscription on construction so authentication and
reachability problems surface synchronously from NewStream rather
than landing on the Errors channel after the goroutine starts.
* Background pull loop calls PullMessages against the
SubscriptionReference Address returned by Create. Each
NotificationMessage is fed through Decode and pushed on the Events
channel, with context cancellation honoured between every step so a
Close cannot get stuck behind a long-server-side-wait pull.
* Errors during a pull are surfaced on a separate Errors channel using
a non-blocking send; a stalled consumer drops older errors instead
of blocking the loop. The loop sleeps briefly (ctx-aware) and
retries — automatic subscription recreation lands in the
reconnect-on-error commit.
* Close cancels the context, waits for the run goroutine to exit,
Unsubscribes the pull point and closes Events/Errors. sync.Once
keeps it idempotent.
Design seams
------------
* caller interface (CallMethod + SendSoap) abstracts *onvif.Device so
tests can substitute fakeCaller without an HTTP server. deviceCaller
is the production adapter; newStream takes the interface, NewStream
takes the concrete *onvif.Device. The same shape lets a future commit
add WithClassifier / WithClock / WithCaller options if the architect
reviewer's pluggable-classifier note becomes urgent.
* now func() time.Time is a Stream field so a future clock-injecting
test (renew timing, observed-at determinism) can swap it.
* unmarshalNode keys on the local XML name, sidestepping namespace
matching since SOAP envelopes from different vendors prefix the
PullMessagesResponse and CreatePullPointSubscriptionResponse with
arbitrary tev:/tev1:/... bindings. This is the same trick the agent's
getXMLNode used; lifting it here lets the agent eventually drop its
copy.
Options and defaults
--------------------
PullTimeout 5s, MessageLimit 10, InitialTermination 60s, BufferSize 16
match what the existing agent code uses. TopicFilter defaults to empty
so AXIS cameras work out of the box — the verified topic table is
intentionally the routing layer, not a server-side filter, because the
agent will frequently want digital I/O and motion on the same stream.
Tests cover the create-then-pull-then-close happy path, that pulls
target the SubscriptionReference Address (not the device endpoint),
construction failure on CreatePullPoint error, context-cancel exits
the loop cleanly with channels closed, transient pull errors land on
Errors without stopping decode of subsequent good messages, idempotent
Close, and Options default values. -race clean.
Adds the Decode entry point that converts the ONVIF wire form into the
package's typed Event. The agent (and any other consumer) no longer has
to walk NotificationMessage.Message.Message.Data.SimpleItem chains and
hand-special-case per-vendor data item names.
Decoding rules
--------------
* Topic -> Kind via the verified Classify table.
* PropertyOperation parses Initialized/Changed/Deleted; absent or
unrecognised -> PropertyUnknown (the attribute is optional per
WS-Notification).
* UtcTime parses RFC3339Nano first, RFC3339 second, normalised to UTC.
Absent or unparseable -> DeviceTime is zero. Camera clocks drift; the
type doc already steers callers to prefer Timestamp.
* State extraction scans Data items in order for the first boolean-like
value (true/false/1/0/active/inactive, case-insensitive). This handles
every vendor data item in the verified table — IsMotion, State,
IsTamper, LogicalState, active, Motion, triggered, SoundDetection,
TamperingDetection — without a per-kind switch.
* Edge-triggered topics (LineDetector/Crossed with only ObjectId) yield
StateUnknown, matching the topic-rule doc note.
* Source and Data are full ONVIF SimpleItem name->value maps so callers
retain multi-item info (AXIS AOA active+classType+confidence, digital
I/O InputToken+LogicalState, analytics VideoSourceConfigurationToken+
Rule). Empty notifications yield nil maps, matching the Event
zero-value contract from types_test.go.
* Topic, Source and Data are always populated even when Kind is
KindUnknown, so consumers can log/route unclassified events.
Tests cover the AXIS motion happy path, the inactive case, the Hanwha
numeric-string variant, the Avigilon 'active' literal, multi-item AOA
decode, the LineDetector edge-trigger semantic, unknown-topic wire
preservation, every PropertyOperation literal, RFC3339 with sub-second
and timezone offsets, and case-insensitive State extraction.
Parallel expert review of the four-commit scaffold surfaced 13 actionable
items split across API design, ONVIF domain accuracy, Go idiomaticity and
test rigor. This change addresses them before the Stream type lands, when
the public surface is still cheap to move.
API shape (hard-to-reverse before tagging)
------------------------------------------
* Rename EventKind -> Kind and EventState -> State to avoid the
stream.EventKind / stream.EventState stutter when imported.
* Restructure Event for non-lossy decode:
- Source string and RawValue string replaced with Source/Data maps so
multi-item ONVIF Source and Data lists (e.g. AXIS AOA emitting
active+classType+confidence; DigitalInput carrying InputToken+
LogicalState) are preserved.
- Add DeviceID so a single channel can fan in events from multiple
cameras.
- Add DeviceTime parsed from wsnt:UtcTime alongside the local
observation Timestamp. The earlier doc-comment decision to bake-in
'drop UtcTime' was a policy disguised as an API; expose both and let
callers choose.
Classifier accuracy (ONVIF domain audit)
----------------------------------------
* Introduce KindImageQuality for tns1:VideoSource/ImageTooDark|Bright|
Blurry. These are imaging-quality alarms that integrators route
separately because they fire on sunset/dawn/condensation, not tamper.
Previously mis-classified as KindTampering.
* Add tns1:VideoSource/GlobalSceneChange -> KindTampering, which is the
real lens-cover signal on firmwares without TamperDetector.
* Anchor the TamperDetector rule to 'TamperDetector/Tamper' so a
hypothetical 'TamperDetectorLog' path cannot match.
* Narrow MyRuleDetector from container-match to an explicit whitelist
(HumanDetect, VehicleDetect, PeopleDetect, ObjectsInside, FaceDetect).
Bosch publishes Counter and Occupancy under MyRuleDetector too; those
must not classify as ObjectDetected.
* Add the AXIS Guard suite (MotionGuard, FenceGuard, LoiteringGuard) ->
KindMotion. Common on AXIS deployments configured with these apps
instead of basic VMD.
* Drop the bogus Device1ScenarioANY test fixture; AOA uses numeric
scenarios (Device1Scenario1, Device1Scenario2). The 'ANY' suffix was a
borrow from the older Guard suite's Camera1ProfileANY pattern.
* Document the edge-trigger semantics of LineDetector/Crossed in the rule
comment so decoder consumers do not expect a State boolean.
Tests
-----
* String tests now use t.Run subtests so failures name the case.
* TestKindStringsAreUnique guards against accidental String() aliasing
when adding new kinds.
* TestEventFieldAssignmentRoundTrip exercises the new field set
including DeviceID, Source/Data maps and DeviceTime.
* Canonicalisation table now covers: double slash, colon-only segment,
trailing colon, multi-colon-in-segment, leading/trailing slash,
no-colon passthrough. Locks the actual behaviour so future refactors
see regressions.
* False-positive negatives: Counter and Occupancy under MyRuleDetector,
AudioEncoderConfiguration, RelayFailure, DigitalInputConfiguration,
TamperDetectorLog, MotionRecording/Started — all assert KindUnknown.
* TestClassifyRuleOrder_ObjectAnalyticsBeforeGenericObjects pins the
ordering invariant called out by the architect reviewer.
Documentation
-------------
* doc.go trimmed so it does not advertise NewStream / Events / Errors /
Close before those identifiers exist — the godoc reader will no longer
see dead names. Re-expanded when the Stream type lands.
Deferred to the Stream commit
-----------------------------
* PropertyUnknown vs PropertyUnset disambiguation — kept as
PropertyUnknown for now with a clarified doc comment; revisit when the
decoder needs to distinguish 'absent on wire' from 'unparseable'.
* Classifier pluggability (WithClassifier option) — meaningful only once
there is a Stream; revisit at that commit.
The rest of github.com/kerberos-io/onvif uses stretchr/testify (assert,
require) consistently — Device_test.go, event/type_test.go,
media2/types_test.go, ws-discovery/networking_test.go. Migrate the two
new test files in event/stream from stdlib t.Errorf to the same testify
convention so the package fits in without local style variation.
No production-code change; no behaviour change.
Cross-checked each topic string against public sources before adding the
rule, and inlined the citation next to the rule it supports so future
maintainers can audit the table:
* Hikvision motion: CellMotionDetector/Motion (Hikvision PDF on third
party motion troubleshooting) plus the VideoSource/MotionAlarm
fallback emitted by newer firmware.
* Hikvision tamper-class scene change: VideoSource/ImageTooDark|Bright|
Blurry — present in the ONVIF topic namespace; treated as Tampering
for routing.
* Bosch motion: VideoAnalytics/MotionAlarm (Bosch metadata/IVA PDF) —
NOT VideoSource/MotionAlarm. The earlier 'tnsbosch:MotionAlarm' guess
in PR #194 was wrong; Bosch uses standard tns1 namespace under
VideoAnalytics.
* Hanwha (Samsung Wisenet): VideoAnalytics/tnssamsung:MotionDetection,
VideoAnalytics/tnssamsung:TamperingDetection,
AudioAnalytics/tnssamsung:SoundDetection — confirmed via HA #66493
capture.
* Avigilon: per-segment-namespaced serialisation
(tns1:Device/tns1:Trigger/tns1:Relay) folded by canonicalization.
Documented in Avigilon's own ONVIF subscription guide.
* Object analytics: LineDetector/Crossed, FieldDetector/ObjectsInside
and the MyRuleDetector container for vendor rule names (Bosch IVA,
Dahua SMD) — sourced from ONVIF Analytics Service Spec v22.06.
* AXIS Object Analytics: prefix match on ObjectAnalytics/ to absorb
the dynamic Device1Scenario<N> suffixes (AXIS counting-data docs).
Empirical topic table cross-checked with openvideolibs/onvif-parsers
(Apache-2.0), the package the Home Assistant ONVIF integration imports —
referenced from the package doc-comment.
Test cases now cover the verified topic for every supported vendor plus
case-sensitivity and canonicalization. No code change for callers: the
public API is still just Classify(topic) -> EventKind.
Adds a Classify function that maps ONVIF topic strings to EventKind so the
agent does not need to know AXIS vs Hikvision vs Bosch topic conventions.
The classifier canonicalizes topics by stripping XML-namespace prefixes from
each path segment, which collapses vendor variants like
'tns1:Device/tnssamsung:DigitalInput' and 'tns1:Device/Trigger/DigitalInput'
to a single matchable form.
Motion coverage on day one:
* tns1:VideoSource/MotionAlarm (AXIS, Bosch, Dahua, ...)
* tns1:VideoAnalytics/<vendor>:MotionAlarm
* tns1:RuleEngine/CellMotionDetector/Motion (ONVIF standard, Hikvision)
* tns1:RuleEngine/MotionRegionDetector/Motion (AXIS region rule)
* tnsaxis:CameraApplicationPlatform/ObjectAnalytics/...
Also covers Tamper, DigitalInput, Relay (DigitalOutput), object analytics
and audio alarms, so the same Stream can replace the agent's ad-hoc digital
I/O polling without losing coverage.
Introduces a new event/stream sub-package that will host the long-running,
channel-based consumer for ONVIF device events. This commit only lays down
the value types — EventKind, EventState, PropertyOperation and the Event
struct — together with Stringer methods and zero-value tests.
The intent is to give callers a vendor-neutral surface (Motion, DigitalInput,
etc.) so they do not need to special-case AXIS, Hikvision, Avigilon, Hanwha,
Bosch or Dahua topic strings. Decoding, topic classification and the Stream
type itself land in follow-up commits.
