fix: ONVIF events record 0 alarm frames for momentary alarms (DetectedSound) refs #5301

src/zm_monitor.cpp

@@ -2150,6 +2150,13 @@ bool Monitor::Analyse() {
     // Need to guard around event creation/deletion from Reload()
     std::lock_guard<std::mutex> 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,9 +2179,14 @@ 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);
+        // 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
 
@@ -2679,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) {

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,9 @@ 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()) {
+              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());
               }
@@ -765,13 +766,17 @@ 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()) {
+                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);
                 }
               } 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 {
@@ -789,11 +794,15 @@ void ONVIF::WaitForMessage() {
             } else {
               if (alarms.count(last_topic) == 0) {
                 alarms[last_topic] = AlarmEntry{last_value, alarm_termination(last_topic)};
-                if (!isAlarmed()) {
+                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;
               }
             }
           }
@@ -1810,7 +1819,17 @@ void ONVIF::expire_stale_alarms(const SystemTimePoint &now) {
 
 void ONVIF::SetNoteSet(Event::StringSet &noteSet) {
   std::unique_lock<std::mutex> 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);
@@ -1823,7 +1842,6 @@ void ONVIF::SetNoteSet(Event::StringSet &noteSet) {
 
 ONVIF::ONVIF(Monitor *parent_) :
   parent(parent_)
-  ,alarmed_(false)
   ,healthy_(false)
   ,closes_event(false)
 {

src/zm_monitor_onvif.h

@@ -96,15 +96,51 @@ 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<bool> active_{false};   // a topic is currently active
+  std::atomic<bool> pending_{false};  // an activation not yet seen by the observer
+};
+
 class ONVIF {
   friend class Monitor;
 
  public:
   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<bool> alarmed_;
+  AlarmLatch alarm_;
   std::atomic<bool> healthy_;
   bool closes_event;
   std::string last_topic;
@@ -164,6 +200,11 @@ class ONVIF {
     SystemTimePoint termination_time;
   };
   std::unordered_map<std::string, AlarmEntry> 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;

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

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 <http://www.gnu.org/licenses/>.
+ */
+
+#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());
+}