From 290b89ab6f6a7f31a8b14bdaa1dc31e4192b731f Mon Sep 17 00:00:00 2001 From: Isaac Connor Date: Sun, 18 Oct 2026 18:09:32 -0400 Subject: [PATCH 1/2] fix: latch ONVIF alarms so a momentary pulse is not missed refs #5301 The ONVIF alarm was a plain atomic bool that the poll thread set and the analysis thread sampled once per frame. A momentary alarm -- a DetectedSound pulse the camera reports active then inactive between two analysis frames -- was raised and cleared on the raw level before any frame sampled it, so the recorded event had no alarm frames. A motion alarm stays active for seconds and so was always sampled, which is why MotionAlarm never showed the problem. Replace the bool with an AlarmLatch: raise() marks the level active and that an activation is waiting to be seen, clear() drops the level, and the analysis thread calls observeAlarm() once it has sampled the alarm. isAlarmed() stays true from a raise until the level is down and the activation has been observed, so a pulse is reported on at least one frame however short it was. A still-active alarm is unaffected. The latch is released only on a frame that scored, so a pulse arriving just after the isAlarmed() check is kept for the next frame rather than lost. The activation guards in WaitForMessage() now test the raw level (alarm_.active()) instead of isAlarmed(), so a new topic still raises the level while an earlier pulse's latch is still pending. Add Catch2 coverage for AlarmLatch (tests/zm_onvif_alarm_latch.cpp), testable without a gSOAP context like the other ONVIF free-function tests. Co-Authored-By: Claude Opus 4.8 --- src/zm_monitor.cpp | 10 ++- src/zm_monitor_onvif.cpp | 8 +-- src/zm_monitor_onvif.h | 42 ++++++++++- tests/CMakeLists.txt | 1 + tests/zm_onvif_alarm_latch.cpp | 123 +++++++++++++++++++++++++++++++++ 5 files changed, 173 insertions(+), 11 deletions(-) create mode 100644 tests/zm_onvif_alarm_latch.cpp diff --git a/src/zm_monitor.cpp b/src/zm_monitor.cpp index d91fb2c231..70389a2342 100644 --- a/src/zm_monitor.cpp +++ b/src/zm_monitor.cpp @@ -2172,9 +2172,13 @@ bool Monitor::Analyse() { onvif->setNotes(noteSet); noteSetMap[MOTION_CAUSE] = noteSet; cause += "ONVIF"; - // If the camera isn't going to send an event close, we need to close it here, but only after it has actually triggered an alarm. - //if (!Event_Poller_Closes_Event && state == ALARM) - //onvif->setAlarmed(false); + // This frame has recorded the alarm, so release the latch. A momentary + // alarm (a DetectedSound pulse the camera has already cleared) was held + // until a frame sampled it, instead of being missed between two frames + // (refs #5301); a still-active alarm stays alarmed for the next frame. + // Released only on a frame that scored, so a pulse arriving after the + // isAlarmed() check above is kept for the next frame rather than lost. + onvif->observeAlarm(); } // end ONVIF_Trigger #endif diff --git a/src/zm_monitor_onvif.cpp b/src/zm_monitor_onvif.cpp index 14e32ae282..b8eb26077a 100644 --- a/src/zm_monitor_onvif.cpp +++ b/src/zm_monitor_onvif.cpp @@ -154,7 +154,6 @@ bool ONVIFIsActionNotSupported(int result, const char *subcode, const char *faul ONVIF::ONVIF(Monitor *parent_) : parent(parent_) - ,alarmed_(false) ,healthy_(false) ,closes_event(false) ,soap(nullptr) @@ -725,7 +724,7 @@ void ONVIF::WaitForMessage() { // Camera reports an existing alarm we didn't know about Debug(2, "ONVIF Syncing with camera: alarm is already active for topic: %s", last_topic.c_str()); alarms[last_topic] = AlarmEntry{last_value, alarm_termination(last_topic)}; - if (!isAlarmed()) { + if (!alarm_.active()) { setAlarmed(true); Info("ONVIF Alarm already active on subscription (Initialized): %s", last_topic.c_str()); } @@ -765,7 +764,7 @@ void ONVIF::WaitForMessage() { Info("ONVIF Alarm On (Changed to active): topic=%s value=%s", last_topic.c_str(), last_value.c_str()); if (alarms.count(last_topic) == 0) { alarms[last_topic] = AlarmEntry{last_value, alarm_termination(last_topic)}; - if (!isAlarmed()) { + if (!alarm_.active()) { Info("ONVIF Triggered Start Event on topic: %s", last_topic.c_str()); setAlarmed(true); } @@ -789,7 +788,7 @@ void ONVIF::WaitForMessage() { } else { if (alarms.count(last_topic) == 0) { alarms[last_topic] = AlarmEntry{last_value, alarm_termination(last_topic)}; - if (!isAlarmed()) { + if (!alarm_.active()) { setAlarmed(true); } } else { @@ -1823,7 +1822,6 @@ void ONVIF::SetNoteSet(Event::StringSet ¬eSet) { ONVIF::ONVIF(Monitor *parent_) : parent(parent_) - ,alarmed_(false) ,healthy_(false) ,closes_event(false) { diff --git a/src/zm_monitor_onvif.h b/src/zm_monitor_onvif.h index b29bcc862a..764374993f 100644 --- a/src/zm_monitor_onvif.h +++ b/src/zm_monitor_onvif.h @@ -96,6 +96,39 @@ bool ONVIFIsActionNotSupported(int result, const char *subcode, const char *faul // Forward declaration class Monitor; +// A level alarm raised by one thread and sampled by a slower one. +// +// The ONVIF poll thread raises and clears the alarm as the camera reports a +// topic active or inactive; the analysis thread samples it once per frame. A +// momentary alarm -- a DetectedSound pulse that goes active then inactive +// between two analysis frames -- was raised and cleared on the raw level before +// any frame sampled it, so the event recorded no alarm frames (refs #5301). A +// motion alarm stays active for seconds and so was always sampled. +// +// raise() marks the level active and remembers that an activation is waiting to +// be seen. clear() drops the level. observe() is the analysis thread saying it +// has sampled the alarm. alarmed() stays true from a raise() until the level is +// down AND the activation has been observed, so a pulse is reported on at least +// one frame however short it was. observe() while the level is still up is a +// no-op on the outcome -- it stays alarmed until clear(). +class AlarmLatch { + public: + void raise() { active_.store(true, std::memory_order_release); + pending_.store(true, std::memory_order_release); } + void clear() { active_.store(false, std::memory_order_release); } + void observe() { pending_.store(false, std::memory_order_release); } + bool alarmed() const { + return active_.load(std::memory_order_acquire) || + pending_.load(std::memory_order_acquire); + } + // The raw level, ignoring an unobserved pulse: whether a topic is active now. + bool active() const { return active_.load(std::memory_order_acquire); } + + private: + std::atomic active_{false}; // a topic is currently active + std::atomic pending_{false}; // an activation not yet seen by the observer +}; + class ONVIF { friend class Monitor; @@ -103,8 +136,11 @@ class ONVIF { explicit ONVIF(Monitor *parent_); ~ONVIF(); void start(); - bool isAlarmed() const { return alarmed_.load(std::memory_order_acquire); } - void setAlarmed(bool p_alarmed) { alarmed_.store(p_alarmed, std::memory_order_release); } + bool isAlarmed() const { return alarm_.alarmed(); } + void setAlarmed(bool p_alarmed) { if (p_alarmed) alarm_.raise(); else alarm_.clear(); } + // The analysis thread has sampled isAlarmed(); release the latch so a + // momentary pulse clears once it has been seen (refs #5301). + void observeAlarm() { alarm_.observe(); } bool isHealthy() const { return healthy_.load(std::memory_order_acquire); } void setHealthy(bool p_healthy) { healthy_.store(p_healthy, std::memory_order_release); } #ifdef WITH_GSOAP @@ -115,7 +151,7 @@ class ONVIF { protected: Monitor *parent; - std::atomic alarmed_; + AlarmLatch alarm_; std::atomic healthy_; bool closes_event; std::string last_topic; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 1b082d0cbf..a2c2933cde 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -41,6 +41,7 @@ set(TEST_SOURCES zm_packet_lock.cpp zm_monitor_action.cpp zm_monitor_signal.cpp + zm_onvif_alarm_latch.cpp zm_onvif_alarm_termination.cpp zm_onvif_auth_error.cpp zm_onvif_poll_wait.cpp diff --git a/tests/zm_onvif_alarm_latch.cpp b/tests/zm_onvif_alarm_latch.cpp new file mode 100644 index 0000000000..d195beb71f --- /dev/null +++ b/tests/zm_onvif_alarm_latch.cpp @@ -0,0 +1,123 @@ +/* + * This file is part of the ZoneMinder Project. See AUTHORS file for Copyright information + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program. If not, see . + */ + +#include "zm_catch2.h" +#include "zm_monitor_onvif.h" + +// AlarmLatch is defined outside the WITH_GSOAP guard, so these run regardless +// of whether gSOAP is available. + +TEST_CASE("AlarmLatch starts clear", "[onvif]") { + AlarmLatch latch; + REQUIRE_FALSE(latch.alarmed()); + REQUIRE_FALSE(latch.active()); +} + +TEST_CASE("AlarmLatch level follows raise and clear", "[onvif]") { + AlarmLatch latch; + + latch.raise(); + REQUIRE(latch.alarmed()); + REQUIRE(latch.active()); + + latch.observe(); // a long alarm, observed while still up + REQUIRE(latch.alarmed()); + REQUIRE(latch.active()); + + latch.clear(); + REQUIRE_FALSE(latch.active()); + REQUIRE_FALSE(latch.alarmed()); +} + +TEST_CASE("AlarmLatch holds a pulse cleared before it is observed", "[onvif]") { + // The #5301 case: a DetectedSound pulse goes active then inactive on the poll + // thread between two analysis frames. Without the latch the raw level was + // already down by the time a frame sampled it and no alarm frame was recorded. + AlarmLatch latch; + + latch.raise(); + latch.clear(); // ON then OFF, no frame sampled it in between + + REQUIRE_FALSE(latch.active()); // raw level is down + REQUIRE(latch.alarmed()); // but the pulse is still reported + + latch.observe(); // the next analysis frame samples it + REQUIRE_FALSE(latch.alarmed()); // and it is released +} + +TEST_CASE("AlarmLatch reports a pulse on exactly one sample", "[onvif]") { + AlarmLatch latch; + + latch.raise(); + latch.clear(); + + REQUIRE(latch.alarmed()); // frame N sees the pulse + latch.observe(); + REQUIRE_FALSE(latch.alarmed()); // frame N+1 does not see it again +} + +TEST_CASE("AlarmLatch observe without an alarm is a no-op", "[onvif]") { + AlarmLatch latch; + latch.observe(); + REQUIRE_FALSE(latch.alarmed()); + REQUIRE_FALSE(latch.active()); +} + +TEST_CASE("AlarmLatch reports a second pulse after the first is released", "[onvif]") { + AlarmLatch latch; + + latch.raise(); + latch.clear(); + latch.observe(); + REQUIRE_FALSE(latch.alarmed()); + + latch.raise(); + latch.clear(); + REQUIRE(latch.alarmed()); // the second pulse is latched too + latch.observe(); + REQUIRE_FALSE(latch.alarmed()); +} + +TEST_CASE("AlarmLatch stays alarmed while the level is up even after observe", "[onvif]") { + // A still-active alarm (e.g. ongoing motion) must not be released by a sample; + // only clear() ends it. + AlarmLatch latch; + + latch.raise(); + latch.observe(); + latch.observe(); + REQUIRE(latch.alarmed()); + REQUIRE(latch.active()); + + latch.clear(); + REQUIRE_FALSE(latch.alarmed()); +} + +TEST_CASE("AlarmLatch re-raise before observe is still reported", "[onvif]") { + // Two pulses arrive before a frame samples: the latch is still set, and one + // observe releases it (the two momentary pulses merge into one alarm frame). + AlarmLatch latch; + + latch.raise(); + latch.clear(); + latch.raise(); + latch.clear(); + REQUIRE(latch.alarmed()); + + latch.observe(); + REQUIRE_FALSE(latch.alarmed()); +} From eff658e319855e008c198cc4d5863918dc9f10e5 Mon Sep 17 00:00:00 2001 From: Isaac Connor Date: Tue, 20 Oct 2026 08:49:01 -0400 Subject: [PATCH 2/2] fix: release the ONVIF alarm latch only when a video frame records it refs #5301 The latch was released in Analyse() at the isAlarmed() sample, before the packet was committed. Two paths then spent a momentary DetectedSound pulse without recording it: - A video packet not yet decoded makes Analyse() return at the decode gate before packet->score is set, and the same packet is retried. The sample -- and the release -- ran on every retry, so the pulse was gone by the time the packet was decoded and scored, and the event recorded no alarm frame. This is why raising AlarmFrameCount or the earlier hold did not help: the retries of one undecoded packet consumed the latch. - An audio packet (these cameras alarm on AudioAnalytics/DetectedSound, so audio packets are present) could sample and release the latch, but alarm_frames counts video frames, so the pulse was again recorded on no counted frame. Defer the release to the point where a video packet is committed with its score, so the latch is spent only on a frame the event actually writes. Also keep the topic/value of the last activation and report it from SetNoteSet() when the alarms map has already been cleared, so a momentary pulse's event note still names the cause (e.g. AudioAnalytics/Audio/ DetectedSound/true) instead of a bare "ONVIF". Co-Authored-By: Claude Opus 4.8 --- src/zm_monitor.cpp | 31 ++++++++++++++++++++++++------- src/zm_monitor_onvif.cpp | 22 +++++++++++++++++++++- src/zm_monitor_onvif.h | 5 +++++ 3 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/zm_monitor.cpp b/src/zm_monitor.cpp index 70389a2342..139eec74d2 100644 --- a/src/zm_monitor.cpp +++ b/src/zm_monitor.cpp @@ -2150,6 +2150,13 @@ bool Monitor::Analyse() { // Need to guard around event creation/deletion from Reload() std::lock_guard lck(event_mutex); int score = 0; +#ifdef WITH_GSOAP + // Whether the ONVIF alarm was sampled for this packet. The latch is only + // released once the packet is committed (packet->score set), not here, + // because Analyse() can return early at the decode gate and retry the same + // packet (refs #5301). + bool onvif_alarmed = false; +#endif // A link wake (see CheckOndemand) counted on this packet, cleared once the // packet's score has been applied. bool linked_wake = false; @@ -2172,13 +2179,14 @@ bool Monitor::Analyse() { onvif->setNotes(noteSet); noteSetMap[MOTION_CAUSE] = noteSet; cause += "ONVIF"; - // This frame has recorded the alarm, so release the latch. A momentary - // alarm (a DetectedSound pulse the camera has already cleared) was held - // until a frame sampled it, instead of being missed between two frames - // (refs #5301); a still-active alarm stays alarmed for the next frame. - // Released only on a frame that scored, so a pulse arriving after the - // isAlarmed() check above is kept for the next frame rather than lost. - onvif->observeAlarm(); + // Remember that we sampled the alarm, but do not release the latch here. + // For a video packet that is not yet decoded, Analyse() returns below at + // the decode gate before this packet's score is committed, and the same + // packet is retried. Releasing here would spend a momentary DetectedSound + // pulse on a frame that is never recorded, so the decoded retry sees no + // alarm and the event gets no alarm frame (refs #5301). Release only once + // the packet is actually scored, below. + onvif_alarmed = true; } // end ONVIF_Trigger #endif @@ -2683,6 +2691,15 @@ bool Monitor::Analyse() { } // end if signal packet->score = score; +#ifdef WITH_GSOAP + // Release the latch only once a video packet is committed with its score, + // because alarm_frames counts video frames. Releasing at the isAlarmed() + // check above would spend a momentary pulse on a packet that Analyse() + // discards and retries at the decode gate, or on an audio packet that is + // never counted as an alarm frame, leaving the event with none (refs #5301). + if (onvif and onvif_alarmed and packet->codec_type == AVMEDIA_TYPE_VIDEO) + onvif->observeAlarm(); +#endif } else { Debug(3, "trigger == off"); if (event) { diff --git a/src/zm_monitor_onvif.cpp b/src/zm_monitor_onvif.cpp index b8eb26077a..974d2995e9 100644 --- a/src/zm_monitor_onvif.cpp +++ b/src/zm_monitor_onvif.cpp @@ -724,6 +724,8 @@ void ONVIF::WaitForMessage() { // Camera reports an existing alarm we didn't know about Debug(2, "ONVIF Syncing with camera: alarm is already active for topic: %s", last_topic.c_str()); alarms[last_topic] = AlarmEntry{last_value, alarm_termination(last_topic)}; + last_alarm_topic_ = last_topic; + last_alarm_value_ = last_value; if (!alarm_.active()) { setAlarmed(true); Info("ONVIF Alarm already active on subscription (Initialized): %s", last_topic.c_str()); @@ -764,6 +766,8 @@ void ONVIF::WaitForMessage() { Info("ONVIF Alarm On (Changed to active): topic=%s value=%s", last_topic.c_str(), last_value.c_str()); if (alarms.count(last_topic) == 0) { alarms[last_topic] = AlarmEntry{last_value, alarm_termination(last_topic)}; + last_alarm_topic_ = last_topic; + last_alarm_value_ = last_value; if (!alarm_.active()) { Info("ONVIF Triggered Start Event on topic: %s", last_topic.c_str()); setAlarmed(true); @@ -771,6 +775,8 @@ void ONVIF::WaitForMessage() { } else { // Update existing alarm value and refresh termination time alarms[last_topic] = AlarmEntry{last_value, alarm_termination(last_topic)}; + last_alarm_topic_ = last_topic; + last_alarm_value_ = last_value; } } } else { @@ -788,11 +794,15 @@ void ONVIF::WaitForMessage() { } else { if (alarms.count(last_topic) == 0) { alarms[last_topic] = AlarmEntry{last_value, alarm_termination(last_topic)}; + last_alarm_topic_ = last_topic; + last_alarm_value_ = last_value; if (!alarm_.active()) { setAlarmed(true); } } else { alarms[last_topic] = AlarmEntry{last_value, alarm_termination(last_topic)}; + last_alarm_topic_ = last_topic; + last_alarm_value_ = last_value; } } } @@ -1809,7 +1819,17 @@ void ONVIF::expire_stale_alarms(const SystemTimePoint &now) { void ONVIF::SetNoteSet(Event::StringSet ¬eSet) { std::unique_lock lck(alarms_mutex); - if (alarms.empty()) return; + + if (alarms.empty()) { + // The analysis thread only calls this while isAlarmed() is true, so an empty + // map here means a momentary pulse whose topic the camera has already cleared. + // Name it from the last activation so the event note still says what fired, + // rather than a bare "ONVIF" (refs #5301). + if (!last_alarm_topic_.empty()) { + noteSet.insert(last_alarm_topic_ + "/" + last_alarm_value_); + } + return; + } for (const auto &[topic, entry] : alarms) { noteSet.insert(topic + "/" + entry.value); diff --git a/src/zm_monitor_onvif.h b/src/zm_monitor_onvif.h index 764374993f..e6b753250d 100644 --- a/src/zm_monitor_onvif.h +++ b/src/zm_monitor_onvif.h @@ -200,6 +200,11 @@ class ONVIF { SystemTimePoint termination_time; }; std::unordered_map alarms; + // The topic/value of the most recent activation, kept so SetNoteSet can still + // name the cause of a momentary pulse that has already been erased from + // `alarms` by the time the analysis thread samples it (refs #5301). + std::string last_alarm_topic_; + std::string last_alarm_value_; bool expire_alarms_enabled; int timestamp_validity_seconds; // WS-Security timestamp validity window std::mutex alarms_mutex;