Closed Bug 698328 Opened 8 years ago Closed 7 years ago

AudioTrack.cpp backend for libcubeb

Categories

(Core :: Audio/Video, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: kinetik, Assigned: padenot)

References

Details

Attachments

(2 files, 6 obsolete files)

+++ 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.
Depends on: 698809
No longer depends on: 698809
Blocks: 750596
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
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)
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)?
Following the status then consider to port on B2G.
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.
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.
Summary: AudioTrack backend for libcubeb → AudioTrack.cpp backend for libcubeb
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.
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.
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.
Attached patch Enable cubeb for fennec. (obsolete) — Splinter Review
This is necessary for the incoming patch.
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.
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?
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.
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.
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.
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.
I already tried changing the sessionId, it does not make the crash go away.
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.
Attachment #713936 - Attachment is obsolete: true
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.
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).
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)
Attachment #717003 - Attachment is obsolete: true
And now with the makefile changes. And less printfs.
Attachment #719118 - Flags: review?(kinetik)
Attachment #719113 - Attachment is obsolete: true
Attachment #719113 - Flags: review?(kinetik)
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+
Attached patch patch (obsolete) — Splinter Review
> ::: 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)
Attachment #713933 - Attachment is obsolete: true
Attachment #719118 - Attachment is obsolete: true
Attachment #721307 - Attachment description: Enable cubeb for fennec. → patch
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.
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+.
(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.
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)
Attachment #721307 - Attachment is obsolete: true
Attachment #721307 - Flags: review?(kinetik)
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+
Depends on: 850713
Depends on: 850770
This updates the libcubeb sources, but needs 850770 and 850713 to land before.
Blocks: 850121
https://hg.mozilla.org/mozilla-central/rev/8e0441770277
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.