Skip to content

feature: Allow use of secondary stream for analysis/motion detection - #4965

Open
madcamel wants to merge 15 commits into
ZoneMinder:masterfrom
madcamel:secondary_stream
Open

feature: Allow use of secondary stream for analysis/motion detection#4965
madcamel wants to merge 15 commits into
ZoneMinder:masterfrom
madcamel:secondary_stream

Conversation

@madcamel

@madcamel madcamel commented Jul 4, 2026

Copy link
Copy Markdown

It's really hot outside and my NVR was melting down to zmc CPU usage. Here's a fun fix with some help from the slop machine. This is an absolutely massive improvement, my machine went from flamethrower to mostly idle.

-snip-

What

Uses SecondPath (should be configured as a lower resolution stream from the camera) for frame analysis when AnalysisSource is set to Secondary.

AnalysisSource already exists in the schema and UI but was a no-op. This wires it up.

Why

In Modect, zmc software-decodes the entire primary stream just to feed motion
detection, even with no live viewers. On a 5MP/10fps monitor that is roughly a
full CPU core (measured: ~59% libavcodec + ~13% libswscale). Most cameras
already publish a low-res substream; analysing that instead cuts idle decode
cost, while motion events still record the primary at full resolution.

How it works

  • New SecondStreamThread (one per monitor, started only when SecondPath is
    set and AnalysisSource=Secondary) owns its own FFmpeg_Input on the
    substream, reconnects with exponential backoff, and publishes each decoded
    frame into a mutex-guarded mailbox at the substream's native resolution. It
    never enqueues packets into the packetqueue, so the single-video-stream
    invariants there are untouched.
  • Monitor::getMotionSourceImage() feeds Analyse(). In secondary mode it
    returns the mailbox image, upscaled to camera dimensions only when a frame is
    actually scored (a cheap sequence/age peek gates the copy + upscale). Zones
    stay in primary coordinates and need no changes, because the analysis image is
    scaled to the same camera_width x camera_height the primary path already
    produces.
  • The monitor's AnalysisImage setting (YChannel vs FullColour) drives whether
    the sidecar extracts the Y plane or swscales to colour.
  • effective_decoding downgrades DECODING_ALWAYS to on-demand while secondary
    analysis is active, so the primary decodes only for live viewers. Alarm/event
    flow is unchanged: scores attach to the primary packet, so pre-event buffering
    and recording use the primary as before.
  • The sidecar forces software decoding (FFmpeg_Input::set_no_hwaccel()): the
    D1 substream is cheap on the CPU, hardware-format frames cannot be consumed by
    the mailbox, and this avoids contending with the primary over the VAAPI device.
  • FFmpeg_Input gains an Open(filename, AVDictionary**) overload so the
    sidecar can pass rtsp_transport and a socket read timeout (timeout for
    ffmpeg >= 5.0, stimeout for older). The existing single-argument Open()
    delegates to it unchanged.

Interaction with existing SecondPath use

SecondPath is already consumed elsewhere, so reviewers should be aware:

  • FfmpegCamera opens the substream as an audio source (mSecondInput) when
    the primary has no audio stream; go2rtc and RTSP2Web restreaming also open it.
  • The sidecar adds one more in-process connection. On a monitor with
    AnalysisSource=Secondary and no primary audio, zmc will hold two
    connections to the substream (audio + analysis); with restreaming enabled the
    camera may see more. This is additive load only — the sidecar owns a separate
    FFmpeg_Input and shares no state with mSecondInput, so the existing
    teardown paths are unaffected. Cameras with tight session limits are the
    constraint here.

Behaviour when the substream is unavailable

  • No frame yet, or newest frame older than 10s: treated as "no motion data" —
    no scoring, no forced state change, no alarm storm while blind.
  • Read failure: the sidecar closes and reconnects with backoff (1s -> 30s),
    which also keepalives the RTSP session so it does not sit in CLOSE-WAIT.

Upgrade note

Because AnalysisSource was previously a no-op, any monitor already set to
Secondary in the database will change behaviour after upgrade: it will begin
opening and analysing the substream. A monitor with a stale or misconfigured
SecondPath will log reconnect errors and report "no motion data" rather than
analysing the primary. Worth a mention in release notes.

Testing

  • Builds clean (cmake --build . --target zmc), no warnings.
  • Dropped CPU load by 80%
  • Works for me.jpg

Limitations

  • Analysis inherits the substream's spatial precision (a D1 frame upscaled to
    primary dimensions is blocky). Zone areas and pixel-count thresholds stay
    valid because the analysis image is at primary resolution.
  • If the substream aspect ratio differs from the primary, the upscale stretches
    it; zones still function.

Comment thread src/zm_monitor.cpp Outdated
// that DetectMotion and the reference image match the primary coordinate
// space and zones need no changes.
secondary_image.Assign(secondary_image_native);
if ((secondary_image.Width() != static_cast<unsigned int>(camera_width)) or

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense to upscale. Only makes sense to downscale. Zone coordinates are percentages now. Motion detect results scan be scaled to match either image.

Also, we should still honour y-image for motion detection.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by that I mean we can use the y-image on the secondary stream for even LESS cpu.

Comment thread src/zm_monitor.cpp Outdated
// full-res primary. When active, the primary is only decoded on demand (for
// live viewers), so DECODING_ALWAYS is downgraded to viewer-driven decoding.
secondary_analysis = (analysis_source == ANALYSIS_SECONDARY) and !second_path.empty();
effective_decoding = decoding;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we are encoding in videostore? This isn't enough. We should honour the setting. If the user wants decoding=ondemand, they can set that. We will just always decode the secondary.

@connortechnology

Copy link
Copy Markdown
Member

THanks for taking this on, we have been slowing working towards it but you have jumped right in, which should force us to move faster.

That being said, the reason it was taking so long is it is a bit more complicated than this initial effort addresses. I have concerns about doing the open and capture outside of the Camera object. Not opposed to it being it's own thread, but I think it should be injecting into the packet queue. I'm not convinced that this implementation will stay synced between the streams. We need to sync and link the two streams, likely in wallclock units instead of pts/dts.

And now I will sic the AI on it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Wires the existing AnalysisSource=Secondary setting to actually analyze a monitor’s configured SecondPath substream, reducing CPU use by avoiding full-resolution primary-stream decoding when no live viewers are present.

Changes:

  • Adds SecondStreamThread to decode the secondary/substream in a sidecar thread and publish the latest analysis-ready frame.
  • Updates Monitor::Analyse() to source motion detection input from the sidecar when AnalysisSource=Secondary, and introduces effective_decoding to make primary decoding viewer-driven in that mode.
  • Extends FFmpeg_Input with an Open(..., AVDictionary**) overload plus a “force software decode” option to support the sidecar’s RTSP/timeouts and non-HW decode needs.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/zm_second_stream_thread.h Declares the sidecar substream decode thread and its mailbox-style image accessors.
src/zm_second_stream_thread.cpp Implements substream open/reconnect/backoff, frame conversion, and mailbox publishing.
src/zm_monitor.h Adds secondary-analysis state, sidecar ownership, and getMotionSourceImage() API.
src/zm_monitor.cpp Routes motion detection through the secondary stream when configured; adjusts decode policy via effective_decoding; manages sidecar lifecycle.
src/zm_ffmpeg_input.h Adds Open(filename, AVDictionary**) and a flag to force software decoding.
src/zm_ffmpeg_input.cpp Plumbs AVDictionary options into avformat_open_input and skips HW codecs when software decode is forced.
src/CMakeLists.txt Adds zm_second_stream_thread.cpp to the build.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/zm_second_stream_thread.h Outdated
Comment on lines +17 to +21
// Decodes a monitor's low-res analysis substream (SecondPath) in its own thread
// and publishes the latest frame as an analysis-ready Image, scaled/converted to
// the primary camera dimensions. Used when AnalysisSource=Secondary so that
// motion detection does not require software-decoding the full-resolution
// primary stream while nobody is watching live.
Comment thread src/zm_second_stream_thread.h Outdated
Image latest_image_;
bool have_image_;
uint64_t sequence_;
SystemTimePoint capture_time_;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to add some further info here: If we use a steady clock, we should start it at capture, and then base timestamps for both streams off that start point.

Comment on lines +34 to +38
Stop(); // Signal any running thread to terminate first
if (thread_.joinable()) thread_.join();
terminate_ = false;
thread_ = std::thread(&SecondStreamThread::Run, this);
}
Comment thread src/zm_second_stream_thread.cpp Outdated
Comment on lines +223 to +227
int dummy[4];
int in_range, out_range, brightness, contrast, saturation;
sws_getColorspaceDetails(convert_context_, reinterpret_cast<int **>(&dummy), &in_range,
reinterpret_cast<int **>(&dummy), &out_range,
&brightness, &contrast, &saturation);
Comment on lines +97 to +101
if (StringToUpper(url.substr(0, 4)) == "RTSP") {
// Bound socket reads so a dead substream is detected (and the thread can be
// joined on shutdown) instead of blocking forever in av_read_frame. The
// RTSP option was renamed stimeout -> timeout in ffmpeg 5.0; set both so the
// bound applies regardless of the linked ffmpeg version.
Comment thread src/zm_ffmpeg_input.cpp
return Open(filepath, nullptr);
}

int FFmpeg_Input::Open(const char *filepath, AVDictionary **options) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could just use AVDictionary **options = nullptr in the declaration to avoid two ::Open functions.

@madcamel

madcamel commented Jul 5, 2026

Copy link
Copy Markdown
Author

Thanks for the review and feedback. I'm just flying by the seat of my pants, I'm not very familiar with zm's non-perl code. To be frank I'm avoiding the packet queue because that would add a lot of complexity and touches a lot of things I don't understand. It's probably over my head.

I will make a decent attempt at addressing the other issues you've raised as time permits, it would be really nice to have this functionality in mainline.

madcamel added a commit to madcamel/zoneminder that referenced this pull request Jul 16, 2026
sws_getColorspaceDetails() writes int* table pointers through its
inv_table/table out-params. The sidecar passed reinterpret_cast<int **>
of an int[4] array, which is not type-safe and only the returned
out_range is actually used afterwards. Pass real int* variables instead.

Addresses a Copilot review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
madcamel added a commit to madcamel/zoneminder that referenced this pull request Jul 16, 2026
Only RTSP-specific read timeouts were set, so Stop()/Join() could hang in
av_read_frame when SecondPath is a non-RTSP network protocol. Set
rw_timeout unconditionally; the rtsp demuxer ignores it (it uses its own
timeout option) while other avio-based protocols honour it.

Addresses a Copilot review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
madcamel added a commit to madcamel/zoneminder that referenced this pull request Jul 16, 2026
After a restart (PrimeCapture reconnect or SecondPath change) the mailbox
kept serving the pre-restart frame. Reset have_image_ under the mutex in
Start() so no stale image is handed out. sequence_ stays monotonic across
restarts so consumers holding last_secondary_sequence still see a reset
frame as fresh rather than stale-equal.

Addresses a Copilot review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
madcamel added a commit to madcamel/zoneminder that referenced this pull request Jul 16, 2026
… options

Replace the two Open(filename) / Open(filename, options) overloads with a
single Open(const char *filename, AVDictionary **options = nullptr). The
no-options overload merely delegated; avformat_open_input already tolerates
a null options pointer. Existing zero-argument-options callers are
unchanged.

Addresses connortechnology's review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
madcamel added a commit to madcamel/zoneminder that referenced this pull request Jul 16, 2026
The header claimed the published Image was scaled/converted to the primary
camera dimensions, but the mailbox now stores the frame at the substream's
native resolution and the consumer rebuilds zones at that size. Update the
SecondStreamThread header comment and the matching getMotionSourceImage
comment to describe the actual contract.

Addresses a Copilot review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
madcamel added a commit to madcamel/zoneminder that referenced this pull request Jul 16, 2026
Remove the effective_decoding member that silently downgraded
DECODING_ALWAYS to on-demand when AnalysisSource=Secondary. That override
broke VideoWriter=Encode (videostore needs the primary decoded
continuously regardless of viewers) and ignored explicit user intent. The
decode-decision sites now use the user's decoding value directly; the
analysis thread still runs off the substream via the existing
(decoding != DECODING_NONE) && !secondary_analysis gate, so motion
detection needs no primary decode when the user picks Ondemand.

PrimeCapture logs an Info suggesting Ondemand when secondary analysis is
paired with Decoding=Always. Update the Decoding help text to explain the
pairing and that Encode recording still requires decoding.

Addresses connortechnology's review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
madcamel added a commit to madcamel/zoneminder that referenced this pull request Jul 16, 2026
The sidecar stamped frames with system_clock and consumers computed age
against system_clock::now(), so an NTP step corrupted the staleness test.
Base both streams' capture times on the steady clock instead: the sidecar
stamps frames with steady_clock, and every ZMPacket now carries a
steady_clock stamp (timestamp_steady), set in all constructors and
re-stamped back-to-back with the system timestamp at both capture stamping
sites. Because the comparison never touches the system clock, it is immune
to NTP steps at any time.

getMotionSourceImage declares the substream stalled only when the packet
under analysis was captured more than kSecondaryStaleThreshold AFTER the
newest substream frame. The test is one-sided on purpose: a frame newer
than the packet just means the analysis thread lags capture (packetqueue
backlog) while the substream is healthy, and analysis pairs with the
freshest frame as before, so backlogged packets still score. The fresh and
stalled verdicts are recomputed from the metadata GetLatestImage returns,
so the scored image and the verdict refer to the same frame even if the
mailbox advanced after PeekLatest. The stall debug message logs the
packet-minus-frame skew that drove the decision.

PeekLatest and GetLatestImage expose the frame's steady capture time and
drop the unused age out-param. The one-sided stall check lives in
zm_secondary_sync.h as a pure inline helper so it can be unit-tested. The
packet copy constructor preserves the original capture stamp.

Full per-packet matching (a frame queue rather than the latest-frame
mailbox) is only meaningful with packetqueue injection, which stays out of
scope for this branch.

Addresses Copilot review comments on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
madcamel added a commit to madcamel/zoneminder that referenced this pull request Jul 16, 2026
Unit-test the one-sided SecondaryFrameStalled helper from
zm_secondary_sync.h with Catch2: frames captured with or slightly before
the packet score, an analysis backlog (frame newer than the packet, even
by minutes) must still score, a dead substream (packet far ahead of a
frozen frame) is rejected, and the threshold boundary is exercised on both
sides. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
madcamel added a commit to madcamel/zoneminder that referenced this pull request Jul 16, 2026
The substream sidecar's OpenInput duplicated FfmpegCamera::OpenFfmpeg's
mapping of the monitor Method setting (rtpMulti/rtpRtsp/rtpRtspHttp/rtpUni)
to the ffmpeg rtsp_transport option. Factor it into a
zm_set_rtsp_transport_method free function in zm_ffmpeg and call it from
both sites. The primary path's behavior is unchanged; site-specific
differences (the sidecar's timeout/stimeout and rw_timeout options) stay at
the call sites. The sidecar now also warns on an unknown method or a failed
option set, matching the primary. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
madcamel and others added 13 commits July 16, 2026 13:28
…Secondary)

Add a per-monitor SecondStreamThread sidecar that decodes the configured
SecondPath substream in its own thread and publishes each frame as a
motion-analysis image. When AnalysisSource=Secondary and a SecondPath is set,
Analyse() scores against the substream instead of the primary, and the primary
is decoded on-demand (viewer-driven) rather than always, so the full-resolution
stream is not software-decoded while nobody is watching live.

- zm_second_stream_thread.{h,cpp}: sidecar owning its own FFmpeg_Input, with
  reconnect backoff (which also keepalives the substream RTSP session) and a
  latest-image mailbox holding the frame at native substream resolution.
- FFmpeg_Input::Open overload taking an AVDictionary so the sidecar can set
  rtsp_transport and a socket read timeout.
