Closed
Bug 698328
Opened 13 years ago
Closed 11 years ago
AudioTrack.cpp backend for libcubeb
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: kinetik, Assigned: padenot)
References
Details
Attachments
(2 files, 6 obsolete files)
17.40 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
26.18 KB,
patch
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #694484 +++ We'll need to support Android releases earlier than 2.3 for some time, so we'll also need a AudioTrack/JNI backend for libcubeb. This can roughly be adapted from the existing code in sydney_audio_android.c and Doug's Flash/Pepper plugin patches. It probably makes the most sense to build this before the OpenSL version, since it'll work on all platforms, and the OpenSL backend can only provide incremental improvements (i.e. lower overhead, but probably not lower latency) over an AudioTrack backend.
Assignee | ||
Comment 1•11 years ago
|
||
So roc told me to write this. I'll go with pthreads, since they seem to be available on Android, and AudioTrack is a push-style API.
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•11 years ago
|
||
Also, [1] seems a decent way to get a callback to fire, I'll try to use it instead of a timer/epoll based solution. If I can figure how to get the JVM to callback to the native code. In unrelated news, the OpenSL backend works quite nicely on my Galaxy Nexus, on Fennec. [1]: http://developer.android.com/reference/android/media/AudioTrack.html#setPositionNotificationPeriod%28int%29
(In reply to Paul Adenot (:padenot) from comment #2) > In unrelated news, the OpenSL backend works quite nicely on my Galaxy Nexus, > on Fennec. Great. Please submit a patch to turn it on in mozilla-central! (in a new bug of course)
Assignee | ||
Comment 4•11 years ago
|
||
So, I've got the thing in a state where I can play a media on android using AudioTrack and cubeb. One at a time, because I'm not sure yet of the best way to know when I have to call the data callback, so I use a pthread and a nanosleep. I could probably use [1], but I'm not super confident about this JNI stuff, and I tried finding docs on how to implement the listener that we need in C++ so the JVM could call my callback when it is time to refill the buffer, without success. Also, I'm not sure how to make the JVM run the callback on the right thread. Do we have someone I could ask questions? Another solution would be to use epoll/timerfd, and the return value from the write call (to check if we are writing too much). This would be much easier. [1]: http://developer.android.com/reference/android/media/AudioTrack.OnPlaybackPositionUpdateListener.html
Assignee: nobody → paul
(In reply to Paul Adenot (:padenot) from comment #4) > Do we have someone I could ask questions? Try cpeterson or snorp (James Willcox)?
Comment 6•11 years ago
|
||
Following the status then consider to port on B2G.
Comment 7•11 years ago
|
||
Marco, we have an opensl backend for B2G. It just needs a bit of polishing. We are not going to use this AudioTrack backend because it requires Java.
Assignee | ||
Comment 8•11 years ago
|
||
Ha, I forgot to say that I talked to snorp, and he suggested that I dlopen libmedia.so instead of using JNI, and then using the AudioTrack.cpp methods directly. In theory, ABI changes could break us. In practice, he says that there are no changes between versions/vendors/whatevers, and I found that info on a couple mailing lists as well. Also, if we get the OpenSL backend working, we can use the hacky dlopened stuff only on android 2.2. Also, this has nothing to do with B2G.
Assignee | ||
Updated•11 years ago
|
Summary: AudioTrack backend for libcubeb → AudioTrack.cpp backend for libcubeb
Comment 9•11 years ago
|
||
Yeah 2.2 is pretty dead. We definitely don't want to use AudioTrack regardless on B2G since API changes break us. B2G is currently using AudioFlinger so this backend is an option. But it is a wrong option.
Comment 10•11 years ago
|
||
Hi Paul & Michael, As I knew that the B2G now is using libSydney -> AudioTrack (from AudioTrack.cpp) -> AudioFlinger. So from Comment 8, B2G v1 now doesn't need to consider any Java or JNI code for AudioTrack.java. As comment 9, so the final plan of audio backend will be the flow below or any others? 1. openSL -> AudioTrack (from .cpp) -> AudioFlinger Thanks.
Assignee | ||
Comment 11•11 years ago
|
||
So, I talked to snorp, and he told me that it would be worth it to try to |dlopen("/system/lib/libmedia.so")|, as he had the idea for the flash plugin implementation. So I'm trying that.
Assignee | ||
Comment 12•11 years ago
|
||
This is necessary for the incoming patch.
Assignee | ||
Comment 13•11 years ago
|
||
So, this implements a cubeb backend that dlopens libmedia.so. It only "works" on Android 4.1 (because the ABI changed a bit, but it is easy to add the support for other versions, I had a 4.1 device only, I just got a 2.2 device that I'll try tomorrow.). The implementation currently segfault when recreating a stream (when seekeing, for example), at [1]. I suspect the return value of |cblk->pointer()| is a dangling pointer, and I see suspicious things in logcat: E/IMemory (32006): binder=0x588693f8 transaction failed fd=-2147483647, size=0, err=-2147483646 (Unknown error 2147483646) E/IMemory (32006): cannot dup fd=-2147483647, size=0, err=-2147483646 (Bad file number) E/IMemory (32006): cannot map BpMemoryHeap (binder=0x588693f8), size=0, fd=-1 (Bad file number) Anyway, I'm an Android repo to debug that. At the same time, I'm adding support for 2.2 (now that I have a device to test) that might not have the same problem. We'll see. Having a working 2.2 backend might be enough if we polish the OpenSL backend enough so it can be used by default. [1]: https://android.googlesource.com/platform/frameworks/av/+/android-4.1.1_r5/media/libmedia/AudioTrack.cpp, line 899, I belive mCblk is dangling, to &mCblk->flags references garbage. I don't have much info since I test on my personal phone that has no debug info.
Reporter | ||
Comment 14•11 years ago
|
||
Marco hit a very similar looking bug in the OpenSL backend, see bug 839007 for the fix. May be applicable, or at least give you a pointer?
Comment 15•11 years ago
|
||
Hi Paul & Matthew, I really met the issue in comment 13 on openSL backend too and the log is the same. But the patch on bug 839007 is another issue which caused crash. For issue on comment 13, the return value of |cblk->pointer()| is null. I am investigating it now.
Comment 16•11 years ago
|
||
Hi Paul, I found it can be re-produced in bankend of sydney + audiotrack, but it is protected by a workaround now (pleaser refer to the link) http://mxr.mozilla.org/mozilla-central/source/media/libsydneyaudio/src/sydney_audio_gonk.cpp#211 I don't the reason but maybe you can try on you side.
Assignee | ||
Comment 17•11 years ago
|
||
Marco, it feels like a reference counting problem, then. Something needs to be kept alive, but isn't. To avoid leaking, can you try to have some sort of global state to store the first AudioTrack, so we can delete it right after creating a second one? I believe simply having a global static variable should do the trick. In the meantime, I'll go read the Android source, see if we can figure this out.
Comment 18•11 years ago
|
||
Hi Paul, In sydney_audio_gonk, it used a static variable to keep only first AudioTrack instance already. By the way, could you try to use different sessionId to create AudioTrack? I will try it on sydney version tomorrow though. Thanks.
Assignee | ||
Comment 19•11 years ago
|
||
I already tried changing the sessionId, it does not make the crash go away.
Assignee | ||
Comment 20•11 years ago
|
||
This backend works (on 2.2), but has not been tested thoroughly. I'm waiting on a 2.2 device (that already been shipped) so I can run mochitests and do more extensive testing.
Assignee | ||
Updated•11 years ago
|
Attachment #713936 -
Attachment is obsolete: true
Comment 21•11 years ago
|
||
Hi Paul, Do you fix the crash issue on this new patch? I doesn't see you keep an AudioTrack alive before a new one created. Or any other for the crash issue? Thanks.
Assignee | ||
Comment 22•11 years ago
|
||
mchen, for some reason, on Android 2.2, I don't observe the crash, that occurs only on Android 4.2 (I only have a 4.2 and a 2.2 device, so I'm not sure it happens elsewhere).
Assignee | ||
Comment 23•11 years ago
|
||
This is green on try [1] on 2.2. It crashes on (at least) 4.0. Considering we have an OpenSL backend in a decent state (at least it seemed to be decent in my not-super-scientific tests), I'm wondering if I should make this new backend work on 2.3+. It is probably not super long to do, it used to work (apart from the workaround mchen did that we can do here as well). I've also stressed it during a night, with a page that randomly performs load/play/pause on <audio> element, and Fennec was alive and working in the morning. [1]: https://tbpl.mozilla.org/?tree=Try&rev=eaa72905d63e
Attachment #719113 -
Flags: review?(kinetik)
Assignee | ||
Updated•11 years ago
|
Attachment #717003 -
Attachment is obsolete: true
Assignee | ||
Comment 24•11 years ago
|
||
And now with the makefile changes. And less printfs.
Attachment #719118 -
Flags: review?(kinetik)
Assignee | ||
Updated•11 years ago
|
Attachment #719113 -
Attachment is obsolete: true
Attachment #719113 -
Flags: review?(kinetik)
Reporter | ||
Comment 25•11 years ago
|
||
Comment on attachment 719118 [details] [diff] [review] AudioTrack.cpp backend for libcubeb. r= Review of attachment 719118 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this! Don't forget to add yourself to AUTHORS. Here's a first pass of review comments: ::: media/libcubeb/src/Makefile.in @@ +28,5 @@ > cubeb_opensl.c \ > $(NULL) > +else > +CSRCS = \ > + cubeb_audiotrack.c \ Also need to remove the Gonk test from configure.in to get MOZ_CUBEB defined? ::: media/libcubeb/src/cubeb_audiotrack.c @@ +4,5 @@ > + * This program is made available under an ISC-style license. See the > + * accompanying file LICENSE for details. > + */ > + > +#include "cubeb/cubeb.h" Include this after the system headers. @@ +5,5 @@ > + * accompanying file LICENSE for details. > + */ > + > +#include "cubeb/cubeb.h" > +#include <jni.h> Is this needed? We're not actually using the JNI interface now, right? @@ +6,5 @@ > + */ > + > +#include "cubeb/cubeb.h" > +#include <jni.h> > +#include <assert.h> #define NDEBUG so the asserts are active all the time. If we end up hitting any regularly, convert those into correctly handled errors. @@ +106,5 @@ > + status_t (*get_position)(void* instance, uint32_t* position); > + /* static */ int (*get_output_frame_count)(int* frame_count, int stream); > + /* static */ int (*get_output_latency)(uint32_t* frame_count, int stream); > + /* static */ int (*get_output_samplingrate)(int* frame_count, int stream); > +}; I think all of the stuff from the Android headers need to go in a separate include with appropriate licensing/attribution, and a clear link to the original source file and version. @@ +110,5 @@ > +}; > + > +struct cubeb { > + void * library; > + struct AudioTrack class; Since "class" is reserved in C++, it'd be nice to use "klass" (common in JNI code) or something. That way it's one less thing to deal with if switching to (or forced to use) a C++-only compiler. @@ +181,5 @@ > + > +char const * > +cubeb_get_backend_id(cubeb * context) > +{ > + return "android_native_audiotrack"; Make this "audiotrack". @@ +201,5 @@ > + cubeb_data_callback data_callback, > + cubeb_state_callback state_callback, > + void * user_ptr) > +{ > + struct cubeb_stream *s; Space between * and s. And call the local variable stm. @@ +215,5 @@ > + return CUBEB_ERROR_INVALID_FORMAT; > + } > + > + /* Recent Android have a getMinFrameCount method. On Froyo, we have to compute it by hand. */ > + if (cubeb_is_froyo(ctx)) { Maybe move this entire block into a helper function. @@ +250,5 @@ > + return CUBEB_ERROR; > + } > + } > + > + s = calloc(1, sizeof(struct cubeb_stream)); Use sizeof(*stm). @@ +258,5 @@ > + s->data_callback = data_callback; > + s->state_callback = state_callback; > + s->user_ptr = user_ptr; > + s->params = stream_params; > + pthread_mutex_init(&s->mutex, NULL); Check error code. Is there documentation about the thread-safety of AudioTrack? I'm wondering about, for example, calling get_position while the callback is in flight. I don't understand what this is protecting right now, as it's only held inside the refill callback and in stream_destroy. @@ +260,5 @@ > + s->user_ptr = user_ptr; > + s->params = stream_params; > + pthread_mutex_init(&s->mutex, NULL); > + > + s->instance = malloc(SIZE_AUDIOTRACK_INSTANCE); calloc this. VLC's audio_output/audiotrack.c has a nice safety feature: allocate slightly more than necessary here, then set the last 4 bytes of the allocation to a known value (0xbaadbaad) and assert that the memory location still has that value after calling the constructor. @@ +274,5 @@ > + if (cubeb_is_froyo(ctx)) { > + ctx->class.ctor_froyo(s->instance, > + AUDIO_STREAM_TYPE_MUSIC, > + s->params.rate, > + AUDIO_FORMAT_PCM_16_BIT, It doesn't look like AudioTrack supports float32. Need to check stream_params.format and return a format error if float32 was requested. @@ +312,5 @@ > + > +void > +cubeb_stream_destroy(cubeb_stream * stream) > +{ > + cubeb* ctx = stream->context; Unused? @@ +367,5 @@ > +{ > + switch (event) { > + case EVENT_MORE_DATA: { > + cubeb_stream* stream = user; > + int32_t got = 0; data_callback returns a long. @@ +374,5 @@ > + pthread_mutex_lock(&stream->mutex); > + /* If instance is null, then we are destroying the stream. Because at that > + * point, AudioTrack::stop has been called, this callback > + * will not be called again. */ > + if (stream->instance == NULL) { If instance is null and this thread is holding the mutex, something must have gone wrong. stream_destroy grabs the lock, sets instance to null, drops the lock, and destroys the resources. stream_destroy should be implemented in such a way that refill does not run after it's called amd if that's not possible we'll need to be _very_ careful with the resources refill uses. @@ +391,5 @@ > + got = stream->data_callback(stream, stream->user_ptr, b->raw, b->frameCount); > + > + if (got != b->frameCount) { > + ALOG("Start draining."); > + stream->draining = 1; How many buffers does AudioTrack use internally? If, for example, the buffers are 500ms long and there are four of them (say we requested 2s latency), it seems like this will trigger drain as soon as the next callback for a 500ms buffer happens, but there may be another 1500ms of audio to play. It looks like BUFFER_END is unimplemented in AudioTrack, but maybe we can set a position marker at the last write position, then handle drain when that's hit? @@ +418,5 @@ > +} > + > +/* We are running on froyo if we found the right AudioTrack constructor */ > +static int > +cubeb_is_froyo(cubeb * ctx) I prefer to have the public/exported functions at the bottom of the file, declared in the same order as the header. Everything else (private/static/helpers/etc.) should be placed above it. So please move this and cubeb_refill above cubeb_init and add/remove declarations as necessary. Maybe also rename this to audiotrack_version_is_froyo or similar.
Attachment #719118 -
Flags: review?(kinetik)
Attachment #719118 -
Flags: review-
Attachment #719118 -
Flags: feedback+
Assignee | ||
Comment 26•11 years ago
|
||
> ::: media/libcubeb/src/Makefile.in > @@ +28,5 @@ > > cubeb_opensl.c \ > > $(NULL) > > +else > > +CSRCS = \ > > + cubeb_audiotrack.c \ > > Also need to remove the Gonk test from configure.in to get MOZ_CUBEB defined? So, I (re)implemented the 4.0+ compatibility, and somehow, I can't repro the crash mchen and I were seeing earlier, despite trying quite hard, so I've included the configure.in modifications. > @@ +106,5 @@ > > + status_t (*get_position)(void* instance, uint32_t* position); > > + /* static */ int (*get_output_frame_count)(int* frame_count, int stream); > > + /* static */ int (*get_output_latency)(uint32_t* frame_count, int stream); > > + /* static */ int (*get_output_samplingrate)(int* frame_count, int stream); > > +}; > > I think all of the stuff from the Android headers need to go in a separate > include with appropriate licensing/attribution, and a clear link to the original > source file and version. So, the struct of function pointers is not from Android (or maybe function prototype should have proper attribution? I actually have no idea about that). I moved everything else in a separate file, that has, for each definition, one (or more) link(s) to the Android sources (depending on which version we use) in the official Google repository. I chose to have stripped down version of their enums, but maybe this is not the correct way to proceed. > Is there documentation about the thread-safety of AudioTrack? I'm wondering > about, for example, calling get_position while the callback is in flight. > > I don't understand what this is protecting right now, as it's only held inside > the refill callback and in stream_destroy. So, because there is no comment that says "this class is threadsafe, have fun", I read carefuly all the methods that we use when we are running multiple threads, to make sure we are not doing something risky, on the versions that we are running on (from what I've read, if we check 2.2 and 4.2, we capture most of the changes). To follow along, here are the files where the methods we are calling are located: - 2.2: https://android.googlesource.com/platform/frameworks/base/+/android-2.2.3_r2.1/media/libmedia/AudioTrack.cpp https://android.googlesource.com/platform/frameworks/base/+/android-2.2.3_r2.1/include/private/media/AudioTrackShared.h - 4.2: https://android.googlesource.com/platform/frameworks/av/+/android-4.2.2_r1/media/libmedia/AudioTrack.cpp 2.2's AudioTrack uses a lock, and explicit |lock| and |unlock| methods. The lock is inside the |AudioTrackThread| class. 4.2's AudioTrack prefers to use an |AutoLock| RAII class. The lock is in the AudioTrack class. AudioTrack::getPosition - 4.2: line 699, using an |AutoLock|. - 2.2: line 578, not using a lock, but |mCblk->server| is volatile (AudioTrackShared.h, line 44). AudioTrack::start - 4.2: line 379, grabbing the lock using an |AutoLock|. - 2.2: line 315, grabbing the lock (or early return if the AudioTrack is going to be shutdown soon). AudioTrack::stop - 4.2: line 437, grabbing the lock using an |AutoLock|. - 2.2: line 362, grabbing the thread's lock. AudioTrack::AudioTrack - 4.2: line 283 (the ctor calls |set|), thread is created, does not run yet. - 2.2: line 224 (the ctor calls |set|), thread is created, does not run yet. Shutdown sequence: The worrying part is that our callback should not be called when we have freed the stream. It turns out that it cannot be called, because in both versions (4.2, 2.2), the dtor calls |requestExitAndWait| on the thread, (that does what it says [1]). [1]: 4.2, line 831 https://android.googlesource.com/platform/frameworks/native/+/android-4.2.2_r1/libs/utils/Threads.cpp 2.2, line 767 https://android.googlesource.com/platform/frameworks/base/+/android-2.2.3_r2.1/libs/utils/Threads.cpp Considering all that, I don't think we actually need to lock anything. (Also, I tested, and it works just fine). > How many buffers does AudioTrack use internally? If, for example, the buffers > are 500ms long and there are four of them (say we requested 2s latency), it > seems like this will trigger drain as soon as the next callback for a 500ms > buffer happens, but there may be another 1500ms of audio to play. > > It looks like BUFFER_END is unimplemented in AudioTrack, but maybe we can set a > position marker at the last write position, then handle drain when that's hit? I don't understand what you mean. This triggers a drain when cubeb returns less frames than asked for, like every other backend.
Attachment #721307 -
Flags: review?(kinetik)
Assignee | ||
Updated•11 years ago
|
Attachment #713933 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #719118 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #721307 -
Attachment description: Enable cubeb for fennec. → patch
Comment 27•11 years ago
|
||
FWIW, I would prefer that we use the opensl backend for 2.3+, and only use this for 2.2. At the very least, this makes supporting new Android versions less tricky.
Assignee | ||
Comment 28•11 years ago
|
||
Michael, when bug 837564 lands, we will be able to decide which cubeb backend we want to use depending on the version. As you say, the plan is to use OpenSL on Android 2.3+.
Reporter | ||
Comment 29•11 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #26) > I don't understand what you mean. This triggers a drain when cubeb returns > less > frames than asked for, like every other backend. My question is about when the drain completes, that is, when state_callback will be called with CUBEB_STATE_DRAINED. That needs to happen when we're sure all of the buffered audio has been played. In this implementation, state_callback will be called the next time cubeb_refill runs after stm->draining is set. If AudioTrack uses, for argument's sake, 4 buffers of 500ms internally, this code will cause the drain to complete in 500ms or earlier (whenever cubeb_refill is next called), even if the remaining 3 queued buffers contain audio. What we need is a way to complete the drain when the last audio queued has played. Assuming AudioTrack doesn't expose the details of it's internal buffers, one technique will be to use the position marker stuff I mentioned before. In other existing cubeb backends, drain is handled the following ways: alsa: no (working) drain API, so uses a timer (duration computed from audio queued) pulse: no (working) drain API, so uses a timer (duration computed from audio queued) audioqueue: counts buffers returned to free pool audiounit: drain completes on next callback, but there are only 2x ~11ms buffers winmm: counts buffers returned to free pool If my explanation still doesn't make sense, ping me on IRC.
Assignee | ||
Comment 30•11 years ago
|
||
This makes perfect sense, thanks. This is now implemented (using setMarkerPosition), and seems to work fine (I checked I can hear the last bit of audio, and the audiotrack callback is called up to a couple times after the cubeb callback returns less than asked for).
Attachment #722232 -
Flags: review?(kinetik)
Assignee | ||
Updated•11 years ago
|
Attachment #721307 -
Attachment is obsolete: true
Attachment #721307 -
Flags: review?(kinetik)
Reporter | ||
Comment 31•11 years ago
|
||
Comment on attachment 722232 [details] [diff] [review] AudioTrack.cpp backend for libcubeb. r= Review of attachment 722232 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Please do the usual github pull request/update.sh dance before checking in. ::: media/libcubeb/src/cubeb_audiotrack.c @@ +286,5 @@ > + stm->state_callback = state_callback; > + stm->user_ptr = user_ptr; > + stm->params = stream_params; > + > + stm->instance = calloc(SIZE_AUDIOTRACK_INSTANCE, 1); Swap SIZE_AUDIO_TRACK_INSTANCE and 1 arguments here, since logically we're allocating 1 AudioTrack.
Attachment #722232 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 32•11 years ago
|
||
This updates the libcubeb sources, but needs 850770 and 850713 to land before.
Reporter | ||
Comment 33•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3080f18ba53d
Reporter | ||
Comment 34•11 years ago
|
||
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/2850e56ee75d Probably due to bug 850713.
Reporter | ||
Comment 35•11 years ago
|
||
Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0441770277
Comment 36•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8e0441770277
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•