Closed Bug 1015932 Opened 6 years ago Closed 6 years ago

Create a way to share the OpenSLES engine between cubeb and WebRTC

Categories

(Core :: Audio/Video, defect, P1)

32 Branch
x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: padenot, Assigned: gcp)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 3 obsolete files)

What needs to happen:
- Have a header that export a function with C linkage that allows one to get an engine (the opensles backend for cubeb is written in C, the code that we are looking at in webrtc is written in C++):

  > int mozilla_get_sles_engine(SLObjectItf ** aObject);

- Implement a thread safe singleton to back this function. This function will not get called very rarely, this should not be too hard. It is important to not put this in nsLayoutStatics, because it might load up a bunch of stuff we might not need until we actually play audio, I'm not sure.
- Patch cubeb and WebRTC by putting the code to get the engine in a separate function with the same signature as the one above.
- Patch the code in WebRTC and cubeb to add a compile time switch between the singleton and the code that creates the engine. #defines should work fine. I checked, cubeb and WebRTC conveniently use the same options when getting an engine. Bonus points for upstream-able code.
Assignee: nobody → gpascutto
Given that both users can try to call OpenSLES Destroy on the returned engine object, I think we need some reference counting and a destroy function as well.
I figured that both LoadManager/LoadMonitor and this are sitting among the other WebRTC stuff in content/media, but are a bit different. So let's put them in a seperate place.
Attachment #8433365 - Flags: review?(rjesup)
Attached patch Patch 2. Add OpenSLESProvider (obsolete) — Splinter Review
WIP
Comment on attachment 8433365 [details] [diff] [review]
Patch 1. Add systemservices subdir to content/media

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

Need a build peer for this
Attachment #8433365 - Flags: review?(ted)
Attachment #8433365 - Flags: review?(rjesup)
Attachment #8433365 - Flags: review+
Priority: -- → P1
Target Milestone: --- → mozilla33
The problem of the patches here is that libcubeb gets pulled in the unit tests (signaling, mediapipeline, etc), and the current patches leave the relevant function in libxul, so now the unit tests end up depending on libxul/MOZILLA_INTERNAL_API stuff, which probably isn't as good.

I changed the first patch to leave LoadManager where it is, and only dump the OpenSLESProvider in a subdir, build a new lib there, then add that lib to the unit tests (works), but then libxul failed to link and I couldn't figure out yet how to get the new lib in the link there.
You should implement that in a different way: in cubeb, don't use MOZILLA_INTERNAL_API stuff. Instead, have another header that will be different when building in gecko and outside gecko.

The header exposes a single function (say, cubeb_get_sles_engine).

When building cubeb in gecko, you use a header that rebinds to content/media/systemservices stuff.

Outside gecko (stand alone cubeb build, unit tests, etc.), it just calls the opensles function directly.

We've been using this method for the speex resampler: when building in gecko, we link against the in-tree resampler, otherwise, we build our own copy.
Comment on attachment 8433365 [details] [diff] [review]
Patch 1. Add systemservices subdir to content/media

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

::: content/media/systemservices/moz.build
@@ +7,5 @@
> +EXPORTS += [
> +]
> +
> +UNIFIED_SOURCES += [
> +]

I don't think you need these.
Attachment #8433365 - Flags: review?(ted) → review+
Banging my head against bug 1038799 made me realize this problem was as simple as MOZ_EXPORT.
Attachment #8433369 - Attachment is obsolete: true
Attachment #8458764 - Flags: review?(rjesup)
Attachment #8433370 - Attachment is obsolete: true
Attachment #8458765 - Flags: review?(rjesup)
Attachment #8458765 - Flags: review?(paul)
Comment on attachment 8458764 [details] [diff] [review]
Patch 2. v2 Add OpenSLESProvideropensles-provider

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

::: content/media/systemservices/moz.build
@@ +13,5 @@
> +    ]
> +
> +include('/ipc/chromium/chromium-config.mozbuild')
> +
> +LIBRARY_NAME = 'gksystemservices'

gklayout, leftover from rearranging the patches
Comment on attachment 8458764 [details] [diff] [review]
Patch 2. v2 Add OpenSLESProvideropensles-provider

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

::: content/media/systemservices/OpenSLESProvider.cpp
@@ +70,5 @@
> +                                const SLEngineOption *aOptions)
> +{
> +  MutexAutoLock lock(mLock);
> +  LOG(("Getting OpenSLES engine"));
> +  // XXX: Validate options are the same

bug #?

@@ +79,5 @@
> +    return SL_RESULT_SUCCESS;
> +  } else {
> +    int res = ConstructEngine(aObjectm, aOptionCount, aOptions);
> +    if (res == SL_RESULT_SUCCESS) {
> +      // XXX: Store engine options

bug?

::: content/media/systemservices/OpenSLESProvider.h
@@ +53,5 @@
> +    SLEngineOption *mSLEngineOptions;
> +    int mSLEngineOptionCount;
> +    int mSLEngineUsers;
> +    void *mOpenSLESLib;
> +    mozilla::Mutex mLock;

What does the lock protect?  COmment, and if possible put it above the items it protects
Attachment #8458764 - Flags: review?(rjesup) → review+
Attachment #8458765 - Flags: review?(rjesup) → review+
Comment on attachment 8458765 [details] [diff] [review]
Patch 3. v2 Use OpenSLESProvider in webrtc/libcubebbbbb

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

::: media/libcubeb/src/cubeb_opensl.c
@@ +268,4 @@
>    res = f_slCreateEngine(&ctx->engObj, 1, opt, 0, NULL, NULL);
> +#else
> +  res = mozilla_get_sles_engine(&ctx->engObj, 1, opt);
> +#endif

Don't do that. Wrap the call in a swappable header. This header exposes a single function (cubeb_get_slel_engine).

The function in the first header (when in the mozilla codebase) calls mozilla_get_sles_engine, and won't be in the main cubeb repo.

The function in the second header (when built stand alone) calls f_slCreateEngine directly.

This way, you don't have to define MOZILLA_INTERNAL_API in cubeb.

@@ +452,5 @@
> +#else
> +  if (ctx->engObj) {
> +    mozilla_destroy_sles_engine(&ctx->engObj);
> +  }
> +#endif

Same remark.
Attachment #8458765 - Flags: review?(paul) → review-
Blocks: 1042051
Like this?
Attachment #8458765 - Attachment is obsolete: true
Attachment #8460224 - Flags: review?(paul)
Comment on attachment 8460224 [details] [diff] [review]
Patch 3. v3 Use OpenSLESProvider in webrtc/libcubeb

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

Yes. I'll do the remaining things to properly upstream.

::: media/libcubeb/src/cubeb-sles.h
@@ +1,5 @@
> +#ifndef _CUBEB_SLES_H_
> +#define _CUBEB_SLES_H_
> +#include <OpenSLESProvider.h>
> +#define cubeb_get_sles(u, v, w, x, y, z) mozilla_get_sles_engine(u, v, w)
> +#define cubeb_destroy_sles(u) mozilla_destroy_sles_engine(u)

Using a macro is a bit brutal, isn't it? Can't you just wrap that in a function?

Also, can you name those cubeb_get_sles_engine so it's clear what we are talking about?

@@ +4,5 @@
> +#define cubeb_get_sles(u, v, w, x, y, z) mozilla_get_sles_engine(u, v, w)
> +#define cubeb_destroy_sles(u) mozilla_destroy_sles_engine(u)
> +// Upstream:
> +// #define cubeb_get_sles(u, v, w, x, y, z) f_slCreateEngine(u, v, w, x, y, z)
> +// #define cubeb_destroy_sles(u) (*(*u))->Destroy(*u);

Upstream will only include this code.

The update.sh script in the mozilla-central/media/libcubeb will swap the upstream header for the this one that call the singleton, so you can remove those three lines for now.
Attachment #8460224 - Flags: review?(paul) → review+
Hmm, did I break something?

I/PRLog   ( 7918): -2031637304[8865d480]: OpenSLESProvider being initialized
I/PRLog   ( 7918): -2031637304[8865d480]: Getting OpenSLES engine
I/PRLog   ( 7918): -2031637304[8865d480]: Returning new engine

I/PRLog   ( 7918): 1961933512[7363f080]: Getting OpenSLES engine
I/PRLog   ( 7918): 1961933512[7363f080]: Returning existing engine, 2 users
W/libOpenSLES( 7918): Leaving Object::Realize (SL_RESULT_PRECONDITIONS_VIOLATED)
I/PRLog   ( 7918): 1961933512[7363f080]: Freeing engine, 1 users left

I/PRLog   ( 7918): 1961933512[7363f080]: Getting OpenSLES engine
I/PRLog   ( 7918): 1961933512[7363f080]: Returning existing engine, 2 users
W/libOpenSLES( 7918): Leaving Object::Realize (SL_RESULT_PRECONDITIONS_VIOLATED)
F/libc    ( 7918): /home/morbo/hg/mozilla-central/media/webrtc/trunk/webrtc/modules/audio_device/opensl/../android/opensles_input.cc:129: int32_t webrtc::OpenSlesInput::Init(): assertion "false" failed
F/libc    ( 7918): Fatal signal 11 (SIGSEGV) at 0xdeadbaad (code=1), thread 8034 (Gecko)
Bah, I wonder if I messed up when I tested this originally. If I'm reading this correctly, you can't call Realize on a reused OpenSLES engine, so this should never have worked. I'll try pushing the Realize call into this broker as well.

http://code.metager.de/source/xref/android/2.3.7/system/media/opensles/libopensles/IObject.c#97
Okay, that worked. Notable thing in the log:

I/PRLog   (17485): 1961842256[7363f080]: Getting OpenSLES engine
I/PRLog   (17485): 1961842256[7363f080]: Returning existing engine, 3 users
I/PRLog   (17485): 1961842256[7363f080]: Not realizing already realized engine

Who's the third?
Comment on attachment 8460871 [details] [diff] [review]
Patch 4. Move Realize calls into the broker

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

::: content/media/systemservices/OpenSLESProvider.h
@@ +55,1 @@
>      void DestroyEngine(SLObjectItf * aObjectm);

Should reset mIsRealized.
Here's an analysis of the callers, showing that there's a leak in (our use?) of the webrtc.org stack, causing 3 OpenSLES engines to be allocated:
https://pastebin.mozilla.org/5596342
Attachment #8460871 - Flags: review?(paul) → review+
sorry had to backout this changes for cross platform bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=44691227&tree=Mozilla-Inbound
Oops, somewhere during rearranging the patches LoadManager got lost. New try:
https://tbpl.mozilla.org/?tree=Try&rev=b1b75d7003c3
I dumbly assumed that the red on try that looked like infra would probably go green on inbound.
It's not entirely clear to me why Gonk can't use the includes from the Android SDK but we use this fixup all over the tree so I guess it's ok here too.

https://tbpl.mozilla.org/?tree=Try&rev=2544b4dc6d4b
Attachment #8463347 - Flags: review?(ted)
Blocks: 1045018
Comment on attachment 8463347 [details] [diff] [review]
Patch 5. Fix Gonk

Stealing review per IRC request. Looks good!
Attachment #8463347 - Flags: review?(ted) → review+
You need to log in before you can comment on or make changes to this bug.