Skip to content

Fix h2m LFE remap for ambisonics to Sound System H (#48) - #49

Open
jingbo-marquis wants to merge 2 commits into
mainfrom
ISSUE-48
Open

jingbo-marquis wants to merge 2 commits into
mainfrom
ISSUE-48

Conversation

@jingbo-marquis

Copy link
Copy Markdown
Contributor

The remap compared LFE slot indices (lfe1/lfe2) against the matrix column index i instead of the output slot n being assigned. For Sound System H (lfe1 = 3, lfe2 = 9, 22 columns) the second LFE skip came one column late (map[8] = 9 instead of 10): column 8 was moved into the LFE2 slot and overwritten by LFE generation, and slot 10 kept a duplicate of column 10. Other layouts are unaffected because their indices never drift apart before the last skip. Skip LFE slots by the output slot being assigned:

while (n == lfe1 || n == lfe2) n++;

Only map[8] for Sound System H changes. Companion: AOMediaCodec/roar#1.

Signed-off-by: Jingbo Hou <jingbo.hou@samsung.com>

@yeroro yeroro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Is it possible to add a test that would have caught this?

- Add test_hoa_sound_system_h.c: renders one-hot HOA signals (1OA/3OA)
  to Sound System H and asserts non-LFE output slots are non-silent and
  pairwise distinct, catching remap misassignment (duplicate/silent slots)
- Add TEST_ASSERTF macro to the test framework for printf-style failure
  messages (reports the exact slot indices on failure)
- Register test_hoa_sound_system_h in both CMake and Bazel builds

Signed-off-by: Jingbo Hou <jingbo.hou@samsung.com>
@jingbo-marquis

Copy link
Copy Markdown
Contributor Author

Good catch! Is it possible to add a test that would have caught this?

added a test that covers this case: test_hoa_sound_system_h.c

@trsonic trsonic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

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.

3 participants