Add AAudio backend #559
Add AAudio backend #559
Conversation
|
Thank you for that. It needs more iterations since it's big and I need to catch up with the AAudio specifics. For now, I am answering some of your questions
That's correct. However, we need to guarantee that the
Yes. It's not a big problem but other backends do not do that so it's better to follow the same behavior here and not fire the stop event on drain. A cubeb client must call explicitly |
There's no hurry. I'm still fixing up some details as well.
Makes sense, this is already guaranteed in the backend.
Okay, I changed that. |
|
Regarding the supported methods I would say that we need to support at least the methods that the OpenSL ES backend supports (check here). From a quick look, we are missing only two. Do you think we can add support to those easily? I am also adding more people to look at it. Thank you! |
|
AAudio has no volume concept but |
|
For clarification: we can implement |
Hmm I don't think that this is what we want here. I believe the jni stuff we do on opensl es are adequate and relatively easy to import. That said, I am not sure how accurate the guessing can be. If you think that it can be good enough, please let us know, to consider it. |
|
@nyorain, thank you very much for doing this! This member is for doing A/V sync, it needs to be the time it take between returning from a callback that has output data to the time one hears this buffer physically. It's a static number, for a particular stream setting and device configuration (but is expected to change if the underlying device changes). As you can see, this is not the prettiest code, but has been determined to answer a correct number for a variety of use cases: Bluetooth device using hands-free, Bluetooth device using a2dp, normal wired headphones, speakers, external card, etc., and provide good A/V sync there (also for reporting the latency via We can certainly add it after merging this however, you've done enough work here :-). The volume is simpler, and we sometimes implement it in software as you saw. On some backends, it's implemented using OS APIs, this allows a particular scenario to work better:
If implemented in the cubeb backend, and depending on the OS, etc., this can produce a burst of audio at 1.0 (because it's still buffered somewhere in the OS), and then play at 0.1 volume. Using the APIs from the OS solves this (since the volume is applied later in the pipeline). Implementing in software is what we should do here I think. Thanks again for working on this! |
|
You're welcome, I'm using this library in my own projects and it works quite well, so thanks for writing it in the first place!
The reasoning for including the volume api in cubeb makes sense, I added a software implementation. With AAudio, the scenario you describe shouldn't be a problem (at least not if the restarting of the stream happens after the state callback was called with STOPPED) since stopping an output stream will still first output all buffered data.
I don't use the latency functionality or A/V sync and wouldn't be sure how to properly test that the jni latency functions return valid values for this backend so I guess it's indeed the best to move this to a separate PR. |
src/cubeb_aaudio.c
Outdated
| // NOTE: not sure about this. | ||
| // But when this stream contains input and output stream, their | ||
| // sample rates have to match, don't they? | ||
| assert(!target_sample_rate || target_sample_rate == input_stream_params->rate); |
There was a problem hiding this comment.
Yes, this is guaranteed by the function calling this.
| @@ -217,6 +224,11 @@ cubeb_init(cubeb ** context, char const * context_name, char const * backend_nam | |||
| #endif | |||
| #if defined(USE_OPENSL) | |||
| opensl_init, | |||
| #endif | |||
| // TODO: should probably be preferred over OpenSLES when available. | |||
| // Initialization will fail on old android devices. | |||
There was a problem hiding this comment.
You can change the order to do this: if a particular function fails, the following ones will be called, but if one succeeds, this function returns.
There was a problem hiding this comment.
Yes, I did that for testing and for my own cubeb builds. But should it already be done here at this stage of the backend?
src/cubeb_aaudio.c
Outdated
| // The DRAINING state means that we want to stop the streams but | ||
| // may not have done so yet. | ||
| // The aaudio docs state that returning STOP from the callback isn't | ||
| // enough, the stream has to be stopped from another threads |
|
Yes, this is ready for review. Just one minor thing left (that i know of): there currently can be crashes (failed ringbuffer assertion) when restarting an app with this backend since we use ALOGV. That seems to use a function-static/global variable and those are not always reset when the app restarts on android (but the audio thread will change obviously, triggering the invalid thread_id assertion in the async logger ringbuffer). I'll either have to remove the calls to ALOGV or explicitly reset the thread ids in the static ringbuffer in aaudio_init somehow. If we just consider ALOGV broken on android and move on (it's equally broken for the other android backends, note how it's never used there), that should at least be documented somewhere I guess. If you find the current threaded solution for handling stream state too complicated (using an additional notifier thread), I could revert a6422f6. See the commit message for more details (and a link to a post on the problem I did). Before this commit, we just used constant looping with short sleeping in between. |
Yeah, don't spend too muh time with that. If you have a handy solution go for it otherwise just remove the call and go on. The logic is not broken but there are limitations and they are not your fault. Please, document it as you said. |
There was a problem hiding this comment.
Created Bug 1609372 that will track the importing of the backend.
src/cubeb_aaudio.c
Outdated
| @@ -0,0 +1,1373 @@ | |||
| /* ex: set tabstop=2 shiftwidth=2 expandtab: | |||
There was a problem hiding this comment.
Would be possible to turn it to c++ instead of c.
src/cubeb_aaudio.c
Outdated
| new_state = STREAM_STATE_STOPPED; | ||
| stm->state_callback(stm, stm->user_ptr, CUBEB_STATE_DRAINED); | ||
| } | ||
|
|
|
Sorry, my review is not ready. I meant to make an individual comment about the bug. The two comments above are real but more will come. |
|
The backend was tested with my own dummy audio apps (that cover all cases like stop, drain, input + output or seperate streams). I could look into setting up a build environment for firefox on android but I expect that to be a lot of work, not sure when I'll have time for that tbh. Without symbols or android log I have no idea what the issue is, unfortunately. |
Is it something that you can send me privately and speed up my testing?
No need to spend time on that. We can go on as is for now and debug the Firefox case later. |
|
Did the crash occur on a device with API level 26 (i.e. Android 8.0)? According to https://github.com/google/oboe/blob/master/docs/AndroidAudioHistory.md#80-oreo---api-26-august-21-2017 AAudio is not recommended on 8.0 as it might trigger a crash |
|
Damn, completely forgot about this. Would still like to help you getting this into cubeb as I'm still using the library.
Ok so I wrote up a "minimal" testing example. Minimal in this case still means it has dependencies since setting up logging and message loop and all that on android just requires some code (I also wanted the example to be testable on non-android system for comparison). The code is here. It tests almost all features of cubeb I guess, starting/stopping/draining the stream. When tapping the screen (or clicking the window on desktop systems) it toggles between recording audio and playing back the recorded audio. In my quick testing it behaved the same as the pulseaudio backend (except I'm getting a lot of I uploaded the apk I tested with here. I have full understanding if you don't want to execute random APKs though. To make it work you would also have to manually enable the 'record audio' permission for the app in android settings (since it's a pure NDK app), otherwise creating the AAudio input stream will fail and the example throw an exception. In case you want to compile it yourself, I'll gladly help with issues. The android-log tag it uses is
Yeah maybe that's a problem, I think I tested it with Android 9 back when I created the PR and with Android 10 today. |
|
I've imported this into a Firefox build, it works well, I haven't seen crashes, but I'm running Android 9 on a Sony XZ1 Compact (I hear there are issues on Android 8). I should receive a Pixel and a cheaper phone running Android 8 in the next few days. @nyorain Thanks a lot for this. It's largely working, but some work is needed:
You've done most of the work, and we need this for Firefox (OpenSL ES is deprecated in NDK r21d, and we'd rather have a bullet-proof AAudio backend sooner than later), I'd be happy to finish this and test on more devices, with more involved test scenarios, etc. |
|
I'll gladly port it to C++ (am more used to it anyways) and fix the remaining TODOs, will be looking into the clock issue. Should be able to do this until next week. |
For sure, it's not particularly exciting. We'll take care of this part, or any part you don't feel like working on, just let us know where and when we can help in any way. |
|
Rebased and updated the backend to C++11, let me know if it's what you intended. One config option specifies whether we pass the low latency flag to the aaudio stream builder (aaudio documentation states that this flag might have an impact on battery life). I enabled that config option by default in cmake now and would consider it sensible to leave it like this but it's up to you what should be enabled by default in cubeb in the end. In my tests, it significantly decreased the output buffer size. The other flag (that I left disabled but maybe it's useful to you) is to use exclusive sharing mode. The aaudio documentation states that this can reduce the latency further. |
This removes the constant checking for state changes with 5ms of sleep in between for the state thread. We need a new thread to wakeup the state thread reliably without blocking in the audio thread. For a more detailed and theoretical explanation of the problem and solution (specifically written for this commit), see: https://nyorain.github.io/lock-free-wakeup.html Now, will only do this time-based sleeping when actively waiting for a state change. We can't implement that with a blocking call to AAudioStream_waitForStateChange since that can't be woken up and we furthermore might have to wait for multiple streams at once. This also fixes some issues and race conditions with stream destruction and adds some more documentation.
From a quick look, this is what I had in mind, thank you very much. I've received test devices, and can test this on Android 8, 9 and 11, now, on a Sony XZ1 Compact, Moto G5 and Pixel 4 devices.
cubeb usually makes the decision, per platform, depending on the latency requested, at runtime. This allows having a battery preserving mode for playing regular media (videos, etc.), and a "lowest latency possible in non-exclusive" when doing Web Audio API, WebRTC or other interactive scenarios.
We don't use exclusive mode in Firefox, but it might well be useful for others, or for an opt-in mode or something, thanks! |
Makes sense, could be done here as well. Everything I could do right now is just guesswork though, this should probably be based on queried latencies or general android guarantees. So I would leave it together with the other latency stuff for you. Let me know how the testing goes, I'll do what I can to fix remaining errors in the code. |
| // that only serves as notifier backup (in case the notification happens | ||
| // right between the state thread checking and going to sleep in which case | ||
| // this thread will kick in and signal it right again). | ||
| static void notifier_thread(cubeb* ctx) |
There was a problem hiding this comment.
That should work indeed, the cost of a single thread is acceptable.
It's still unclear to me whether notifying a condition variable is real-time safe on all operating systems that are supported by cubeb. The importance of all this is mitigated by the fact that we only need this when draining.
There was a problem hiding this comment.
This is good to merge as is, I'll quickly followup with lots of little things that are needed before it reaches parity with the opensl backend for all usage, at which point we can move it to have a higher precedence than the opensl backend, that will still be used as fallback. I'm also going to have it run the complete audio-related test suite of Firefox, that usually finds lots of edge-cases.
Thank you very much for doing this and then improving it!
|
Merging this now, and I'll open another PR (or maybe even multiple PRs) for improvements on latency, picking the correct buffer size, A/V sync, etc. It is build by default when available, but not used by default (opensl is always preferred except when AAudio is requested and available). |
This implements a first AAudio backend with support for input, output and duplex streams. It uses callbacks for best latency and is completely lock-free in the audio thread (as the aaudio docs recommend). I could only test it on one device. Some additional functions could be implemented via guesswork, jni or other APIs (see the SLES backend), I didn't do that for now and would prefer to keep this PR down to a first basic implementation. There are a couple of NOTEs in the code in places that could need improvements or at stuff I wasn't sure about. I will continue to fix out minor details but am already thankful for input, especially regarding the general backend design.
The cubeb docs don't state this so i assumed it's ok if the state callback is called from a different
thread than the audio callback and that they don't have to be mutually exclusive, i.e. can be called at the same time. Is that correct?
We furthermore currently send a callback with STATE_STOPPED some time after STATE_DRAINED since that simplifies the backend state logic and isn't strictly wrong. Does that break expected behavior? That could be changed without much work.
The backend is currently not used over SLES (i.e. never unless explicitly requested) but i guess this should be changed when it's well tested as it can be expected to provide better latency.