fix: hold a momentary ONVIF alarm past the close-on-alarm event split refs #5301

src/zm_monitor.cpp

@@ -1229,6 +1229,7 @@ bool Monitor::connect() {
         onvif = nullptr;
       }
       onvif = new ONVIF(this);
+      onvif->setMinimumAlarmFrames(alarm_frame_count);
       onvif->start();
     }  // End ONVIF Setup
 
@@ -2172,9 +2173,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
 

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 &noteSet) {
 
 ONVIF::ONVIF(Monitor *parent_) :
   parent(parent_)
-  ,alarmed_(false)
   ,healthy_(false)
   ,closes_event(false)
 {

src/zm_monitor_onvif.h

@@ -96,15 +96,81 @@ 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 arms a hold of minimum_hold_ observations.
+// clear() drops the level. observe() is the analysis thread saying it has
+// sampled the alarm, which spends one unit of the hold. alarmed() stays true
+// from a raise() until the level is down AND the hold is spent, so a pulse is
+// reported on at least minimum_hold_ frames however short it was. observe()
+// while the level is still up does not end the alarm -- it stays alarmed until
+// clear().
+//
+// minimum_hold_ defaults to one frame. Holding for more (set_minimum_hold)
+// keeps a momentary pulse alarmed across an event split: with close-on-alarm
+// and continuous recording the running event is closed and a new one opened
+// when the alarm begins, and a one-frame pulse otherwise leaves all its scored
+// frames in the event that is closing, so the new event records none (refs
+// #5301).
+class AlarmLatch {
+ public:
+  // Report a momentary pulse for at least this many observe() calls. Clamped to
+  // a minimum of one so a raise is always seen on at least one frame.
+  void set_minimum_hold(unsigned int frames) {
+    minimum_hold_.store(frames < 1 ? 1 : frames, std::memory_order_release);
+  }
+  void raise() {
+    active_.store(true, std::memory_order_release);
+    hold_.store(minimum_hold_.load(std::memory_order_acquire), std::memory_order_release);
+  }
+  void clear() { active_.store(false, std::memory_order_release); }
+  void observe() {
+    unsigned int remaining = hold_.load(std::memory_order_acquire);
+    while (remaining > 0 &&
+           !hold_.compare_exchange_weak(remaining, remaining - 1,
+                                        std::memory_order_acq_rel, std::memory_order_acquire)) {
+    }
+  }
+  bool alarmed() const {
+    return active_.load(std::memory_order_acquire) ||
+           hold_.load(std::memory_order_acquire) > 0;
+  }
+  // The raw level, ignoring an unspent hold: 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<unsigned int> hold_{0};        // observe() calls still owed to a pulse
+  std::atomic<unsigned int> minimum_hold_{1};  // frames a pulse is held for
+};
+
 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(); spend one unit of the hold so a
+  // momentary pulse clears once it has been reported for long enough (refs #5301).
+  void observeAlarm() { alarm_.observe(); }
+  // Hold a momentary alarm for one frame beyond alarm_frame_count observations.
+  // Reaching ALARM takes alarm_frame_count scored frames, which is where a
+  // continuous event is split on close-on-alarm; holding one frame longer leaves
+  // at least one scored frame for the new event rather than all of them in the
+  // event that is closing (refs #5301).
+  void setMinimumAlarmFrames(int alarm_frame_count) {
+    alarm_.set_minimum_hold(alarm_frame_count > 0 ? alarm_frame_count + 1 : 2);
+  }
   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 +181,7 @@ class ONVIF {
 
  protected:
   Monitor *parent;
-  std::atomic<bool> alarmed_;
+  AlarmLatch alarm_;
   std::atomic<bool> healthy_;
   bool closes_event;
   std::string last_topic;

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,184 @@
+/*
+ * 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());
+}
+
+TEST_CASE("AlarmLatch minimum hold reports a pulse across several samples", "[onvif]") {
+  // With a hold of 3, a momentary pulse stays alarmed for three samples, so its
+  // scored frames survive the close-on-alarm event split (refs #5301).
+  AlarmLatch latch;
+  latch.set_minimum_hold(3);
+
+  latch.raise();
+  latch.clear();  // ON then OFF before any frame sampled it
+
+  for (int i = 0; i < 3; i++) {
+    REQUIRE(latch.alarmed());  // frames N..N+2 each see the pulse
+    latch.observe();
+  }
+  REQUIRE_FALSE(latch.alarmed());  // frame N+3 does not
+}
+
+TEST_CASE("AlarmLatch minimum hold: active level outlasts a spent hold", "[onvif]") {
+  // An ongoing alarm sampled more times than the hold stays alarmed on the raw
+  // level; only clear() ends it.
+  AlarmLatch latch;
+  latch.set_minimum_hold(2);
+
+  latch.raise();
+  latch.observe();
+  latch.observe();
+  latch.observe();  // hold spent, but the topic is still active
+  REQUIRE(latch.alarmed());
+  REQUIRE(latch.active());
+
+  latch.clear();
+  REQUIRE_FALSE(latch.alarmed());
+}
+
+TEST_CASE("AlarmLatch minimum hold: re-raise restores the full hold", "[onvif]") {
+  AlarmLatch latch;
+  latch.set_minimum_hold(3);
+
+  latch.raise();
+  latch.clear();
+  latch.observe();  // spend one unit of the first pulse's hold
+
+  latch.raise();    // a new pulse restores the full hold
+  latch.clear();
+  for (int i = 0; i < 3; i++) {
+    REQUIRE(latch.alarmed());
+    latch.observe();
+  }
+  REQUIRE_FALSE(latch.alarmed());
+}
+
+TEST_CASE("AlarmLatch minimum hold of zero is clamped to one", "[onvif]") {
+  AlarmLatch latch;
+  latch.set_minimum_hold(0);
+
+  latch.raise();
+  latch.clear();
+  REQUIRE(latch.alarmed());  // still reported on at least one frame
+  latch.observe();
+  REQUIRE_FALSE(latch.alarmed());
+}