694484 - OpenSL backend for libcubeb

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What's the latency like?

Comment 2

[Matthew Gregan [:kinetik]]

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bugger

Comment 4

[Ralph Giles (:rillian)]

Maybe we should contribute cubeb to android :)

Comment 5

[Michael Wu [:mwu]]

Attachment: WIP opensl backend

Was poking at this this weekend and probably won't have time this week to look at it.

Comment 6

[Michael Wu [:mwu]]

Attachment: WIP v2

Comment 7

[Michael Wu [:mwu]]

Attachment: Add cubeb opensl backend

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.

Comment 8

[Michael Wu [:mwu]]

(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 9

[Matthew Gregan [:kinetik]]

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.

Comment 10

[Michael Wu [:mwu]]

(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.

Comment 11

[Michael Wu [:mwu]]

Attachment: Add cubeb opensl backend, v2

Review comments addressed

Comment 12

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8eee25223a3e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8eee25223a3e

Comment 14

[Matthew Gregan [:kinetik]]

Upstreamed: https://github.com/kinetiknz/cubeb/commit/9d44b732da8f1cbe8ff8f721b2c2ce18c6a11bf4