- Monitor::getMotionSourceImage() resolves the analysis image (substream mailbox
  vs primary Y/colour) and upscales the substream to camera dimensions only when
  a frame is actually scored, so zones keep working in primary coordinates with
  no changes. effective_decoding downgrades DECODING_ALWAYS to on-demand under
  secondary analysis. Sidecar lifecycle wired into PrimeCapture()/Pause().

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
FFmpeg_Input::Open() selects a hardware decoder whenever get_decoder_data
returns one (VAAPI, etc.) and get_frame() returns the frame without transferring
it to system memory. The substream sidecar then received an AV_PIX_FMT_VAAPI
frame whose data[0] is a GPU surface handle, and ProduceImage() dereferenced it
as a CPU Y plane, segfaulting zmc when VAAPI was enabled.

Add FFmpeg_Input::set_no_hwaccel() to skip hardware decoders (default off, so
other callers are unchanged) and use it in the sidecar: the D1 substream is
cheap to decode on the CPU and this also avoids contending with the primary
over the VAAPI device. ProduceImage() additionally rejects any hardware frame
instead of dereferencing it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ilbox copies

- The RTSP read timeout was set only via `stimeout`, which ffmpeg 5.0 renamed to
  `timeout`; on the target's ffmpeg 7.x the option was ignored, so a stalled
  substream could block av_read_frame indefinitely and hang shutdown in Join().
  Set both option names.
- getMotionSourceImage() called GetLatestImage() every analysed packet, deep
  copying the mailbox image under the mutex even when the frame had not advanced
  and the copy was discarded. Add SecondStreamThread::PeekLatest() to check
  sequence/age cheaply and only copy when the frame will actually be used.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The substream images were built with the Image(w,h,colours,subpix) constructor,
which sets linesize = width*colours (unaligned). But Image::Scale and all of ZM's
swscale paths treat buffers as align-32: SWScale::Convert fills the source frame
with av_image_fill_arrays(..., 32), i.e. stride FFALIGN(width,32). For a width that
is not a multiple of 32 (e.g. a 720-wide D1 substream) the scaler read each row at
the aligned stride while the data was width-packed, drifting every row and shearing
the analysis image into horizontal bands.

Build the images with the align-32 constructor (buffer=nullptr, allocation=0) and
copy the decoded Y plane with av_image_copy_plane, which bridges the decoder's
stride (frame->linesize[0]) to the image's align-32 stride. Applies to both the
YChannel and FullColour paths.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… on teardown

When a camera has no audio stream on its primary input but a SecondPath is
configured, OpenFfmpeg opens the second path via mSecondInput (a
unique_ptr<FFmpeg_Input>) and aliases its AVFormatContext into the base
Camera::mSecondFormatContext for audio muxing. mSecondInput owns that context
and closes it (avformat_close_input) in its destructor, but Camera::~Camera()
also freed the same pointer with avformat_free_context, double-freeing it and
crashing in avformat_free_context -> av_opt_free -> av_opt_next on shutdown.

This surfaced now because AnalysisSource=Secondary requires a SecondPath, which
activates the camera's pre-existing second-input audio path on monitors that
previously had none. Clear the non-owning alias in ~FfmpegCamera() before the
base destructor runs, mirroring the existing 'and !mSecondInput' guard that
already protects mAudioCodecContext in FfmpegCamera::Close().

Reproduced on monitor 4 (segfault on every zmc teardown) and verified fixed
(clean SIGTERM shutdown across repeated teardown tests with the substream
active).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…m resolution

AnalysisSource=Secondary had two coupled defects that tied motion detection to
the on-demand primary decoder, which only runs while a live viewer is watching:

1. Analyse() skipped motion detection unless the primary packet was decoded
   (the `decoding != DECODING_NONE` / `!packet->decoded` gate) and unless
   Ready() was true. Ready() checks `decoding_image_count > ready_count`, and
   decoding_image_count only advances when a primary frame is decoded. So with
   no viewer the primary is never decoded, detection never runs, and the first
   view bootstraps decoding_image_count past ready_count permanently (it is
   monotonic) - leaving analysis latched on afterwards. Net effect: no motion
   detection while unwatched, and a permanent CPU step-up after the first view.

   Fix: bypass the decode wait when secondary_analysis, and advance the warmup
   counter in the non-decoding path so Ready() progresses without a viewer.

