FEAT(client): Add HRTF-based spatialization#7085
FEAT(client): Add HRTF-based spatialization#7085hiinaspace wants to merge 9 commits intomumble-voip:masterfrom
Conversation
FFTConvolver (MIT) provides real-time partitioned convolution via overlap-add FFT. Used for HRTF impulse response convolution. Only FFTConvolver.cpp, AudioFFT.cpp, and Utilities.cpp are compiled; the rest of the submodule is unused.
Uses libmysofa for SOFA HRTF file loading with bilinear interpolation between measured directions, and FFTConvolver for real-time overlap-add convolution. Per-source state is tracked via a double-buffered ConvolverSet ping-pong that crossfades over one block (~10 ms) on direction change, preserving convolver overlap-add accumulators for click-free transitions. Direction vectors are normalized at entry; zero-length vectors (source coincident with listener) fall back to forward (+Z) to avoid crossfade thrashing. Listener-local coordinates map to SOFA Cartesian as: sofaX = dirZ, sofaY = -dirX, sofaZ = dirY.
CIPIC Subject 124 (MIT KEMAR mannequin), distributed under a permissive academic license. 558-sample HRIR at 44.1 kHz; libmysofa resamples to the mixer rate at load time. This is the same dataset used by Steam Audio as its default HRTF.
Six test cases covering: - SOFA file loading (default path resolution) - Cardinal direction HRIR distinctness (catches coordinate mapping bugs) - Impulse convolution energy bounds - Numerical stability over 1000 random directions - Left/right ear asymmetry for off-axis sources - Zero direction vector stability (coincident source/listener) - Variable frameCount crossfade direction tracking
Adds optional `hrtf` build flag (default ON). When enabled: - Detects libmysofa via find_pkg() with pkg-config fallback - Compiles FFTConvolver sources from the vendored submodule - Sets USE_HRTF compile definition (guards all HRTF code) - Bakes source-tree SOFA path as MUMBLE_HRTF_DEFAULT_SOFA for dev builds; installed builds use QStandardPaths::AppDataLocation - Registers TestHrtfSpatializer in the test suite
Adds the infrastructure hooks needed for HRTF binaural mixing, with no change to audio behaviour. All additions are guarded by USE_HRTF. - AudioOutput.h: include HrtfSpatializer.h and declare m_hrtfSpatializer - removeBuffer(): capture pointer-as-sourceId before delete so the HRTF per-source convolver state can be released alongside the buffer - initializeMixer(): construct and load the HrtfSpatializer - mix(): lift cameraDir/cameraAxis/right into outer-scope hrtfCamera* variables so they are accessible in the per-buffer loop below The per-buffer mixing loop is not yet modified; the HRTF convolution path is added in the following commit.
Settings fields bHrtf (bool, default false) and qsHrtfFile (QString, default empty = use bundled KEMAR SOFA) are added unconditionally to the POSITIONAL_AUDIO_SETTINGS macro block. The UI widgets (checkbox, line edit, browse button) are hidden at runtime when USE_HRTF is not defined, so the settings file format is stable regardless of build flags.
When USE_HRTF is defined and the user has enabled HRTF in settings,
the positional audio path replaces per-channel gain+ITD with binaural
convolution:
- Listener-local direction computed from the hrtfCamera* vectors saved
in the preceding structural commit
- Opus stereo downmixed to mono before spatialisation
- calcGain(1.0f, len) provides distance-only attenuation; the HRTF IR
encodes all directional cues (ILD/ITD)
- Gain ramped linearly across each block (matching the non-HRTF path)
to avoid clicks on rapid distance changes
- Buffer pointer address used as sourceId, unique for all source types
The existing per-channel gain+ITD block is retained verbatim as the
non-HRTF fallback inside an `#ifdef / } else { / #endif` bracket so
that the lines themselves are unchanged in this diff. Use `git diff -w`
to verify that only new code was added and no logic was modified.
WalkthroughThis pull request introduces HRTF (Head-Related Transfer Function) binaural audio spatialization support to Mumble. The implementation adds a new HrtfSpatializer class that loads SOFA impulse response files and spatializes mono audio sources to stereo output based on source direction and listener orientation. Integration includes a third-party FFTConvolver submodule for convolution processing, UI controls for enabling HRTF and selecting SOFA files, CMake build configuration with libmysofa dependency management, settings persistence, and comprehensive unit tests. The feature is guarded by a USE_HRTF compile-time flag to maintain compatibility with builds that exclude HRTF support. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mumble/AudioConfigDialog.cpp`:
- Around line 791-792: The assignments to s.bHrtf and s.qsHrtfFile are
overwriting stored HRTF settings even when HRTF support is compiled out; wrap
the two lines that read qcbHrtf->isChecked() and qleHrtfFile->text() in a
compile-time guard so they only run when USE_HRTF is defined (e.g. `#ifdef`
USE_HRTF ... `#endif`), leaving s.bHrtf and s.qsHrtfFile untouched when the
feature is disabled to preserve existing values; reference the symbols s.bHrtf,
s.qsHrtfFile, qcbHrtf, qleHrtfFile and the USE_HRTF macro when making the
change.
In `@src/mumble/AudioOutput.cpp`:
- Around line 816-823: The allocation for buffer->pfVolume in AudioOutput.cpp
only initializes channel 0, leaving pfVolume[1..nchan-1] uninitialized; modify
the pfVolume allocation branch (the code that does buffer->pfVolume = new
float[nchan]) to initialize all elements to -1.0f (e.g., loop i from 0 to nchan
and set buffer->pfVolume[i] = -1.0f) before using pfVolume[0] and applying gain
so that all channels are safe if HRTF is later disabled.
- Around line 239-242: removeSource() is being called on HrtfSpatializer from
non-audio threads (removeBuffer() via bufferInvalidated or removeUser()),
violating HrtfSpatializer's thread-affinity rule; instead enqueue the removal to
run on the audio mixing thread. Modify the code path that currently calls
m_hrtfSpatializer->removeSource(hrtfSourceId) to push a task/command (e.g., a
lambda or id) into the audio mixer's task/command queue or a thread-safe
pending-removals list that the mix loop processes, and in the mix loop execute
HrtfSpatializer::removeSource(hrtfSourceId) there; ensure the enqueueing occurs
where removeBuffer()/removeUser() can reach and that the mix thread drains and
performs the actual removeSource call.
- Around line 231-235: The HRTF spatializer uses 32-bit unsigned IDs causing
pointer truncation on 64-bit systems; change HrtfSpatializer to use
std::uintptr_t for IDs by updating the class signature (spatialize and
removeSource to accept std::uintptr_t) and the member map type to
std::unordered_map<std::uintptr_t, std::unique_ptr<SourceState>> m_sources, then
update both call sites that currently cast buffer pointers to unsigned int to
instead cast via reinterpret_cast<std::uintptr_t>(ptr) and store/pass that value
(e.g., the hrtfSourceId capture in AudioOutput.cpp and the spatialize()
invocation), and add the proper include for std::uintptr_t (<cstdint> or
<cstddef>).
In `@src/mumble/AudioOutput.ui`:
- Around line 712-751: The tab order is missing the new HRTF widgets, so add
them into the existing tab sequence (e.g., in setupUi or the .ui taborder
section) by inserting setTabOrder calls (or the equivalent UI XML <tabstops>) to
include qcbHrtf, qlHrtfFile (if focusable) / qleHrtfFile, and qpbHrtfBrowse in
the correct logical position; ensure qcbHrtf comes before qleHrtfFile and
qpbHrtfBrowse (or match surrounding controls) so keyboard navigation visits
qcbHrtf → qleHrtfFile → qpbHrtfBrowse in the expected order.
In `@src/mumble/HrtfSpatializer.cpp`:
- Around line 64-71: After successfully loading a new SOFA (the block that sets
m_sofa, resizes m_irLeft/m_irRight and returns true), invalidate or reinitialize
per-source convolution state so existing sources do not keep old IRs: locate the
HrtfSpatializer reload/init routine containing m_sofa, m_irLeft, m_irRight,
m_filterLength and ensure you clear or reset any per-source convolvers (e.g. the
container that holds source convolvers or per-source IR caches) by either
destroying/recreating them or marking them dirty so they rebuild with the new
HRTF; follow the same reset logic used when a source changes direction to update
all static sources immediately after the SOFA load succeeds.
- Around line 100-113: The code currently ignores FFTConvolver::init's boolean
return; update the block that calls set.convL->init(...) and
set.convR->init(...) to capture both return values, and only set set.initialized
= true if both inits return true (and handle the failure case by leaving
set.initialized false and optionally resetting or releasing
set.convL/set.convR); ensure you use m_blockSize and m_filterLength when calling
init and check for the zero-block-size failure path so uninitialized convolvers
aren't marked ready.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.gitmodules3rdparty/fftconvolverdata/hrtf/default.sofasrc/mumble/AudioConfigDialog.cppsrc/mumble/AudioConfigDialog.hsrc/mumble/AudioOutput.cppsrc/mumble/AudioOutput.hsrc/mumble/AudioOutput.uisrc/mumble/CMakeLists.txtsrc/mumble/HrtfSpatializer.cppsrc/mumble/HrtfSpatializer.hsrc/mumble/Settings.hsrc/mumble/SettingsKeys.hsrc/mumble/SettingsMacros.hsrc/tests/CMakeLists.txtsrc/tests/TestHrtfSpatializer/CMakeLists.txtsrc/tests/TestHrtfSpatializer/TestHrtfSpatializer.cpp
| s.bHrtf = qcbHrtf->isChecked(); | ||
| s.qsHrtfFile = qleHrtfFile->text(); |
There was a problem hiding this comment.
Avoid overwriting HRTF settings when the feature is compiled out.
With USE_HRTF disabled, the hidden controls will still save default/empty values and wipe a user’s previous HRTF settings. Consider preserving the existing values when HRTF is not available.
🔧 Suggested guard to preserve existing values
- s.bHrtf = qcbHrtf->isChecked();
- s.qsHrtfFile = qleHrtfFile->text();
+#ifdef USE_HRTF
+ s.bHrtf = qcbHrtf->isChecked();
+ s.qsHrtfFile = qleHrtfFile->text();
+#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mumble/AudioConfigDialog.cpp` around lines 791 - 792, The assignments to
s.bHrtf and s.qsHrtfFile are overwriting stored HRTF settings even when HRTF
support is compiled out; wrap the two lines that read qcbHrtf->isChecked() and
qleHrtfFile->text() in a compile-time guard so they only run when USE_HRTF is
defined (e.g. `#ifdef` USE_HRTF ... `#endif`), leaving s.bHrtf and s.qsHrtfFile
untouched when the feature is disabled to preserve existing values; reference
the symbols s.bHrtf, s.qsHrtfFile, qcbHrtf, qleHrtfFile and the USE_HRTF macro
when making the change.
| #ifdef USE_HRTF | ||
| // Capture the pointer value as source ID before deletion. | ||
| const auto hrtfSourceId = | ||
| static_cast< unsigned int >(reinterpret_cast< uintptr_t >(iter.value())); | ||
| #endif |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the specific file and lines mentioned
cd src/mumble && wc -l AudioOutput.cpp && head -n 240 AudioOutput.cpp | tail -n 20Repository: mumble-voip/mumble
Length of output: 853
🏁 Script executed:
# Search for HRTF-related definitions and usage patterns
rg -n "HrtfSpatializer|spatialize|removeSource|sourceId" --type-list | head -20Repository: mumble-voip/mumble
Length of output: 451
🏁 Script executed:
# Find files containing HrtfSpatializer
fd -e h -e hpp -e cpp | xargs rg -l "HrtfSpatializer" 2>/dev/nullRepository: mumble-voip/mumble
Length of output: 227
🏁 Script executed:
# Examine HrtfSpatializer header to see class definition and method signatures
cat -n src/mumble/HrtfSpatializer.hRepository: mumble-voip/mumble
Length of output: 4748
🏁 Script executed:
# Check the removeSource method and class structure
rg -A 10 -B 2 "removeSource|spatialize" src/mumble/HrtfSpatializer.hRepository: mumble-voip/mumble
Length of output: 1323
🏁 Script executed:
# Check HrtfSpatializer implementation
head -n 100 src/mumble/HrtfSpatializer.cppRepository: mumble-voip/mumble
Length of output: 3103
🏁 Script executed:
# Search for the map/container that stores HRTFs
rg -n "map|Map|unordered_map" src/mumble/HrtfSpatializer.h src/mumble/HrtfSpatializer.cppRepository: mumble-voip/mumble
Length of output: 230
🏁 Script executed:
# Look at broader context in AudioOutput.cpp around the problematic code
sed -n '220,250p' src/mumble/AudioOutput.cppRepository: mumble-voip/mumble
Length of output: 1048
🏁 Script executed:
# Search for all removeSource calls in AudioOutput.cpp
rg -n "removeSource" src/mumble/AudioOutput.cppRepository: mumble-voip/mumble
Length of output: 116
🏁 Script executed:
# Search for spatialize calls in AudioOutput.cpp
rg -n "spatialize" src/mumble/AudioOutput.cppRepository: mumble-voip/mumble
Length of output: 143
🏁 Script executed:
# Get full context around spatialize calls
rg -B 5 -A 5 "spatialize" src/mumble/AudioOutput.cppRepository: mumble-voip/mumble
Length of output: 622
🏁 Script executed:
# Check if there are any other places where sourceId is created from a pointer
rg -n "hrtfSourceId\|sourceId.*reinterpret\|uintptr_t" src/mumble/AudioOutput.cpp | head -20Repository: mumble-voip/mumble
Length of output: 44
🏁 Script executed:
# Look for the broader context where spatialize is called to see how sourceId is derived
sed -n '780,820p' src/mumble/AudioOutput.cppRepository: mumble-voip/mumble
Length of output: 2115
Use 64‑bit IDs throughout the HRTF spatializer to prevent pointer truncation collisions.
Casting buffer pointers to unsigned int truncates their upper bits on 64‑bit systems. Two different buffers may collide in the lower 32 bits, causing the same HRTF state to be reused across different sources and corrupting spatialization.
Update both call sites (lines 785–786 for spatialize() and 232–234 for removeSource()) and the HrtfSpatializer class to use std::uintptr_t throughout:
spatialize(std::uintptr_t sourceId, ...)removeSource(std::uintptr_t sourceId)std::unordered_map< std::uintptr_t, std::unique_ptr< SourceState > > m_sources
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mumble/AudioOutput.cpp` around lines 231 - 235, The HRTF spatializer uses
32-bit unsigned IDs causing pointer truncation on 64-bit systems; change
HrtfSpatializer to use std::uintptr_t for IDs by updating the class signature
(spatialize and removeSource to accept std::uintptr_t) and the member map type
to std::unordered_map<std::uintptr_t, std::unique_ptr<SourceState>> m_sources,
then update both call sites that currently cast buffer pointers to unsigned int
to instead cast via reinterpret_cast<std::uintptr_t>(ptr) and store/pass that
value (e.g., the hrtfSourceId capture in AudioOutput.cpp and the spatialize()
invocation), and add the proper include for std::uintptr_t (<cstdint> or
<cstddef>).
| #ifdef USE_HRTF | ||
| if (m_hrtfSpatializer) { | ||
| m_hrtfSpatializer->removeSource(hrtfSourceId); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the AudioOutput.cpp file around lines 239-242
fd -e cpp -e h | grep -i "AudioOutput" | head -20Repository: mumble-voip/mumble
Length of output: 397
🏁 Script executed:
# Run the provided script to locate removeBuffer/removeUser call sites
rg -n "removeBuffer\(|removeUser\(" --type cppRepository: mumble-voip/mumble
Length of output: 2132
🏁 Script executed:
# Check for HrtfSpatializer class definition and documentation
rg -n "class HrtfSpatializer" --type cpp --type hRepository: mumble-voip/mumble
Length of output: 117
🏁 Script executed:
# Look for the removeSource method definition in HrtfSpatializer
rg -n "removeSource" --type cpp --type hRepository: mumble-voip/mumble
Length of output: 402
🏁 Script executed:
# Examine AudioOutput.cpp around lines 239-242
sed -n '220,260p' src/mumble/AudioOutput.cppRepository: mumble-voip/mumble
Length of output: 1290
🏁 Script executed:
# Look at the removeBuffer and removeUser methods to understand thread context
sed -n '217,295p' src/mumble/AudioOutput.cppRepository: mumble-voip/mumble
Length of output: 2248
🏁 Script executed:
# Check HrtfSpatializer.h for thread documentation
cat src/mumble/HrtfSpatializer.hRepository: mumble-voip/mumble
Length of output: 3999
🏁 Script executed:
# Check HrtfSpatializer implementation around removeSource
sed -n '210,230p' src/mumble/HrtfSpatializer.cppRepository: mumble-voip/mumble
Length of output: 245
Defer HRTF source removal to the audio mixing thread.
HrtfSpatializer documentation explicitly states all methods must run on the audio mixing thread. However, removeBuffer() can be called from non-audio contexts (via the bufferInvalidated signal or from removeUser()), causing removeSource() at line 241 to violate thread affinity. Queue the removal for execution within the mix loop instead of calling it directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mumble/AudioOutput.cpp` around lines 239 - 242, removeSource() is being
called on HrtfSpatializer from non-audio threads (removeBuffer() via
bufferInvalidated or removeUser()), violating HrtfSpatializer's thread-affinity
rule; instead enqueue the removal to run on the audio mixing thread. Modify the
code path that currently calls m_hrtfSpatializer->removeSource(hrtfSourceId) to
push a task/command (e.g., a lambda or id) into the audio mixer's task/command
queue or a thread-safe pending-removals list that the mix loop processes, and in
the mix loop execute HrtfSpatializer::removeSource(hrtfSourceId) there; ensure
the enqueueing occurs where removeBuffer()/removeUser() can reach and that the
mix thread drains and performs the actual removeSource call.
| if (!buffer->pfVolume) { | ||
| buffer->pfVolume = new float[nchan]; | ||
| buffer->pfVolume[0] = -1.0f; | ||
| } | ||
| const float oldGain = (buffer->pfVolume[0] >= 0.0f) ? buffer->pfVolume[0] : gain; | ||
| buffer->pfVolume[0] = gain; | ||
| const float gainInc = (gain - oldGain) / static_cast< float >(frameCount); | ||
|
|
There was a problem hiding this comment.
Initialize all pfVolume channels in the HRTF path.
If HRTF is later disabled for an active buffer, the non‑HRTF path can read uninitialized pfVolume[1], which can spike gain ramps. Initialize all channels to -1.0f.
🛠️ Proposed fix
- if (!buffer->pfVolume) {
- buffer->pfVolume = new float[nchan];
- buffer->pfVolume[0] = -1.0f;
- }
+ if (!buffer->pfVolume) {
+ buffer->pfVolume = new float[nchan];
+ for (unsigned int s = 0; s < nchan; ++s) {
+ buffer->pfVolume[s] = -1.0f;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!buffer->pfVolume) { | |
| buffer->pfVolume = new float[nchan]; | |
| buffer->pfVolume[0] = -1.0f; | |
| } | |
| const float oldGain = (buffer->pfVolume[0] >= 0.0f) ? buffer->pfVolume[0] : gain; | |
| buffer->pfVolume[0] = gain; | |
| const float gainInc = (gain - oldGain) / static_cast< float >(frameCount); | |
| if (!buffer->pfVolume) { | |
| buffer->pfVolume = new float[nchan]; | |
| for (unsigned int s = 0; s < nchan; ++s) { | |
| buffer->pfVolume[s] = -1.0f; | |
| } | |
| } | |
| const float oldGain = (buffer->pfVolume[0] >= 0.0f) ? buffer->pfVolume[0] : gain; | |
| buffer->pfVolume[0] = gain; | |
| const float gainInc = (gain - oldGain) / static_cast< float >(frameCount); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mumble/AudioOutput.cpp` around lines 816 - 823, The allocation for
buffer->pfVolume in AudioOutput.cpp only initializes channel 0, leaving
pfVolume[1..nchan-1] uninitialized; modify the pfVolume allocation branch (the
code that does buffer->pfVolume = new float[nchan]) to initialize all elements
to -1.0f (e.g., loop i from 0 to nchan and set buffer->pfVolume[i] = -1.0f)
before using pfVolume[0] and applying gain so that all channels are safe if HRTF
is later disabled.
| <item row="9" column="1" colspan="3"> | ||
| <widget class="QCheckBox" name="qcbHrtf"> | ||
| <property name="toolTip"> | ||
| <string>Enable Head-Related Transfer Function (HRTF) binaural audio processing for headphones</string> | ||
| </property> | ||
| <property name="whatsThis"> | ||
| <string>When enabled, HRTF processing uses a measured head-related transfer function to render spatial audio binaurally. This provides improved elevation perception and front/back disambiguation compared to standard panning. Requires headphones for best effect.</string> | ||
| </property> | ||
| <property name="text"> | ||
| <string>Use HRTF binaural audio (headphones recommended)</string> | ||
| </property> | ||
| </widget> | ||
| </item> | ||
| <item row="10" column="1"> | ||
| <widget class="QLabel" name="qlHrtfFile"> | ||
| <property name="text"> | ||
| <string>HRTF File</string> | ||
| </property> | ||
| <property name="buddy"> | ||
| <cstring>qleHrtfFile</cstring> | ||
| </property> | ||
| </widget> | ||
| </item> | ||
| <item row="10" column="2"> | ||
| <widget class="QLineEdit" name="qleHrtfFile"> | ||
| <property name="toolTip"> | ||
| <string>Path to a custom SOFA file. Leave empty to use the default HRTF.</string> | ||
| </property> | ||
| <property name="placeholderText"> | ||
| <string>Default HRTF</string> | ||
| </property> | ||
| </widget> | ||
| </item> | ||
| <item row="10" column="3"> | ||
| <widget class="QPushButton" name="qpbHrtfBrowse"> | ||
| <property name="text"> | ||
| <string>Browse...</string> | ||
| </property> | ||
| </widget> | ||
| </item> |
There was a problem hiding this comment.
Add the new HRTF controls to the tab order.
The UI defines an explicit tab order, but the new HRTF widgets aren’t included, so keyboard navigation will skip them.
♿ Suggested tab order update
<tabstop>qsBloom</tabstop>
<tabstop>qsbBloom</tabstop>
+ <tabstop>qcbHrtf</tabstop>
+ <tabstop>qleHrtfFile</tabstop>
+ <tabstop>qpbHrtfBrowse</tabstop>
<tabstop>qcbLoopback</tabstop>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mumble/AudioOutput.ui` around lines 712 - 751, The tab order is missing
the new HRTF widgets, so add them into the existing tab sequence (e.g., in
setupUi or the .ui taborder section) by inserting setTabOrder calls (or the
equivalent UI XML <tabstops>) to include qcbHrtf, qlHrtfFile (if focusable) /
qleHrtfFile, and qpbHrtfBrowse in the correct logical position; ensure qcbHrtf
comes before qleHrtfFile and qpbHrtfBrowse (or match surrounding controls) so
keyboard navigation visits qcbHrtf → qleHrtfFile → qpbHrtfBrowse in the expected
order.
| if (sofa && err == MYSOFA_OK) { | ||
| m_sofa = sofa; | ||
| m_irLeft.resize(static_cast< size_t >(m_filterLength)); | ||
| m_irRight.resize(static_cast< size_t >(m_filterLength)); | ||
| qDebug("HrtfSpatializer: Loaded SOFA file \"%s\" (filterLen=%d @ %d Hz)", | ||
| qPrintable(path), m_filterLength, m_mixerFreq); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Reloading HRTF should invalidate per-source convolvers.
After a successful reload, existing sources keep old IRs until a direction change occurs. Static sources will never update. Clear or reinit per-source state on successful load.
🔄 Proposed fix
if (sofa && err == MYSOFA_OK) {
m_sofa = sofa;
m_irLeft.resize(static_cast< size_t >(m_filterLength));
m_irRight.resize(static_cast< size_t >(m_filterLength));
+ m_sources.clear();
qDebug("HrtfSpatializer: Loaded SOFA file \"%s\" (filterLen=%d @ %d Hz)",
qPrintable(path), m_filterLength, m_mixerFreq);
return true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mumble/HrtfSpatializer.cpp` around lines 64 - 71, After successfully
loading a new SOFA (the block that sets m_sofa, resizes m_irLeft/m_irRight and
returns true), invalidate or reinitialize per-source convolution state so
existing sources do not keep old IRs: locate the HrtfSpatializer reload/init
routine containing m_sofa, m_irLeft, m_irRight, m_filterLength and ensure you
clear or reset any per-source convolvers (e.g. the container that holds source
convolvers or per-source IR caches) by either destroying/recreating them or
marking them dirty so they rebuild with the new HRTF; follow the same reset
logic used when a source changes direction to update all static sources
immediately after the SOFA load succeeds.
| float delayL, delayR; | ||
| mysofa_getfilter_float(m_sofa, sofaX, sofaY, sofaZ, | ||
| m_irLeft.data(), m_irRight.data(), &delayL, &delayR); | ||
|
|
||
| if (!set.convL) { | ||
| set.convL = std::make_unique< fftconvolver::FFTConvolver >(); | ||
| set.convR = std::make_unique< fftconvolver::FFTConvolver >(); | ||
| } | ||
|
|
||
| set.convL->init(static_cast< size_t >(m_blockSize), | ||
| m_irLeft.data(), static_cast< size_t >(m_filterLength)); | ||
| set.convR->init(static_cast< size_t >(m_blockSize), | ||
| m_irRight.data(), static_cast< size_t >(m_filterLength)); | ||
| set.initialized = true; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect FFTConvolver init/process contracts
rg -n "class FFTConvolver|void init|bool init|int init|return" 3rdparty/fftconvolver/FFTConvolver.h -C2Repository: mumble-voip/mumble
Length of output: 138
🌐 Web query:
libmysofa mysofa_getfilter_float return value error handling documentation
💡 Result:
mysofa_getfilter_float(...) has no return value (it’s a void C API / extern "C" fn ... with no return in Rust bindings), so there is no direct “return-code” based error handling for this call. It is expected to always fill the output buffers/delays for a valid, opened MYSOFA_EASY*. [1], [2]
Where errors are reported instead (documented path):
- Errors are reported when opening/loading a SOFA file via
mysofa_open*/mysofa_open*_advanced, using theint *errout-parameter (andNULLreturn on failure), as shown in the project usage docs. [3] - The error codes are the
MYSOFA_*constants (e.g.,MYSOFA_OK,MYSOFA_READ_ERROR,MYSOFA_INVALID_FORMAT,MYSOFA_NO_MEMORY, etc.). [2]
Practical handling guidance (based on the documented API shape):
- Check
hrtf != NULLaftermysofa_open*and handleerr. - Only call
mysofa_getfilter_floatafter a successful open; otherwise there’s no additional per-call error code to inspect. [3]
Sources
- Rust binding signature for
mysofa_getfilter_float(no return type): https://docs.rs/libmysofa-sys/latest/libmysofa_sys/fn.mysofa_getfilter_float.html MYSOFA_*error-code constants list (where errors are surfaced in the API): https://docs.rs/libmysofa-sys/latest/libmysofa_sys/- libmysofa usage docs showing
mysofa_open(...,&err)/NULLcheck andmysofa_getfilter_float(...)usage: https://hoene.github.io/libmysofa/
🏁 Script executed:
#!/bin/bash
# Find FFTConvolver header
fd -name "FFTConvolver.h"Repository: mumble-voip/mumble
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Find and examine HrtfSpatializer.cpp context around initialization
fd -name "HrtfSpatializer.cpp" -exec head -150 {} \; | grep -A 50 -B 10 "m_sofa\|mysofa_open"Repository: mumble-voip/mumble
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Search for FFTConvolver usage and init patterns across codebase
rg -n "FFTConvolver\|init\(" --type cpp -C 2 | head -80Repository: mumble-voip/mumble
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Find FFTConvolver header properly
fd FFTConvolver.hRepository: mumble-voip/mumble
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Find HrtfSpatializer.cpp
fd HrtfSpatializer.cppRepository: mumble-voip/mumble
Length of output: 146
🏁 Script executed:
#!/bin/bash
# Search for FFTConvolver class definition
rg "class FFTConvolver|void init|bool init" --type cppRepository: mumble-voip/mumble
Length of output: 4717
🏁 Script executed:
#!/bin/bash
# Read HrtfSpatializer.cpp to see the context
cat src/mumble/HrtfSpatializer.cppRepository: mumble-voip/mumble
Length of output: 7817
🏁 Script executed:
#!/bin/bash
# Search for FFTConvolver includes and external references
rg "FFTConvolver|#include.*fft" --type cpp --type hRepository: mumble-voip/mumble
Length of output: 610
🌐 Web query:
FFTConvolver init return type error handling C++ audio library
💡 Result:
In the widely used C++ audio convolution library HiFi-LoFi/FFTConvolver, FFTConvolver::init(...) is explicitly designed as a simple success/fail API:
- Signature / return type:
bool init(size_t blockSize, const Sample* ir, size_t irLen);(raw.githubusercontent.com) - When it returns
false: only ifblockSize == 0(it callsreset()first, then fails). (raw.githubusercontent.com) - When it returns
true:- Normal successful initialization. (raw.githubusercontent.com)
- Also returns
trueif the impulse response becomes effectively empty (it trims near-zero samples at the end; ifirLenreaches 0 it returnstrueearly). (raw.githubusercontent.com)
Implication for error handling: there’s no richer error reporting (no error codes/enum, no exceptions); from the library’s perspective, the only “init error” you can detect via the return value is blockSize == 0. (raw.githubusercontent.com)
Citations:
- 1: https://raw.githubusercontent.com/HiFi-LoFi/FFTConvolver/non-uniform/FFTConvolver.h
- 2: https://raw.githubusercontent.com/HiFi-LoFi/FFTConvolver/non-uniform/FFTConvolver.cpp
- 3: https://raw.githubusercontent.com/HiFi-LoFi/FFTConvolver/non-uniform/FFTConvolver.cpp
- 4: https://raw.githubusercontent.com/HiFi-LoFi/FFTConvolver/non-uniform/FFTConvolver.cpp
- 5: https://raw.githubusercontent.com/HiFi-LoFi/FFTConvolver/non-uniform/FFTConvolver.cpp
🏁 Script executed:
#!/bin/bash
# Search for any FFTConvolver references, documentation, or build configuration
rg "fftconvolver|FFTConvolver" --type toml --type cmake --type txt -iRepository: mumble-voip/mumble
Length of output: 693
Check FFTConvolver::init return value before marking convolver as initialized.
FFTConvolver::init returns bool and can fail (specifically when blockSize == 0). The code currently ignores the return value and unconditionally sets set.initialized = true at line 109, which could leave uninitialized convolvers marked as ready. This should be checked before proceeding.
(Note: mysofa_getfilter_float is a void function with no return value; libmysofa error reporting happens during file open via mysofa_open, which is already properly handled in loadHRTF.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mumble/HrtfSpatializer.cpp` around lines 100 - 113, The code currently
ignores FFTConvolver::init's boolean return; update the block that calls
set.convL->init(...) and set.convR->init(...) to capture both return values, and
only set set.initialized = true if both inits return true (and handle the
failure case by leaving set.initialized false and optionally resetting or
releasing set.convL/set.convR); ensure you use m_blockSize and m_filterLength
when calling init and check for the zero-block-size failure path so
uninitialized convolvers aren't marked ready.
Replaces the previous bundled HRTF (an unnormalized libmysofa sample) with cipic_124.sofa from ValveSoftware/steam-audio@f88bd4e, which has a flat front-incidence response (±5 dB, 100 Hz–8 kHz) and avoids the spectral coloration that made spatialized audio sound like a bad mic. Adds data/hrtf/README.md documenting provenance, licensing, and guidance on choosing future HRTF files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
I got this building on windows and was able to test with some other people. For anyone interested and trusting, I published a build at https://github.com/hiinaspace/mumble/releases/tag/hrtf-experimental-0.1 , which also includes a simple "spatial room" plugin to make testing different positions easier than the manual placement plugin. Testing made me realize the .sofa file I originally included was unnormalized and sounded bad, so ede3832 replaces it with the default one from steam-audio, along with some research into how it was sourced. Once the general approach here looks solid, I'll (make an AI) address the coderabbit nits. |
1 participant
Implements part of #6597, as a separate option from the current panning and delay method:
I tested this manually talking to myself with the manual location plugin and it seems to work. I only tested the build on linux so far though.
When enabled, HRTF sofa files are parsed and interpolated using libmysofa (with a public domain default), and the resulting impulse responses are convolved with the mono audio using FFTConvolver.
libmysofa is linked to using cmake/pkgconfig. It's fairly widely packaged, but will require some downstream packaging work for repository maintainers. SOFA is a complex enough format that it's probably worth using a library.
FFTConvolver is vendored with a submodule, since it's fairly small. I did note that there's already
smallfftin 3rdparty; however it doesn't implement convolution. I'm ambivalent whether it's better to use FFTConvolver for this or implement a convolver on top of smallfft.Some edge cases and the double-buffered design are adapted from Steam Audio, and it uses the same default HRTF. So in theory this sounds the same as steam audio's binaural spatialization. However, the distance attenuation still uses the current implementation (not steam audio's more complex frequency-dependent distance attenuation). Nor is there any reverb or geometry attenuation or doppler shift. Steam-audio itself has all that stuff, so I also considered linking to steam-audio directly (it's Apache-licensed). For just HRTF though it didn't seem worth pulling in the entire library.
Disclosure: While I guided and reviewed everything, the design doc and code is all AI-generated, including the tests. I think I have pretty good general software engineering taste, but I am not a cpp dev, so I had to trust the AI's taste on the tactical implementation and cmake setup.
Checks