Skip to content

Fix GCC-PHAT normalization pointer desync in beamformer#92

Open
goyalpalak18 wants to merge 2 commits intoeembc:mainfrom
goyalpalak18:fix/abf-phat-normalization
Open

Fix GCC-PHAT normalization pointer desync in beamformer#92
goyalpalak18 wants to merge 2 commits intoeembc:mainfrom
goyalpalak18:fix/abf-phat-normalization

Conversation

@goyalpalak18
Copy link

File + Function

  • File: src/ee_abf_f32.c
  • Function: beamformer_f32_run()

Exact Problem

I noticed that the PHAT normalization loop uses a continue to skip zero-magnitude bins, but it only advances the PHATNORM read pointer (pf32_1) and totally forgets to advance the XY complex array write pointer (pf32_2). Every time a bin has a zero magnitude, pf32_1 advances but pf32_2 stays put. The moment we hit a single zero bin, the pointers desync. From that point forward, every single division applies the wrong bin's magnitude to the wrong XY complex pair.

Root Cause

This boils down to two compounding errors:

  1. Missing epsilon floor: The original Matlab reference explicitly specifies max(abs(XY), 1e-12) to guarantee PHATNORM is never zero. We missed this clamp in the C implementation, allowing th_cmplx_mag_f32() to output exact zeros.
  2. Pointer logic error: When trying to handle those exact zeros to avoid a divide-by-zero, the continue statement skips pf32_2 += 2. The intent was right, but it violates the loop invariant since pf32_2 still needs to advance to stay in sync with pf32_1.

Impact After Fix

  • GCC-PHAT normalization now correctly divides every XY bin by its corresponding magnitude.
  • Beam direction estimation (icorr) is finally accurate.
  • Beamformer output quality directly matches the Matlab reference implementation.
  • All downstream components (AEC, ANR, KWS) receive correctly beamformed audio.
  • AudioMark benchmark scores are fully valid and reproducible across platforms, eliminating a class of silent data corruption that could bias hardware evaluation results.

Signed-off-by: goyalpalak18 <goyalpalak1806@gmail.com>
@llefaucheur
Copy link
Contributor

llefaucheur commented Feb 16, 2026

Thank you for reporting this issue.
It's indeed strange to have this code sequence "if (ftmp == 0) continue;" but in practice, ftmp is only zero when all input samples are zero, which is also a special case where the beamforming process can be ignored.
To clean the code and avoid updating the Audiomark scores, I suggest replacing it with "if (ftmp == 0) ftmp=1;"

Signed-off-by: goyalpalak18 <goyalpalak1806@gmail.com>
@goyalpalak18
Copy link
Author

@llefaucheur Makes sense. Dropped my old logic and applied your fix. Pushed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants