From a3b3282a14e58e7aab1bda25d179d80dbbbb5c14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jean-Micha=C3=ABl=20Celerier?= Date: Fri, 10 Jul 2026 02:24:32 -0400 Subject: [PATCH] pipewire: hold the thread-loop lock across blocking invokes; buffer param ranges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes found by hardware/daemon round-trip testing of ossia score's PipeWire video path (PipeWire 1.3.0): 1. context::invoke_sync issued pw_loop_invoke(block=true) from foreign threads WITHOUT holding the thread-loop lock. The blocking-invoke path in spa/plugins/support/loop.c unconditionally fires the loop-control hooks around its wait (pw_thread_loop's impl_before/impl_after = unlock/lock), which exist precisely to release the caller's lock while it blocks — i.e. the API contract is that the caller holds the lock. Without it, do_unlock underflows the recurse counter ("'this->recurse > 0' failed" on stderr) and is refused, and impl_after then ACQUIRES the mutex on the way out — the calling thread exits owning the loop mutex forever. The next loop iteration blocks in do_lock and any subsequent pw_thread_loop_stop deadlocks in pthread_join. Proven with an instrumented thread-loop.c (abort on underflow): the guilty stack was invoke_sync <- context::unsubscribe <- subscription dtor. Fix: wrap both blocking pw_loop_invoke calls in with_lock (the hooks then release/retake the lock during the wait, as designed). invoke_async (block=false) is unaffected. 2. format_negotiation::build_buffers_param advertised SPA_PARAM_BUFFERS_size/stride as fixed Ints, so any producer with padded rows (GPU-allocated buffers routinely pad strides) failed the buffers-param intersection and the link died before a single frame. Advertise CHOICE_RANGE(tight, tight, INT32_MAX) instead; consumers must read chunk->stride/size per-frame anyway. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_014rZgzE8JjWvHDtaVUhxpLE --- .../libremidi/backends/linux/pipewire/context.hpp | 12 ++++++++++-- include/libremidi/backends/linux/pipewire/format.hpp | 7 ++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/include/libremidi/backends/linux/pipewire/context.hpp b/include/libremidi/backends/linux/pipewire/context.hpp index dfdeec63..a983d0a5 100644 --- a/include/libremidi/backends/linux/pipewire/context.hpp +++ b/include/libremidi/backends/linux/pipewire/context.hpp @@ -308,7 +308,13 @@ class context : public std::enable_shared_from_this } return 0; }; - pw_loop_invoke(m_loop, trampoline, 0, nullptr, 0, /*block=*/true, &f); + // A blocking pw_loop_invoke from a foreign thread fires the loop + // control hooks (thread-loop unlock before the wait / lock after), + // which REQUIRE the caller to hold the thread-loop lock. Calling + // without it underflows the recurse counter (refused unlock) and + // then acquires the mutex on the way out, deadlocking the loop. + with_lock( + [&] { pw_loop_invoke(m_loop, trampoline, 0, nullptr, 0, /*block=*/true, &f); }); } else { @@ -330,7 +336,9 @@ class context : public std::enable_shared_from_this } return 0; }; - pw_loop_invoke(m_loop, trampoline, 0, nullptr, 0, /*block=*/true, &p); + // See the void branch: blocking invokes require the thread-loop lock. + with_lock( + [&] { pw_loop_invoke(m_loop, trampoline, 0, nullptr, 0, /*block=*/true, &p); }); return std::move(*p.result); } } diff --git a/include/libremidi/backends/linux/pipewire/format.hpp b/include/libremidi/backends/linux/pipewire/format.hpp index d83c7eb3..6d11780c 100644 --- a/include/libremidi/backends/linux/pipewire/format.hpp +++ b/include/libremidi/backends/linux/pipewire/format.hpp @@ -320,10 +320,15 @@ inline const spa_pod* format_negotiation::build_buffers_param( data_type = (1u << SPA_DATA_MemPtr); break; } + // size/stride are RANGES with our tight computation as the minimum: + // producers commonly deliver row-padded buffers (GPU-allocated strides), + // and a fixed Int here fails the param intersection against any producer + // whose size differs — killing the link instead of accepting padding. return reinterpret_cast(spa_pod_builder_add_object( &builder, SPA_TYPE_OBJECT_ParamBuffers, SPA_PARAM_Buffers, SPA_PARAM_BUFFERS_buffers, SPA_POD_CHOICE_RANGE_Int(8, 2, 16), SPA_PARAM_BUFFERS_blocks, SPA_POD_Int(blocks), - SPA_PARAM_BUFFERS_size, SPA_POD_Int(size), SPA_PARAM_BUFFERS_stride, SPA_POD_Int(stride), + SPA_PARAM_BUFFERS_size, SPA_POD_CHOICE_RANGE_Int(size, size, INT32_MAX), + SPA_PARAM_BUFFERS_stride, SPA_POD_CHOICE_RANGE_Int(stride, stride, INT32_MAX), SPA_PARAM_BUFFERS_dataType, SPA_POD_CHOICE_FLAGS_Int(data_type))); }