Paul Adenot (:padenot) Reporter
	Description • 10 years ago

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.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 1 • 10 years ago

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.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 2 • 10 years ago

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.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 3 • 10 years ago

WIP

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 4 • 10 years ago

WIP

	Randell Jesup [:jesup] (needinfo me)
	Comment 5 • 10 years ago

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

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 6 • 10 years ago

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.

	Paul Adenot (:padenot) Reporter
	Comment 7 • 10 years ago

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.

	(not currently active) Ted Mielczarek
	Comment 8 • 10 years ago

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.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 9 • 10 years ago

Banging my head against bug 1038799 made me realize this problem was as simple as MOZ_EXPORT.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 12 • 10 years ago

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

	Randell Jesup [:jesup] (needinfo me)
	Comment 13 • 10 years ago

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

	Paul Adenot (:padenot) Reporter
	Comment 14 • 10 years ago

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.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 15 • 10 years ago

Like this?

	Paul Adenot (:padenot) Reporter
	Comment 16 • 10 years ago

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.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 17 • 10 years ago

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)

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 18 • 10 years ago

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

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 19 • 10 years ago

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?

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 21 • 10 years ago

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.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 22 • 10 years ago

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

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 23 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7024109e9ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/3af17bad947f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b652396e9fd7
https://hg.mozilla.org/integration/mozilla-inbound/rev/689b75c98f2f

	Carsten Book [:Tomcat]
	Comment 24 • 10 years ago

sorry had to backout this changes for cross platform bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=44691227&tree=Mozilla-Inbound

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 25 • 10 years ago

Bitrot due to bug 1036894.

https://tbpl.mozilla.org/?tree=Try&rev=4ea07dcebb31

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 26 • 10 years ago

Oops, somewhere during rearranging the patches LoadManager got lost. New try:
https://tbpl.mozilla.org/?tree=Try&rev=b1b75d7003c3

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 27 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4058c55db564
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b23c66ef10
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/10f8441b42a1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6289b18d2127

	Carsten Book [:Tomcat]
	Comment 28 • 10 years ago

sorry had to backout again for https://tbpl.mozilla.org/php/getParsedLog.php?id=44699915&tree=Mozilla-Inbound

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 29 • 10 years ago

I dumbly assumed that the red on try that looked like infra would probably go green on inbound.

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 30 • 10 years ago

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

	Michael Shal [:mshal]
	Comment 31 • 10 years ago

Comment on attachment 8463347 [details] [diff] [review]
Patch 5. Fix Gonk

Stealing review per IRC request. Looks good!

	Gian-Carlo Pascutto [:gcp] Assignee
	Comment 32 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3227bf248435
https://hg.mozilla.org/integration/mozilla-inbound/rev/c85c57d0ccda
https://hg.mozilla.org/integration/mozilla-inbound/rev/da7d102b9f80
https://hg.mozilla.org/integration/mozilla-inbound/rev/d749aec7a3cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/df81f55bf94e

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 33 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/3227bf248435
https://hg.mozilla.org/mozilla-central/rev/c85c57d0ccda
https://hg.mozilla.org/mozilla-central/rev/da7d102b9f80
https://hg.mozilla.org/mozilla-central/rev/d749aec7a3cd
https://hg.mozilla.org/mozilla-central/rev/df81f55bf94e