2. The substream frame was upscaled to the full camera resolution before every
   Delta/Blend/zone check so that zones (camera coordinates) needed no changes -
   paying full-res analysis cost for a low-res source.

   Fix: detect at the substream's native resolution. Zones are percentage-based,
   so rebuilding them at the substream size (discovered from its first frame)
   scales both geometry and pixel thresholds automatically. Adds Monitor
   analysis_width/height (== camera size for primary analysis) and points
   Zone::Setup/Load/copy at them. Zone alarm-image overlays onto the full-res
   analysis image are guarded for the size mismatch, and the signal-reacquire
   reference-image reseed is skipped in secondary mode.

Verified on monitor 4 (704x480 substream, 2592x1944 camera): detection now runs
with no viewer, idle CPU dropped from ~15-20% (or a broken 7.8% with detection
off) to ~4%, the post-view latch is gone, and teardown stays crash-free.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Hardening (from code review):
- Persist the discovered substream size (substream_width/height) across
  Load()/Reload() so a reload does not reset the analysis resolution and force a
  redundant zone rebuild plus a reference-image reset (losing the background).
- Set analysis_width/height before Zone::Load in the switch (Zone::Load builds
  zones at AnalysisWidth()/Height()), and if the reload returns empty while zones
  still exist, revert and retry next frame instead of wiping all zones - a
  transient DB error would otherwise silently disable motion detection.
- Drop the now-unused secondary_image (full-res upscale) member.
sws_getColorspaceDetails() writes int* table pointers through its
inv_table/table out-params. The sidecar passed reinterpret_cast<int **>
of an int[4] array, which is not type-safe and only the returned
out_range is actually used afterwards. Pass real int* variables instead.

Addresses a Copilot review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Only RTSP-specific read timeouts were set, so Stop()/Join() could hang in
av_read_frame when SecondPath is a non-RTSP network protocol. Set
rw_timeout unconditionally; the rtsp demuxer ignores it (it uses its own
timeout option) while other avio-based protocols honour it.

Addresses a Copilot review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
After a restart (PrimeCapture reconnect or SecondPath change) the mailbox
kept serving the pre-restart frame. Reset have_image_ under the mutex in
Start() so no stale image is handed out. sequence_ stays monotonic across
restarts so consumers holding last_secondary_sequence still see a reset
frame as fresh rather than stale-equal.

Addresses a Copilot review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… options

Replace the two Open(filename) / Open(filename, options) overloads with a
single Open(const char *filename, AVDictionary **options = nullptr). The
no-options overload merely delegated; avformat_open_input already tolerates
a null options pointer. Existing zero-argument-options callers are
unchanged.

Addresses connortechnology's review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The header claimed the published Image was scaled/converted to the primary
camera dimensions, but the mailbox now stores the frame at the substream's
native resolution and the consumer rebuilds zones at that size. Update the
SecondStreamThread header comment and the matching getMotionSourceImage
comment to describe the actual contract.

Addresses a Copilot review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Remove the effective_decoding member that silently downgraded
DECODING_ALWAYS to on-demand when AnalysisSource=Secondary. That override
broke VideoWriter=Encode (videostore needs the primary decoded
continuously regardless of viewers) and ignored explicit user intent. The
decode-decision sites now use the user's decoding value directly; the
analysis thread still runs off the substream via the existing
(decoding != DECODING_NONE) && !secondary_analysis gate, so motion
detection needs no primary decode when the user picks Ondemand.

PrimeCapture logs an Info suggesting Ondemand when secondary analysis is
paired with Decoding=Always. Update the Decoding help text to explain the
pairing and that Encode recording still requires decoding.

