OpenSLES candidate #638

croissanne · 2021-09-05T20:01:37Z

Since PortAudio is on GH now I figured I might as well make a PR. I had some conversations on mailing lists & the old tracker ages ago but that sort of died down.

This is an OpenSLES hostapi for PortAudio and up, for Android 14 and up. And I would love some feedback on it. I added a README.md under src/hostapi/opensles which has some extra notes.

Because testing this on Android is a bit of a hassle, there might be some blind spots, but most of the tests included under test/ work fine.

I'm sure there are also some areas where it could conform better to the PortAudio (hostapi-)api.

There's still loads of potential improvements, like exposing more of the OpenSLES specific features, like the ability to choose a performance mode. I think there might be some support for sample rate conversion if low-latency isn't enabled (which is the default). Also croissanne/portaudio_opensles#14 needs investigating.

Linking #154 and #281

Also has contributions from @jwinarske for CMake support and bugfixes, and @darksylinc for more bugfixing.

Output parameters were used to initialize the input stream. Use the input parameters instead.

Pa_CloseStream would call pa_opensles.c's StopStream This would in turn call PaUnixThread_Terminate which looks at the thread's returns value via pthread_join. If the thread's return value is not 0, portaudio interprets there's a pointer to free. However because StreamProcessingCallback never returns a value nor calls PaUnixThreading_EXIT, there's only garbage; causing a crash when attempting to free that garbage ptr.

philburk · 2021-09-06T00:28:37Z

Thank you for this contribution. But please note that OpenSL ES on Android is deprecated in favor of Oboe. Oboe is simpler and has many advantages over OpenSL ES.

https://listserv.cuit.columbia.edu/scripts/wa.exe?A2=PORTAUDIO;e3ed65e8.2102C&S=b

The design of Oboe was inspired by PortAudio. So it should be much easier to implement an Oboe backend.

https://github.com/google/oboe

darksylinc · 2021-09-06T01:30:24Z

Thank you for this contribution. But please note that OpenSL ES on Android is deprecated in favor of Oboe. Oboe is simpler and has many advantages over OpenSL ES.

That's cool but it is sounding like the PR will be rejected for that reason. To that end:

There's currently no Android backend. An OpenSL ES is infinitely better than no Android backend. Currently there is no way to get Android sound via portaudio; while this is a 4-year-old tested backend. It works out of the box and passes both ASAN* and static analysis** and has no compiler warnings
OpenSL ES backend supports older versions of Android oboe can't because OSLES predates it.
Other platforms also have multiple backends (e.g. jack vs alsa vs oss; dsound vs asio vs wasapi vs wdm-ks vs wmme). An Oboe backend can come up later; where portaudio can dynamically select between the two based on availability and superiority; like it does on other platforms

The OSLES backend may not be the best one; but at least it fulfills the role of getting audio output/input.

* I'm not a dev of this backend. I am a user of relatively new portaudio that checked out the OpenSL ES fork because it was the only thing available for Android, and I was in charge of running tests on that backend to ensure it met minimum quality. To that end we ran ASAN (that's how we detected a bug and why I am now listed as a contributor), static analysis and overall code review of the backend.

** scan-build could only detect one issue in line 721 of pa_opensles.c where a nullptr may be dereferenced if portaudio user decides to use the portaudio API without specifying both output / no speakers and without input / no mic; i.e. an invalid usage of portaudio:

    if( framesPerBuffer == paFramesPerBufferUnspecified ) {
        if( outputParameters ) {
            framesPerHostBuffer = (unsigned long) (outputParameters->suggestedLatency * sampleRate);
        }
        else {
            framesPerHostBuffer = (unsigned long) (inputParameters->suggestedLatency * sampleRate);
        }
    }

To the analyzer there is no guarantee inputParameters won't be nullptr if outputParameters is also a nullptr; however we didn't think there was a point of reporting it because this would be an invalid portaudio usage.

While we can't guarantee Gundersanne's work is a high quality implementation; it appears like it is (enough to convince us) and we were extremely surprised it worked out of the box without problems, which is a rare occurrence whenever an Android project is involved.

philburk · 2021-09-06T02:18:49Z

Thank you for the analysis. I agree that this seems like a very good quality implementation. The problem is not with this code but with OpenSL ES. OpenSL ES has many severe limitations that are addressed with AAudio/Oboe.

