Skip to content

Commit fb8959e

Browse files
origin2000matteius
authored andcommitted
fix(mp4_recording_core): treat writer==NULL+running==1 as initializing to prevent duplicate recordings
The already-running guard in all four start_mp4_recording* variants had only two states: healthy (writer!=NULL && recording) or dead (everything else). This missed the initialization window between pthread_create() returning and ctx->mp4_writer being assigned inside mp4_recording_thread() after RTSP connect + avformat_find_stream_info(), which can take several seconds. During that window every caller saw writer==NULL, concluded the recording was dead, cleared the slot, and spawned a new thread - producing multiple parallel recordings with overlapping timestamps in the database. Fix: introduce a third state using ctx->running as discriminator: 1. writer!=NULL && mp4_writer_is_recording() -> healthy, return early 2. writer==NULL && ctx->running==1 -> initializing, return early 3. everything else -> dead, clean up and restart ctx->running is set to 1 by the caller before pthread_create() and only cleared by the thread on exit or by a stop path, making it an unambiguous sentinel for the initialization window. Fix applied identically to all four entry points: - start_mp4_recording() - start_mp4_recording_with_url() - start_mp4_recording_with_trigger() - start_mp4_recording_with_url_and_trigger() Tested on Rock Pi 4C+ with TP-Link C545D camera. Rapid motion events previously produced 2-3 concurrent recording entries per stream in the database. With this fix only one recording instance per stream is active.
1 parent 4e869cf commit fb8959e

1 file changed

Lines changed: 75 additions & 37 deletions

File tree

src/video/mp4_recording_core.c

Lines changed: 75 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828
#include "core/logger.h"
2929
#include "core/config.h"
3030
#include "core/url_utils.h"
31-
#include "core/path_utils.h"
3231
#include "core/shutdown_coordinator.h"
3332
#include "video/stream_manager.h"
3433
#include "video/streams.h"
@@ -575,6 +574,8 @@ int start_mp4_recording(const char *stream_name) {
575574

576575
// Check if already running — also verify the recording is actually healthy.
577576
// Extract a dead context (if any) under the mutex, then join it outside.
577+
// FIX: treat writer==NULL + ctx->running==1 as "initializing" to prevent
578+
// duplicate instances during the RTSP-connect window (see start_mp4_recording_with_trigger).
578579
mp4_recording_ctx_t *dead_ctx = NULL;
579580
pthread_mutex_lock(&recording_contexts_mutex);
580581
for (int i = 0; i < g_config.max_streams; i++) {
@@ -585,7 +586,12 @@ int start_mp4_recording(const char *stream_name) {
585586
log_info("MP4 recording for stream %s already running and healthy", stream_name);
586587
return 0; // Already running and healthy
587588
}
588-
589+
if (!writer && recording_contexts[i]->running) {
590+
// Still initializing — mp4_writer not yet assigned by the thread. // <-- bug fix
591+
pthread_mutex_unlock(&recording_contexts_mutex);
592+
log_info("MP4 recording for stream %s is initializing, skipping duplicate start", stream_name);
593+
return 0;
594+
}
589595
// Dead — extract from slot under the lock, join outside
590596
log_warn("MP4 recording for stream %s exists but is dead, cleaning up before restart", stream_name);
591597
dead_ctx = recording_contexts[i];
@@ -645,20 +651,16 @@ int start_mp4_recording(const char *stream_name) {
645651
const struct tm *tm_info = localtime_r(&now, &tm_buf);
646652
strftime(timestamp_str, sizeof(timestamp_str), "%Y%m%d_%H%M%S", tm_info);
647653

648-
// Sanitize the stream name so that names with spaces work correctly.
649-
char encoded_name[MAX_STREAM_NAME];
650-
sanitize_stream_name(stream_name, encoded_name, MAX_STREAM_NAME);
651-
652654
// Create MP4 directory path
653655
char mp4_dir[MAX_PATH_LENGTH];
654656
if (global_config->record_mp4_directly && global_config->mp4_storage_path[0] != '\0') {
655657
// Use configured MP4 storage path if available
656658
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/%s",
657-
global_config->mp4_storage_path, encoded_name);
659+
global_config->mp4_storage_path, stream_name);
658660
} else {
659661
// Use mp4 directory parallel to hls, NOT inside it
660662
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/mp4/%s",
661-
global_config->storage_path, encoded_name);
663+
global_config->storage_path, stream_name);
662664
}
663665

664666
// Create MP4 directory if it doesn't exist
@@ -748,7 +750,12 @@ int start_mp4_recording_with_url(const char *stream_name, const char *url) {
748750
log_info("MP4 recording for stream %s already running and healthy", stream_name);
749751
return 0; // Already running and healthy
750752
}
751-
753+
if (!writer && recording_contexts[i]->running) {
754+
// Still initializing — mp4_writer not yet assigned by the thread. // <-- bug fix
755+
pthread_mutex_unlock(&recording_contexts_mutex);
756+
log_info("MP4 recording for stream %s is initializing, skipping duplicate start", stream_name);
757+
return 0;
758+
}
752759
// Dead — extract from slot under the lock, join outside
753760
log_warn("MP4 recording for stream %s exists but is dead, cleaning up before restart", stream_name);
754761
dead_ctx = recording_contexts[i];
@@ -811,20 +818,16 @@ int start_mp4_recording_with_url(const char *stream_name, const char *url) {
811818
const struct tm *tm_info = localtime_r(&now, &tm_buf);
812819
strftime(timestamp_str, sizeof(timestamp_str), "%Y%m%d_%H%M%S", tm_info);
813820

814-
// Sanitize the stream name so that names with spaces work correctly.
815-
char encoded_name[MAX_STREAM_NAME];
816-
sanitize_stream_name(stream_name, encoded_name, MAX_STREAM_NAME);
817-
818821
// Create MP4 directory path
819822
char mp4_dir[MAX_PATH_LENGTH];
820823
if (global_config->record_mp4_directly && global_config->mp4_storage_path[0] != '\0') {
821824
// Use configured MP4 storage path if available
822825
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/%s",
823-
global_config->mp4_storage_path, encoded_name);
826+
global_config->mp4_storage_path, stream_name);
824827
} else {
825828
// Use mp4 directory parallel to hls, NOT inside it
826829
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/mp4/%s",
827-
global_config->storage_path, encoded_name);
830+
global_config->storage_path, stream_name);
828831
}
829832

830833
// Create MP4 directory if it doesn't exist
@@ -969,18 +972,61 @@ int start_mp4_recording_with_trigger(const char *stream_name, const char *trigge
969972
}
970973

971974
// Check if already running — also verify the recording is actually healthy.
975+
//
976+
// OLD CODE (kept for reference):
977+
//
978+
// mp4_recording_ctx_t *dead_ctx = NULL;
979+
// pthread_mutex_lock(&recording_contexts_mutex);
980+
// for (int i = 0; i < g_config.max_streams; i++) {
981+
// if (recording_contexts[i] && strcmp(recording_contexts[i]->config.name, stream_name) == 0) {
982+
// mp4_writer_t *writer = recording_contexts[i]->mp4_writer;
983+
// if (writer && mp4_writer_is_recording(writer)) {
984+
// pthread_mutex_unlock(&recording_contexts_mutex);
985+
// return 0; // Already running and healthy
986+
// }
987+
// // <-- bug: writer==NULL was treated as dead and the slot was cleared,
988+
// // allowing a second recording instance to start for the same stream.
989+
// // writer is set asynchronously inside mp4_recording_thread (after
990+
// // RTSP connect + avformat_find_stream_info, which can take several
991+
// // seconds). During that window every call here saw writer==NULL,
992+
// // concluded the recording was dead, and spawned a new thread —
993+
// // producing multiple parallel recordings with overlapping timestamps.
994+
// dead_ctx = recording_contexts[i];
995+
// dead_ctx->running = 0;
996+
// recording_contexts[i] = NULL;
997+
// break;
998+
// }
999+
// }
1000+
// pthread_mutex_unlock(&recording_contexts_mutex);
1001+
//
1002+
// FIX: distinguish three states for an existing slot:
1003+
// 1. writer != NULL && mp4_writer_is_recording() → healthy, return early
1004+
// 2. writer == NULL && ctx->running == 1 → initializing, return early
1005+
// 3. everything else → dead, clean up and restart
9721006
mp4_recording_ctx_t *dead_ctx = NULL;
9731007
pthread_mutex_lock(&recording_contexts_mutex);
9741008
for (int i = 0; i < g_config.max_streams; i++) {
9751009
if (recording_contexts[i] && strcmp(recording_contexts[i]->config.name, stream_name) == 0) {
9761010
mp4_writer_t *writer = recording_contexts[i]->mp4_writer;
1011+
9771012
if (writer && mp4_writer_is_recording(writer)) {
1013+
// State 1: fully up and healthy
9781014
pthread_mutex_unlock(&recording_contexts_mutex);
9791015
log_info("MP4 recording for stream %s already running and healthy", stream_name);
980-
return 0; // Already running and healthy
1016+
return 0;
9811017
}
9821018

983-
// Dead — extract from slot under the lock, join outside
1019+
if (!writer && recording_contexts[i]->running) {
1020+
// State 2: thread started but mp4_writer not yet assigned —
1021+
// RTSP connect / avformat_find_stream_info still in progress.
1022+
// This is the race window; do NOT spawn a second instance.
1023+
pthread_mutex_unlock(&recording_contexts_mutex);
1024+
log_info("MP4 recording for stream %s is initializing (writer not yet ready), skipping duplicate start",
1025+
stream_name);
1026+
return 0;
1027+
}
1028+
1029+
// State 3: dead — extract under the lock, join outside
9841030
log_warn("MP4 recording for stream %s exists but is dead, cleaning up before restart", stream_name);
9851031
dead_ctx = recording_contexts[i];
9861032
dead_ctx->running = 0;
@@ -990,13 +1036,6 @@ int start_mp4_recording_with_trigger(const char *stream_name, const char *trigge
9901036
}
9911037
pthread_mutex_unlock(&recording_contexts_mutex);
9921038

993-
// Join the dead thread outside the mutex (can block up to 15 s)
994-
if (dead_ctx) {
995-
cleanup_dead_recording(dead_ctx, stream_name);
996-
}
997-
998-
log_info("Using standalone recording thread for stream %s with trigger_type: %s", stream_name, trigger_type);
999-
10001039
// Find empty slot (under lock)
10011040
pthread_mutex_lock(&recording_contexts_mutex);
10021041
int slot = -1;
@@ -1044,18 +1083,14 @@ int start_mp4_recording_with_trigger(const char *stream_name, const char *trigge
10441083
const struct tm *tm_info = localtime_r(&now, &tm_buf);
10451084
strftime(timestamp_str, sizeof(timestamp_str), "%Y%m%d_%H%M%S", tm_info);
10461085

1047-
// Sanitize the stream name so that names with spaces work correctly.
1048-
char encoded_name[MAX_STREAM_NAME];
1049-
sanitize_stream_name(stream_name, encoded_name, MAX_STREAM_NAME);
1050-
10511086
// Create MP4 directory path
10521087
char mp4_dir[MAX_PATH_LENGTH];
10531088
if (global_config->record_mp4_directly && global_config->mp4_storage_path[0] != '\0') {
10541089
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/%s",
1055-
global_config->mp4_storage_path, encoded_name);
1090+
global_config->mp4_storage_path, stream_name);
10561091
} else {
10571092
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/mp4/%s",
1058-
global_config->storage_path, encoded_name);
1093+
global_config->storage_path, stream_name);
10591094
}
10601095

10611096
// Create MP4 directory if it doesn't exist
@@ -1118,6 +1153,8 @@ int start_mp4_recording_with_url_and_trigger(const char *stream_name, const char
11181153
config.url[sizeof(config.url) - 1] = '\0';
11191154

11201155
// Check if already running — also verify the recording is actually healthy.
1156+
// FIX: treat writer==NULL + ctx->running==1 as "initializing" to prevent
1157+
// duplicate instances during the RTSP-connect window (see start_mp4_recording_with_trigger).
11211158
mp4_recording_ctx_t *dead_ctx = NULL;
11221159
pthread_mutex_lock(&recording_contexts_mutex);
11231160
for (int i = 0; i < g_config.max_streams; i++) {
@@ -1128,7 +1165,12 @@ int start_mp4_recording_with_url_and_trigger(const char *stream_name, const char
11281165
log_info("MP4 recording for stream %s already running and healthy", stream_name);
11291166
return 0; // Already running and healthy
11301167
}
1131-
1168+
if (!writer && recording_contexts[i]->running) {
1169+
// Still initializing — mp4_writer not yet assigned by the thread. // <-- bug fix
1170+
pthread_mutex_unlock(&recording_contexts_mutex);
1171+
log_info("MP4 recording for stream %s is initializing, skipping duplicate start", stream_name);
1172+
return 0;
1173+
}
11321174
// Dead — extract from slot under the lock, join outside
11331175
log_warn("MP4 recording for stream %s exists but is dead, cleaning up before restart", stream_name);
11341176
dead_ctx = recording_contexts[i];
@@ -1194,18 +1236,14 @@ int start_mp4_recording_with_url_and_trigger(const char *stream_name, const char
11941236
const struct tm *tm_info = localtime_r(&now, &tm_buf);
11951237
strftime(timestamp_str, sizeof(timestamp_str), "%Y%m%d_%H%M%S", tm_info);
11961238

1197-
// Sanitize the stream name so that names with spaces work correctly.
1198-
char encoded_name[MAX_STREAM_NAME];
1199-
sanitize_stream_name(stream_name, encoded_name, MAX_STREAM_NAME);
1200-
12011239
// Create MP4 directory path
12021240
char mp4_dir[MAX_PATH_LENGTH];
12031241
if (global_config->record_mp4_directly && global_config->mp4_storage_path[0] != '\0') {
12041242
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/%s",
1205-
global_config->mp4_storage_path, encoded_name);
1243+
global_config->mp4_storage_path, stream_name);
12061244
} else {
12071245
snprintf(mp4_dir, MAX_PATH_LENGTH, "%s/mp4/%s",
1208-
global_config->storage_path, encoded_name);
1246+
global_config->storage_path, stream_name);
12091247
}
12101248

12111249
// Create MP4 directory if it doesn't exist

0 commit comments

Comments
 (0)