Skip to content

Publish head-tracking control state without a lock - #27

Draft
olilarkin wants to merge 1 commit into
AOMediaCodec:mainfrom
SpatialAudioKit:lock-free-control-state
Draft

Publish head-tracking control state without a lock#27
olilarkin wants to merge 1 commit into
AOMediaCodec:mainfrom
SpatialAudioKit:lock-free-control-state

Conversation

@olilarkin

Copy link
Copy Markdown

Follow-up to #25, which it replaces.

#25 correctly identified that SetHeadRotation(), EnableHeadTracking() and EnableLimiter() raced with Process() — a torn four-float quaternion write is a real hazard. It closed the race by taking mutex_ in the setters.

The concern is which side of the lock the fix landed on. Process() holds that same mutex "for the entire processing duration" (obr_impl.cc), so the setters now share a lock with the audio thread. absl::Mutex does not implement priority inheritance, so a control thread preempted while holding mutex_ stalls rendering until the scheduler runs it again — and head rotation is written continuously from a sensor thread, so this is contended on every buffer rather than rarely. Trading a torn read for a possible unbounded wait on the audio thread seems like the wrong direction: a renderer should not have to wait on a lower-priority thread.

This PR publishes the three values instead of locking them:

  • the two flags become std::atomic<bool>;
  • the quaternion goes through a new AtomicWorldRotation — a seqlock storing four std::atomic<float>, so concurrent access is defined behaviour rather than a data race that happens to be benign in practice.

std::atomic<WorldRotation> would be simpler, but at 16 bytes it is not lock-free on the platforms obr targets, so the implementation would take a hidden lock on the audio thread — the very thing being avoided.

The seqlock read is deliberately bounded rather than spinning to convergence. A reader that retries until it wins is unbounded when the writer is preempted between its two counter updates, which on an audio thread is no better than the lock this replaces. Load() gives up after a few attempts and returns the caller's previous value, so the read is wait-free. Reusing a rotation for one buffer is inaudible; missing a deadline is not.

Process() now latches all three once at the top of the block. That is also more correct than reading them where they were read before: ProcessingGroup interpolates the rotation across the buffer, so a value that can change partway through is arguably already wrong.

mutex_ is unchanged and still guards audio_elements_ and processing_groups_ — containers being rebuilt, rather than values being handed over. Nothing that runs continuously during playback takes it now.

Testing

  • atomic_rotation_test (new): round trip, the concurrent no-tearing property, and the bounded fallback under a continuous write storm.
  • ObrImplTest.TestConcurrentHeadRotationDuringProcessing (new): renders while a second thread sweeps head rotation and toggles both flags.
  • Both are clean under ThreadSanitizer.
  • The existing 65 renderer and processing-group tests pass unchanged.

No public API change.

Noticed while measuring, reported separately

Running Process() inside a [[clang::nonblocking]] region under RealtimeSanitizer shows 4 malloc / 4 free per render call, from the per-block AudioBuffer group_output and from PeakLimiter. That is independent of this change and of any locking — it happens single-threaded with head tracking disabled — so it is filed on its own rather than bundled here.

AOMediaCodec#25 correctly identified that `SetHeadRotation()`, `EnableHeadTracking()`
and `EnableLimiter()` raced with `Process()`, and a torn 4-float quaternion
write is a real hazard. It closed the race by taking `mutex_` in the
setters. But `Process()` holds that same mutex "for the entire processing
duration" (obr_impl.cc), so the fix also gives the audio thread a lock the
control plane can hold.

`absl::Mutex` does not implement priority inheritance. A control thread
preempted while holding `mutex_` therefore stalls rendering until the
scheduler runs it again, and head rotation is written continuously from a
sensor thread -- so this is contended on every buffer, not rarely. Trading
a torn read for a possible unbounded wait on the audio thread is the wrong
direction: a renderer should never wait on a lower-priority thread.

Publish the three values instead of locking them:

  - the two flags become `std::atomic<bool>`;
  - the quaternion goes through `AtomicWorldRotation`, a seqlock storing
    four `std::atomic<float>`, so concurrent access is defined behaviour
    rather than a race that is merely benign in practice.

`std::atomic<WorldRotation>` would be simpler but is not lock-free at 16
bytes on the platforms obr targets, so it would reintroduce a hidden lock
on the audio thread.

The seqlock read is bounded rather than spinning to convergence. A reader
that retries until it wins is unbounded when the writer is preempted
mid-write, which on an audio thread is no better than the lock this
replaces; instead `Load()` gives up after a few attempts and keeps the
rotation from the previous block. Reusing a rotation for one buffer is
inaudible, missing a deadline is not.

`Process()` now latches all three once at the top of the block. That is
also more correct than reading them where they were read before: the
rotation interpolation in `ProcessingGroup` assumes one rotation per
buffer, and the old code let it change partway through.

`mutex_` is unchanged and still guards the audio element and processing
group collections, which are containers being rebuilt rather than values
being handed over. Nothing that runs continuously during playback takes it.

Tests: `atomic_rotation_test` covers the round trip, the concurrent
no-tearing property and the bounded fallback;
`ObrImplTest.TestConcurrentHeadRotationDuringProcessing` renders while a
second thread sweeps the rotation. Both are clean under ThreadSanitizer,
and the existing 65 renderer and processing-group tests pass unchanged.
@olilarkin
olilarkin marked this pull request as draft August 1, 2026 14:40
@trsonic trsonic added the CLA:no Waiting for contributors to sign the AOMedia CLA label Aug 3, 2026
@trsonic

trsonic commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Thanks @olilarkin for the PR submission. Before we proceed with the review, could you please sign the Contributor License Agreement, as explained in CONTRIBUTING.md. Let us know if you have any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA:no Waiting for contributors to sign the AOMedia CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants