Repository navigation
Set OBR's head rotation and enable flags without the render mutex - #56
Conversation
| 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; |
There was a problem hiding this comment.
Just curious: how is finiteness related to the expected behavior?
There was a problem hiding this comment.
looks like it is not related... I'll post a simplification
| /*!\brief The number of times `Load()` reads the components before it | ||
| * returns the fallback. | ||
| */ | ||
| static constexpr int kMaxLoadAttempts = 4; |
There was a problem hiding this comment.
A quick question: why choose 4 times?
There was a problem hiding this comment.
Load() runs on the render thread once per block, so it must not wait for a writer. A writer that is preempted while the sequence is odd would keep an unbounded retry loop spinning until it runs again. The bound caps the render thread's work at four reads; after that, Process() uses the previous block's rotation, so a new rotation applies one block later.
A Store() is four relaxed float stores between two sequence updates, so a retry after an overlapping write normally succeeds on the next attempt. With 4, a fallback needs writes to overlap four consecutive reads. Any small bound would behave the same way; 4 is not tuned against a measurement.
Added to the kMaxLoadAttempts comment in 340dc49.
There was a problem hiding this comment.
Sounds good, thanks!
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<bool>. - The head rotation is published through AtomicWorldRotation (obr/common/atomic_rotation.h), a sequence lock over four std::atomic<float>; 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.
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.
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.
340dc49 to
e99c734
Compare
|
Rebased onto |
Summary
Implements the design in #46, which @olilarkin proposed in #27: the head rotation and the head-tracking and limiter flags are published without the mutex
Process()holds for the whole render call. This change is written from the description in #46; it does not reuse code from #27.Since #25,
ObrImpl::SetHeadRotation(),EnableHeadTracking()andEnableLimiter()takemutex_. Head rotation usually arrives from a sensor thread on every block, so that thread and the render thread contend for the lock, andabsl::Mutexhas no priority inheritance: a control thread preempted while holding it stalls the render.std::atomic<bool>.AtomicWorldRotation(obr/common/atomic_rotation.h), a sequence lock over fourstd::atomic<float>.std::atomicof 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, under the lock it already holds, and keeps the rotation it used as the fallback for the next block.mutex_still guards the audio elements and processing groups. The three setters no longer take it, and their documentation says so. The public API is unchanged.atomic_rotation_test: a round trip; a reader running against one writer that stores increasing values never sees a mixed or an older value;Load()returns a whole value or the fallback while four writers store continuously.ObrImplTest.TestConcurrentHeadRotationDuringProcessing: 500 blocks rendered while a second thread sweeps the rotation and toggles both flags. It makes no assertion on the output; a ThreadSanitizer build checks the concurrent accesses.obr/common/BUILD,obr/common/tests/BUILDand obr'sCMakeLists.txt.Verification
On macOS arm64 (Apple clang 21):
cmake -S . -B build -DOAR_BUILD_EXAMPLES=ON && cmake --build build && ctest --test-dir build -L 'oar|obr' --output-on-failure: 41 of 41 pass in Release, with-fsanitize=addressand with-fsanitize=thread, with no ThreadSanitizer report.bazel test //...: 27 of 27 pass, including//obr/common/tests:atomic_rotation_test. As in Pin Bazel 8.5.1 and build the standalone obr module without WORKSPACE #47, the macOS run used a localapple_supporttoolchain dependency that is not part of this change.ObrImpl::Process()and throughobr_process(), built once againstmainand once against this branch, and compares an FNV-1a digest of every output sample. All 8 scenarios match, including two with head tracking enabled and the head rotation set before every block (3OA, a 7.1.4 layout, a moving dual object and a moving mono object, with the limiter on and off).Not run here: the CMake and Bazel builds on Linux and Windows, which the CI workflows cover, and the oar root Bazel build.
Closes #46