Publish head-tracking control state without a lock - #27
Draft
olilarkin wants to merge 1 commit into
Draft
Conversation
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.
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #25, which it replaces.
#25 correctly identified that
SetHeadRotation(),EnableHeadTracking()andEnableLimiter()raced withProcess()— a torn four-float quaternion write is a real hazard. It closed the race by takingmutex_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::Mutexdoes not implement priority inheritance, so a control thread preempted while holdingmutex_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:
std::atomic<bool>;AtomicWorldRotation— a seqlock storing fourstd::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:ProcessingGroupinterpolates the rotation across the buffer, so a value that can change partway through is arguably already wrong.mutex_is unchanged and still guardsaudio_elements_andprocessing_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.No public API change.
Noticed while measuring, reported separately
Running
Process()inside a[[clang::nonblocking]]region under RealtimeSanitizer shows 4malloc/ 4freeper render call, from the per-blockAudioBuffer group_outputand fromPeakLimiter. 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.