Closed Bug 694484 Opened 8 years ago Closed 7 years ago

OpenSL backend for libcubeb

Categories

(Core :: Audio/Video, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: kinetik, Assigned: mwu)

References

Details

Attachments

(1 file, 3 obsolete files)

As of Android API level 9 (Gingerbread), a native sound API is available: OpenSL.  The two major benefits of this are: avoiding JNI, and avoiding remoting audio from content processes to the parent process.  I've been experimenting with this recently and will turn that into a libcubeb backend and attach a patch here once it's working well enough for others to use.
The API suffers from all of the same problems discussed here: http://code.google.com/p/android/issues/detail?id=3434

What it does provide us is a native API which we can use directly in non-Java child processes.
Maybe we should contribute cubeb to android :)
Blocks: 698328
Blocks: 750596
Attached patch WIP opensl backend (obsolete) — Splinter Review
Was poking at this this weekend and probably won't have time this week to look at it.
Attached patch WIP v2 (obsolete) — Splinter Review
Attachment #655638 - Attachment is obsolete: true
Depends on: 786690
Attached patch Add cubeb opensl backend (obsolete) — Splinter Review
This appears to work ok *except* when the stream is stopped/pause. However, most of it makes sense so far so I'd like to get it reviewed and landed disabled, and then either one of us can debug the issue afterward.

Also, I suspect we can just use a global output sink instead of making one for every stream, but I'd like to investigate that later.
Assignee: kinetik → mwu
Attachment #656182 - Attachment is obsolete: true
Attachment #656466 - Flags: review?(kinetik)
(In reply to Michael Wu [:mwu] from comment #7)
> This appears to work ok *except* when the stream is stopped/pause.

To clarify, it seems like the get_position code stops producing values that the users expects after a video is stopped.
Comment on attachment 656466 [details] [diff] [review]
Add cubeb opensl backend

Review of attachment 656466 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome, thank you!

I'm fine with landing this with the review comments addressed, but the current patch will enabled this by default--once MOZ_CUBEB is defined, nsAudioStream::AllocateStream will prefer the cubeb backend over the sydneyaudio one.  Add an Android/B2G specific define in nsAudioStream.cpp's PrefChanged that defaults media.use_cubeb to false on those platforms.

(In reply to Michael Wu [:mwu] from comment #8)
> To clarify, it seems like the get_position code stops producing values that
> the users expects after a video is stopped.

Does it return an error, garbage, or something else?  Some of the other backends hit something similar (i.e. get position fails on non-running streams), so they track the last reported write position and return it when the stream is stopped/paused.

::: configure.in
@@ +5452,5 @@
>      case "$target" in
>      *-android*|*-linuxandroid*)
> +        if test -n "$gonkdir"; then
> +            AC_DEFINE(MOZ_CUBEB)
> +        fi

It'd be nice to build this for Android to, but I assume we'd need to dlopen libOpenSLES and what-not to deal with Android < 2.3?

::: media/libcubeb/src/cubeb_opensl.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 <assert.h>

Add #undef NDEBUG so that asserts are fatal in all builds.

@@ +36,5 @@
> +{
> +  void *buf = stm->queuebuf[stm->queuebuf_idx];
> +
> +  long written = stm->data_callback(stm, stm->user_ptr,
> +                                    buf, stm->queuebuf_len / stm->framesize);

Drain needs to be implemented for the case where data_callback returns >= 0 && < stm->queuebuf_len / stm->framesize.  We don't need to do that to land this patch, but it'll be needed before this backend can be enabled.

@@ +121,5 @@
> +  SLDataFormat_PCM format;
> +
> +  format.formatType = SL_DATAFORMAT_PCM;
> +  format.numChannels = stream_params.channels;
> +  format.samplesPerSec = stream_params.rate * 1000;

Add a comment that samplesPerSec is in mHz.

@@ +149,5 @@
> +  stm->user_ptr = user_ptr;
> +
> +  stm->framesize = stream_params.channels * sizeof(int16_t);
> +  stm->bytespersec = stream_params.rate * stm->framesize;
> +  stm->queuebuf_len = stm->bytespersec * latency / 1000;

This needs to be divided by the number of queuebufs to honour the requested latency.  Also, round the computed buffer size up by framesize.

@@ +155,5 @@
> +  stm->queuebuf[1] = malloc(stm->queuebuf_len);
> +  assert(stm->queuebuf[0]);
> +  assert(stm->queuebuf[1]);
> +
> +  SLDataLocator_BufferQueue loc_bufq = {SL_DATALOCATOR_BUFFERQUEUE, 2};

For the cubeb backends where I've got control over the number of buffers, I use 4 buffers to balance refill frequency vs underrun protection.
Attachment #656466 - Flags: review?(kinetik) → review+
(In reply to Matthew Gregan [:kinetik] from comment #9)
> Comment on attachment 656466 [details] [diff] [review]
> (In reply to Michael Wu [:mwu] from comment #8)
> > To clarify, it seems like the get_position code stops producing values that
> > the users expects after a video is stopped.
> 
> Does it return an error, garbage, or something else?  Some of the other
> backends hit something similar (i.e. get position fails on non-running
> streams), so they track the last reported write position and return it when
> the stream is stopped/paused.
> 

The video seems to get offset by the duration that the stream is paused.

> ::: configure.in
> @@ +5452,5 @@
> >      case "$target" in
> >      *-android*|*-linuxandroid*)
> > +        if test -n "$gonkdir"; then
> > +            AC_DEFINE(MOZ_CUBEB)
> > +        fi
> 
> It'd be nice to build this for Android to, but I assume we'd need to dlopen
> libOpenSLES and what-not to deal with Android < 2.3?
> 

Yeah. Though, that might actually be easy. I think slCreateEngine is the only symbol we need to pull from it.
Review comments addressed
Attachment #656466 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8eee25223a3e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 788402
Blocks: gonk-jb
Blocks: 791269
Blocks: 792109
You need to log in before you can comment on or make changes to this bug.