Lock the OBR mutex in head-tracking and limiter setters - #23
Closed
trsonic wants to merge 10 commits into
Closed
Conversation
Ports the fork's MODULE.bazel, BUILD.bazel, extensions.bzl and third_party/pffft.BUILD onto the public repo layout (code at the repository root instead of liboar/).
Bazel globs do not cross package or module boundaries; the vendored OBR subtree's BUILD and MODULE.bazel files would stop the top-level targets from picking up its sources.
Azimuths outside (-180, 180] (e.g. +270 for -90) skipped the front-back fold and hit the closest-speaker fallback, whose linear angle difference picks the wrong speaker whenever the true separation crosses the +/-180 seam. Normalize azimuths once at metadata ingestion so every downstream consumer sees canonical angles, and make the fallback distance circular.
Renders aliased azimuth pairs (+270 vs -90 etc.) to stereo and requires bit-identical output, and checks lateral sources collapse onto opposite speakers. Before the fix a source at +270 landed on the left speaker.
enable_testing() at the top level puts the obr unit tests and the liboar examples in one ctest registry; the examples are registered under an "oar" label (mirroring the obr subtree's "obr" label) derived from the directory's target list so new examples cannot be silently left out. The CI workflow's hardcoded example list and separate obr ctest invocation collapse into a single label-filtered ctest run.
_open stored the address of the stack-local pout in ear_renderer->out_sp_layout, which lives for the renderer's lifetime. The pointer is only dereferenced during _open itself (matrix lookups), so the bug is latent, but any future reader of out_sp_layout.sp_layout.predefined_sp after _open returns would hit a dangling stack pointer. Null it before returning, matching how the input-layout paths already reset pin/cin.
The ARM NEON matrix-render, OBR resampler/sh_hrir DSP, and LFE filter sample-rate fixes were merged upstream (AOMediaCodec/oar #14, #15 and the matrix_render include commit), so they drop off the pending list. The OLR azimuth wrapping, ctest registration, and EAR dangling-pointer fixes take their place.
SetHeadRotation(), EnableHeadTracking() and EnableLimiter() wrote world_rotation_, head_tracking_enabled_ and limiter_enabled_ without acquiring mutex_, while Process() reads all three under the lock for the duration of each rendered block. Head rotation in particular is typically updated from a sensor thread while audio renders, so the unsynchronized quaternion write could be observed torn mid-block, and all three are data races regardless. Every other ObrImpl mutator already takes the mutex; make these three do the same.
Collaborator
Author
|
Withdrawing for now; will resubmit later. |
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
1 participant
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.
SetHeadRotation(),EnableHeadTracking()andEnableLimiter()wroteworld_rotation_,head_tracking_enabled_andlimiter_enabled_without acquiringmutex_, whileProcess()reads all three under the lock for the duration of each rendered block. Head rotation in particular is typically updated from a sensor thread while the audio thread renders, so the unsynchronized 4-float quaternion write could be observed torn mid-block — and all three writes are data races regardless.Every other
ObrImplmutator already takes the mutex; this makes the remaining three do the same. The setters are only called from the C API wrappers (obr_capi.cpp), so no caller holds the lock and there is no deadlock path.No functional change on a single thread; existing tests pass unchanged.