Android support for media/libsydneyaudio

RESOLVED FIXED

Status

()

Core
Audio/Video
--
enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mwu, Assigned: mwu)

Tracking

Trunk
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 447592 [details] [diff] [review]
Add android backend for libsydneyaudio
Attachment #447592 - Flags: review?(kinetik)
Looks great!  I haven't tested this as I don't have an Android device.

+  at.class = (*jenv)->NewGlobalRef(jenv,
+                               (*jenv)->FindClass(jenv,
+                                              "android/media/AudioTrack"));

One instance of this is leaked at shutdown.  Not sure it's possible to do anything about, as sydneyaudio doesn't expose a library init/destroy API.  We could certainly add one.  nsAudioStream is already set up to call functions to init and destroy a library.

Alternatively, we could refcnt it and free it when the last sa_stream is destroyed.

+  if ((*jenv)->PushLocalFrame(jenv, 4)) {

Is this (and the Pop) necessary?  Question goes for the other places it's used.

+    (*jenv)->NewObject(jenv, at.class, at.constructor,
+                     3 /* STREAM_MUSIC */,
+                     s->rate,
+                     s->channels == 1 ?
+                       4 /* CHANNEL_OUT_MONO */ : 12 /* CHANNEL_OUT_STEREO */,
+                     2 /* ENCODING_PCM_16BIT  */,
+                     (s->channels * s->rate) / 2,
+                     1 /* MODE_STREAM */);

Turn these into named constants at the top of the file and include a comment about where the values came from, please.

+  jbyteArray bytearray = (*jenv)->NewByteArray(jenv, nbytes);
+  jbyte *byte = (*jenv)->GetByteArrayElements(jenv, bytearray, NULL);

Test for failure?  Even if PushLocalFrame guaranteed there's enough memory for the local refs, there may not be enough memory for the actual array.

+sa_stream_get_write_size(sa_stream_t *s, size_t *size) {
[...]
+  *size = (s->rate * s->channels) / 2;

Some callers of this use it to work out the largest non-blocking write they can make.  Is it guaranteed that a write of the size calculated here will never block?

+sa_stream_set_volume_abs(sa_stream_t *s, float vol) {

We never call this (or get_volume_abs), so you can just make them UNSUPPORTED if you like.
(Assignee)

Comment 2

8 years ago
(In reply to comment #1)
> One instance of this is leaked at shutdown.  Not sure it's possible to do
> anything about, as sydneyaudio doesn't expose a library init/destroy API.  We
> could certainly add one.  nsAudioStream is already set up to call functions to
> init and destroy a library.
> 
> Alternatively, we could refcnt it and free it when the last sa_stream is
> destroyed.
> 
Now that you mention it, I think the android widget code actually leaks these at shutdown too.

Either option is fine with me though I would want to do a follow up if you'd prefer to add a destroy api to sydneyaudio.

> +  if ((*jenv)->PushLocalFrame(jenv, 4)) {
> 
> Is this (and the Pop) necessary?  Question goes for the other places it's used.
> 
I think so. We need to keep local references to any object java gives us.

> +    (*jenv)->NewObject(jenv, at.class, at.constructor,
> +                     3 /* STREAM_MUSIC */,
> +                     s->rate,
> +                     s->channels == 1 ?
> +                       4 /* CHANNEL_OUT_MONO */ : 12 /* CHANNEL_OUT_STEREO */,
> +                     2 /* ENCODING_PCM_16BIT  */,
> +                     (s->channels * s->rate) / 2,
> +                     1 /* MODE_STREAM */);
> 
> Turn these into named constants at the top of the file and include a comment
> about where the values came from, please.
> 
Ok.

> +  jbyteArray bytearray = (*jenv)->NewByteArray(jenv, nbytes);
> +  jbyte *byte = (*jenv)->GetByteArrayElements(jenv, bytearray, NULL);
> 
> Test for failure?  Even if PushLocalFrame guaranteed there's enough memory for
> the local refs, there may not be enough memory for the actual array.
> 
Right, will do.

> +sa_stream_get_write_size(sa_stream_t *s, size_t *size) {
> [...]
> +  *size = (s->rate * s->channels) / 2;
> 
> Some callers of this use it to work out the largest non-blocking write they can
> make.  Is it guaranteed that a write of the size calculated here will never
> block?
> 
I don't think this API ever blocks. This size is just a random guess.

> +sa_stream_set_volume_abs(sa_stream_t *s, float vol) {
> 
> We never call this (or get_volume_abs), so you can just make them UNSUPPORTED
> if you like.
Ok.
(Assignee)

Updated

8 years ago
Attachment #447592 - Flags: review?(kinetik)
(Assignee)

Comment 3

8 years ago
Created attachment 448569 [details] [diff] [review]
Add android backend for libsydneyaudio, v2

comments addressed.
Attachment #447592 - Attachment is obsolete: true
Attachment #448569 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #448569 - Flags: review? → review?(kinetik)
Comment on attachment 448569 [details] [diff] [review]
Add android backend for libsydneyaudio, v2

> I think so. We need to keep local references to any object java gives us.

Any reference that we're keeping beyond the lifetime of the call into our code needs to be a global reference (like you're doing with the class ID).  As far as I understand, Push/PopLocalRef is only useful to guarantee up front that a minimum number of local references can be created temporarily (which we don't need as we're error checking everything individually), or to create a new local reference stack (e.g. for handling lots of local refs in a loop).

+  streamCount++;

Streams can be opened on multiple threads (i.e. multiple threads may be calling sa_stream_open at the same time), so it's possible for the read-modify-write here to race and end up corrupting the ref count.  Rather than introduce locking or atomics, maybe it'd be simpler to create a globalref to the class once per stream and store it in the sa_stream state?  The method IDs are stable and can be shared safely, I think.

On the topic of thread safety, the sydneyaudio API is supposed to be thread safe.  I think the code as it is is fine as long as the underlying AudioTrack is thread safe.  My main concern would be whether it's safe to call getPlaybackHeadPosition while a write is in progress on another thread.

+  jbyte *byte = (*jenv)->GetByteArrayElements(jenv, bytearray, NULL);

I think this could fail too (in some VMs, it create a copy internally, so requires memory allocations, etc.), so check for NULL and error out.

r+ with those.  Thanks!
Attachment #448569 - Flags: review?(kinetik) → review+
(Assignee)

Comment 5

8 years ago
(In reply to comment #4)
> (From update of attachment 448569 [details] [diff] [review])
> > I think so. We need to keep local references to any object java gives us.
> 
> Any reference that we're keeping beyond the lifetime of the call into our code
> needs to be a global reference (like you're doing with the class ID).  As far
> as I understand, Push/PopLocalRef is only useful to guarantee up front that a
> minimum number of local references can be created temporarily (which we don't
> need as we're error checking everything individually), or to create a new local
> reference stack (e.g. for handling lots of local refs in a loop).
> 
In these cases though, we're calling from our code (from our own thread) into the JVM so the code needs some place to store localrefs. Otherwise how would java know when to free the localrefs? It doesn't know when our function exits.

> +  streamCount++;
> 
> Streams can be opened on multiple threads (i.e. multiple threads may be calling
> sa_stream_open at the same time), so it's possible for the read-modify-write
> here to race and end up corrupting the ref count.  Rather than introduce
> locking or atomics, maybe it'd be simpler to create a globalref to the class
> once per stream and store it in the sa_stream state?  The method IDs are stable
> and can be shared safely, I think.
> 
Oh huh. Will do.

> On the topic of thread safety, the sydneyaudio API is supposed to be thread
> safe.  I think the code as it is is fine as long as the underlying AudioTrack
> is thread safe.  My main concern would be whether it's safe to call
> getPlaybackHeadPosition while a write is in progress on another thread.
> 
I'm not too familiar with the code but from skimming the audiotrack code, it looks ok. getPlaybackHeadPosition just ends up reading a variable, I think, which the write code doesn't touch directly. 

> +  jbyte *byte = (*jenv)->GetByteArrayElements(jenv, bytearray, NULL);
> 
> I think this could fail too (in some VMs, it create a copy internally, so
> requires memory allocations, etc.), so check for NULL and error out.
> 
How sad. Will add a check here too.

> r+ with those.  Thanks!
\o/
(In reply to comment #5)
> In these cases though, we're calling from our code (from our own thread) into
> the JVM so the code needs some place to store localrefs. Otherwise how would
> java know when to free the localrefs? It doesn't know when our function exits.

Oh, duh.  I forgot we're using the invocation interface.  The documentation for what happens with localrefs when using the invocation interface seems to be pretty poor, but as far as I can tell they would stay alive until the owning thread is detached from the VM, so we should be managed them ourselves like you're already doing.  Sorry!
(Assignee)

Comment 7

8 years ago
Created attachment 449091 [details] [diff] [review]
Add android backend for libsydneyaudio, v3
Attachment #448569 - Attachment is obsolete: true
(Assignee)

Comment 8

8 years ago
Created attachment 449093 [details] [diff] [review]
Add android backend for libsydneyaudio, v3.1

Forgot to update the patch within the patch
Attachment #449091 - Attachment is obsolete: true
(Assignee)

Comment 9

8 years ago
Thanks for the review!

http://hg.mozilla.org/mozilla-central/rev/cac6db5caec8
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.