diff --git a/CLAUDE.md b/CLAUDE.md index b5987e1..126282d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -42,6 +42,8 @@ cmake --build build --parallel Output: `build/obs-irl-source.so` (Linux/macOS) or `build/RelWithDebInfo/obs-irl-source.dll` (Windows). +`-DIRL_CHECKED_LOCKS=ON` (also automatic in Debug builds) makes a lock-contract violation abort on the spot, naming the offending file and line, instead of hanging the stream. Worth using whenever you touch the threading model: it is the difference between "OBS froze" and "src/receiver-audio.c:766 took a lock its caller already held". See the header comment in `include/irl-threading.h`. Development only — the check has no recovery path, so it stops the process. + `-DIRL_BUNDLED_FFMPEG=OFF` falls back to linking a system or obs-deps FFmpeg. That path still works for a quick compile check, but it reintroduces the per OBS line binding the bundled stack exists to remove, so it is not what releases use. `scripts/verify-plugin.sh` is not optional polish. It asserts the two properties that make the bundled stack correct and that a successful compile does not prove: that the binary carries no `libav*` dependency, and that it exports nothing but `obs_module_*`. CI runs it (and a `dumpbin` equivalent on Windows) on every build. @@ -104,7 +106,7 @@ Buffer regulation happens through playback speed only, asymmetric like IRLToolki - **Main/OBS thread**: calls create, destroy, update, tick, get_properties, and the activate/deactivate/show/hide callbacks (used only when "Close Stream When Inactive" is on) - **Receiver thread**: owns demux/decode FFmpeg state. Writes to the audio buffer (mutex protected) and pushes decoded video frames (PTS pre-converted to nanoseconds) onto the video queue. Never blocks on GPU or OBS video delivery. - **Video thread**: pops the video queue, does the HW frame transfer, paces each frame to its due time, then converts (owns sws_ctx) and calls `obs_source_output_video`. Queue overflow drops the oldest frame (`video_queue_drops`). The pacing queue it holds those frames in needs no lock — the receiver thread never touches it, and a clear is routed through `video_clear_pending` — but its counters are mirrored under `video_queue_lock` for the stats line. -- **Audio thread**: drains the jitter buffer and submits audio to OBS via `obs_source_output_audio`, paced against the sample counter output clock. Shared timing state is protected by `audio_state_lock` (lock order: `audio_state_lock` before the buffer mutex). +- **Audio thread**: drains the jitter buffer and submits audio to OBS via `obs_source_output_audio`, paced against the sample counter output clock. Shared timing state is protected by `audio_state_lock` (lock order: `audio_state_lock` before the buffer mutex). The thread takes `audio_state_lock` once around the whole of `irl_pump_audio_once`, so nothing reachable from the pump may take it again: the mutex is a plain non-recursive one, and a nested acquire hangs the audio thread and then the video thread queued behind it. Buffer-mutex calls (`audio_buffer_peek_state`, `audio_buffer_fill_ms_locked`, the reads) nest underneath it, which is the documented order. Config fields marked `/* hot */` in `struct irl_config` are written by `irl_source_update` while the worker threads run, so every cross-thread read goes through `os_atomic_load_long` / `os_atomic_load_bool` (not C11 `_Atomic`, which MSVC does not support without an experimental flag). The remaining fields are only written while the threads are stopped, where `irl_thread_create` and `irl_thread_join` supply the happens-before edge. diff --git a/CMakeLists.txt b/CMakeLists.txt index 9c171fb..812d35b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -396,6 +396,30 @@ endif() # ── C standard ─────────────────────────────────────────────── target_compile_features(obs-irl-source PRIVATE c_std_11) +# glibc hides PTHREAD_MUTEX_ERRORCHECK and pthread_mutexattr_settype() behind +# __USE_UNIX98, which a strict -std=c11 build does not define. CMake asks for +# gnu11 by default, so this only bites when C extensions are turned off — pin +# the visibility rather than depend on that default. macOS and the BSDs +# declare both unconditionally, so they need nothing. +if(UNIX AND NOT APPLE) + target_compile_definitions(obs-irl-source PRIVATE _GNU_SOURCE) +endif() + +# ── Checked locks ──────────────────────────────────────────── +# Turn a lock-contract violation (a nested acquire, or an unlock by a thread +# that does not hold the mutex) into an immediate abort naming the offending +# line, instead of a hung stream that only reproduces on POSIX. See the +# header comment in include/irl-threading.h. Costs a mutex type change and +# a branch per lock, so it is a development aid rather than a shipping mode. +option(IRL_CHECKED_LOCKS "Abort on lock-contract violations (dev builds)" OFF) +target_compile_definitions(obs-irl-source PRIVATE + $<$:IRL_CHECKED_LOCKS> +) +if(IRL_CHECKED_LOCKS) + target_compile_definitions(obs-irl-source PRIVATE IRL_CHECKED_LOCKS) + message(STATUS "obs-irl-source: checked locks enabled") +endif() + # ── Compiler warnings ──────────────────────────────────────── if(MSVC) target_compile_options(obs-irl-source PRIVATE /W4 /wd4100) diff --git a/include/audio-buffer.h b/include/audio-buffer.h index 8e2d792..e26d991 100644 --- a/include/audio-buffer.h +++ b/include/audio-buffer.h @@ -73,8 +73,12 @@ struct audio_buffer { * Initialise the buffer. Allocates storage with headroom above `max_ms` so * the configured max is not an audible old-audio drop point. * Call after the first decoded audio frame reveals the stream parameters. + * + * Returns false only if the lock could not be created, in which case the + * buffer is left zeroed and must not be used (a storage allocation failure + * still returns true and leaves an empty, usable ring). */ -void audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, +bool audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, int bytes_per_sample, int target_ms, int min_ms, int max_ms); diff --git a/include/irl-threading.h b/include/irl-threading.h index 367fe4d..5e853ab 100644 --- a/include/irl-threading.h +++ b/include/irl-threading.h @@ -33,6 +33,47 @@ #pragma once +/* ── Checked locks ──────────────────────────────────────────── + * + * Two of the plugin's locks carry contracts the compiler cannot see. The + * audio thread holds audio_state_lock across the whole of + * irl_pump_audio_once(), so nothing the pump reaches may take it again; the + * video thread waits on a condition variable under video_queue_lock, which + * needs that lock held exactly once. Breaking either hangs OBS with no + * diagnostic beyond a stalled stream. + * + * Worse, it does not hang everywhere. CRITICAL_SECTION is recursive, so a + * nested acquire is a no-op on Windows and a deadlock on Linux and macOS — + * the bug ships from a Windows desk and detonates on someone else's machine. + * That is exactly how the audio pump's double lock reached a release. + * + * A checked build turns the hang into an immediate, located abort: POSIX + * mutexes become PTHREAD_MUTEX_ERRORCHECK, where a re-acquire returns + * EDEADLK rather than blocking forever and an unlock by a thread that does + * not hold the mutex returns EPERM; Win32 reads CRITICAL_SECTION's own + * recursion count for the nested-acquire half of the same signal. + * + * Enabled by -DIRL_CHECKED_LOCKS=ON and in Debug builds (see CMakeLists.txt). + * It is a development aid, not a shipping mode: once a lock call has failed + * there is nothing to recover to, because the caller would run on + * unprotected state and its matching unlock would fail in turn. */ +#ifdef IRL_CHECKED_LOCKS + +#include +#include + +static inline void irl_lock_abort(const char *what, int code, const char *file, + int line) +{ + blog(LOG_ERROR, + "[irl-source] Lock contract violated at %s:%d: %s (code %d). " + "See the threading model in CLAUDE.md", + file, line, what, code); + abort(); +} + +#endif /* IRL_CHECKED_LOCKS */ + #ifdef _WIN32 #ifndef WIN32_LEAN_AND_MEAN @@ -71,6 +112,43 @@ static inline void irl_mutex_unlock(irl_mutex_t *m) LeaveCriticalSection(m); } +#ifdef IRL_CHECKED_LOCKS +/* RecursionCount is part of the public RTL_CRITICAL_SECTION layout and is + * incremented by the Enter below, so anything past 1 means this thread was + * already inside. */ +static inline void irl_mutex_lock_checked(irl_mutex_t *m, const char *file, + int line) +{ + EnterCriticalSection(m); + if (m->RecursionCount > 1) { + irl_lock_abort("this thread already holds this lock", + (int)m->RecursionCount, file, line); + } +} + +/* Catches unlocking a section nobody holds. It does not catch unlocking one + * that a *different* thread holds: that needs an owner identity, and the only + * ones available are the undocumented OwningThread encoding or a field of our + * own — which would make sizeof(irl_mutex_t) depend on IRL_CHECKED_LOCKS, + * while the type is embedded by value in struct irl_source and struct + * audio_buffer. A mutex whose layout varies between translation units is the + * bug described at the top of this file; it is not worth re-creating for a + * debug aid. POSIX reports that case as EPERM. + * + * Reading RecursionCount unsynchronised races with other threads, but only + * ever toward a false negative: it cannot read 0 while this thread holds the + * section, so a correct build is never aborted. */ +static inline void irl_mutex_unlock_checked(irl_mutex_t *m, const char *file, + int line) +{ + if (m->RecursionCount == 0) { + irl_lock_abort("unlock of a lock no thread holds", 0, file, + line); + } + LeaveCriticalSection(m); +} +#endif /* IRL_CHECKED_LOCKS */ + static inline int irl_cond_init(irl_cond_t *c) { InitializeConditionVariable(c); @@ -159,6 +237,7 @@ static inline void irl_thread_join(irl_thread_t *t) #else /* !_WIN32 */ +#include #include #include #include @@ -169,7 +248,28 @@ typedef pthread_t irl_thread_t; static inline int irl_mutex_init(irl_mutex_t *m) { +#ifdef IRL_CHECKED_LOCKS + /* No fallback to a default mutex if the attribute cannot be set: a + * checked build that silently produced unchecked mutexes would report + * a clean run while catching nothing. + * + * glibc gates PTHREAD_MUTEX_ERRORCHECK and pthread_mutexattr_settype() + * on __USE_UNIX98, which a strict -std=c11 build does not define. The + * build defines _GNU_SOURCE there (see CMakeLists.txt). */ + pthread_mutexattr_t attr; + int ret = pthread_mutexattr_init(&attr); + if (ret != 0) + return ret; + + ret = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_ERRORCHECK); + if (ret == 0) + ret = pthread_mutex_init(m, &attr); + + pthread_mutexattr_destroy(&attr); + return ret; +#else return pthread_mutex_init(m, NULL); +#endif } static inline void irl_mutex_destroy(irl_mutex_t *m) @@ -187,6 +287,32 @@ static inline void irl_mutex_unlock(irl_mutex_t *m) pthread_mutex_unlock(m); } +#ifdef IRL_CHECKED_LOCKS +static inline void irl_mutex_lock_checked(irl_mutex_t *m, const char *file, + int line) +{ + int ret = pthread_mutex_lock(m); + if (ret == EDEADLK) { + irl_lock_abort("this thread already holds this lock", ret, + file, line); + } else if (ret != 0) { + irl_lock_abort("pthread_mutex_lock failed", ret, file, line); + } +} + +static inline void irl_mutex_unlock_checked(irl_mutex_t *m, const char *file, + int line) +{ + int ret = pthread_mutex_unlock(m); + if (ret == EPERM) { + irl_lock_abort("this thread does not hold this lock", ret, + file, line); + } else if (ret != 0) { + irl_lock_abort("pthread_mutex_unlock failed", ret, file, line); + } +} +#endif /* IRL_CHECKED_LOCKS */ + static inline int irl_cond_init(irl_cond_t *c) { #if defined(__APPLE__) @@ -266,3 +392,12 @@ static inline void irl_thread_join(irl_thread_t *t) } #endif /* _WIN32 */ + +#ifdef IRL_CHECKED_LOCKS +/* Function-like macros rather than checks inside irl_mutex_lock() itself, so + * the abort names the *caller's* file and line — the offending lock/unlock is + * the one thing a contributor needs to see, and it is never in this header. + * Both backends define the _checked helpers above. */ +#define irl_mutex_lock(m) irl_mutex_lock_checked((m), __FILE__, __LINE__) +#define irl_mutex_unlock(m) irl_mutex_unlock_checked((m), __FILE__, __LINE__) +#endif diff --git a/src/audio-buffer.c b/src/audio-buffer.c index 1e8a4a7..c63bc6d 100644 --- a/src/audio-buffer.c +++ b/src/audio-buffer.c @@ -16,6 +16,7 @@ #include #include +#include #include #include "../include/audio-buffer.h" @@ -160,12 +161,26 @@ static void pts_consume(struct audio_buffer *buf, size_t bytes_consumed) /* ── Public API ───────────────────────────────────────────── */ -void audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, +bool audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, int bytes_per_sample, int target_ms, int min_ms, int max_ms) { memset(buf, 0, sizeof(*buf)); + /* Initialise the lock before publishing buf->data: stats readers + * (e.g. proc_handler) gate locking on buf->data != NULL, so the + * mutex must already be live when data becomes visible. + * + * It also has to happen before sample_rate is set, which is what + * audio_buffer_free() reads as its "init ran" marker: leaving the + * struct zeroed here keeps free() from destroying a mutex that was + * never created. */ + if (irl_mutex_init(&buf->lock) != 0) { + blog(LOG_ERROR, + "[irl-source] Failed to create audio buffer lock"); + return false; + } + buf->sample_rate = sample_rate; buf->channels = channels; buf->bytes_per_sample = bytes_per_sample; @@ -174,11 +189,6 @@ void audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, buf->min_ms = min_ms; buf->max_ms = max_ms; - /* Initialise the lock before publishing buf->data: stats readers - * (e.g. proc_handler) gate locking on buf->data != NULL, so the - * mutex must already be live when data becomes visible. */ - irl_mutex_init(&buf->lock); - /* Allocate enough headroom that Max Buffer is not an audible hard * trim point. Old audio is dropped only by explicit recovery paths. */ buf->capacity = ms_to_bytes(buf, max_ms * 4); @@ -187,6 +197,7 @@ void audio_buffer_init(struct audio_buffer *buf, int sample_rate, int channels, buf->data = bzalloc(buf->capacity); if (!buf->data) buf->capacity = 0; + return true; } bool audio_buffer_reconfigure(struct audio_buffer *buf, int sample_rate, diff --git a/src/irl-source.c b/src/irl-source.c index b00eba2..ca7d6ad 100644 --- a/src/irl-source.c +++ b/src/irl-source.c @@ -402,9 +402,32 @@ void *irl_source_create(obs_data_t *settings, obs_source_t *source) struct irl_source *ctx = bzalloc(sizeof(*ctx)); ctx->source = source; ctx->current_speed = 1.0f; - irl_mutex_init(&ctx->audio_state_lock); - irl_mutex_init(&ctx->video_queue_lock); - irl_cond_init(&ctx->video_queue_cond); + /* Bail out rather than run on primitives that were never created: + * every lock/unlock below this point would be undefined behaviour. + * Nothing is registered with libobs yet, so freeing ctx and returning + * NULL is the whole cleanup — libobs logs the failure and never calls + * irl_source_destroy for a create that returned NULL. */ + if (irl_mutex_init(&ctx->audio_state_lock) != 0) { + blog(LOG_ERROR, + "[irl-source] Failed to create audio state lock"); + bfree(ctx); + return NULL; + } + if (irl_mutex_init(&ctx->video_queue_lock) != 0) { + blog(LOG_ERROR, + "[irl-source] Failed to create video queue lock"); + irl_mutex_destroy(&ctx->audio_state_lock); + bfree(ctx); + return NULL; + } + if (irl_cond_init(&ctx->video_queue_cond) != 0) { + blog(LOG_ERROR, + "[irl-source] Failed to create video queue condition variable"); + irl_mutex_destroy(&ctx->video_queue_lock); + irl_mutex_destroy(&ctx->audio_state_lock); + bfree(ctx); + return NULL; + } config_load(&ctx->config, settings); apply_async_audio_mode(ctx); diff --git a/src/receiver-audio.c b/src/receiver-audio.c index 270a7a8..3a58566 100644 --- a/src/receiver-audio.c +++ b/src/receiver-audio.c @@ -637,16 +637,15 @@ static void irl_audio_maybe_reanchor_offset(struct irl_source *ctx, ctx->audio_buf.target_ms) return; - /* Lock order note: fill query above takes and releases the buffer - * mutex on its own; the state lock below is never held across it. */ - irl_mutex_lock(&ctx->audio_state_lock); + /* audio_state_lock is already held (see irl_pump_audio_once). The + * fill query above takes and releases the buffer mutex underneath it, + * which is the documented order (state lock, then buffer mutex). */ ctx->audio_out_anchor_ns = now + chunk_ns; ctx->audio_out_samples = 0; ctx->latest_audio_obs_end_ts_ns = 0; ctx->latest_audio_buffered_end_pts_ns = 0; ctx->audio_playout_offset_baseline_set = false; ctx->audio_conceal_fade_pending = true; - irl_mutex_unlock(&ctx->audio_state_lock); ctx->audio_offset_reanchors++; ctx->audio_quality_events++; @@ -704,6 +703,10 @@ static void audio_check_drain_progress(struct irl_source *ctx, int fill_ms, /* ── Pump ─────────────────────────────────────────────────── */ +/* Caller holds audio_state_lock for the whole call (irl_audio_thread). See + * the declaration in receiver-internal.h: nothing on this path may re-take + * it. Buffer-mutex calls (peek/read/fill) nest underneath it, which is the + * documented lock order. */ bool irl_pump_audio_once(struct irl_source *ctx) { bool low_latency = ctx->config.low_latency_audio; @@ -758,15 +761,13 @@ bool irl_pump_audio_once(struct irl_source *ctx) int chunk_count = 0; bool has_audio = audio_buffer_peek_state(&ctx->audio_buf, &peek, &fill_ms, &chunk_count); - /* The receiver thread reads this for the stats line, so publish it - * under the shared timing lock like the rest of the cross-thread - * state. peek_state released the buffer mutex before returning, so - * nothing is nested here and the documented order (audio_state_lock - * before the buffer mutex) still holds. */ - irl_mutex_lock(&ctx->audio_state_lock); + /* The receiver thread reads this for the stats line; audio_state_lock + * is already held for the whole pump, so the publish is covered. + * peek_state took and released the buffer mutex underneath it, which + * is the documented order (audio_state_lock before the buffer + * mutex). */ if (fill_ms > ctx->audio_fill_peak_ms) ctx->audio_fill_peak_ms = fill_ms; - irl_mutex_unlock(&ctx->audio_state_lock); if (has_audio && maybe_trim_hidden_audio_backlog(ctx, fill_ms, chunk_count)) @@ -927,9 +928,9 @@ void irl_handle_audio_frame(struct irl_source *ctx, AVFrame *frame) &ctx->audio_buf, out_rate, out_channels, bytes_per_sample, target_ms, min_ms, max_ms); } else { - audio_buffer_init(&ctx->audio_buf, out_rate, - out_channels, bytes_per_sample, - target_ms, min_ms, max_ms); + reconfigured = audio_buffer_init( + &ctx->audio_buf, out_rate, out_channels, + bytes_per_sample, target_ms, min_ms, max_ms); } ctx->audio_out_primed = false; ctx->audio_out_anchor_ns = 0; diff --git a/src/receiver-internal.h b/src/receiver-internal.h index 482a754..ca2dad8 100644 --- a/src/receiver-internal.h +++ b/src/receiver-internal.h @@ -13,6 +13,11 @@ void irl_close_ffmpeg(struct irl_source *ctx); void irl_prepare_new_connection(struct irl_source *ctx); bool irl_wait_for_reconnect(struct irl_source *ctx); void irl_handle_stream_read_error(struct irl_source *ctx, int read_ret); +/* Caller must hold audio_state_lock: the pump owns the output clock and the + * playout mapping outright, so it reads and writes them without re-taking the + * lock anywhere below this call. The lock is NOT recursive — an inner + * irl_mutex_lock(&ctx->audio_state_lock) on this path self-deadlocks the + * audio thread. */ bool irl_pump_audio_once(struct irl_source *ctx); void irl_handle_audio_packet(struct irl_source *ctx, AVPacket *pkt, AVFrame *frame); diff --git a/src/receiver.c b/src/receiver.c index 96258a8..18e60df 100644 --- a/src/receiver.c +++ b/src/receiver.c @@ -29,6 +29,11 @@ void *irl_audio_thread(void *data) bool pumped = false; for (int i = 0; i < 16 && os_atomic_load_bool(&ctx->thread_active); i++) { + /* The whole pump runs under audio_state_lock, so + * nothing it calls may take that lock again — the + * mutex is not recursive and a nested acquire hangs + * this thread, and with it the video thread waiting + * behind it. */ irl_mutex_lock(&ctx->audio_state_lock); bool ok = irl_pump_audio_once(ctx); irl_mutex_unlock(&ctx->audio_state_lock);