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

Add Android audio drivers based on OpenSLES and Oboe #464

Merged
merged 184 commits into from
Mar 27, 2019

Conversation

atsushieno
Copy link
Contributor

( A related but very outdated PR on this topic https://sourceforge.net/p/fluidsynth/code-git/merge-requests/3/ )

This set of changes brings audio drivers for Android, either OpenSLES or Oboe. The changes in the original sources are kept minimal so that it should be easily maintained, as long as the build system works (which is actually somewhat complicated).

There are still couple of changes in the original tree, regarding custom sfloader for Android. Hopefully they are harmless to others who don't care about Android.

(I had a couple of other fixes when Android NDK had switched to clang, but they were already fixed in master, which thus reduced significant amount of my changes!)

I know there is another Oboe-based implementation by @VolcanoMobile, but their sources are quite different from master branch here and I see no open code, so I didn't care about any conflict (e.g. android directory collision).

See android/README.Android.md for more details.

(squashed merge recommended, there's a handful of experimental changes and reverts.)

It's annoying to be asked root permission every time.
…ch of issues.

- With this mode latency adjustment became totally unnecessary, therefore
  atsushieno/fluidsynth-midi-service-j#2 is practically fixed.
- With this fix atsushieno/fluidsynth-midi-service-j#4 is completely gone.
@derselbst
Copy link
Member

I made cmake to only query for C++ if -Denable-oboe=1. Also, all those linker errors when building with ubsan occurred due to the missing -fsanitize flag when linking. This should be fixed as well. However, ubsan runtime lib requires NDK r17 or newer. This should be fine though.

The fluid_opensles.c source isn't strictly C90 complaint. But since usually clang or gcc will be used to compile this and as long as nobody complains, this should be fine as well.

I have a slight preference to move the android/ folder into the doc/ folder. Let me know if would cause too much trouble for you.

Apart from that I consider this to be ready to merge. In any case, please give it a last test and let me know whether you want to rebase the commit history. If you don't want to, I would squash it. In every case: before I press the merge button, I will format the code to match our style guide.

@atsushieno
Copy link
Contributor Author

Thanks for a bunch of code cleanup.

I have a slight preference to move the android/ folder into the doc/ folder. Let me know if would cause too much trouble for you.

No, it is totally fine. If those android-specific code is awkward or in the way, I can publish them somewhere outside. I also have some work-in-progress code for new Android Q AMIDI that currently stays near it (also relocatable).

@atsushieno
Copy link
Contributor Author

And forgot to mention, I'm okay with either style for rebasing or squashing. Thanks.

@derselbst
Copy link
Member

If those android-specific code is awkward or in the way, I can publish them somewhere outside.

Not at all. The doc/ directory is absolutely fine.

And forgot to mention, I'm okay with either style for rebasing or squashing.

Ok then. I will squash this in 2 to 3 days, having some time for a final test run.


This really is a great feature for fluidsynth, thank you very much! Looking forward to Q AMIDI!

@atsushieno
Copy link
Contributor Author

I think e7496a9 has caused some regression:

2019-03-26 19:57:26.514 2683-2683/name.atsushieno.fluidsynthmidideviceservicej E/AndroidRuntime: FATAL EXCEPTION: main
    Process: name.atsushieno.fluidsynthmidideviceservicej, PID: 2683
    java.lang.UnsatisfiedLinkError: Unable to load library 'fluidsynth':
    dlopen failed: library "libclang_rt.ubsan_standalone-i686-android.so" not found
    dlopen failed: library "libclang_rt.ubsan_standalone-i686-android.so" not found
    dlopen failed: library "libclang_rt.ubsan_standalone-i686-android.so" not found

libclang_rt.ubsan_standalone-i686-android.so exists but it is like 2.5MB of shared library that we app developers wouldn't like to add as extra dependency. Could we bring back old ubsan related build options?

@atsushieno
Copy link
Contributor Author

This actually fixed it (but probably you want different solution).

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 240ab1d..1cf7dc8 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -199,6 +199,13 @@ if ( CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID STREQUAL "Clang" OR CMAKE_C_
   
 endif ( CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID STREQUAL "Clang" OR CMAKE_C_COMPILER_ID STREQUAL "Intel" )
 
+# Options for CLANG only
+if ( CMAKE_C_COMPILER_ID STREQUAL "Clang" )
+  set ( CMAKE_C_FLAGS_DEBUG "${CMAKE_C_FLAGS_DEBUG} -fsanitize-trap=undefined" )
+  set ( CMAKE_SHARED_LINKER_FLAGS_DEBUG "${CMAKE_SHARED_LINKER_FLAGS_DEBUG} -fsanitize-trap=undefined" )
+endif ( CMAKE_C_COMPILER_ID STREQUAL "Clang" )
+
+
 # Windows
 unset ( WINDOWS_SUPPORT CACHE )
 unset ( WINDOWS_LIBS CACHE )

@derselbst
Copy link
Member

derselbst commented Mar 26, 2019

libclang_rt.ubsan_standalone-i686-android.so exists but it is like 2.5MB of shared library that we app developers wouldn't like to add as extra dependency.

This would only affect debug builds, which app developers probably wouldn't deploy.

But anyway, since ubsan is currently only useful for me, let's get rid of this hack and add an option to enable it explicitly on demand. Doing so will also compel you to provide it at runtime: edb8707

...unless ofc you prefer to override the CFLAGS and append -fsanitize-trap=undefined

@atsushieno
Copy link
Contributor Author

Okay, I confirmed that the latest changes worked for me.

@derselbst
Copy link
Member

Great, thanks!

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

Successfully merging this pull request may close these issues.

3 participants