Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ipc4: mixin: Fix HiFi3 impl of 24-bit mixing #9792

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

serhiy-katsyuba-intel
Copy link
Contributor

AE_ADD24S() expects input arguments to be Q9.23 values. Therefore, negative 24-bit values in a 32-bit container should have their sign extended to the upper 8 bits. All other five implementations of 24-bit mixing (IPC3 mixer's generic and HIFI3, IPC4 mixin's generic, IPC4 mixin's mix with gain generic and HIFI3) perform sign extension prior to mixing and do not rely on samples being already sign-extended.

AE_ADD24S() expects input arguments to be Q9.23 values. Therefore,
negative 24-bit values in a 32-bit container should have their sign
extended to the upper 8 bits. All other five implementations of 24-bit
mixing (IPC3 mixer's generic and HIFI3, IPC4 mixin's generic, IPC4
mixin's mix with gain generic and HIFI3) perform sign extension prior
to mixing and do not rely on samples being already sign-extended.

Signed-off-by: Serhiy Katsyuba <[email protected]>
@serhiy-katsyuba-intel
Copy link
Contributor Author

Note for reviewers why without this fix CI still succeeds:

  • On Windows 24-bit audio in 32-bit container comes in upper 3 bytes (MSB 24in32 format). Host copier converts that to LSB 24in32 by shifting right by 8 bits. Such shifting extends sign, therefore samples coming to mixin were already sign extended.
  • Not sure why Linux CI succeeds without this fix. Probably some upper layer on Linux already does sign extension for 24in32 samples.

@kv2019i kv2019i requested a review from singalsu January 23, 2025 11:42
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Great catch! Seppo to check as well. I think in most cases this is hidden/non-issue as the 24bit in upper 32bit is the dominant format also in Linux, and the non-sign extended 24bit with data lowe lower bytes, is really only used when talking with hardware that requires this format.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@singalsu pls review

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.

4 participants