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

[code] add dnn sources to CMake when compiling on arm with RTCD #340

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

Conversation

pbodilis
Copy link

Compiling the project with the following in a CMakeLists.txt:

set(OPUS_BUILD_TESTING OFF CACHE INTERNAL "")
set(OPUS_BUILD_PROGRAMS OFF CACHE INTERNAL "")


# Machine learning features are available only for amr64 (on arm)
if(${ANDROID_ABI} STREQUAL arm64-v8a)
    set(SW_OPUS_ML ON CACHE INTERNAL "")
else()
    set(SW_OPUS_ML OFF CACHE INTERNAL "")
endif()

set(OPUS_DEEP_PLC ${SW_OPUS_ML} CACHE INTERNAL "")
set(OPUS_DRED ${SW_OPUS_ML} CACHE INTERNAL "")
set(OPUS_OSCE ${SW_OPUS_ML} CACHE INTERNAL "")
set(OPUS_DNN ${SW_OPUS_ML} CACHE INTERNAL "")

# BUILD_SHARED_LIBS which is false by default on the project's scope
# CMAKE_POSITION_INDEPENDENT_CODE is set to ON on the project's scope in the main cmakelists
FetchContent_Declare(
    opus
    URL ${ARCHIVE_OPUS}
    SOURCE_DIR  ${CMAKE_CURRENT_SOURCE_DIR}/${DIRLIBOPUS}
)
FetchContent_MakeAvailable(opus)

This does compile using NDK 26.2, but linkage fails with "undefined symbols" DNN_COMPUTE_CONV2D_IMPL DNN_COMPUTE_LINEAR_IMPL DNN_COMPUTE_ACTIVATION_IMPL

The proposed merge request fixes the linkage issue

@j-schultz
Copy link

We've been running into the same issue with our armv7 builds (arm64 builds were fine). With this patch the DNN_COMPUTE_ACTIVATION_IMPL and DNN_COMPUTE_CONV2D_IMPL problems are gone, but DNN_COMPUTE_LINEAR_IMPL is still not found, so I suppose there is still a difference in the CMake build somewhere regarding different ARM architectures (we're configuring both architectures with the same features, just to see if the ML features are even feasible on armv7).

@pbodilis
Copy link
Author

I personally deactivated the use of the ML-based features for armv7, as it seems to require neon to run. armv7-only devices are pretty rare anyway, too bad for them :)

@j-schultz
Copy link

Yeah, in the meantime I have also resorted to just disabling this option for ARMv7 on Android. It would be nice CMake outright refused to build this configuration if it ends up in linking errors. (just to be clear, I don't expect that to be adressed in this PR, but if Opus devs see this, maybe that would be some inspiration to make the CMake script a bit more sturdy).

@jmvalin
Copy link
Member

jmvalin commented Apr 26, 2024

I personally deactivated the use of the ML-based features for armv7, as it seems to require neon to run. armv7-only devices are pretty rare anyway, too bad for them :)

Technically, the code can run without Neon and can use RTCD to turn Neon on-off dynamically. Practically, I don't think there's any non-Neon ARM chips that will run the DNN C code in real-time. Even 32-bit Neon chips will likely struggle. That being said, I think there are ARMv8 chips running 32-bit binaries and those should be able to run the DNN code with Neon. And those should be possible to detect at run-time to avoid running the DNN code on CPUs that are too slow.

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