Addresses connortechnology's review comment on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The sidecar stamped frames with system_clock and consumers computed age
against system_clock::now(), so an NTP step corrupted the staleness test.
Base both streams' capture times on the steady clock instead: the sidecar
stamps frames with steady_clock, and every ZMPacket now carries a
steady_clock stamp (timestamp_steady), set in all constructors and
re-stamped back-to-back with the system timestamp at both capture stamping
sites. Because the comparison never touches the system clock, it is immune
to NTP steps at any time.

getMotionSourceImage declares the substream stalled only when the packet
under analysis was captured more than kSecondaryStaleThreshold AFTER the
newest substream frame. The test is one-sided on purpose: a frame newer
than the packet just means the analysis thread lags capture (packetqueue
backlog) while the substream is healthy, and analysis pairs with the
freshest frame as before, so backlogged packets still score. The fresh and
stalled verdicts are recomputed from the metadata GetLatestImage returns,
so the scored image and the verdict refer to the same frame even if the
mailbox advanced after PeekLatest. The stall debug message logs the
packet-minus-frame skew that drove the decision.

PeekLatest and GetLatestImage expose the frame's steady capture time and
drop the unused age out-param. The one-sided stall check lives in
zm_secondary_sync.h as a pure inline helper so it can be unit-tested. The
packet copy constructor preserves the original capture stamp.

Full per-packet matching (a frame queue rather than the latest-frame
mailbox) is only meaningful with packetqueue injection, which stays out of
scope for this branch.

Addresses Copilot review comments on PR ZoneMinder#4965. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
madcamel and others added 2 commits July 16, 2026 13:28
Unit-test the one-sided SecondaryFrameStalled helper from
zm_secondary_sync.h with Catch2: frames captured with or slightly before
the packet score, an analysis backlog (frame newer than the packet, even
by minutes) must still score, a dead substream (packet far ahead of a
frozen frame) is rejected, and the threshold boundary is exercised on both
sides. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The substream sidecar's OpenInput duplicated FfmpegCamera::OpenFfmpeg's
mapping of the monitor Method setting (rtpMulti/rtpRtsp/rtpRtspHttp/rtpUni)
to the ffmpeg rtsp_transport option. Factor it into a
zm_set_rtsp_transport_method free function in zm_ffmpeg and call it from
both sites. The primary path's behavior is unchanged; site-specific
differences (the sidecar's timeout/stimeout and rw_timeout options) stay at
the call sites. The sidecar now also warns on an unknown method or a failed
option set, matching the primary. refs ZoneMinder#4965

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@madcamel

Copy link
Copy Markdown
Author

I've addressed many of the issues in your code review. Admittedly 90% vibe coded. Keeping the extra monotonic timestamps is somewhat janky but it works. Because it's not using the packet queue I've tried to keep this more a bolt-on than a fully integrated solution.

I have been running this on my instance for a couple days now and it seems to be stable and perform correctly.

@connortechnology

Copy link
Copy Markdown
Member

THere is a lot in here that I like, and I will likely cherry-pick some of it to reduce the size.

I like having a second thread doing capture of the second stream, and my code in zm_ffmpeg_camera is kindof a hack that i would like to replace with your approach. But I really feel we need to be injecting from both capture threads into the packet queue sorted by wall clock. I'm not sure we need the steady clock stuff, I think the existing system clock stuff will do. Maybe claude can defend itself on that.

Anyways, those are some thoughts, I will look it over and think about it some more.

@madcamel

Copy link
Copy Markdown
Author

Thanks again for looking it over. The work you do on ZM is very appreciated. It may not be modern and shiny but it's extremely reliable. Even if this code is not up to your standards I hope something can be gained from it!

@IgorA100

Copy link
Copy Markdown
Contributor

I'm following this PR, and it would be great if the existing AnalysisFPSLimit parameter was finally fully implemented. This would further reduce CPU usage. Motion analysis is probably possible with 5 FPS, and if the camera delivers 25 FPS, the gains would be significant. For example, for live viewing of the second stream, a high FPS is desirable, but for analysis, a significantly lower FPS is desirable.

@madcamel

Copy link
Copy Markdown
Author

The sidecar should already be honoring AnalysisFPSLimit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants