Holding a DatabaseLogPause on the dbQueue thread for its whole life kept the
errors of every queued write out of Logs too: Monitor_Status updates, Events,
Frames and Stats rows. Under lock contention most "Giving up on query" errors
were only in the log files.
Mark each queued row with whether the logger queued it, and pause database
logging only around those inserts into Logs and the backlog warning. The loop
stays closed: a line about a queued write queues one Logs insert, which logs
nothing to the database.
Test: an error from a queued write reaches Logs.
From review by @nabbi on #5208.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
zmDbDo() kept its calling thread out of the database for the whole query,
which stopped its own "Success running sql query" line from queueing another
Logs insert. But the loop only starts where the Logs rows are inserted, on
the dbQueue thread, and pausing every zmDbDo() also kept the calling thread's
errors about its query out of Logs: "Can't run query" and the lock contention
retries were only in the log files.
Hold one Logger::DatabaseLogPause on the dbQueue thread for its whole life,
in zmDbQueue::process(), and none in zmDbDo(). Lines the queue thread logs,
the backlog warning included, still stay out of the database; every other
thread's query lines reach it, errors and, at a debug database level, the
success lines too.
Tests: the queue thread's inserts into Logs log nothing and the row count
settles, a query on another thread logs its success line at debug, and an
error from a failed query reaches Logs. The last two fail with the pause in
zmDbDo().
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: validate alarm packets in zmalarm-server.py
The alarm server accepted any packet from any host and mapped it to a
monitor by the Address field inside the packet, so a host on the network
could raise an alarm or a zmtrigger event on any configured camera.
Every malformed packet (short header, non-JSON body, unknown address)
raised an uncaught exception that ended the process, and a client that
connected without sending blocked the accept loop indefinitely.
- only accept an alarm whose Address matches the address it came from
and a monitor that uses that address
- read the header and body with a 5 s timeout, cap the body at 64 KiB,
log and drop bad packets instead of exiting
- resolve camera hostnames in monitor Paths and skip monitors without a
camera address instead of failing at startup
- pass the alarm description to the zmtrigger thread instead of reading
a global, and talk to zmtrigger over a plain socket (telnetlib was
removed in Python 3.13)
- honour the bare port number zmpkg.pl passes from ZM_OPT_ALS_PORT and
add --bind= to choose the listening address (default unchanged)
- move the pyzm imports into main() so the parser can be tested; add
tests/python/test_zmalarm_server.py
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: accept alarms from cameras behind NAT in zmalarm-server.py
The source check required the packet's Address field to equal the address
the packet came from. A camera behind NAT reports its LAN address there
while its packets arrive from the NAT address, so its alarms were dropped.
The Address field is no credential, so the monitor is now chosen by the
sender: an alarm is accepted only from an address that belongs to a
monitor, the camera address in its Path or one listed for it under
[alarmserver] in secrets.ini, and it goes to one of that address's
monitors. The Address field only picks among several monitors on one
address, for cameras behind a shared NAT address. A host that belongs to no
monitor still cannot raise an alarm, and a camera cannot raise one on
another camera's monitor.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: cancel the forced alarms zmalarm-server.py raised when it stops
An alarm packet armed the monitor, slept 60 s in a thread and disarmed it.
When zmdc stopped or restarted the server inside that window (SIGTERM, which
Python's default handler ends at once) the disarm never ran and the monitor
stayed in forced alarm, recording, until someone cancelled it by hand.
Keep the forced alarms the server raised in one place, one per monitor: a
new alarm for a monitor in forced alarm arms it again, as every packet did,
and moves its end instead of starting a second timer that would cut it
short. On the way out, SIGTERM included, the server cancels every alarm
still running and exits 0, so zmdc no longer logs the stop as abnormal. An
arm that fails is logged instead of ending the server.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
After saving a zone, actions/zone.php redirected to the REFERER the zone form
posted, which the form fills from the Referer its page was loaded with. A
zone editor who reached the zone page through a link on another site was sent
to that site after saving: an open redirect, usable to lead someone from a
trusted ZoneMinder page to a look-alike login. Forging the POST itself is
blocked by the CSRF token, so it needs an editor who followed such a link.
localRedirectUrl() returns the target only if it is a query-only URL or an
absolute http(s) URL on the host and port of the current request, and the
fallback otherwise: another host, //host, backslashes, user@host, other
schemes and control characters all give the fallback. The zone save falls
back to the zones list, as it already does with no Referer (#5207).
tests/php/test_local_redirect_url.php covers 21 cases.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* test: cover lines logged to the database while a query runs
Live-database tests ([notCI][dblive]) for how the logger and zmDbDo
interact:
- a line logged on one thread while another thread is inside zmDbDo
reaches the Logs table
- zmDbDo's own debug lines stay out of Logs when the database level is
debug, so inserting a Logs row cannot log another one without end
- a dbQueue backlog over 40 rows does not deadlock threads that log
The first fails on master: zmDbDo sets the process-wide database log
level to NOLOG for the whole query, so the line is dropped.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: stop dropping other threads' database log lines during queries
zmDbDo lowered the logger's database level to NOLOG for as long as its
query ran, so that its own lines (and, at a debug database level, the line
about every insert into Logs) could not queue more inserts into Logs. The
level is shared by the whole process, so every line any other thread
logged in that window never reached the Logs table or the web log view,
while the log file and syslog still had it. The save and restore of that
shared level from several threads is also a data race: a thread can
restore a stale NOLOG and leave database logging off.
Replace the toggle with Logger::DatabaseLogPause, a thread_local count
that keeps only the current thread's lines out of the database. zmDbDo
and the queue's backlog warning use it; mDatabaseLevel is no longer
written outside of initialise().
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: warn about the database queue backlog outside the queue's lock
zmDbQueue::process logged its "db queue size has grown larger" warning
while holding the queue mutex. logPrint holds the logger's mutex while it
pushes a Logs row, which takes the queue mutex. With more than 40 rows
waiting, a thread logging to the database and the queue thread could each
hold one of the two locks and wait for the other, stopping every thread
that logs.
Take the backlog size under the lock and warn after releasing it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Gapless playback loads a new event page for each event, and each load moved
the video three times and ended in a page-wide wipe:
- video.js sets the player up on DOMContentLoaded without a size, while the
first changeScale() ran from initPage() on window load. The browser could
paint a collapsed video with the controls drawn up under it, and
changeScale() measured that collapsed layout (an empty inline-block still
leaves a line box), which made the video too small by about 17px.
- skin.js's ResizeObserver on the navbar statistics called endOfResize() for
the size it reports when it starts observing, and for every width change of
the figures, so changeScale() ran again 250ms later and corrected the size.
- With the stats panel turned off, initPage() hid it and widened the video's
column only after load.
- The page-leave roll-up in skin.js animated the content to nothing on every
reload, a wipe from the bottom up between events.
Size the video on DOMContentLoaded, after video.js has the player, and
remember the size per monitor (zmEventVideoSize<Id>) so event.php opens the
next page at it; changeScale() still corrects it if the window or layout
changed. Render the stats panel and the video's column width from the
zmEventStats cookie in event.php. Rescale for the navbar only when its height
changes. Move between events through goToEvent(), which sets
skipPageLeaveAnimation so the roll-up is skipped for that reload.
Measured in Chrome at 1920x915 on a 1280x720 event, each frame's video size
and controls position: before, every page went 1x0, 1144x644, 1174x660 with
the controls starting near the top of the video, and the content shrank from
851px to 363px before the reload. After, the page reached by gapless playback
paints at 1174x660 with the controls in place and does not move, and there is
no roll-up. The first event of a session still sizes once, as no size has
been remembered yet. Window resize and the stats button still rescale, for
both video.js and zms events.
tests/js/event-gapless-layout.test.js covers goToEvent() and the
DOMContentLoaded sizing; two of its three tests fail without this change.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
requestField() read Model[field] whenever the request's Model entry was an
array, empty or not. CakePHP's Model::set() treats the bare top-level fields
as the model's data when that entry is empty (_setAliasData), so
{"Monitor":[],"EventEndCommand":"x"} passed the System-edit check on the
monitor command fields, which found no value, while save() stored x. A
Monitors=Edit user without System=Edit could set EventStartCommand or
EventEndCommand, which zmc runs through /bin/sh.
Every other check built on requestField() had the same gap: the target
MonitorId when adding or editing an event, the EventId when adding a frame,
and the ParentId when editing a group.
Read Model[field] only when the Model entry is non-empty, as set() does, and
otherwise the bare field; a non-empty entry that is not an array yields
nothing, since set() skips it.
tests/php/test_api_request_field.php runs the real method against each
request shape.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ffmpeg reaches the same network cameras libvlc did, so carrying a second
streaming library -- with its own threading, its own bugs and its own
build dependency -- buys nothing. The preceding migration moves existing
monitors to Ffmpeg first, so Monitor::Load cannot meet a type it no
longer recognises, which is Fatal and would stop zmc rather than skip
the monitor.
Drops LibvlcCamera, the LIBVLC camera type and its parse and
construction arms, LIBVLC_SRC and Camera::IsLibvlc, the ZM_NO_LIBVLC
option and the libvlc probe, and the HAVE_LIBVLC and HAVE_VLC_VLC_H
defines. Since the camera resolved libvlc through dlopen rather than
linking it, this removes a headers-only build dependency and a runtime
one, and nothing else needed relinking.
kOptionSeparators stays -- the Ffmpeg path still parses the Options
field with it -- but it is no longer a contract shared between two
readers, so its comment and the test that pins its behaviour are
reworded rather than removed.
Built clean from a fresh cmake configure, which no longer lists libVLC
among optional libraries; unit suite passes (147 cases, 12531
assertions); no libvlc strings remain in zmc.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Y6FieTwEXuLhhR4e2yiax
Groundwork for moving decoding out of the camera and into the decoder thread.
The parsing of DecoderHWAccelName sat inline in FfmpegCamera::OpenFfmpeg(),
where nothing could test it, and the ai_server branch has a second copy of the
same logic in its Monitor-side decoder open. Moving the decoder without pulling
this out first would carry the duplication across rather than end it.
No behaviour change: same iteration over the types this build offers, same
comma-separated priority list, same warn-and-skip for a name libavutil does not
know, same order.
Being a free function it can now be tested, which it could not be before. The
cases covered are the ones an operator's setting actually runs into -- a single
name, a priority list, spaces around the commas, a typo among good entries,
empty entries -- asserted against whatever device types the host reports rather
than against a fixed list, so the test travels. This host offers seven, so no
section skipped for want of data.
Confirmed non-vacuous by mutation: dropping TrimSpaces fails one case, and
accepting an unknown name instead of skipping it fails two.
Build clean with no warnings; suite 147 cases / 12531 assertions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Y6FieTwEXuLhhR4e2yiax
With ZM_LOG_DEBUG off, Logger::initialise() clamped every target level
(file, database, syslog, terminal) to INFO as well as the overall level.
SIGUSR1 raises the overall level, but the effective level is the lower of
that and the highest target level, so it stayed at INFO and the signal
did nothing.
Only cap the overall level. Targets keep their configured levels, so
SIGUSR1 behaves as if ZM_LOG_DEBUG_LEVEL had been incremented: targets
configured for debug start receiving it, targets at INFO stay at INFO.
Startup behaviour is unchanged since the effective level is still INFO.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
(cherry picked from commit af970628c3fe5fe76d163a30747cf534babbb8a6)
RemoteCameraHttp read the camera's Content-Length with atoi() and then asked
ReadData() for that many bytes, which passes the count straight to
Buffer::read_into(). Buffer::expand() worked in int: once the allocation passed
INT_MAX the spare space went negative, it grew again, and mAllocation + count
wrapped to a small value, so the following memcpy or read() wrote the camera's
data past a small heap block. Whoever answers the camera's HTTP request, often
on a plain-HTTP link, could crash zmc in a loop or corrupt its heap.
- RemoteCameraHttp::ParseContentLength() replaces the four atoi() calls. It
refuses a length that is not a number or is larger than twice the monitor's
raw frame size plus one network buffer (capped at 1 GiB), and GetResponse()
fails the response instead of buffering it.
- Buffer::expand() does its arithmetic in size_t, grows to exactly what is
needed, and refuses to hold more than INT_MAX bytes, since read_into()
returns the count as an int.
- Buffer::read_into() refuses a read that does not fit after expand() rather
than reading past the allocation.
tests/zm_buffer.cpp covers reads through a pipe, an oversized read, data kept
across a shift and a grow, and the Content-Length bounds.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FOSCAMR2C presetGoto and presetSet interpolated the preset into
"SELECT Label FROM ControlPresets WHERE Preset = $preset" and sent the selected
Label to the camera as name=. For presetGoto the preset came straight from the
web request, so any user who could see the monitor could run a SELECT against
the ZoneMinder database and have the value (the auth hash secret, password
hashes) delivered to the camera link, or use it as a boolean oracle.
- FOSCAMR2C: bind the preset in every statement, refuse a preset that is not a
number, and look ControlPresets up by MonitorId as Dahua and PSIA do.
- buildControlCommand(): refuse a non-numeric presetGoto preset, and a control
name that is not a plain method name. zmcontrol calls the name as a method and
the options are split back out of the command string on spaces.
- zmcontrol.pl: refuse a command that is not a plain method name, so a qualified
Package::sub name can't call a sub outside the control module.
- canView()/canEdit() with a monitor id returned only the monitor check and
ignored the area, so canView('Control', $mid) passed for anyone who could see
the monitor. Require the area permission as well; Monitors stays decided by
visibleMonitor()/editableMonitor().
tests/php/test_control_command.php covers buildControlCommand() and the
monitor-scoped canView()/canEdit(); tests/perl/test_foscamr2c_preset_sql.pl
checks the statements FOSCAMR2C prepares and what it sends to the camera.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Filter::canView(), canEdit() and canDelete() ask the user object
$u->canEdit('System'), $u->canView('Events') and so on, but ZM\User had no such
methods. The calls fell through to ZM_Object::__call(), which stored the argument
as a property and returned it, so every check returned a truthy area name. Any
account with Events=View passed as a System editor and could save and run a
filter with AutoExecute, an OS command run by zmfilter.pl. The same fall-through
made the filter ownership and filterdebug checks pass for everyone.
Add canView(), canEdit() and canCreate() to ZM\User. They take the level from the
user's own field and from their role, the way the global functions in auth.php do
for the logged-in user, and refuse anything that is not a permission area.
Monitor::commandFieldChangesNotAllowed() now uses canEdit('System') so a role's
System=Edit counts there too.
In the filter action, require edit on a stored filter before applying the
submitted fields when saving over it, since the fields can name a new owner;
stop after a refused save instead of starting the filter in the background; and
require edit on the filter for start/stop/restart, which had no check.
tests/php/test_user_permissions.php drives the real ZM\User, ZM\User_Role and
ZM\Filter classes. The older filter tests stubbed the user with its own
canView/canEdit, which is why they passed while the real class had neither.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Annotate() clamped the label position to [0, width - text_width] in unsigned
arithmetic. A label wider (or taller) than the frame wrapped that bound to about
four billion, so the requested LabelX/LabelY was used unchanged and glyph rows were
written past the end of the frame buffer on every captured frame. The label text,
position and size are monitor settings any editor of that monitor can change.
Compute the bounds in signed arithmetic and floor them at zero. Draw a glyph only
when its whole cell fits in the row and a line only when it fits in the frame,
rather than checking after the glyph is already drawn, and skip foreground pixels
that fall outside the row (the shipped font reaches one column past the cell).
LabelSize indexed the font variants with size - 1 through at(), so 0 or anything
above 4 threw out_of_range in the capture thread. Clamp it to the valid sizes in
Annotate() and centreCoord().
Add tests that draw into a buffer followed by a guard region, covering
oversized, overlong, overtall and out-of-range label sizes in each pixel format.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: free a stream's monitor and playback buffer when nph-zms exits
Every nph-zms exited with ~1 MB still allocated, reported by LeakSanitizer
on every stream:
- The monitor's zones each hold a shared_ptr back to it, so releasing the
stream's reference never freed the Monitor, its zones (a full-frame zone
image each), camera or links. zmc breaks the cycle with disconnect() on
exit; zms never did. ~StreamBase now calls it.
- The buffered-playback SwapImage array (connkey + buffer) was never
deleted. MonitorStream now frees it in its destructor.
Freed at process exit either way, so nothing grows, but it shows up in
every leak report and hides real ones.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* test: check a stream frees its monitor and zones on destruction refs #5186
A stream holds a monitor with one zone; once the stream is destroyed, a
weak_ptr to the monitor must have expired. Each Zone holds a shared_ptr
back to its Monitor, so without ~StreamBase breaking the cycle the monitor,
its zones and their images outlive the stream. Fails on 82901ac1f
(REQUIRE( watched.expired() )), passes with the fix. Needs no database or
sanitizer, so ci-cpp-tests runs it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* refactor: hold the buffered-playback array in a unique_ptr refs #5186
temp_image_buffer is now a std::unique_ptr<SwapImage[]>, so it is freed
with the stream without a hand-written destructor, and the leak cannot
come back by dropping that line. The array is private and only allocated
in runStream(), so a unit test cannot see it freed without a sanitizer;
under LeakSanitizer, buffered nph-zms streams leaked 48,008 bytes from
runStream() on 82901ac1f and nothing with this branch.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* fix: free decoded frames and images once no consumer can read them
Packets stay in the packet queue for passthrough's keyframe alignment and
for pre-event frames, but each also kept its decoded AVFrame until it left
the queue, and with SaveJPEGs set (the default) its Image too: ~15 MB per
packet at 5MP. The frames also pinned libavcodec's buffer pool at the
queue's depth. With on-demand decoding and viewers, or a keyframe interval
longer than MaxImageBufferCount, zmc grew to several GB.
- Analyse() releases in_frame, and the y_image view into it, once the
packet has been analysed; later readers use packet->image. It is kept
when an encoder has no image to work from.
- PacketQueue::releaseDecoded() frees every decoded copy of packets that
all iterators have passed and that lie before where an event could still
start (openEvent's pre-event walk back, to a keyframe for passthrough).
The compressed packets stay queued.
Event output (capture/analysis JPEGs, snapshot, alarm, video) is unchanged.
refs #4860
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* docs: say which reload can widen the window under releaseDecoded refs #4860
The comment said the release boundary can't move back because events only
open on the analysis thread. That holds for events, but a RELOAD action
(zmu --reload) runs Monitor::Reload() on zmc's main loop and can widen
pre_event_video_packet_count or set keep_keyframes without clearing the
queue. Say so, and what an event right after it loses: capture JPEGs for
released passthrough frames; on encode those packets are already gone.
Raised in review of #5185.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: never start an event on packets whose decoded data was released refs #4860
releaseDecoded() frees the images and decoder frames of packets before the
earliest point an event could start, computed with the current pre-event
window. A RELOAD action (zmu --reload) can widen that window, or turn on
keep_keyframes, without clearing the queue. On passthrough the queue still
holds the previous GOP until the next keyframe trims it, so the next event
could walk back into packets already released: its video was complete, but
those frames had no capture JPEGs. On a 15 fps camera with a 30-frame GOP,
widening PreEventCount 5 -> 100 left frames 1-30 without JPEGs in about a
third of the events right after the reload.
get_event_start_packet_it() now treats the first packet at or after
released_before_ as the front of the queue and warns when it stops there.
For passthrough that packet is the keyframe releaseDecoded() stopped on;
after a reload from encode to passthrough it may not be, so the start
moves forward to the next keyframe. With an unchanged window the walk
never reaches the boundary. After a widening reload the next event gets up
to one GOP fewer pre-event frames than before #4860's change, instead of
frames without images.
Raised in review of #5185.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
With its player left on Auto, a monitor with go2rtc enabled had
selectNextPlayer() pick go2rtc_webrtc first, and with ZM_GO2RTC_PATH
empty select_go2rtc() then alerted "ZM_GO2RTC_PATH is empty", on the
montage for every such monitor, although nobody had chosen go2rtc.
The fallback chain already skipped go2rtc for a monitor without it
enabled; skip it when ZM_GO2RTC_PATH is empty too. A monitor or browser
that chose go2rtc reaches select_go2rtc() directly and still gets the
alert.
A node test drives selectPlayer() through the restart chain for Auto
without a path, Auto with one, and go2rtc chosen explicitly.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
go2rtc serves its API under /api, and its websocket at /api/ws. zmc
appends /streams to ZM_GO2RTC_PATH and the browser appends /ws, so the
setting only worked if it ended in /api. The natural value to enter is
the server address, http://host:1984, and with that zmc reached
/streams, which is go2rtc's RTSPtoWeb compatibility endpoint, so no
stream was ever registered, and the browser's /ws was a 404. Where
another player was available the failure went unnoticed.
Normalise the setting where URLs are built from it: a trailing slash is
dropped and /api is added unless the path already ends in it, which also
covers go2rtc behind a reverse proxy at a sub-path.
- Go2RTCApiEndpoint() in zm_utils, used by zmc's Go2RTCManager.
- go2rtcApiUrl() in skin.js, used by MonitorStream and the console
thumbnail stream. It assigns the path once, since stripping the slash
from a bare host's "/" and appending gave "//api", as the old
pathname += '/ws' gave "//ws".
- The option's help says the suffix is optional.
Catch2 and node tests cover bare hosts, trailing slashes, an existing
/api, a proxy sub-path and credentials in the URL.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: schedule ONVIF subscription renewal for short and unreported lifetimes refs #5179
The Beward camera from #5165 grants a 60 second subscription and its
RenewResponse carries no TerminationTime. Three things combined so that
ZM renewed after every PullMessages that returned messages (61 renewals
in 74 seconds of the reporter's log):
- The renewal was scheduled a fixed ONVIF_RENEWAL_ADVANCE_SECONDS (60)
before termination, which for a 60 second subscription is its creation
time. ONVIFNextRenewalTime() now uses the smaller of that advance and
half the remaining lifetime.
- Renew() only logged a missing TerminationTime and left
next_renewal_time in the past. assume_renewal_times() now schedules
from the lifetime we asked for, capped at what the camera last granted
(ONVIFAssumedLifetime()), so this camera renews every 30 seconds.
- IsRenewalNeeded() was only checked after a response carrying messages,
so a camera quiet for longer than its subscription was never renewed.
It is now also checked after an empty (SOAP_EOF) poll.
Since renewal is now checked after every poll, a camera that answers
Renew with ActionNotSupported has renewal disabled rather than being
asked again on each poll, matching the existing handling of unusable
TerminationTimes.
Tests cover the renewal time for long, short and very short
subscriptions, and the assumed lifetime with no, smaller and larger
previously granted lifetimes.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: only disable ONVIF renewal for ActionNotSupported, keep sub-second grants refs #5179
Review follow-ups on the renewal scheduling change:
- Renew() treated soap->error == 12 as ActionNotSupported, but 12 is
gSOAP's generic SOAP_FAULT. With renewal now disabled on that path, a
NotAuthorized or InvalidArgVal fault from Renew would have stopped
renewal for good while marking the subscription healthy.
ONVIFIsActionNotSupported() checks the fault subcode and string for
ActionNotSupported (wsa: or ter:); any other fault now takes the
existing cleanup and re-subscribe path.
- granted_lifetime truncated the remaining time to whole seconds. A
termination less than a second away stored 0, which reads as "never
reported", so the next Renew without a TerminationTime assumed the full
requested 300 seconds. ONVIFGrantedLifetime() rounds up instead.
Tests cover ActionNotSupported subcodes and strings against other faults
and non-fault results, and the granted lifetime for whole, fractional and
sub-second remainders.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: count an assumed ONVIF renewal lifetime from the request, not the response refs #5179
When a RenewResponse has no TerminationTime, assume_renewal_times()
started the assumed lifetime when the response arrived, but the camera
counts it from when it received the request. A slow response pushed the
deadline and the next renewal later by the response time: with
subscription_timeout=10 and a 6 second response, the camera's deadline
was request+10 but the renewal was scheduled for request+11. In absolute
renewal mode it also ignored the exact deadline that was sent.
Renew() now records the time just before building the request and the
deadline it asks for: request time plus subscription_timeout, or the
absolute whole-second time it sends. ONVIFAssumedTermination() replaces
ONVIFAssumedLifetime() and returns that deadline, capped at request time
plus the lifetime the camera last granted. The renewal is scheduled from
the request time; if the response arrived after it, it is already due and
the next poll renews.
Tests cover no, smaller and larger previous grants, an absolute deadline
kept exactly, and the slow-response case from review.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
ZM asks the camera to hold each PullMessages for up to pull_timeout
seconds (default 1). The Beward camera from #5165 answers in 10-30ms with
no messages, which gSOAP reports as SOAP_EOF, and Run() issued the next
request at once: ~2,500 PullMessages in 74 seconds of the reporter's log
(~34/s), each with a fresh UsernameToken digest.
Time each PullMessages. When it comes back with no messages (SOAP_EOF, or
a SOAP_OK response with no NotificationMessage) before the Timeout,
WaitForMessage() now waits out the remainder in 100ms steps, staying
responsive to terminate_ and zm_terminate. A camera that ignores the
long-poll is then polled at most once per pull_timeout; one that holds it
is polled again immediately as before. Events raised during the wait stay
queued in the camera's pull point and arrive with the next request, so
the added latency is at most pull_timeout.
ONVIFEarlyPollWait() computes the remainder and is tested for an
immediate answer, a full hold, a partial hold and sub-millisecond
elapsed times.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Per-topic alarm expiry takes each alarm's deadline from the
PullMessagesResponse TerminationTime. Beward cameras send their
CurrentTime as the TerminationTime in every response, so an alarm was
stored with a deadline that had already passed and the sweep at the end
of the same WaitForMessage() pass removed it about 25us later. The
analysis thread checks onvif->isAlarmed() once per frame, never saw the
monitor alarmed, and no event was recorded even though the log showed
"ONVIF Triggered Start Event".
Move the TerminationTime handling into a free function,
ONVIFAlarmTermination(), which applies the camera clock offset as before
and returns false when the adjusted time is not after now. The alarm then
gets no expiry and is cleared by the camera's explicit State=false
message, as it was before per-topic expiry was added. The Debug line now
also prints CurrentTime so this camera behaviour is visible in logs.
Tests cover TerminationTime equal to and before CurrentTime, a future
TerminationTime, a skewed camera clock, a missing CurrentTime keeping the
previous offset, and a missing TerminationTime.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
view_video.php took the end of the range straight from the request and never
checked it against the file:
if (!empty($matches[2])) $end = intval($matches[2]);
$length = $end - $begin + 1;
so on a 1000 byte file:
bytes=0-99999 206, Content-Length 100000, body 1000 bytes
bytes=5000-6000 206, Content-Range naming bytes that do not exist
bytes=500-100 206, Content-Length -399
bytes=1000- 206, Content-Length 0
bytes=-100 200 with the whole file, not the last 100 bytes
A client that is told to expect 100000 bytes and gets 1000 does not see a bad
request, it sees a truncated file, and reports the video as broken. The suffix
form was not recognised at all because the pattern required a digit before the
dash.
This is reached once per fragment by the byte-range HLS manifest VideoStore
writes, every fragment being a Range against the one mp4, so a player that
asks for anything the file cannot supply gets a body that does not match its
own Content-Length rather than an answer it can act on.
Parse the header properly: clamp a range that runs past the end, because a
client may ask for more than is there and is entitled to what is there;
answer 416 with "Content-Range: bytes */size" when the range cannot be
satisfied at all, so the client learns the real length; and read "-N" as the
last N bytes. Length is now derived from the range being served rather than
the one requested, and the send loop counts down by the bytes it actually
read, so Content-Length and the body cannot disagree.
Only the first range of a multi-range request is served, as before. A
multipart/byteranges body is not worth building for this, and falling back to
sending the whole representation is not an option when these are event videos
of hundreds of megabytes; Content-Range names exactly what was sent.
The parsing is its own dependency-free include so it can be tested without a
database, and tests/php/test_http_range.php covers each case above plus a
sweep asserting that every range it ever returns lies inside the file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#5174 reports an event's m3u8 as invalid and proposes moving every fragment's
byte range 8 bytes further into the file, from @17203 to @17211.
That reading comes from ffprobe's trace, whose second column is the offset of
the box *body* -- the start plus the 8 byte box header -- not the start. In
the manifest quoted there the first fragment is 1434876@17203, and the trace
has the moof body at 17211 and the mdat ending at 1452079, so the range spans
exactly moof(264) + mdat(1434612) = 1434876 from the start of the moof. The
proposed change would cut the moof header off every segment.
The manifest is not contiguous, which is what draws the eye: the init range
is ftyp+moov and the fragments start 16KB later, because reserve_region puts
the leading sidx in between and the sidx has to END where the fragments
BEGIN. Nothing fetches those bytes and HLS does not require byte ranges to
abut.
So assert it rather than argue it. Against sidx-moof.mp4, using the box
walker the sidx tests already keep for the purpose, check that a fragment
starts at its moof box rather than 8 bytes in, that fragments run back to
back, that the init range is the header boxes and nothing else, and that the
gap between them is exactly the reserved index.
This says nothing about the browser error that prompted the report, which is
not quoted in the issue; it only rules the byte ranges in or out.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
writeM3U8 truncated and rewrote the whole playlist every time a fragment
completed. That is O(fragments) of writing per fragment, so an event pays
O(fragments squared) overall. For the ten minute events a section length
produces the manifest is about 55KB and the cost is invisible. It stops being
invisible when an event does not close.
On a box here, four monitors stopped closing their events after a reboot and
ran for 51 hours. Their manifests reached 17MB and 470,000 lines, and strace
showed where the writes were going: in one window the mp4 took 3 writes while
index.m3u8 took 195, opened O_WRONLY|O_CREAT|O_TRUNC. The cameras produced
0.4MB/s of video between them and the disk was absorbing 24MB/s at 100%
utilisation, 106ms average write latency.
That is a loop rather than just waste. One rewrite took 3.48s of wall clock
against a fragment arriving every 1.2s, so the event thread could never catch
up, the packet queue stayed full ("Analysis is not keeping up" every three
seconds), and the analysis thread is where the section length check that would
have closed the event lives. The growth starved the only thing that could stop
it.
An EVENT playlist is append only: a fragment's three lines never change once
written, and only the header depends on anything global. So write the new
fragments to the end, and fall back to a full rewrite when something above
them would differ -- a changed target duration or init segment end, the
different url the close path passes, a different path, or the closing
ENDLIST. The remembered byte count is checked against the file before
appending, so a manifest that something else has truncated, replaced or
removed is rebuilt rather than appended to; any failure zeroes the state,
which makes a rewrite the answer to anything unexpected.
The header and fragment text are split into m3u8Header, m3u8Fragment and
m3u8TargetDuration so the property that matters can be tested without
standing up a VideoStore: appending one fragment at a time produces a byte
identical manifest to writing the whole thing at once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every query on Frames filters on EventId and orders or ranges by FrameId,
and nothing needs a frame by Id. The Id primary key was a clustered index
nothing read, and EventId_FrameId_idx was a secondary index every query
used. On a copy of a local table that secondary index was half the
table's size. With (EventId, FrameId) as the primary key the index is
gone, and an event's rows are stored together, so deleting an event is a
range delete.
zm_update-1.39.36.sql:
- converts AI_Detections.FrameId from Frames.Id to the per-event frame
number, drops its foreign key to Frames and indexes (EventId, FrameId).
A composite foreign key cannot replace it: ON DELETE SET NULL would
have to null the NOT NULL EventId, and Frames rows are written in
batches, so a detection can be recorded before its frame row.
- removes duplicate (EventId, FrameId) rows, keeping the earliest.
- rebuilds Frames with the new primary key.
- removes ON UPDATE CURRENT_TIMESTAMP from Frames.TimeStamp. Any UPDATE
of a frame row was overwriting its capture time.
Each step checks the current schema first, so the migration can be
re-run.
REST API: view, edit and delete take /frames/<action>/<EventId>/<FrameId>.json.
The old single-Id URLs return 404. CakePHP 2 has no composite keys, so
the model's primaryKey is EventId. That keeps Event's dependent cascade
delete limited to the event's own frames. The controller writes with
explicit (EventId, FrameId) conditions instead of save(), which would
match rows on EventId alone. Edit no longer changes EventId or FrameId.
view=image with fid but no eid used to look up Frames.Id. It now returns
404.
The Perl Frame class is identified by (EventId, FrameId).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ajax/frames.php collected Frames.Id values from the event's rows and then
filtered with a FrameId IN (...) term, which FilterTerm translated to
Id IN (...). The search query had no EventId condition of its own. Once
the term matches on FrameId, which is only unique within an event, it
returns rows from every event, so the query is now anchored on the event
with the same WHERE EventId clause as the unfiltered list.
FilterTerm now maps FrameId to the FrameId column. The frames list
thumbnail links address the image by eid and fid instead of Frames.Id.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
finalize() located the mfra by reading the trailing mfro with fopen and
fseeko and decoding it by hand, the same job zm_mp4::media_end() does for
the sidx scan. Use media_end() for both, so the last HLS fragment and the
index agree on where the media ends.
media_end() is also stricter: it requires an mfra box of the stated size
at the offset the mfro points to, where the old code accepted any size up
to the file length. A trailer that does not check out now leaves the final
fragment running to EOF, as a missing trailer already did.
A new test covers media_end() against the fixture's real mfra and three
damaged trailers: an mfro size off by four, one larger than the file, and
no mfro at all.
refs #5144
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
build_sidx_region() wrote starts_with_SAP=1, SAP type 1 into every
reference. That holds for frag_keyframe recordings, where every fragment
opens on a keyframe, but a monitor whose encoder options add frag_duration
or frag_size gets fragments cut mid-GOP, and the index then claimed a
decodable start where there was none.
scan_fragments() now reads the sync flag of each fragment's first video
sample: trun first_sample_flags, else the first entry's sample_flags, else
the tfhd default_sample_flags, else the trex default (read_video_track()
now keeps it). A reference with no SAP gets 0 for starts_with_SAP, type and
delta; a merged reference takes the flag of its first fragment. parse_traf()
also skips tfhd default_sample_size, which it never needed before.
Tests: remuxing the fixture with frag_keyframe and a 250 ms frag_duration
gives about a dozen references, of which exactly the three that begin at a
keyframe are SAPs; the frag_keyframe fixture scans as all SAPs, and the
byte-for-byte comparison with the reference tool is unchanged; merging takes
the first fragment's flag. The remux sequence the muxer test used is now a
helper shared by both.
refs #5144
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every fragmented event reserved 64 KiB for its leading sidx, whatever its
length. On a short motion clip of 1-2 MB that is 3-6% of the file.
zm_mp4::reserve_size() sizes the region for twice the fragments expected
in one section: section length divided by the GOP, which is the encoder's
gop_size when encoding and the packet queue's longest keyframe interval
otherwise, over the capture fps. It rounds up to whole 4 KiB blocks and
stays between 4 KiB (337 references) and the previous 64 KiB, which is
still taken when the section length, GOP or fps is unknown. The default
600 s section at a 1 s GOP now reserves 16 KiB.
A low guess does not lose the index: an event with more fragments than
the region holds has neighbouring fragments merged into one reference,
as before, which only coarsens seeking. VideoStore keeps the size it
reserved so finalize() fills that exact region, and Monitor gains a
GetSectionLength() accessor.
refs #5144
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
VideoStore::open() reserves a 64 KiB free box between moov and the first
fragment, but only when the MOV muxer wrote the moov in the header and
moof+mdat fragments follow it. finalize() scans the fragments and fills the
region with free padding followed by a sidx that ends at the first moof, so
FFmpeg-based players (Chromium, Android WebView, Electron) treat the index as
complete and start playback without visiting every fragment. On any parse or
write failure the region stays a free box.
fixes#5144
ReadJpeg() stored the JPEG header's width and height in the Image before
calling WriteBuffer(). WriteBuffer() reallocates only when the requested
size differs from the current one, so it saw no change and kept the
existing buffer and linesize, and the scanline loop then decoded every
row of a larger file past its end. A File monitor re-reads its source
into a monitor-sized image on every capture, so whoever can write that
file could overflow zmc's heap. Stored event JPEGs read by zms take the
same path.
Leave width and height to WriteBuffer(), as DecodeJpeg() already does.
FileCamera::Capture() also now refuses a file whose dimensions do not
match the monitor, since everything downstream is sized for the monitor.
Add a Catch2 case that reads a 256x192 JPEG into a 64x48 image. It
segfaulted before this change and passes after.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With ZM_AUTH_HASH_IPS on, the client address bound into the auth hash was
taken from the left-most X-Forwarded-For value whenever the header was
present, both when PHP generated the hash (getRemoteAddr()) and when PHP or
zms validated it (getAuthUser(), zmLoadAuthUser()). The header is client
controlled, so anyone holding a leaked hash could replay it from anywhere by
sending the address it was bound to.
Add ZM_AUTH_TRUSTED_PROXIES, a list of exact reverse proxy addresses.
X-Forwarded-For is now used only when REMOTE_ADDR is one of them, and is read
from the right, skipping hops that are themselves listed proxies, so values a
client prepends are never chosen. With the option empty, the default, the
header is ignored and REMOTE_ADDR is used.
PHP (web/includes/Network.php getRemoteAddr()) and C++ (ClientAddress() in
zm_utils, used by zmLoadAuthUser()) implement the same rule so generation and
validation continue to agree. Every PHP caller already routes through
getRemoteAddr(), so session.php and auth.php need no change.
Reverse proxy users who enable ZM_AUTH_HASH_IPS must list their proxy in the
new option; until they do, hashes bind to the proxy address, which still
validates but no longer distinguishes clients. This is the behaviour change
that the #4921 work avoided by trusting the header.
The option is added through ConfigData only, like other recent options;
zmupdate.pl --freshen inserts it, zms falls back to the compiled-in default and
PHP treats an undefined constant as empty, so no schema migration is needed.
Tests: ClientAddress Catch2 case; tests/php/test_remote_addr.php updated for
the trusted-proxy rule.
refs GHSA-72rf-54rm-798c
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit acea5889dec0826f595cb736147a5fdc9c94a2a9)
Saving a user from the web ui took the whole auth path down:
PHP Fatal error: Uncaught TypeError: strcasecmp(): Argument #1 ($string1)
must be of type string, array given in includes/auth.php:197
#0 auth.php(197): strcasecmp()
#1 auth.php(528): getAuthUser()
#2 auth.php(687): userFromSession()
reached from ?view=user&uid=2. That page's form posts user[Username],
user[Password], user[Name] and the rest, so $_REQUEST['user'] is an array on
every save from it, and getAuthUser() read that parameter as the username to
filter on and handed it to strcasecmp(). Under PHP 8 a string function given an
array is a TypeError rather than a warning, so the request died with a 500.
The same shape arrives from anyone who cares to send it, and not only on a page
that needs a session. userFromSession() reads user, pass, username, password
and auth straight out of the request, and the credential branches run before
anyone is logged in, so ?username[]=x&password[]=y reaches validateUser() with
arrays on an install that has never seen the caller before.
requestString() returns a parameter only when it is a string and null
otherwise, which is what the callers already do with a parameter that was not
sent. An array is not a username, a password or an auth hash.
master no longer has the strcasecmp line the report names, so it does not fatal
in that exact spot, but it reads the same unvalidated values: $filterUser is
bound as a query parameter and the credentials still reach validateUser(). This
fixes the class rather than the one line, and backports to 1.38 where the
reported line lives.
The test lifts requestString() out of auth.php and evaluates it alone, because
including auth.php needs a database; test_auth_no_include_side_effects.php
sidesteps the same dependency the same way. 8 cases, covering the form's array,
the login parameters, a nested array and the strings that must still pass
through. 5 of them fail without this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 1f4aa1a16c1b63fdf34a6f2c6aef6590dbd45acd)
kPacketInterval * i with a size_t i gives the deadline an unsigned
duration; once the producer falls behind, libc++'s sleep_until turns the
negative remaining time into a near-infinite sleep and the benchmark
hangs. Multiply by a signed index.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
zm::UnixSocket copies the path with a truncating strncpy, so a
PATH_SOCKS long enough to overflow sun_path made the server bind one
file while chmod, chown and unlink acted on another, and the client
connect to a truncated, different path. Both now refuse such a path with
an error. Start() also closes the listener on its own failure paths.
SendMedia indexed the two-entry sequence array with the stream id; a
caller passing StreamId::Monitor would have written past it. Ignore
anything that is not video or audio.
Tests: server and client refuse a 200 character path; a Monitor stream
id is ignored and does not disturb the video sequence.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
on_media admitted a packet whenever its generation matched the one the
session was built for. A restarted zmc starts again at generation 0, so
if the camera came back with different parameters its replayed keyframe
passed that check and went into the old codec's packer before the main
thread rebuilt the session.
Move the HELLO and generation bookkeeping into RtspSessionTracker, a
small class with no locking or RTSP types. Every HELLO and every
disconnect clears the confirmed flag; Update() asks the tracker for a
plan (none, adopt, rebuild), acts on it and confirms; on_media feeds the
packers only for a confirmed session at the confirmed generation. A set
that failed to build (unsupported codec) is not retried until a new
HELLO arrives, so a bad camera does not rebuild on every pass.
Tests: a new suite for the tracker covers the complete-set rule, the
HELLO-to-rebuild window, generation reuse after a producer restart with
changed and with unchanged parameters, audio appearing, disappearing and
changing, a generation without video, a failed build, and HELLOs for
streams the server does not serve.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The cached snapshot was framed when the status last changed, so after a
generation bump or further events a new consumer received it stamped
with a stale generation and an old event-sequence baseline. Keep only
the body and frame it in AcceptClient, so the header carries the
generation and sequence in effect at the moment of connection.
Tests: a snapshot cached at generation 0 with no events is delivered to
a later consumer with the current generation and sequence.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Two problems with announcing streams one at a time. SetAudioParams only
bumped the generation when audio had been announced before, so audio
joining a video-only stream was appended to the current generation after
its video HELLO, which the protocol says is the last HELLO of a
generation. And a re-prime that changed both streams bumped twice:
consumers saw, and a connecting consumer was handed, an intermediate
generation pairing the new audio with the old video, which
zm_rtsp_server built a session for and tore down again.
Add StreamSocket::SetStreams(video, audio), which applies the whole set
under one lock: unchanged is a no-op, the first announcement stays in
generation 0, and any change once a video HELLO has gone out - new
parameters, a stream appearing, a stream disappearing - is exactly one
bump with every remaining stream re-announced, audio first. The single
stream setters and the new ClearVideoParams are wrappers over the same
logic, and a null or codec-less parameter set means "no such stream".
PrimeCapture passes both streams in one call.
Tests: audio joining an announced video stream bumps the generation;
SetStreams keeps the initial announcement in generation 0, changes both
streams in one generation with consistent pairing, and drops a video
stream the source no longer has.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The alarmed/filtered pixel check methods handed Overlay() the raw GRAY8
scoring mask. Overlay keys on a non-zero source pixel and copies that byte, so
the highlight took whatever shape one byte has in the destination format: the
red channel on an RGB32 monitor, which is the whole reason alarms have always
come out red; all three channels on RGB24, giving white; luma alone on YUV420.
The zone's configured Alarm Colour was honoured only on the blob path, which
goes through HighlightEdges.
Generalise HighlightEdges into BuildHighlight, which takes an edges_only flag
and otherwise fills every marked pixel, and build the highlight for the pixel
methods the same way the blob path already builds its outline: in the
capture's own pixel format, carrying alarm_rgb, once scoring is finished with
the GRAY8 mask. HighlightEdges stays as a thin wrapper so the blob path and
its callers are unchanged.
This is a behaviour change: monitors left on the default red see no
difference, but a zone configured with any other Alarm Colour now paints that
colour instead of red or white.
Reverting the zone hunk fails the new test in all three of its sections.
Full suite: 148 cases, 12535 assertions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WBHBB95RBX7D9p8ge2WDZb
Monitor::connect() returns false with shared_data still null on every one of
its failure paths: the mmap file cannot be opened (wrong ownership, e.g.
after a package upgrade), fstat fails, ftruncate cannot grow it (/dev/shm out
of space -- a container with the default 64MB tmpfs hits this quickly, since
one 720x480 monitor with 10 buffers already asks for ~20MB and a 1080x720 one
asks for ~62MB), or mmap itself fails.
zmc's startup loop reacts by retrying:
while (!monitor->connect() and !zm_terminate) {
Warning("Couldn't connect to monitor %d", monitor->Id());
monitor->SetHeartbeatTime(std::chrono::system_clock::now());
sleep(1);
}
so the first thing it does after a failed connect is write through the null
pointer. zmc dies with SIGSEGV at address 0x80 instead of retrying, which
presents as a monitor that will not start and a capture daemon that keeps
crashing. The accessors on either side of these already guard with
`if (shared_data && shared_data->valid)`.
Reverting the guard fails the new test with SIGSEGV at zm_monitor_shm.cpp:66,
and restoring it passes. Same fix as release-1.38's a550545e4.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WBHBB95RBX7D9p8ge2WDZb
Reverts the previous commit's always-on measurement. Decoding audio for every
monitor that has it spends CPU on a number almost nothing reads, which is the
wrong trade even though it did solve the chicken and egg of picking a
threshold without ever seeing a level.
Measure when something is actually going to use the reading instead:
- AudioDetection is on, as before, so nothing changes for a monitor that
scores on audio; or
- somebody asked. SharedData gains audio_level_until, a wall clock second
the capture thread keeps measuring up to. The monitor editor's new level
meter pushes it forward while it is on screen and the measurement lapses a
few seconds after the page is left, so nothing has to send a stop and a
crashed browser cannot leave a monitor decoding forever.
When the reading stops being wanted the decoder is released and the published
level and peak are cleared, so a stale number is not left looking current and
an old peak does not land on the next frame row written.
audio_level_until is carved out of analysis_pad rather than appended, so
SharedData stays 888 bytes and no existing offset moves; the static_asserts,
Memory.pm and Monitor.php are updated together and all three now agree the
field is at +880.
The meter itself is on the audio settings, shown whether or not
AudioDetection is checked, because the level is what you need in order to
choose a threshold. It draws the threshold currently in the input as a mark on
the bar so a reading can be judged against it before saving, and a monitor
whose zmc is not running reads "no reading" rather than a confident 0, which
would be indistinguishable from silence.
Frames.AudioLevel is therefore 0 again on monitors that do not score on audio.
That is what the graph already treats as "no audio data", so it draws no line
rather than a flat one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
Gating the measurement on AudioDetection made the graph useless for the job
it is most wanted for. AudioThreshold is a per-device number -- the floor on
one camera's mic is nothing like another's -- so it has to be measured before
it can be set, but nothing was measured until it was already set. Enabling
detection with a guessed threshold to find out what the real one should be is
backwards.
The level is now read for every monitor with decodable audio. AudioDetection
governs only whether crossing the threshold contributes a score, which is
what the setting is named for. shared_data->audio_alarm stays 0 when it is
off, so nothing downstream changes for a monitor that does not want audio
alarms.
Nothing here depends on Analysing either. The measurement is in
Monitor::Capture, which runs on whatever Analysing is set to, and frame rows
come from Event::AddFrame, which a continuously recording monitor reaches
through the RECORDING_ALWAYS path with motion detection off. So a monitor
that only records continuously still gets levels on its rows.
Since Monitor::Capture retries Open on every audio packet until it succeeds,
and that now happens for every monitor with audio rather than the handful with
detection on, AudioDetector remembers a codec it has already failed to find a
decoder for. Without it a stream ZoneMinder cannot decode logs a warning at
the audio packet rate for as long as the monitor runs. A reconnect bringing a
different codec is still tried.
The cost is one audio decode per monitor with audio, where before it was one
per monitor with detection enabled. That is small next to the video path, but
it is not nothing on a box with many cameras.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
The cue strip under the video showed one flat red band per alarm period. It
tried to encode the score as a bar height, 'height: '+frame.Score+'px', but
never could: the frames ajax did not return Score, so every bar got
'height: undefinedpx' and fell back to the stylesheet's height: 100%. So the
motion level it looks like it is drawing has never actually been drawn.
Replace it with a line graph of both series over the length of the event. The
alarm periods stay, as a pale wash behind the lines, so nothing that was
readable before is lost.
The two series do not share a vertical scale. Audio level is 0-100 by
construction, but a motion score is a sum over zones with no upper bound, so
pinning both to 0-100 would flatten the audio line against the floor on any
event scoring above 100. Each is scaled to its own maximum, and hovering reads
out the exact values, which is what the numbers are wanted for. An event whose
rows are all zero -- recorded before the column existed, or by a monitor with
AudioDetection off -- draws no audio line at all, rather than a flat line
claiming silence was measured.
The readout goes into the existing #indicator, which already tracks the mouse
across the whole progress bar, instead of a second tooltip competing for the
same pixels.
The geometry lives in web/js/LevelGraph.js so it can be tested without a DOM,
and so the polyline and the hover readout cannot disagree about where a given
second sits. The bar grows from 1.25em to fit the graph, which needed two
consequential CSS changes: #indicator now spans the taller bar, and
.progressBox drops from 0.66 opacity to 0.25, because at full strength the
played part of the graph is unreadable behind it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
zm_update-1.39.31.sql gave monitors audio detection, but the level only ever
existed in shared memory, so it was gone the moment the frame passed and there
was nothing for the event view to plot. Add Frames.AudioLevel next to Score, on
the same 0-100 dBFS-derived scale the threshold uses.
What is stored is the peak since the previous row, not the level at the instant
the row was written. Frames rows are written well below the capture rate --
only alarm, bulk and score-increasing frames get one -- so sampling at write
time would drop exactly the short loud noises worth seeing on a timeline.
AudioDetector accumulates the peak as it decodes and Event::AddFrame takes it
where the row is built, which clears it so each row covers its own interval.
The Event constructor takes and discards it once, otherwise an event's first
row reports the loudest moment since the previous event ended.
This needs no shared memory change: zma is now an offline re-analysis tool and
the live analysis runs in a thread of zmc, alongside the capture thread that
runs the decoder, so the peak can stay in the AudioDetector. SharedData keeps
its documented 888-byte layout and its fixed offsets.
The frames ajax returns the column, and Score with it. elements in
web/ajax/status.php is a whitelist that never listed Score, which is why the
event view's cue strip has been reading an undefined Score off every frame.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
changeRate's reverse branch did not use the selected rate as the reverse
speed. It looked it up as rates[rates.indexOf(-rate)-1]/100, stepping one
entry further down the shared rate list, so every reverse rate ran a notch
too slow: -1/2x played at 1/4x and -16x at 10x. -1/4x was worse than slow,
because one step below 25 in that list is 0, so revSpeed came out 0 and the
video sat still while the ui claimed it was rewinding.
The rate the user picked is the speed, so use it.
Leaving reverse through the dropdown also leaked the rewind interval, which
only pauseClicked and vjsPlay ever stopped. Picking a forward rate after a
reverse one left it running, so it went on dragging currentTime backwards and
resetting playbackRate to 0 on every tick while the player was supposedly
running forwards. The teardown is now stopRewind(), split out of
stopFastRev() because stopFastRev rewrites the rate select to 1x, which would
undo the choice changeRate is in the middle of applying.
stopFastRev no longer reads the rate back out of the player to decide what to
put in the select and the cookie, for the same reason streamFastFwd stopped
doing it: videojs can defer the set until the tech is ready, so the getter
still answers with the rate we just left.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
Clicking fast forward once too many at 16x threw out of video.js:
TypeError: HTMLMediaElement.playbackRate setter: Value being assigned is
not a finite floating-point value.
playbackRate@video.min.js
streamFastFwd@..._views_js_event-....js
streamFastFwd stepped the shared rate list by indexing it directly,
rates[rates.indexOf(current)+1]. At the top of the list that is rates[15],
undefined, and undefined/100 is NaN, which Firefox refuses outright.
The guard meant to prevent this ran after the assignment rather than before
it, and read the rate back from the player to decide, so it could only
disable the button once the bad value had already been sent. It was reachable
in normal use because streamPlay() re-enables the button whatever rate we are
at, so play-then-fast-forward at 16x throws every time; picking 16x from the
rate dropdown gets there too, since changeRate does not touch button state.
indexOf also answers -1 for a rate that is not in the list, and -1+1 indexes
rates[0], which is -1600: stepping forwards from an unlisted rate asked for
16x reverse.
streamFastRev had the same fault at the other end. rates[0-1] is undefined, so
revSpeed became NaN and every tick of the rewind interval then handed
currentTime a NaN.
Both now go through stepRate, which snaps an unlisted rate to the nearest
listed one and returns null rather than walking off either end, so the caller
disables the button and leaves the player alone instead of assigning
something it cannot use. streamFastFwd also sets the dropdown and cookie from
the rate it just asked for rather than reading it back, because the stack in
the report shows videojs deferring the set until the tech is ready, at which
point the getter still answers with the old rate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
The library is AGPL-3.0-or-later so it is not shipped, and the admin installs
it at skins/<skin>/assets/audioMotion-analyzer/src/audioMotion-analyzer.js.
The instructions for doing that had drifted from what the code loads:
- help.txt and the OPTIONS_WHATTODISPLAY help gave the bare
https://cdn.jsdelivr.net/npm/audiomotion-analyzer@X.X.X URL, which resolves
to the package's "main" entry, the minified UMD bundle dist/index.js. The
install path is the package's src/ ES module, so the URL needs the explicit
/src/audioMotion-analyzer.js suffix. Same for the download links in the
AudioMotionVersionNotInstalled and AudioMotionVersionWrongVersion messages.
- The install path was written as /skins/MySkin/..., a placeholder that does
not correspond to any skin.
- RequiresAudioMotionEnabled named only the file, not where it goes.
- assets/version documented every other asset in that directory but not this
one, leaving no explanation for the otherwise empty directory.
help.txt no longer restates the required version, so 4.5.4 stays declared only
by SUPPORTED_AUDIO_MOTION_ANALYZER_VERSION as intended, and assets/version
points at that constant rather than duplicating it.
Add tests/js/audiomotion-paths.test.js to hold the PHP feature probe, the
dynamic import, help.txt and both lang catalogues to the same path, the same
download URL and the same version.
Also gitignore the installed library so a local install is not committed back
into a GPL-2.0 tree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
Correct three problems in the stream socket generation tracking added by
the previous review fix-up:
- ClearAudioParams bumped the generation without restarting the video
sequence or dropping the cached keyframe, so a late joiner received a
HELLO at generation N+1 followed by a KEYFRAME still stamped N, and
sequences did not restart as documented. It now does the same full bump
as SetVideoParams/SetAudioParams.
- zm_rtsp_server rebuilt the xop session whenever the generation changed,
even with identical codec parameters. Generations restart at 0 when zmc
restarts, so every zmc restart dropped the RTSP clients; the original
code kept the session in that case. Unchanged parameters now just adopt
the new generation without a teardown.
- Deciding whether audio belongs to the current generation by comparing
per-stream generation numbers raced the two HELLOs of a generation
(double rebuild when Update() ran between them) and cannot tell a
producer restart apart. The producer now sends the audio HELLO before
the video HELLO within a generation (on connect and on every bump, and
PrimeCapture announces audio before video), so the video HELLO always
completes a generation's parameter set. The consumer forgets the
previous generation's HELLOs when a new generation starts and on
disconnect, and builds only once the video HELLO of the latest
generation is in. It also records whether audio was announced at build
time rather than whether a packer was created, so an unsupported audio
codec no longer triggers a rebuild on every pass.
The ordering guarantee is documented in the protocol header and the
stream socket docs. Tests pin the audio-first order on connect and on a
video reconfigure, and the keyframe drop and sequence restart on audio
removal.
refs #5143
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T4UcdJLt1bxwdpcigGxZRD
Address several stream socket review findings on the transport, its
consumer client and the wire protocol:
- ParseAllowedUids rejects negative, out-of-range and non-round-tripping
uids instead of wrapping or truncating them (e.g. 2^32 no longer
becomes uid 0).
- StreamSocketClient backs off after a connection the producer closes
before any message, so a rejected consumer (uid allow-list, client
limit) no longer busy-loops; a rejection is not reported as a
disconnect.
- SendMedia drops packets for a stream that has no announced HELLO, and
ClearAudioParams forgets a previously announced audio stream (bumping
the generation and re-issuing the surviving video HELLO), so a stale
audio HELLO is never replayed and media never precedes its HELLO.
- Header pts_us is encoded as signed (two's-complement) microseconds so
negative and AV_NOPTS_VALUE timestamps survive the wire; the dump tool
decodes it as signed and tracks sequence gaps per generation so a
generation reset is not mistaken for packet loss.
Tests cover the uid rejections, the audio HELLO clearing and media
guard, the connection-rejection backoff, and signed pts round-trips.
refs #5143
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T4UcdJLt1bxwdpcigGxZRD
