Skip to content

Conversation

@agosdahu
Copy link
Contributor

@agosdahu agosdahu commented Dec 5, 2024

Added Arch structure, containing function pointers to optimizable functions.
Original C implementations can be overridden at runtime with faster, CPU-specific implementations.

Copy link
Contributor

@hkchung72 hkchung72 left a comment

Choose a reason for hiding this comment

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

It's good suggestion.

@yilun-zhangs
Copy link
Collaborator

Hi Agosdahu,
Because this patch is only about optimization based on ARM CPU architecture, but not IAMF functional things,
so I think it is better to add pre-processor definition in CmakeLists.txt to disable it defaultly.

@agosdahu
Copy link
Contributor Author

Hi @yilun-zhangs ,
May I ask for clarifications first?
This patch introduces no actual optimisation, it only prepares the possibility to separate platform dependent code (currently arm and x86 only) for upcoming optimisations. (you can find two more patches, planned to be submitted, on the source fork of this PR)

I am unsure which of the following options would you have me do:

  1. Have a flag in CMake which enables this separation altogether, set to disabled by default and user have a choice.
  2. Migrate the target platform detection from checking the compiler provided standard macros to having some CMake logic to define symbols, such as IAMF_ARCH_DETECTED_ARM?
  3. Have both solution 1. and 2. together
  4. Something else entirely

The current solution was mindful about e.g.: the existence of the VS2022 project file as well and aimed to provide a universal, build system agnostic and portable solution.

@yilun-zhangs
Copy link
Collaborator

Hi @agosdahu

Thanks for clarifying it to me detailly.

In my opinion, solution 1 is necessary.
As you said, the solution is aimed to provide a universal, build system, I think soluiton 2 is alternative, it depends on your convenience.

Thanks for your understanding.

@agosdahu
Copy link
Contributor Author

Hi @yilun-zhangs

Thank you for clarifying!
I am happy to push the requested change, however, could you confirm that you want all architecture specific code to be disabled by default?

Enabling them by default may be more beneficial given the current configuration of CI builds and especially if users generally prefer performant code.

@yilun-zhangs
Copy link
Collaborator

yilun-zhangs commented Feb 5, 2025

Hi @agosdahu
I'm afraid the architecture specific code would affect the building on other platforms, so I think it is better to disabled by default,
If users prefer performance code, they could open it manually.

Of course, in the future, if all architecture specific code are complete, we could consider to open it by default.

@jwcullen
Copy link
Collaborator

jwcullen commented Feb 5, 2025

@yilun-zhangs,

With IAMF supporting so many channels, any optimizations are beneficial. Performance gains can be particularly significant on certain devices. It's not uncommon for this type of thing. Other "reference implementation" libraries, like libopus have platform specific optimizations.

This PR simply adds a slot for platform specific code, but when there is no platform-specific code it will use the original implementation for those functions. So it seems OK to default it to on. It seems to work fine even if optimizations are added one at a time. Missing functions will fallback to the original implementation. Also, the GitHub actions CI still proves it builds on several platforms.

@agosdahu, can you sync to the latest upstream. I think the MacOS CI is failing because you are missing 4826ee2.

Added Arch structure, containing function pointers to optimizable
functions. Original C implementations can be overridden at runtime with
faster, CPU-specific implementations.
@yilun-zhangs yilun-zhangs merged commit d69085c into AOMediaCodec:main Feb 21, 2025
3 checks passed
@yilun-zhangs
Copy link
Collaborator

Hi @agosdahu
I have merged this PR, but there is one error while compiling VS2022 Project
src\iamf_dec\arch.h(25,1): error C2016: C requires that a struct or union have at least one member
could you fix it firstly?

@agosdahu agosdahu deleted the cpu_arch_spec branch February 21, 2025 07:21
@agosdahu
Copy link
Contributor Author

@yilun-zhangs
PR #134 addresses the empty struct issue as well.
I could also open a PR with only the empty struct fix if needed

Luiz-Monad pushed a commit to Luiz-Monad/libiamf that referenced this pull request Sep 15, 2025
Introduce possibility of CPU-specific optimization
Luiz-Monad pushed a commit to Luiz-Monad/libiamf that referenced this pull request Sep 15, 2025
Introduce possibility of CPU-specific optimization
Luiz-Monad pushed a commit to Luiz-Monad/libiamf that referenced this pull request Sep 28, 2025
Introduce possibility of CPU-specific optimization
Luiz-Monad pushed a commit to Luiz-Monad/libiamf that referenced this pull request Oct 6, 2025
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.

5 participants