fix: mp4_writer close sequence, start_time correction, external motion trigger for UDT slave streams

[origin2000]

Fix 1:

mp4_writer_close() correct close sequence (DB update before av_write_trailer)

Problem

When a detection or motion recording was finalized, the database record was updated
with end_time, file size, and is_complete = true before the MP4 container was
fully written to disk.

This caused two concrete defects:

1. Inaccurate duration in the database. The old helper get_recording_end_time()
   called avformat_open_input() on a file that was still open for writing. FFmpeg
   either failed to read the duration or returned 0, so end_time was computed as
   start_time + 0 — effectively making every detection clip appear to have zero or
   wrong duration in the recordings list.

2. Potential file corruption risk. Marking a recording as is_complete = true
   while the AVFormatContext was still open meant that any consumer (UI, storage
   manager) could act on an incomplete file.

Root Cause

The original mp4_writer_close() executed in this order:

1. DB update (end_time, size, is_complete=true)   ← file not yet closed
2. Stop recording thread
3. av_write_trailer()
4. avio_closep()
5. avformat_free_context()
6. free(writer)

get_recording_end_time() was called as part of step 1 and tried to probe the
still-open file with avformat_open_input(), producing unreliable results.

Fix

The close sequence is reordered so the file is fully finalized on disk before
any database or cache update:

1. Stop recording thread          ← no more packets written
2. av_write_trailer()             ← MP4 container complete
3. avio_closep()                  ← file handle released
4. avformat_free_context()        ← FFmpeg resources freed
5. stat() → file size             ← accurate size now readable
6. get_mp4_file_duration_seconds() ← probe closed file for real duration
7. DB update (end_time, size, is_complete=true)
8. storage cache update
9. Destroy mutexes, free writer

A new helper get_mp4_file_duration_seconds(const char *path) is introduced in
mp4_writer_core.c (declared in mp4_writer.h) to encapsulate the
avformat_open_input / avformat_find_stream_info probe on the closed file.
The old get_recording_end_time() static function is removed.

Additionally, mp4_writer_thread.c now resets writer->creation_time to
current_time on each segment rotation, so that the new segment's duration is
measured from the correct baseline rather than from the original writer creation time.

Files Changed

File	Change
src/video/mp4_writer_core.c	Reordered close sequence; replaced get_recording_end_time() with get_mp4_file_duration_seconds(); added #include <errno.h>
include/video/mp4_writer.h	Added get_mp4_file_duration_seconds() declaration; added bool start_time_corrected field to mp4_writer_t
src/video/mp4_writer_thread.c	Reset creation_time and start_time_corrected on segment rotation

---

Fix 2:

Correct start_time in database after pre-buffer flush for detection recordings

Problem

When a detection recording is started, the database record is created immediately
via add_recording_metadata() using time(NULL) as start_time. However, the
Unified Detection Thread (UDT) then flushes a circular pre-event buffer into the
MP4 file — this buffer can contain several seconds of footage that precede the
detection event.

As a result, the start_time stored in the database was consistently later than
the actual first frame in the file, by as much as pre_buffer_seconds. This
caused:

• Incorrect clip durations in the recordings list
• Timeline display misalignment in the web UI (clips appear shifted forward)
• The pre-buffer footage was present in the file but invisible/inaccessible via the
  recorded time range shown in the UI

Root Cause

udt_start_recording() called add_recording_metadata() with start_time = now
(the moment the detection triggered), before flush_prebuffer_to_recording()
was called. The pre-buffer flush writes packets that are up to pre_buffer_seconds
older than now, but the database was never updated to reflect this earlier start.

Fix

After flush_prebuffer_to_recording() completes, the actual pre-buffer duration
is read via packet_buffer_get_stats(). If the duration is non-zero, a new
database function update_recording_start_time() is called to correct the stored
start_time to now - pre_buffer_duration. The mp4_writer_t::creation_time
field is updated in parallel so that the writer's own duration tracking stays
consistent.

A new boolean field mp4_writer_t::start_time_corrected is set after the first
correction to prevent a redundant update on segment rotation.

// After flush_prebuffer_to_recording():
if (!ctx->mp4_writer->start_time_corrected && pre_dur > 0 &&
    ctx->current_recording_id > 0) {
    time_t corrected = now - (time_t)pre_dur;
    ctx->mp4_writer->creation_time = corrected;
    ctx->mp4_writer->start_time_corrected = true;
    update_recording_start_time(ctx->current_recording_id, corrected);
}

New API

// include/database/db_recordings.h
int update_recording_start_time(uint64_t id, time_t start_time);

Executes UPDATE recordings SET start_time = ? WHERE id = ? — a minimal,
targeted correction that does not touch any other fields.

Files Changed

File	Change
include/database/db_recordings.h	Added update_recording_start_time() declaration
src/database/db_recordings.c	Implemented update_recording_start_time()
include/video/mp4_writer.h	Added bool start_time_corrected field to mp4_writer_t
src/video/unified_detection_thread.c	Call update_recording_start_time() after pre-buffer flush in both local detection and external trigger paths
src/video/mp4_segment_recorder.c	Added explanatory comment at started_cb invocation

---

Fix 3:

Propagate ONVIF motion events to Unified Detection Thread-managed slave streams

Problem

LightNVR supports cross-stream motion triggering via motion_trigger_source: when
stream A detects motion via ONVIF, the event is propagated to any stream B that
lists A as its trigger source. This is the intended architecture for multi-lens
cameras where one lens provides ONVIF motion events and another does not.

However, the propagation in process_motion_event() pushed the event only onto
the ONVIF motion event queue. If the slave stream was configured to use the
Unified Detection Thread (UDT) — which is the correct and only viable architecture
for a stream without its own ONVIF profile — the UDT thread never received the
event. The slave stream therefore never started recording, silently dropping all
motion-triggered recordings.

Root Cause

The ONVIF event queue (push_event()) is consumed exclusively by
event_processor_thread_func() in onvif_motion_recording.c. The Unified
Detection Thread reads from a completely separate code path and has no mechanism
to receive events from the ONVIF queue. Sending an ONVIF queue event to a
UDT-managed stream was silently ignored.

Affected Camera Configurations

This bug affects any multi-lens camera that:

1. Exposes multiple physical lenses as separate RTSP streams
2. Provides only a single ONVIF endpoint shared across all streams
3. Reports identical VideoSourceConfigurationToken values for all profiles,
   making ONVIF-level stream distinction impossible
4. Makes secondary streams accessible only via proprietary RTSP paths, with no
   independent ONVIF profile

In this configuration, the correct LightNVR setup is:

• Primary stream: ONVIF subscription for motion detection (onvif model)
• Secondary stream(s): UDT-managed RTSP, triggered via motion_trigger_source

This is a documented limitation of several manufacturers' ONVIF implementations
and is not specific to any single device. Tested with TP-Link C545D (dual-lens,
wide + PTZ).

Fix

A second propagation path is added alongside the existing ONVIF queue path:

For each slave stream with motion_trigger_source == current stream:
  Path A: push_event()                         ← ONVIF-managed slaves (unchanged)
  Path B: unified_detection_notify_motion()    ← UDT-managed slaves (new)

Both paths are always attempted for every matching slave stream. Whichever system
is not managing that stream silently ignores the call — there is no double-trigger
risk for streams managed by either system exclusively.

unified_detection_notify_motion()

A new public function in unified_detection_thread.c:

void unified_detection_notify_motion(const char *stream_name, bool motion_active);

It writes to a new atomic_int external_motion_trigger field on the UDT context:

• 1 = motion active (start / keep-alive)
• 2 = motion ended
• 0 = idle (initial state / reset by UDT thread after processing)

The write is intentionally non-blocking — it only sets the atomic flag and returns
immediately, making it safe to call from the ONVIF event processor thread without
any locking concerns.

UDT main loop

On each keyframe, the UDT thread checks external_motion_trigger via
atomic_exchange(..., 0) (consuming the flag atomically):

• Value 1 (motion active):
  • If UDT_STATE_BUFFERING: start recording, flush pre-buffer, correct start_time
  • If UDT_STATE_POST_BUFFER: return to UDT_STATE_RECORDING
  • If UDT_STATE_RECORDING: refresh last_detection_time (extends recording)
• Value 2 (motion ended):
  • If UDT_STATE_RECORDING: transition to UDT_STATE_POST_BUFFER

This mirrors the behaviour of a locally-triggered detection event, including the
pre-buffer flush and start_time correction (see related PR).

Files Changed

File	Change
include/video/unified_detection_thread.h	Added atomic_int external_motion_trigger field to unified_detection_ctx_t; declared unified_detection_notify_motion()
src/video/unified_detection_thread.c	Implemented unified_detection_notify_motion(); added external trigger polling in UDT main loop; initialized external_motion_trigger to 0 in context setup
src/video/onvif_motion_recording.c	Added #include "video/unified_detection_thread.h"; added Path B (unified_detection_notify_motion()) call in process_motion_event() propagation loop

---

[origin2000]

@matteius : sorry for the rather comprehensive PR. This my first github thingy. have now spent couple of days wading through the sources. finally decided to buy Claude for help. Tested this in my environment positively the last 24hours, hope I didn't create any side effects

[Copilot]

Pull request overview

This PR fixes recording metadata accuracy and adds support for propagating ONVIF motion events to streams managed by the Unified Detection Thread (UDT), with additional start/end time corrections for MP4 segments.

Changes:

• Reorders mp4_writer_close() to finalize the MP4 file before probing duration and updating DB metadata; adds get_mp4_file_duration_seconds().
• Adds an external-motion propagation path from ONVIF motion recording to UDT-managed “slave” streams via an atomic trigger.
• Introduces DB support for correcting recordings.start_time after pre-buffer flush; resets writer timing state on segment rotation.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/video/unified_detection_thread.c	Adds external motion trigger handling and start_time correction after pre-buffer flush.
include/video/unified_detection_thread.h	Adds external_motion_trigger field + API for external motion notification.
src/video/onvif_motion_recording.c	Propagates motion events to UDT-managed slave streams.
src/video/mp4_writer_core.c	Reorders close sequence; probes finalized file duration; updates DB after finalization.
include/video/mp4_writer.h	Adds start_time_corrected field and duration-probe helper declaration.
src/video/mp4_writer_thread.c	Resets creation_time / start_time_corrected on rotation for segment timing.
src/video/mp4_segment_recorder.c	Clarifies comment about DB entry creation aligning to first keyframe.
src/database/db_recordings.c	Adds update_recording_start_time() to correct start_time post pre-buffer flush.
include/database/db_recordings.h	Declares update_recording_start_time().

---

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

[Copilot]

External motion triggers are only processed inside if (is_keyframe && current_state != UDT_STATE_POST_BUFFER), which means a motion "keep-alive" received while already in POST_BUFFER will never be consumed. This makes the ext_trigger == 1 && current_state == UDT_STATE_POST_BUFFER branch unreachable and can let a recording stop even though external motion is still active. Consider handling external_motion_trigger regardless of POST_BUFFER state (or move the exchange above the state guard) so POST_BUFFER can be extended/returned to RECORDING as intended.

[origin2000]

Confirmed and fixed. The atomic_exchange has been moved outside the != UDT_STATE_POST_BUFFER guard into its own if (is_keyframe) block. The POST_BUFFER → RECORDING branch on ext_trigger == 1 is now reachable. A note in the comment explicitly marks it as "previously unreachable".

[Copilot]

The comments say the UDT thread reads external_motion_trigger "on each packet iteration", but the implementation currently checks it only when is_keyframe is true. Either update the documentation/comments to match the keyframe-driven behavior or move the check outside the keyframe condition if you intend it to react on any packet boundary.

[origin2000]

Accepted and applied. Comment updated in unified_detection_thread.h to reflect the actual keyframe-driven polling behavior. Per-packet handling is intentionally not used — keyframes are the natural trigger point for detection decisions.

[Copilot]

The struct comment says the UDT main loop polls external_motion_trigger "on every packet", but process_packet() currently consumes it only on video keyframes. Please align this comment with the actual behavior (keyframe-driven polling) or adjust the code if the intent is truly per-packet handling.

Suggested change

	// The UDT main loop polls this flag on every packet and treats a rising edge as
	// equivalent to a local detection event (starts/extends recording).
	// The UDT processing path checks this flag when handling eligible video
	// keyframes and treats a rising edge as equivalent to a local detection
	// event (starts/extends recording).

[origin2000]

Accepted and applied. Comment updated in unified_detection_thread.h to reflect the actual keyframe-driven polling behavior. Per-packet handling is intentionally not used — keyframes are the natural trigger point for detection decisions. Thank you for the precise suggestion.

[Copilot]

mp4_writer_stop_recording_thread() can detach the recording thread on timeout, leaving it running with a live tctx->writer pointer. mp4_writer_close() then continues and eventually free(writer), which can cause a use-after-free if the detached thread is still executing. Consider making mp4_writer_stop_recording_thread() report whether the thread was joined vs detached and, if detached, avoid freeing/closing writer resources (or block until exit) to keep the writer alive as long as the thread may access it.

[Copilot]

mp4_writer_close() derives end_time as writer->creation_time + duration. This assumes creation_time matches the DB start_time for current_recording_id, but in the continuous writer thread the DB start_time is set in on_segment_started_cb() (first keyframe) and creation_time is currently set at rotation time, which can differ by up to a GOP interval. To keep end_time accurate, consider baselining off the DB start_time (fetch metadata) or set writer->creation_time to the same timestamp used when the DB row is created.

Suggested change

	/* Derive end_time from the MP4 container duration (most accurate). *
	* Fall back to wall-clock time(NULL) if the file cannot be read. */
	time_t end_time = time(NULL);
	double dur = get_mp4_file_duration_seconds(writer->output_path);
	if (dur >= 0.0) {
	end_time = writer->creation_time + (time_t)dur;
	log_info("Actual MP4 duration for %s: %.1f s → end_time=%ld",
	writer->output_path, dur, (long)end_time);
	} else {
	log_warn("Could not read duration from '%s', using wall-clock end_time",
	writer->output_path);
	/* Use the finalized file timestamp as the end_time baseline instead of
	* deriving it from writer->creation_time, which may not match the DB
	* start_time for this recording. Fall back to wall-clock time(NULL)
	* if stat() did not succeed. Read MP4 duration for logging only. */
	time_t end_time = (size_bytes > 0 || stat(writer->output_path, &st) == 0)
	? st.st_mtime
	: time(NULL);
	double dur = get_mp4_file_duration_seconds(writer->output_path);
	if (dur >= 0.0) {
	log_info("Actual MP4 duration for %s: %.1f s; using file end_time=%ld",
	writer->output_path, dur, (long)end_time);
	} else {
	log_warn("Could not read duration from '%s', using file/wall-clock end_time=%ld",
	writer->output_path, (long)end_time);

[origin2000]

Accepted with a refinement. The observation is correct — creation_time is set at rotation time, while the DB start_time is recorded at the first keyframe, so creation_time + dur carries a potential GOP-interval drift.
We adopt the st.st_mtime baseline as suggested, but track stat_ok explicitly as a bool rather than reusing the size_bytes > 0 heuristic from Copilot's snippet — that heuristic would leave st uninitialized if both stat() calls failed. With stat_ok the logic is unambiguous: end_time = stat_ok ? st.st_mtime : time(NULL).
get_mp4_file_duration_seconds() is retained for diagnostic logging — it confirms the encoded duration matches expectations without influencing end_time.

[Copilot]

On rotation, creation_time is reset to current_time, but the DB start_time for the new segment is created later in on_segment_started_cb() when the first keyframe arrives. If mp4_writer_close() uses creation_time as the baseline for end_time, this can still skew durations by the keyframe wait time. Consider updating writer->creation_time inside on_segment_started_cb() to match the DB start_time (or stop using creation_time for DB end_time calculation).

[origin2000]

This concern is resolved by the fix applied in response to the previous comment. mp4_writer_close() now derives end_time from st.st_mtime (the file's last-modified timestamp after avio_closep) rather than creation_time + dur, so the keyframe-wait skew between rotation time and on_segment_started_cb() no longer affects end_time accuracy.
The creation_time = current_time assignment on rotation is retained for diagnostics; its comment has been updated to reflect that it is no longer the end_time baseline. The start_time_corrected reset remains necessary for the pre-buffer start_time correction path in the UDT.