fix(audio): stop the audio pump re-taking the lock its caller already holds

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.
 

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
+    $<$<CONFIG:Debug>: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)

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);
 

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 <stdlib.h>
+#include <util/base.h>
+
+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 <errno.h>
 #include <pthread.h>
 #include <stdint.h>
 #include <time.h>
@@ -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

src/audio-buffer.c

@@ -16,6 +16,7 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include <util/base.h>
 #include <util/bmem.h>
 
 #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,

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);

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;