OpenSL ES backend supports older versions of Android oboe can't because OSLES predates it.

Actually Oboe is a supports both AAudio and OpenSL ES. Oboe uses AAudio after Android 8.1 (API 27) and falls back to using OpenSL ES on earlier versions. Oboe runs on devices back to API 16. It probably runs on API 14 but it is hard to find devices that old for testing.

Other platforms also have multiple backends

There is currently no need for multiple back ends on Android. An Oboe back end is sufficient.

We could add this implementation now and then add an Oboe implementation later. But once we add an OpenSL ES implementation then it would be very difficult to remove it without breaking some app.

OpenSL ES is deprecated and all work on it is frozen. For native Android APIs we are putting our efforts into AAudio/Oboe.

philburk · 2021-09-06T02:19:27Z

Sorry. I closed it accidentally. Discussion is still open.

croissanne · 2021-09-06T08:28:51Z

The bulk of this was written a while back, and Android is a bit of a moving target. But imo if it's deprecated (I didn't realize actually ^_^) then let's leave it at that, no point in maintaining or merging deprecated stuff.

It will live on in my fork regardless. I'll ultimately leave it up to you to decide if this is worth having, but I'll close this for now then.

Be-ing · 2021-09-06T13:19:34Z

CMakeLists.txt

@@ -278,6 +278,16 @@ elseif(UNIX)
    target_compile_definitions(PortAudio PRIVATE PA_USE_COREAUDIO=1)
    set(PKGCONFIG_LDFLAGS_PRIVATE
      "${PKGCONFIG_LDFLAGS_PRIVATE} -framework CoreAudio -framework AudioToolbox -framework AudioUnit -framework CoreFoundation -framework CoreServices")
+  elseif(ANDROID)


Why is this gated on both UNIX and ANDROID? OpenSLES is not exclusive to either. I think this should be treated like JACK and allowed on all operating systems if the required library is found.

The implementation does contain some android-specific code, which could be preprocessored out where necessary of course to make this a universal unix implementation.

If you want to make this compatible with non-Android systems, great, but otherwise I think it's fine to add a comment here in CMakeLists.txt explaining this has Android specific code even though OpenSL ES is not exclusive to Android.

Be-ing · 2021-09-06T13:23:25Z

CMakeLists.txt

+      cmake_dependent_option(OPENSLES "Enable support for OpenSLES" ON OPENSLES_FOUND OFF)
+
+      if(OPENSLES)
+        target_link_libraries(PortAudio PRIVATE "${OPENSLES_LIBRARIES}")


Export an INTERFACE library target from the find module rather than directly using ${OPENSLES_LIBRARIES}

Be-ing · 2021-09-06T13:29:02Z

src/os/unix/pa_unix_hostapis.c

+#if PA_USE_OPENSLES
+        PaOpenSLES_Initialize,
+#endif


I'm not sure if there are any OpenSL ES implementations for Windows, but if there are, this should be added to pa_win_hostapis.c as well.

I had a look around, there seems to be 1 other implementation around according to wikipedia, and I'm pretty sure the 2nd implementation is not open source :/

IMO we should apply YAGNI

Even if OSLES is supported on Windows I can't imagine it being the better alternative to any of the other available backends

And if another platform is truly benefited from OSLES, then that can be evaluated when the time arrives

Be-ing · 2021-09-06T13:30:00Z

Unless someone is actively working on an Oboe backend or intends to do so imminently, I think it would be good to merge this.

Be-ing · 2021-09-06T14:08:22Z

Please add a condition to the cmake_build.yml matrix to build this on GitHub Actions. You can use https://github.com/marketplace/actions/setup-android-ndk

Be-ing · 2021-09-06T14:34:42Z

If this is adapted to build without Android, it could be useful to merge even if there is also an Oboe backend.

croissanne · 2021-09-06T14:41:19Z

@Be-ing Thank you for the review, & all reasonable suggestions. Especially the CMake stuff needs a little more work I agree.

I'll reopen again for visibility, to see what some of the other maintainers think. I don't really have plans to implement an Oboe backend. Regarding this PR I'm happy either way, and depending on what the consensus is I'm of course happy to finish it up according to review comments.

jwinarske · 2021-09-06T15:02:33Z

@Be-ing happy to make changes to the CMake. Also would consider implementing Oboe backend.

philburk · 2021-09-06T15:08:11Z

I don't know of any other platforms that support OpenSL ES.

Even if OSLES is supported on Windows I can't imagine it being the better alternative to any of the
other available backends

I agree. I think OpenSL ES is a dead end.

Be-ing · 2021-09-06T15:10:51Z

Also would consider implementing Oboe backend.

That would be great!

I don't know of any other platforms that support OpenSL ES.

In that case, if @jwinarske (or anyone else) writes an Oboe backend, let's not merge this.

philburk · 2021-09-06T18:57:38Z

I am currently working on an Oboe backend for PortAudio.

croissanne and others added 12 commits September 5, 2021 21:44

opensles: OpenSLES hostapi

a1ae20a

CMake: Support for OpenSLES

3152c76

opensles: Fix segfault when opening input stream only

f607c51

Output parameters were used to initialize the input stream. Use the input parameters instead.

opensles: Fix warning about memset being implicitly declared

825a914

opensles: Mark interthread communication variables volatile

2424017

opensles: Add building and testing instructions in README

d8d6d70

opensles: Honor stream abort call

23f5220

opensles: Split input and output into different structs

b22ab53

opensles: Removed unused volume itf

2f91184

opensles: Removed unused header

55d55b6

opensles: Float support for audio output for Android 21 and above

72e9692

philburk closed this Sep 6, 2021

philburk reopened this Sep 6, 2021

croissanne closed this Sep 6, 2021

Be-ing reviewed Sep 6, 2021

View reviewed changes

croissanne reopened this Sep 6, 2021

philburk closed this Sep 6, 2021

mgeier mentioned this pull request Nov 20, 2021

Is it possible to use it for android? spatialaudio/python-sounddevice#381

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenSLES candidate #638

OpenSLES candidate #638

croissanne commented Sep 5, 2021 •

edited

Loading

philburk commented Sep 6, 2021

darksylinc commented Sep 6, 2021 •

edited

Loading

philburk commented Sep 6, 2021

philburk commented Sep 6, 2021

croissanne commented Sep 6, 2021 •

edited

Loading

Be-ing Sep 6, 2021

croissanne Sep 6, 2021

Be-ing Sep 6, 2021

Be-ing Sep 6, 2021 •

edited

Loading

Be-ing Sep 6, 2021

croissanne Sep 6, 2021

darksylinc Sep 6, 2021

Be-ing commented Sep 6, 2021

Be-ing commented Sep 6, 2021

Be-ing commented Sep 6, 2021

croissanne commented Sep 6, 2021 •

edited

Loading

jwinarske commented Sep 6, 2021

philburk commented Sep 6, 2021

Be-ing commented Sep 6, 2021

philburk commented Sep 6, 2021

OpenSLES candidate #638

OpenSLES candidate #638

Conversation

croissanne commented Sep 5, 2021 • edited Loading

philburk commented Sep 6, 2021

darksylinc commented Sep 6, 2021 • edited Loading

philburk commented Sep 6, 2021

philburk commented Sep 6, 2021

croissanne commented Sep 6, 2021 • edited Loading

Be-ing Sep 6, 2021

Choose a reason for hiding this comment

croissanne Sep 6, 2021

Choose a reason for hiding this comment

Be-ing Sep 6, 2021

Choose a reason for hiding this comment

Be-ing Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

Be-ing Sep 6, 2021

Choose a reason for hiding this comment

croissanne Sep 6, 2021

Choose a reason for hiding this comment

darksylinc Sep 6, 2021

Choose a reason for hiding this comment

Be-ing commented Sep 6, 2021

Be-ing commented Sep 6, 2021

Be-ing commented Sep 6, 2021

croissanne commented Sep 6, 2021 • edited Loading

jwinarske commented Sep 6, 2021

philburk commented Sep 6, 2021

Be-ing commented Sep 6, 2021

philburk commented Sep 6, 2021

croissanne commented Sep 5, 2021 •

edited

Loading

darksylinc commented Sep 6, 2021 •

edited

Loading

croissanne commented Sep 6, 2021 •

edited

Loading

Be-ing Sep 6, 2021 •

edited

Loading

croissanne commented Sep 6, 2021 •

edited

Loading