From 74e6d3ca8c0e22aa281d1f9958317941c81eee37 Mon Sep 17 00:00:00 2001 From: Tomasz Rudzki Date: Tue, 13 Oct 2026 13:26:44 +0200 Subject: [PATCH 1/3] Set OBR's head rotation and enable flags without the render mutex Since #25, ObrImpl::SetHeadRotation(), EnableHeadTracking() and EnableLimiter() take mutex_, the absl::Mutex that Process() holds for the whole render call. Head rotation usually arrives from a sensor thread on every block, so the audio thread and that thread contend for the lock, and absl::Mutex has no priority inheritance: a control thread preempted while holding it stalls the render (#46). The design is the one @olilarkin proposed in #27, implemented here from its description in #46: - The head-tracking and limiter flags are std::atomic. - The head rotation is published through AtomicWorldRotation (obr/common/atomic_rotation.h), a sequence lock over four std::atomic; std::atomic of the whole 16-byte quaternion is not guaranteed to be lock-free on every platform OBR targets. Store() serializes concurrent writers with a compare-and-swap on the sequence, with acquire ordering on success so that each writer's stores follow the previous writer's. Load() makes at most four attempts and then returns the caller's previous value, so the read never waits. - Process() reads the three values once at the start of the block and keeps the rotation it used, which it passes as the fallback for the next block. - mutex_ still guards the audio elements and processing groups; the three setters no longer take it. The public API is unchanged. atomic_rotation_test checks a round trip, that a reader running against one writer storing increasing values never sees a mixed or an older value, and that Load() returns a whole value or the fallback while four writers store continuously. ObrImplTest.TestConcurrentHeadRotationDuringProcessing renders 500 blocks while a second thread sweeps the rotation and toggles both flags, and requires finite output. --- src/renderer/obr/obr_capi/obr/CMakeLists.txt | 6 + .../obr/obr_capi/obr/obr/common/BUILD | 1 + .../obr_capi/obr/obr/common/atomic_rotation.h | 108 +++++++++++++++ .../obr/obr_capi/obr/obr/common/tests/BUILD | 9 ++ .../obr/common/tests/atomic_rotation_test.cc | 131 ++++++++++++++++++ .../obr/obr_capi/obr/obr/renderer/obr_impl.cc | 37 ++--- .../obr/obr_capi/obr/obr/renderer/obr_impl.h | 33 ++++- .../obr/obr/renderer/tests/obr_impl_test.cc | 52 +++++++ 8 files changed, 353 insertions(+), 24 deletions(-) create mode 100644 src/renderer/obr/obr_capi/obr/obr/common/atomic_rotation.h create mode 100644 src/renderer/obr/obr_capi/obr/obr/common/tests/atomic_rotation_test.cc diff --git a/src/renderer/obr/obr_capi/obr/CMakeLists.txt b/src/renderer/obr/obr_capi/obr/CMakeLists.txt index 083e73c..383376a 100644 --- a/src/renderer/obr/obr_capi/obr/CMakeLists.txt +++ b/src/renderer/obr/obr_capi/obr/CMakeLists.txt @@ -154,6 +154,7 @@ set(SourceFiles obr/audio_buffer/simd_utils.cc obr/audio_buffer/simd_utils.h obr/common/ambisonic_utils.h + obr/common/atomic_rotation.h obr/common/constants.h obr/common/status_macros.h obr/common/misc_math.h @@ -315,6 +316,10 @@ add_executable(ambisonic_utils_test obr/common/tests/ambisonic_utils_test.cc) target_link_libraries(ambisonic_utils_test PRIVATE obr GTest::gtest_main) add_test(NAME ambisonic_utils_test COMMAND ambisonic_utils_test) +add_executable(atomic_rotation_test obr/common/tests/atomic_rotation_test.cc) +target_link_libraries(atomic_rotation_test PRIVATE obr GTest::gtest_main) +add_test(NAME atomic_rotation_test COMMAND atomic_rotation_test) + add_executable(misc_math_test obr/common/tests/misc_math_test.cc) target_link_libraries(misc_math_test PRIVATE obr GTest::gtest_main) add_test(NAME misc_math_test COMMAND misc_math_test) @@ -347,6 +352,7 @@ set_tests_properties( ambisonic_rotator_test spherical_angle_test ambisonic_utils_test + atomic_rotation_test misc_math_test test_util_test PROPERTIES LABELS "obr" diff --git a/src/renderer/obr/obr_capi/obr/obr/common/BUILD b/src/renderer/obr/obr_capi/obr/obr/common/BUILD index b20ef17..1b34057 100644 --- a/src/renderer/obr/obr_capi/obr/obr/common/BUILD +++ b/src/renderer/obr/obr_capi/obr/obr/common/BUILD @@ -6,6 +6,7 @@ cc_library( name = "common", hdrs = [ "ambisonic_utils.h", + "atomic_rotation.h", "constants.h", "misc_math.h", "status_macros.h", diff --git a/src/renderer/obr/obr_capi/obr/obr/common/atomic_rotation.h b/src/renderer/obr/obr_capi/obr/obr/common/atomic_rotation.h new file mode 100644 index 0000000..2f788d4 --- /dev/null +++ b/src/renderer/obr/obr_capi/obr/obr/common/atomic_rotation.h @@ -0,0 +1,108 @@ +/* + * Copyright (c) 2026, Alliance for Open Media. All rights reserved + * + * This source code is subject to the terms of the BSD 3-Clause Clear License + * and the Alliance for Open Media Patent License 1.0. If the BSD 3-Clause Clear + * License was not distributed with this source code in the LICENSE file, you + * can obtain it at www.aomedia.org/license/software-license/bsd-3-c-c. If the + * Alliance for Open Media Patent License 1.0 was not distributed with this + * source code in the PATENTS file, you can obtain it at + * www.aomedia.org/license/patent. + */ + +#ifndef OBR_COMMON_ATOMIC_ROTATION_H_ +#define OBR_COMMON_ATOMIC_ROTATION_H_ + +#include +#include + +#include "obr/common/misc_math.h" + +namespace obr { + +/*!\brief A `WorldRotation` written by one or more threads and read by + * another without a lock. + * + * The four quaternion components are atomic floats behind a sequence + * counter that is odd while a write is in progress. `std::atomic` of the + * whole 16-byte quaternion is not guaranteed to be lock-free on every + * platform OBR targets. + * `Store()` waits for a concurrent `Store()` to finish; `Load()` never waits: + * after `kMaxLoadAttempts` attempts that overlap a write it returns the + * caller's previous value. + */ +class AtomicWorldRotation { + public: + /*!\brief The number of times `Load()` reads the components before it + * returns the fallback. + */ + static constexpr int kMaxLoadAttempts = 4; + + /*!\brief Constructs an identity rotation. */ + AtomicWorldRotation() : AtomicWorldRotation(WorldRotation()) {} + + /*!\brief Constructs a rotation holding `rotation`. */ + explicit AtomicWorldRotation(const WorldRotation& rotation) + : w_(rotation.w()), + x_(rotation.x()), + y_(rotation.y()), + z_(rotation.z()) {} + + AtomicWorldRotation(const AtomicWorldRotation&) = delete; + AtomicWorldRotation& operator=(const AtomicWorldRotation&) = delete; + + /*!\brief Publishes `rotation`. Safe to call from several threads. */ + void Store(const WorldRotation& rotation) { + // Make the sequence odd. Another writer holding it odd is storing four + // floats, so the wait is short while that writer runs; it spins for as + // long as that writer is preempted. The acquire on success orders this + // store after the previous writer's, whose final sequence store it reads. + uint32_t sequence = sequence_.load(std::memory_order_relaxed); + do { + while (sequence & 1u) { + sequence = sequence_.load(std::memory_order_relaxed); + } + } while (!sequence_.compare_exchange_weak(sequence, sequence + 1, + std::memory_order_acquire, + std::memory_order_relaxed)); + // Orders the odd sequence before the component stores, so a reader that + // sees a new component also sees a changed sequence. + std::atomic_thread_fence(std::memory_order_release); + w_.store(rotation.w(), std::memory_order_relaxed); + x_.store(rotation.x(), std::memory_order_relaxed); + y_.store(rotation.y(), std::memory_order_relaxed); + z_.store(rotation.z(), std::memory_order_relaxed); + sequence_.store(sequence + 2, std::memory_order_release); + } + + /*!\brief Returns the last stored rotation, or `fallback` when every one of + * `kMaxLoadAttempts` reads overlapped a `Store()`. + */ + WorldRotation Load(const WorldRotation& fallback) const { + for (int attempt = 0; attempt < kMaxLoadAttempts; ++attempt) { + const uint32_t before = sequence_.load(std::memory_order_acquire); + if (before & 1u) continue; + const float w = w_.load(std::memory_order_relaxed); + const float x = x_.load(std::memory_order_relaxed); + const float y = y_.load(std::memory_order_relaxed); + const float z = z_.load(std::memory_order_relaxed); + // Orders the component loads before the second sequence load. + std::atomic_thread_fence(std::memory_order_acquire); + if (sequence_.load(std::memory_order_relaxed) == before) { + return WorldRotation(w, x, y, z); + } + } + return fallback; + } + + private: + std::atomic sequence_{0}; + std::atomic w_; + std::atomic x_; + std::atomic y_; + std::atomic z_; +}; + +} // namespace obr + +#endif // OBR_COMMON_ATOMIC_ROTATION_H_ diff --git a/src/renderer/obr/obr_capi/obr/obr/common/tests/BUILD b/src/renderer/obr/obr_capi/obr/obr/common/tests/BUILD index 1bc40de..6c3de6a 100644 --- a/src/renderer/obr/obr_capi/obr/obr/common/tests/BUILD +++ b/src/renderer/obr/obr_capi/obr/obr/common/tests/BUILD @@ -9,6 +9,15 @@ cc_test( ], ) +cc_test( + name = "atomic_rotation_test", + srcs = ["atomic_rotation_test.cc"], + deps = [ + "//obr/common", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "misc_math_test", srcs = ["misc_math_test.cc"], diff --git a/src/renderer/obr/obr_capi/obr/obr/common/tests/atomic_rotation_test.cc b/src/renderer/obr/obr_capi/obr/obr/common/tests/atomic_rotation_test.cc new file mode 100644 index 0000000..8a2b697 --- /dev/null +++ b/src/renderer/obr/obr_capi/obr/obr/common/tests/atomic_rotation_test.cc @@ -0,0 +1,131 @@ +/* + * Copyright (c) 2026, Alliance for Open Media. All rights reserved + * + * This source code is subject to the terms of the BSD 3-Clause Clear License + * and the Alliance for Open Media Patent License 1.0. If the BSD 3-Clause Clear + * License was not distributed with this source code in the LICENSE file, you + * can obtain it at www.aomedia.org/license/software-license/bsd-3-c-c. If the + * Alliance for Open Media Patent License 1.0 was not distributed with this + * source code in the PATENTS file, you can obtain it at + * www.aomedia.org/license/patent. + */ + +#include "obr/common/atomic_rotation.h" + +#include +#include +#include + +#include "gtest/gtest.h" +#include "obr/common/misc_math.h" + +namespace obr { +namespace { + +// A rotation whose components are `k`, `k + 1`, `k + 2` and `k + 3`, so a +// load that mixes two stores shows as unequal steps. Integers below 2^24 are +// exact in a float. +WorldRotation Numbered(int k) { + return WorldRotation(static_cast(k), static_cast(k + 1), + static_cast(k + 2), static_cast(k + 3)); +} + +bool IsNumbered(const WorldRotation& r) { + return r.x() - r.w() == 1.0f && r.y() - r.w() == 2.0f && + r.z() - r.w() == 3.0f; +} + +const WorldRotation kFallback(-7.0f, -7.0f, -7.0f, -7.0f); + +bool IsFallback(const WorldRotation& r) { + return r.w() == kFallback.w() && r.x() == kFallback.x() && + r.y() == kFallback.y() && r.z() == kFallback.z(); +} + +TEST(AtomicWorldRotationTest, DefaultIsIdentity) { + const AtomicWorldRotation rotation; + const WorldRotation loaded = rotation.Load(kFallback); + EXPECT_EQ(loaded.w(), 1.0f); + EXPECT_EQ(loaded.x(), 0.0f); + EXPECT_EQ(loaded.y(), 0.0f); + EXPECT_EQ(loaded.z(), 0.0f); +} + +TEST(AtomicWorldRotationTest, LoadReturnsTheLastStore) { + AtomicWorldRotation rotation; + rotation.Store(WorldRotation(0.5f, -0.5f, 0.25f, 0.75f)); + rotation.Store(Numbered(10)); + const WorldRotation loaded = rotation.Load(kFallback); + EXPECT_EQ(loaded.w(), 10.0f); + EXPECT_EQ(loaded.x(), 11.0f); + EXPECT_EQ(loaded.y(), 12.0f); + EXPECT_EQ(loaded.z(), 13.0f); +} + +// One writer stores increasing numbered rotations while the test thread +// loads. Every load returns a whole store or the fallback, and the stores it +// returns never go back in time. +TEST(AtomicWorldRotationTest, NoTornLoadsUnderAConcurrentWriter) { + AtomicWorldRotation rotation(Numbered(0)); + std::atomic stop{false}; + std::thread writer([&] { + for (int k = 1; !stop.load(std::memory_order_relaxed); ++k) { + rotation.Store(Numbered(k & ((1 << 20) - 1))); + } + }); + + float last_w = 0.0f; + int torn = 0; + int backwards = 0; + for (int i = 0; i < 200000; ++i) { + const WorldRotation loaded = rotation.Load(kFallback); + if (IsFallback(loaded)) continue; + if (!IsNumbered(loaded)) { + ++torn; + continue; + } + // The writer wraps at 2^20; a smaller value after a large one is a wrap. + if (loaded.w() < last_w && last_w - loaded.w() < (1 << 19)) ++backwards; + last_w = loaded.w(); + } + stop.store(true, std::memory_order_relaxed); + writer.join(); + + EXPECT_EQ(torn, 0); + EXPECT_EQ(backwards, 0); +} + +// Several writers store as fast as they can. `Load()` returns after at most +// `kMaxLoadAttempts` attempts each time, with a whole store or the fallback. +TEST(AtomicWorldRotationTest, LoadIsBoundedUnderAWriteStorm) { + AtomicWorldRotation rotation(Numbered(0)); + std::atomic stop{false}; + std::vector writers; + for (int t = 0; t < 4; ++t) { + writers.emplace_back([&rotation, &stop, t] { + for (int k = t; !stop.load(std::memory_order_relaxed); k += 4) { + rotation.Store(Numbered(k & ((1 << 20) - 1))); + } + }); + } + + int torn = 0; + int fallbacks = 0; + for (int i = 0; i < 100000; ++i) { + const WorldRotation loaded = rotation.Load(kFallback); + if (IsFallback(loaded)) { + ++fallbacks; + } else if (!IsNumbered(loaded)) { + ++torn; + } + } + stop.store(true, std::memory_order_relaxed); + for (auto& writer : writers) writer.join(); + + EXPECT_EQ(torn, 0); + // Whether a load fell back depends on scheduling; record it. + RecordProperty("fallbacks", fallbacks); +} + +} // namespace +} // namespace obr diff --git a/src/renderer/obr/obr_capi/obr/obr/renderer/obr_impl.cc b/src/renderer/obr/obr_capi/obr/obr/renderer/obr_impl.cc index 825cb84..80038b6 100644 --- a/src/renderer/obr/obr_capi/obr/obr/renderer/obr_impl.cc +++ b/src/renderer/obr/obr_capi/obr/obr/renderer/obr_impl.cc @@ -13,6 +13,7 @@ #include "obr/renderer/obr_impl.h" #include +#include #include #include #include @@ -41,7 +42,6 @@ ObrImpl::ObrImpl(int buffer_size_per_channel, int sampling_rate) sampling_rate_(sampling_rate), head_tracking_enabled_(false), limiter_enabled_(true), - world_rotation_(WorldRotation()), fft_manager_(buffer_size_per_channel_), group_output_(kNumBinauralChannels, buffer_size_per_channel_), planar_input_(0, buffer_size_per_channel_), @@ -179,6 +179,13 @@ void ObrImpl::ProcessPlanar(const float* input, float* output, void ObrImpl::ProcessLocked(const AudioBuffer& input_buffer, AudioBuffer* output_buffer) { + // Read the control state once for the whole block; other threads set it + // without the lock. + const bool head_tracking_enabled = + head_tracking_enabled_.load(std::memory_order_relaxed); + const bool limiter_enabled = limiter_enabled_.load(std::memory_order_relaxed); + latched_world_rotation_ = world_rotation_.Load(latched_world_rotation_); + // Clear the output buffer. output_buffer->Clear(); @@ -229,7 +236,7 @@ void ObrImpl::ProcessLocked(const AudioBuffer& input_buffer, // (May still have passthrough elements processed above.) if (processing_groups_.empty()) { // Apply peak limiter even for passthrough-only configurations. - if (limiter_enabled_) { + if (limiter_enabled) { peak_limiter_->Process(*output_buffer, output_buffer); } return; @@ -238,8 +245,8 @@ void ObrImpl::ProcessLocked(const AudioBuffer& input_buffer, // Process each processing group. for (auto& group : processing_groups_) { group_output_.Clear(); - group.Process(input_buffer, audio_elements_, head_tracking_enabled_, - world_rotation_, &group_output_); + group.Process(input_buffer, audio_elements_, head_tracking_enabled, + latched_world_rotation_, &group_output_); // Add this group's binaural output to the main output buffer. (*output_buffer)[0] += group_output_[0]; @@ -247,7 +254,7 @@ void ObrImpl::ProcessLocked(const AudioBuffer& input_buffer, } // Peak limit the output if enabled. - if (limiter_enabled_) { + if (limiter_enabled) { peak_limiter_->Process(*output_buffer, output_buffer); } } @@ -460,26 +467,20 @@ absl::Status ObrImpl::UpdateObjectChannelPosition(size_t audio_element_index, } void ObrImpl::EnableHeadTracking(bool enable_head_tracking) { - // Acquire mutex: `head_tracking_enabled_` is read by `Process()` on the - // audio thread. - absl::MutexLock lock(&mutex_); - head_tracking_enabled_ = enable_head_tracking; + // No lock: `Process()` reads the flag once per block on the audio thread. + head_tracking_enabled_.store(enable_head_tracking, std::memory_order_relaxed); } void ObrImpl::EnableLimiter(bool enable_limiter) { - // Acquire mutex: `limiter_enabled_` is read by `Process()` on the audio - // thread. - absl::MutexLock lock(&mutex_); - limiter_enabled_ = enable_limiter; + // No lock: `Process()` reads the flag once per block on the audio thread. + limiter_enabled_.store(enable_limiter, std::memory_order_relaxed); } absl::Status ObrImpl::SetHeadRotation(float w, float x, float y, float z) { - // Acquire mutex: `world_rotation_` is read by `Process()` on the audio - // thread, and head rotation updates typically arrive from a sensor thread. - absl::MutexLock lock(&mutex_); - + // No lock: head rotation typically arrives from a sensor thread on every + // block, and `Process()` reads it once per block on the audio thread. // Apply counter-rotation for head tracking. - world_rotation_ = WorldRotation(w, -x, -y, -z); + world_rotation_.Store(WorldRotation(w, -x, -y, -z)); return absl::OkStatus(); } diff --git a/src/renderer/obr/obr_capi/obr/obr/renderer/obr_impl.h b/src/renderer/obr/obr_capi/obr/obr/renderer/obr_impl.h index bd479a1..a73ece8 100644 --- a/src/renderer/obr/obr_capi/obr/obr/renderer/obr_impl.h +++ b/src/renderer/obr/obr_capi/obr/obr/renderer/obr_impl.h @@ -13,6 +13,7 @@ #ifndef OBR_RENDERER_OBR_IMPL_H_ #define OBR_RENDERER_OBR_IMPL_H_ +#include #include #include #include @@ -23,6 +24,7 @@ #include "obr/ambisonic_binaural_decoder/fft_manager.h" #include "obr/ambisonic_binaural_decoder/resampler.h" #include "obr/audio_buffer/audio_buffer.h" +#include "obr/common/atomic_rotation.h" #include "obr/common/misc_math.h" #include "obr/peak_limiter/peak_limiter.h" #include "obr/renderer/audio_element_config.h" @@ -150,6 +152,9 @@ class ObrImpl { float elevation, float distance); /*! \brief Enables or disables head tracking. + * + * Takes no lock, so it can be called from any thread while `Process()` + * runs; the change takes effect at the next `Process()` call. * * \param enable_head_tracking True to enable head tracking, false to disable. */ @@ -188,6 +193,9 @@ class ObrImpl { bool head_locked); /*! \brief Enables or disables the output peak limiter. + * + * Takes no lock, so it can be called from any thread while `Process()` + * runs; the change takes effect at the next `Process()` call. * * \param enable_limiter True to enable the limiter (default), false to * disable. @@ -203,6 +211,12 @@ class ObrImpl { * Y - front (positive front, negative back) * Z - up (positive up, negative down) * + * Takes no lock, so it can be called from any thread, such as a sensor + * thread, while `Process()` runs; the rotation takes effect at the next + * `Process()` call. If a call overlaps the rotation's read at the start of + * `Process()` every time it retries, that block keeps the previous block's + * rotation. + * * @param w * @param x * @param y @@ -264,13 +278,20 @@ class ObrImpl { const int buffer_size_per_channel_; const int sampling_rate_; - bool head_tracking_enabled_; - bool limiter_enabled_; - WorldRotation world_rotation_; + // Control state set from any thread without `mutex_`. `Process()` reads + // each once per call, so a change takes effect at the next block. + std::atomic head_tracking_enabled_; + std::atomic limiter_enabled_; + AtomicWorldRotation world_rotation_; + + // The rotation `Process()` read for its last block, which it keeps when a + // read of `world_rotation_` overlaps a write. Accessed under `mutex_`. + WorldRotation latched_world_rotation_; - // Mutex to protect data accessed in different threads, including the - // group output and planar buffers. `Process()` and `ProcessPlanar()` hold - // it for the whole call. + // Guards the audio elements and processing groups, the planar input and + // output buffers, the group output buffer, the peak limiter and + // `latched_world_rotation_`. `Process()` and `ProcessPlanar()` hold it for + // the whole call; the control state above does not use it. mutable absl::Mutex mutex_; std::vector audio_elements_; diff --git a/src/renderer/obr/obr_capi/obr/obr/renderer/tests/obr_impl_test.cc b/src/renderer/obr/obr_capi/obr/obr/renderer/tests/obr_impl_test.cc index 32a1b35..f567815 100644 --- a/src/renderer/obr/obr_capi/obr/obr/renderer/tests/obr_impl_test.cc +++ b/src/renderer/obr/obr_capi/obr/obr/renderer/tests/obr_impl_test.cc @@ -12,8 +12,11 @@ #include "obr/renderer/obr_impl.h" +#include +#include #include #include +#include #include #include @@ -1183,5 +1186,54 @@ TEST(ObrImplTest, TestProcessPlanarMatchesProcess) { expect_same_output(30); } +// Renders while a second thread sweeps the head rotation and toggles head +// tracking and the limiter. None of those calls takes the mutex `Process()` +// holds; the test checks the output stays finite, and a ThreadSanitizer build +// checks the accesses. +TEST(ObrImplTest, TestConcurrentHeadRotationDuringProcessing) { + const size_t kNumFrames = 256; + ObrImpl renderer(kNumFrames, kSamplingRate); + EXPECT_THAT(renderer.AddAudioElement(AudioElementType::k3OA), IsOk()); + EXPECT_THAT(renderer.AddAudioElement(AudioElementType::kLayout7_1_4_ch), + IsOk()); + + AudioBuffer input_buffer(renderer.GetNumberOfInputChannels(), kNumFrames); + for (size_t ch = 0; ch < input_buffer.num_channels(); ++ch) { + GenerateWhiteNoise(0.1f, &input_buffer[ch], static_cast(100 + ch)); + } + AudioBuffer output_buffer(2, kNumFrames); + + std::atomic stop{false}; + std::atomic failed_rotations{0}; + std::thread control([&] { + for (int i = 0; !stop.load(std::memory_order_relaxed); ++i) { + const float half_angle = 0.005f * static_cast(i % 1257); + if (!renderer + .SetHeadRotation(std::cos(half_angle), 0.0f, 0.0f, + std::sin(half_angle)) + .ok()) { + failed_rotations.fetch_add(1, std::memory_order_relaxed); + } + renderer.EnableHeadTracking(i % 2 == 0); + renderer.EnableLimiter(i % 3 != 0); + } + }); + + int non_finite = 0; + for (int block = 0; block < 500; ++block) { + renderer.Process(input_buffer, &output_buffer); + for (size_t ch = 0; ch < output_buffer.num_channels(); ++ch) { + for (size_t frame = 0; frame < kNumFrames; ++frame) { + if (!std::isfinite(output_buffer[ch][frame])) ++non_finite; + } + } + } + stop.store(true, std::memory_order_relaxed); + control.join(); + + EXPECT_EQ(non_finite, 0); + EXPECT_EQ(failed_rotations.load(), 0); +} + } // namespace } // namespace obr From c7bca553badde00c201562cd2903cf251ea14dfc Mon Sep 17 00:00:00 2001 From: Tomasz Rudzki Date: Wed, 14 Oct 2026 10:54:38 +0200 Subject: [PATCH 2/3] Drop the finiteness check from the concurrent head-rotation test ObrImplTest.TestConcurrentHeadRotationDuringProcessing required finite output and counted SetHeadRotation() calls that did not return OK. Neither check can fail. SetHeadRotation() always returns OK. A rotation read while it is being written mixes the components of two stored quaternions, and OBR renders such a quaternion to finite samples: 4099 blocks with head tracking on and rotations (cos a, 0, 0, sin b), of norm 0 to 1.414, gave no non-finite sample. The test can fail only in a ThreadSanitizer build, which reports a data race between a setter and Process(). With the atomics and the sequence lock replaced by plain members, a TSan build reports a race on the rotation and the test exits non-zero; a Release build passes. The test comment now says that it makes no assertion on the output. --- .../obr/obr/renderer/tests/obr_impl_test.cc | 25 +++++-------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/src/renderer/obr/obr_capi/obr/obr/renderer/tests/obr_impl_test.cc b/src/renderer/obr/obr_capi/obr/obr/renderer/tests/obr_impl_test.cc index f567815..57c6556 100644 --- a/src/renderer/obr/obr_capi/obr/obr/renderer/tests/obr_impl_test.cc +++ b/src/renderer/obr/obr_capi/obr/obr/renderer/tests/obr_impl_test.cc @@ -1188,8 +1188,8 @@ TEST(ObrImplTest, TestProcessPlanarMatchesProcess) { // Renders while a second thread sweeps the head rotation and toggles head // tracking and the limiter. None of those calls takes the mutex `Process()` -// holds; the test checks the output stays finite, and a ThreadSanitizer build -// checks the accesses. +// holds. The test makes no assertion on the output; a ThreadSanitizer build +// checks the concurrent accesses. TEST(ObrImplTest, TestConcurrentHeadRotationDuringProcessing) { const size_t kNumFrames = 256; ObrImpl renderer(kNumFrames, kSamplingRate); @@ -1204,35 +1204,22 @@ TEST(ObrImplTest, TestConcurrentHeadRotationDuringProcessing) { AudioBuffer output_buffer(2, kNumFrames); std::atomic stop{false}; - std::atomic failed_rotations{0}; std::thread control([&] { for (int i = 0; !stop.load(std::memory_order_relaxed); ++i) { const float half_angle = 0.005f * static_cast(i % 1257); - if (!renderer - .SetHeadRotation(std::cos(half_angle), 0.0f, 0.0f, - std::sin(half_angle)) - .ok()) { - failed_rotations.fetch_add(1, std::memory_order_relaxed); - } + renderer + .SetHeadRotation(std::cos(half_angle), 0.0f, 0.0f, + std::sin(half_angle)) + .IgnoreError(); renderer.EnableHeadTracking(i % 2 == 0); renderer.EnableLimiter(i % 3 != 0); } }); - - int non_finite = 0; for (int block = 0; block < 500; ++block) { renderer.Process(input_buffer, &output_buffer); - for (size_t ch = 0; ch < output_buffer.num_channels(); ++ch) { - for (size_t frame = 0; frame < kNumFrames; ++frame) { - if (!std::isfinite(output_buffer[ch][frame])) ++non_finite; - } - } } stop.store(true, std::memory_order_relaxed); control.join(); - - EXPECT_EQ(non_finite, 0); - EXPECT_EQ(failed_rotations.load(), 0); } } // namespace From e99c734d2b1f1c072415d808a11057230eec54ce Mon Sep 17 00:00:00 2001 From: Tomasz Rudzki Date: Fri, 16 Oct 2026 11:05:04 +0200 Subject: [PATCH 3/3] Say why AtomicWorldRotation::Load() stops after four attempts The comment on kMaxLoadAttempts now gives the reason for the bound: Load() runs on the render thread and must not keep retrying while a preempted writer holds the sequence odd. It also says that an attempt fails only when a write overlaps it, that ObrImpl passes the previous block's rotation as the fallback, and that the value 4 is not tuned against a measurement. The change is to the comment only. --- .../obr/obr_capi/obr/obr/common/atomic_rotation.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/renderer/obr/obr_capi/obr/obr/common/atomic_rotation.h b/src/renderer/obr/obr_capi/obr/obr/common/atomic_rotation.h index 2f788d4..535bf6c 100644 --- a/src/renderer/obr/obr_capi/obr/obr/common/atomic_rotation.h +++ b/src/renderer/obr/obr_capi/obr/obr/common/atomic_rotation.h @@ -35,6 +35,14 @@ class AtomicWorldRotation { public: /*!\brief The number of times `Load()` reads the components before it * returns the fallback. + * + * `Load()` runs on the render thread, so it must not wait for a writer: a + * writer preempted while the sequence is odd would keep an unbounded retry + * loop spinning until that writer runs again. An attempt fails only when a + * write overlaps it, so with 4 the fallback needs writes to overlap four + * consecutive attempts. `ObrImpl` passes the rotation it rendered the + * previous block with as the fallback. Any small bound behaves the same + * way; 4 is not tuned against a measurement. */ static constexpr int kMaxLoadAttempts = 4;