Closed Bug 1410456 Opened 7 years ago Closed 6 years ago

Android: Remove access to system library libmedia.so

Categories

(Firefox for Android Graveyard :: Audio/Video, enhancement, P2)

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
enhancement

Tracking

(firefox58 wontfix, firefox59 wontfix, firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: achronop, Assigned: achronop)

References

Details

(Whiteboard: [geckoview:klar])

Attachments

(11 files, 1 obsolete file)

Bug 1410456 - Allow OMT access to Android system audio properties.
59 bytes, text/x-review-board-request
esawin
: review+
Details
Bug 1410456 - use jni methods in place of removed cubeb methods.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1410456 - remove an unused variable that produces a compile warning.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1410456 - remove get min latency implementation because makes use of dlopen.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1410456 - remove get latency implementation because makes use of dlopen.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1410456 - remove preferred sample rate implementation because makes use of dlopen.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1410456 - get mixer latency from JNI instead of system library.
59 bytes, text/x-review-board-request
jchen
: review+
padenot
: review+
Details
Bug 1410456 - Keep the old mechanism for the older versions that jni method is not available.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1410456 - Add new files in update.sh script.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1410456 - Pass in cubeb JNIEnv instead of JavaVM.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1410456 - Update cubeb from upstream to eb3409e.
59 bytes, text/x-review-board-request
achronop
: review+
Details 
Cubeb for OpenSl ES access the system library libmedia.so. In order to remove that we need:

1. Remove the illegal access on cubeb backend.
2. Use the newly exposed methods GetAudioOutputFramesPerBuffer()/GetAudioOutputSampleRate() to set the latency and and stream rate when cubeb stream is initialized (playback and WebRTC).
Assignee: nobody → achronop
Rank: 15
Priority: -- → P2
The push above is on my WIP patches. They work on both of my devices. I have to clarify the playback case. I changed the AudioStream to set for latency the minimum latency give by cubeb (in frames) instead of using 10 ms translated to frames. In addition I need to check with someone from playback how to set the sample rate in case of Android. I am not sure if it is configurable or it depends on the codec etc.
Alex what is the status here? We're going to want to update to the newer SDK pretty soon.
Flags: needinfo?(achronop)
It's close but got distracted due to other priorities? When do you need it?
Flags: needinfo?(achronop)
(In reply to Alex Chronopoulos [:achronop] from comment #8)
> It's close but got distracted due to other priorities? When do you need it?

I don't think there is a hard timeline, but the sooner the better.
A reminder to myself to finish this one before new priorities come in. If software cooperates with my current issue I will be back on it next week. 

The remaining issue is to define how to get the position of the stream since we need a 3rd JNI method for that. One option is to calculate it outside cubeb and leave opensl_stream_get_position() unimplemented (similar to opensl_stream_get_position() and opensl_get_min_latency()). Alternatively we could provide the function pointer inside cubeb. I will run it by Paul to take a decision here and continue.
Flags: needinfo?(achronop)
Comment on attachment 8947491 [details]
Bug 1410456 - Allow OMT access to Android system audio properties.

https://reviewboard.mozilla.org/r/217198/#review223084
Attachment #8947491 - Flags: review?(esawin) → review+
Attachment #8920632 - Attachment is obsolete: true
Comment on attachment 8947497 [details]
Bug 1410456 - get mixer latency from JNI instead of system library.

https://reviewboard.mozilla.org/r/217210/#review223332

::: media/libcubeb/src/cubeb-jni-instances.h:18
(Diff revision 1)
> + *
> + * cubeb_stream_get_position()
> + *
> + * Users that want to use that cubeb api method must "overide"
> + * the methods bellow to return a valid instance of JavaVM
> + * and application's Context object.

Have an example file in cubeb's repo when you do the PR upstrea.

It should compile, but just print an error and return null probaby.

::: media/libcubeb/src/cubeb-jni.cpp:47
(Diff revision 1)
> +  assert(cubeb_jni_ptr);
> +
> +  cubeb_jni_ptr->s_java_vm = javaVM;
> +
> +  // Find audio manager object and make it global to call it from another method
> +  jclass context_class = jni_env->FindClass("android/content/Context");

snort or eugen should have a look at this.
Comment on attachment 8947492 [details]
Bug 1410456 - use jni methods in place of removed cubeb methods.

https://reviewboard.mozilla.org/r/217200/#review223334
Attachment #8947492 - Flags: review?(padenot) → review+
Comment on attachment 8947493 [details]
Bug 1410456 - remove an unused variable that produces a compile warning.

https://reviewboard.mozilla.org/r/217202/#review223338
Attachment #8947493 - Flags: review?(padenot) → review+
Comment on attachment 8947494 [details]
Bug 1410456 - remove get min latency implementation because makes use of dlopen.

https://reviewboard.mozilla.org/r/217204/#review223324

::: commit-message-86444:1
(Diff revision 1)
> +Bug 1410456 - remove get min latency implementation because makes use of dlopen. r?padenot

Bug 1410456 - remove opensl's get_min_latency implementation because it uses dlopen.
Attachment #8947494 - Flags: review?(padenot) → review+
Comment on attachment 8947495 [details]
Bug 1410456 - remove get latency implementation because makes use of dlopen.

https://reviewboard.mozilla.org/r/217206/#review223326

::: commit-message-9d765:1
(Diff revision 1)
> +Bug 1410456 - remove get latency implementation because makes use of dlopen. r?padenot

same.
Attachment #8947495 - Flags: review?(padenot) → review+
Comment on attachment 8947496 [details]
Bug 1410456 - remove preferred sample rate implementation because makes use of dlopen.

https://reviewboard.mozilla.org/r/217208/#review223328

::: commit-message-cf9af:1
(Diff revision 1)
> +Bug 1410456 - remove preferred sample rate implementation because makes use of dlopen. r?padenot

same.
Comment on attachment 8947498 [details]
Bug 1410456 - Keep the old mechanism for the older versions that jni method is not available.

https://reviewboard.mozilla.org/r/217212/#review223340
Attachment #8947498 - Flags: review?(padenot) → review+
Comment on attachment 8947496 [details]
Bug 1410456 - remove preferred sample rate implementation because makes use of dlopen.

https://reviewboard.mozilla.org/r/217208/#review223342
Attachment #8947496 - Flags: review?(padenot) → review+
Attachment #8947497 - Flags: review?(snorp)
Attachment #8947497 - Flags: review?(padenot)
Attachment #8947497 - Flags: review?(esawin)
Comment on attachment 8947497 [details]
Bug 1410456 - get mixer latency from JNI instead of system library.

https://reviewboard.mozilla.org/r/217210/#review223354

lgtm, but jchen is really the JNI expert.
Attachment #8947497 - Flags: review?(snorp) → review+
Attachment #8947497 - Flags: review?(esawin) → review?(nchen)
I also NI jchen about the review because reviewboard sometimes fails to deliver notifications
Flags: needinfo?(jcheng)
Blocks: 1412853
Comment on attachment 8947497 [details]
Bug 1410456 - get mixer latency from JNI instead of system library.

https://reviewboard.mozilla.org/r/217210/#review224552

Sorry for the late response. There are a few issues with reference management, but patch looks good overall!

::: media/libcubeb/src/cubeb-jni-instances.h:31
(Diff revision 1)
> +
> +jobject
> +cubeb_jni_get_context_instance()
> +{
> +  auto context = mozilla::java::GeckoAppShell::GetApplicationContext();
> +  return mozilla::jni::Object::GlobalRef(context).Forget();

I suggest you return a local ref instead of a global ref

::: media/libcubeb/src/cubeb-jni.cpp:8
(Diff revision 1)
> +#include "cubeb-jni-instances.h"
> +
> +#define AUDIO_STREAM_TYPE_MUSIC 3
> +
> +JNIEnv *
> +cubeb_jni_get_env_for_thread(JavaVM * javaVM)

Can you use `mozilla::jni::GetEnvForThread()`? i.e. move `cubeb_jni_get_env_for_thread` to cubeb-jni-instances.h

::: media/libcubeb/src/cubeb-jni.cpp:32
(Diff revision 1)
> +extern "C"
> +cubeb_jni *
> +cubeb_jni_init()
> +{
> +  JavaVM * javaVM = cubeb_jni_get_java_vm();
> +  jobject ctx_obj = cubeb_jni_get_context_instance();

`ctx_obj` should be deleted through `jni_env->DeleteGlobalRef()`

::: media/libcubeb/src/cubeb-jni.cpp:47
(Diff revision 1)
> +  assert(cubeb_jni_ptr);
> +
> +  cubeb_jni_ptr->s_java_vm = javaVM;
> +
> +  // Find audio manager object and make it global to call it from another method
> +  jclass context_class = jni_env->FindClass("android/content/Context");

`context_class` is currently a global ref, so it should be deleted through `jni_env->DeleteLocalRef()`

::: media/libcubeb/src/cubeb-jni.cpp:49
(Diff revision 1)
> +  cubeb_jni_ptr->s_java_vm = javaVM;
> +
> +  // Find audio manager object and make it global to call it from another method
> +  jclass context_class = jni_env->FindClass("android/content/Context");
> +  jfieldID audio_service_field = jni_env->GetStaticFieldID(context_class, "AUDIO_SERVICE", "Ljava/lang/String;");
> +  jstring jstr = (jstring)jni_env->GetStaticObjectField(context_class, audio_service_field);

`jstr` should be deleted through `jni_env->DeleteLocalRef()`

::: media/libcubeb/src/cubeb-jni.cpp:51
(Diff revision 1)
> +  // Find audio manager object and make it global to call it from another method
> +  jclass context_class = jni_env->FindClass("android/content/Context");
> +  jfieldID audio_service_field = jni_env->GetStaticFieldID(context_class, "AUDIO_SERVICE", "Ljava/lang/String;");
> +  jstring jstr = (jstring)jni_env->GetStaticObjectField(context_class, audio_service_field);
> +  jmethodID get_system_service_id = jni_env->GetMethodID(context_class, "getSystemService", "(Ljava/lang/String;)Ljava/lang/Object;");
> +  jobject audio_manager_obj = jni_env->CallObjectMethod(ctx_obj, get_system_service_id, jstr);

`audio_manager_obj` should be deleted through `jni_env->DeleteLocalRef()`

::: media/libcubeb/src/cubeb-jni.cpp:55
(Diff revision 1)
> +  jmethodID get_system_service_id = jni_env->GetMethodID(context_class, "getSystemService", "(Ljava/lang/String;)Ljava/lang/Object;");
> +  jobject audio_manager_obj = jni_env->CallObjectMethod(ctx_obj, get_system_service_id, jstr);
> +  cubeb_jni_ptr->s_audio_manager_obj = reinterpret_cast<jobject>(jni_env->NewGlobalRef(audio_manager_obj));
> +
> +  // Make audio manager class a global reference in order to preserve method id
> +  jclass audio_manager_class = jni_env->FindClass("android/media/AudioManager");

`audio_manager_class` should be deleted through `jni_env->DeleteLocalRef()`. If you don't want to deal with individual `DeleteLocalRef` calls, you can also use `jni_env->PushLocalFrame()/PopLocalFrame()`
Attachment #8947497 - Flags: review?(nchen) → review-
Flags: needinfo?(jcheng)
Comment on attachment 8947497 [details]
Bug 1410456 - get mixer latency from JNI instead of system library.

https://reviewboard.mozilla.org/r/217210/#review224552

> Can you use `mozilla::jni::GetEnvForThread()`? i.e. move `cubeb_jni_get_env_for_thread` to cubeb-jni-instances.h

I'm afraid I cannot, I want to keep it decoupled from the mozilla stuff. This is supposed to be an external library. But I can enhance it if you see that something is missing.

> `ctx_obj` should be deleted through `jni_env->DeleteGlobalRef()`

Since I turn it into a local reference I do not have to delete it any more, right?

> `context_class` is currently a global ref, so it should be deleted through `jni_env->DeleteLocalRef()`

Do you mean it's a local ref? You said global and I guess it's a typo.

> `audio_manager_class` should be deleted through `jni_env->DeleteLocalRef()`. If you don't want to deal with individual `DeleteLocalRef` calls, you can also use `jni_env->PushLocalFrame()/PopLocalFrame()`

This is a genaral question about all local ref deletes. My understanding is that local refs are deleted automatically outside the declared scope. Is this wrong? I will take a look in `jni_env->PushLocalFrame()/PopLocalFrame()` since I am not very familiar but I do not mind really to take care individual deletes. They are not so many. Which method do you suggest?
Comment on attachment 8947497 [details]
Bug 1410456 - get mixer latency from JNI instead of system library.

https://reviewboard.mozilla.org/r/217210/#review224552

> I'm afraid I cannot, I want to keep it decoupled from the mozilla stuff. This is supposed to be an external library. But I can enhance it if you see that something is missing.

That's fine. The existing code looks okay.

> Since I turn it into a local reference I do not have to delete it any more, right?

After turning it into a local ref, you would use `DeleteLocalRef()`

> Do you mean it's a local ref? You said global and I guess it's a typo.

Oops. Yeah should be "local ref"

> This is a genaral question about all local ref deletes. My understanding is that local refs are deleted automatically outside the declared scope. Is this wrong? I will take a look in `jni_env->PushLocalFrame()/PopLocalFrame()` since I am not very familiar but I do not mind really to take care individual deletes. They are not so many. Which method do you suggest?

Local refs are only deleted when crossing the native to Java boundary (e.g. if `cubeb_jni_init` returns directly to Java code). Otherwise, we have to manage the refs ourselves.
Comment on attachment 8947497 [details]
Bug 1410456 - get mixer latency from JNI instead of system library.

https://reviewboard.mozilla.org/r/217210/#review225410

::: media/libcubeb/src/cubeb-jni-instances.h:31
(Diff revision 2)
> +
> +jobject
> +cubeb_jni_get_context_instance()
> +{
> +  auto context = mozilla::java::GeckoAppShell::GetApplicationContext();
> +  return mozilla::jni::Object::LocalRef(context).Forget();

or just `context.Forget()`
Attachment #8947497 - Flags: review?(nchen) → review+
Comment on attachment 8947497 [details]
Bug 1410456 - get mixer latency from JNI instead of system library.

https://reviewboard.mozilla.org/r/217210/#review225562
Attachment #8947497 - Flags: review?(padenot) → review+
Comment on attachment 8952190 [details]
Bug 1410456 - Add new files in update.sh script.

https://reviewboard.mozilla.org/r/221434/#review227222
Attachment #8952190 - Flags: review?(achronop) → review+
Comment on attachment 8952190 [details]
Bug 1410456 - Add new files in update.sh script.

https://reviewboard.mozilla.org/r/221434/#review227224

I r+ myself, commit contains only the README_MOZILLA file update, not source code.
Pushed by achronop@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c170d37b1a04
Allow OMT access to Android system audio properties. r=esawin
https://hg.mozilla.org/integration/autoland/rev/1f75636b5bce
use jni methods in place of removed cubeb methods. r=padenot
https://hg.mozilla.org/integration/autoland/rev/f996b9dce4a6
remove an unused variable that produces a compile warning. r=padenot
https://hg.mozilla.org/integration/autoland/rev/d59060ecd24c
remove get min latency implementation because makes use of dlopen. r=padenot
https://hg.mozilla.org/integration/autoland/rev/f7eae1545d5e
remove get latency implementation because makes use of dlopen. r=padenot
https://hg.mozilla.org/integration/autoland/rev/0ec75a56b4c7
remove preferred sample rate implementation because makes use of dlopen. r=padenot
https://hg.mozilla.org/integration/autoland/rev/18ef18999175
get mixer latency from JNI instead of system library. r=jchen,padenot,snorp
https://hg.mozilla.org/integration/autoland/rev/19f3248502d9
Add new files in update.sh script. r=padenot
https://hg.mozilla.org/integration/autoland/rev/36f6b40dfa88
Update cubeb from upstream to 84e9568. r=achronop
It seems 4.2 does not have the method:

02-19 19:13:47.300  2209  2529 W dalvikvm: JNI WARNING: JNI method called with exception pending
02-19 19:13:47.300  2209  2529 W dalvikvm:              in Ldalvik/system/NativeStart;.run:()V (GetStaticMethodID)
02-19 19:13:47.300  2209  2529 W dalvikvm: Pending exception is:
02-19 19:13:47.300  2209  2529 I dalvikvm: java.lang.NoSuchMethodError: no method with name='getOutputLatency' signature='(I)I' in class Landroid/media/AudioManager;
02-19 19:13:47.300  2209  2529 I dalvikvm: 	at dalvik.system.NativeStart.run(Native Method)
Flags: needinfo?(achronop)
Comment on attachment 8952190 [details]
Bug 1410456 - Add new files in update.sh script.

https://reviewboard.mozilla.org/r/221434/#review228604

::: media/libcubeb/src/android/cubeb-output-latency.h:23
(Diff revision 2)
> +  output_latency * ol = NULL;
> +  ol = calloc(1, sizeof(output_latency));
> +
> +  ol->version = version;
> +
> +  if (ol->version > 17){

Add a comment so that we know this is > 4.2.

::: media/libcubeb/src/android/cubeb-output-latency.h:36
(Diff revision 2)
> +
> +bool
> +cubeb_output_latency_method_is_loaded(output_latency * ol)
> +{
> +  assert(ol && (ol->from_jni || ol->from_lib));
> +  if (ol->version > 17){

Same.

::: media/libcubeb/src/cubeb_opensl.c:83
(Diff revision 2)
>    SLInterfaceID SL_IID_VOLUME;
>    SLInterfaceID SL_IID_RECORD;
>    SLObjectItf engObj;
>    SLEngineItf eng;
>    SLObjectItf outmixObj;
> -  cubeb_jni * jni_obj;
> +  output_latency * p_output_latency;

`output_latency_function` would be clearer here, same for the name of the attribute.
Attachment #8952190 - Flags: review?(padenot) → review+
Comment on attachment 8952190 [details]
Bug 1410456 - Add new files in update.sh script.

https://reviewboard.mozilla.org/r/221434/#review228616
Comment on attachment 8953969 [details]
Bug 1410456 - Pass in cubeb JNIEnv instead of JavaVM.

https://reviewboard.mozilla.org/r/223126/#review229038
Attachment #8953969 - Flags: review?(padenot) → review+
Pushed by achronop@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7bf358e3e01b
Allow OMT access to Android system audio properties. r=esawin
https://hg.mozilla.org/integration/autoland/rev/82f6667c6758
use jni methods in place of removed cubeb methods. r=padenot
https://hg.mozilla.org/integration/autoland/rev/1a9c687ec277
remove an unused variable that produces a compile warning. r=padenot
https://hg.mozilla.org/integration/autoland/rev/6a8ed4bf5d2f
remove get min latency implementation because makes use of dlopen. r=padenot
https://hg.mozilla.org/integration/autoland/rev/f7b646045e06
remove get latency implementation because makes use of dlopen. r=padenot
https://hg.mozilla.org/integration/autoland/rev/e482347bdae3
remove preferred sample rate implementation because makes use of dlopen. r=padenot
https://hg.mozilla.org/integration/autoland/rev/f182ab7885db
get mixer latency from JNI instead of system library. r=jchen,padenot,snorp
https://hg.mozilla.org/integration/autoland/rev/5248a21216ae
Keep the old mechanism for the older versions that jni method is not available. r=padenot
https://hg.mozilla.org/integration/autoland/rev/94457911bb24
Add new files in update.sh script. r=padenot
https://hg.mozilla.org/integration/autoland/rev/7ec175446efd
Update cubeb from upstream to b1ee1ce. r=padenot
02-26 05:46:20.538   811   831 I GeckoDump: ⰲ겿{"action":"test_status","time":1519652780540,"thread":null,"pid":null,"source":"mochitest","test":"/tests/dom/base/test/test_postMessages.html","subtest":"Props match","status":"PASS","js_source":"TestRunner.js"}ⰲ겿
02-26 05:46:20.558   811   831 I GeckoDump: ⰲ겿{"action":"test_status","time":1519652780558,"thread":null,"pid":null,"source":"mochitest","test":"/tests/dom/base/test/test_postMessages.html","subtest":"Props match - using toSource()","status":"PASS","js_source":"TestRunner.js"}ⰲ겿
02-26 05:46:20.568   811   831 I GeckoDump: ⰲ겿{"action":"test_status","time":1519652780560,"thread":null,"pid":null,"source":"mochitest","test":"/tests/dom/base/test/test_postMessages.html","subtest":"Type matches","status":"PASS","js_source":"TestRunner.js"}ⰲ겿
02-26 05:46:20.568   811   831 I GeckoDump: ⰲ겿{"action":"test_status","time":1519652780570,"thread":null,"pid":null,"source":"mochitest","test":"/tests/dom/base/test/test_postMessages.html","subtest":"Array.length matches","status":"PASS","js_source":"TestRunner.js"}ⰲ겿
02-26 05:46:20.668    38    38 D Zygote  : Process 811 terminated by signal (11)
02-26 05:46:20.688   278   446 I ActivityManager: Process org.mozilla.fennec_aurora (pid 811) has died.
02-26 05:46:20.688   278   446 W ActivityManager: Scheduling restart of crashed service org.mozilla.fennec_aurora/org.mozilla.gecko.media.MediaControlService in 5000ms
02-26 05:46:20.688   278   290 I WindowState: WIN DEATH: Window{418d1fd0 u0 org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp}
02-26 05:46:20.698   278   290 W WindowManager: Force-removing child win Window{41a162b8 u0 SurfaceView} from container Window{418d1fd0 u0 org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp}
02-26 05:46:20.717   278   446 W ActivityManager: Force removing ActivityRecord{41913a40 u0 org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp}: app died, no saved state
02-26 05:46:20.747   278   289 W WindowManager: Failed looking up window
02-26 05:46:20.747   278   289 W WindowManager: java.lang.IllegalArgumentException: Requested window android.os.BinderProxy@419576f0 does not exist
02-26 05:46:20.747   278   289 W WindowManager: 	at com.android.server.wm.WindowManagerService.windowForClientLocked(WindowManagerService.java:7630)
02-26 05:46:20.747   278   289 W WindowManager: 	at com.android.server.wm.WindowManagerService.windowForClientLocked(WindowManagerService.java:7621)
02-26 05:46:20.747   278   289 W WindowManager: 	at com.android.server.wm.WindowState$DeathRecipient.binderDied(WindowState.java:1000)
02-26 05:46:20.747   278   289 W WindowManager: 	at android.os.BinderProxy.sendDeathNotice(Binder.java:470)
02-26 05:46:20.747   278   289 W WindowManager: 	at dalvik.system.NativeStart.run(Native Method)
02-26 05:46:20.747   278   289 I WindowState: WIN DEATH: null
02-26 05:46:20.958   408   408 W EGL_emulation: eglSurfaceAttrib not implemented
02-26 05:46:20.968   278   289 W InputMethodManagerService: Got RemoteException sending setActive(false) notification to pid 811 uid 10043
02-26 05:46:21.028   408   408 D dalvikvm: GC_FOR_ALLOC freed 236K, 20% free 4065K/5072K, paused 28ms, total 32ms
02-26 05:46:48.218   278   281 D dalvikvm: GC_CONCURRENT freed 582K, 44% free 5228K/9320K, paused 5ms+28ms, total 152ms
Flags: needinfo?(achronop)
Also a little earlier in this log:

02-26 05:46:18.388   811  1441 D dalvikvm: threadid=19: thread exiting, not yet detached (count=0)
02-26 05:46:18.398   811  1460 D dalvikvm: threadid=20: thread exiting, not yet detached (count=0)
02-26 05:46:18.398   811  1463 D dalvikvm: threadid=24: thread exiting, not yet detached (count=0)
02-26 05:46:18.398   811  1463 D dalvikvm: threadid=24: thread exiting, not yet detached (count=1)
02-26 05:46:18.398   811  1463 E dalvikvm: threadid=24: native thread exited without detaching
02-26 05:46:18.398   811  1463 E dalvikvm: VM aborting
02-26 05:46:18.448   811  1440 D dalvikvm: threadid=17: thread exiting, not yet detached (count=0)
02-26 05:46:18.468   811  1463 W google-breakpad: ExceptionHandler::GenerateDump cloned child 
02-26 05:46:18.468   811  1463 W google-breakpad: 1516üÿÿÿ
02-26 05:46:18.468   811  1463 W google-breakpad: 
02-26 05:46:18.468   811  1463 W google-breakpad: ExceptionHandler::SendContinueSignalToChild sent continue signal to child
02-26 05:46:18.488  1516  1516 W google-breakpad: ExceptionHandler::WaitForContinueSignal waiting for continue signal...

jchen: Can you please check this back out error? Do you think, this problem is about not detaching the newly attached JNIEnv in audio threads? Assuming that I created a try push to solve it, let's see how it goes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3af91498e8e5b04a291a3a88443e0403e8f5d0c
Flags: needinfo?(nchen)
Ok, so you need to call JavaVM->DetachCurrentThread() before a thread dies. In Gecko, is is accomplished by using a pthread key slot [1], which lets you register a thread death callback; we then detach the thread in the callback [2]. You'd need something similar for your code.

[1] https://searchfox.org/mozilla-central/rev/b469db5c90d618f4b202d5ef280e1a78224eb43b/widget/android/jni/Utils.cpp#104
[2] https://searchfox.org/mozilla-central/rev/b469db5c90d618f4b202d5ef280e1a78224eb43b/widget/android/jni/Utils.cpp#83

Alternatively, going back to my suggestion of using `mozilla::jni::GetEnvForThread`

> > Can you use `mozilla::jni::GetEnvForThread()`? i.e. move `cubeb_jni_get_env_for_thread` to cubeb-jni-instances.h
> 
> I'm afraid I cannot, I want to keep it decoupled from the mozilla stuff.
> This is supposed to be an external library. But I can enhance it if you see
> that something is missing.

My idea was to make the library user's responsibility to implement `cubeb_jni_get_env_for_thread`, and Gecko's implementation would be to call `mozilla::jni::GetEnvForThread`. That way you automatically get this detach-on-death behavior.
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #82)
> Ok, so you need to call JavaVM->DetachCurrentThread() before a thread dies.
> In Gecko, is is accomplished by using a pthread key slot [1], which lets you
My implementation is here [1] and try [2] looks green now. Could you please have a look?

> Alternatively, going back to my suggestion of using `mozilla::jni::GetEnvForThread`
Yeah, it was a good suggestion, I realize it now that I see the extra requirements. My opinion back then was that I leave too much work for any potential user of the library. I thought that passing the JavaVM instance is trivial enough from that point of view. On the other hand now I have to detach the newly created threads but not the existing which is also not very nice. Which solution do you thing is better?

[1] https://hg.mozilla.org/try/rev/863ab562503be9a35b6dd0f25e9619bd5eb61063
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3af91498e8e5b04a291a3a88443e0403e8f5d0c

Thanks
Try run of the second option (get JNIEnv directly from the user):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b18f5081b33698618731b70312f6096761d82b28
Tagging this bug as a Klar+GeckoView blocker.
status-firefox58: --- → wontfix
status-firefox59: --- → ?
status-firefox60: --- → affected
Whiteboard: [geckoview:klar]
Comment on attachment 8955116 [details]
Bug 1410456 - Update cubeb from upstream to eb3409e.

https://reviewboard.mozilla.org/r/224272/#review230230

I r+ myself, this is a change in a README file, source code is not affected.
Attachment #8955116 - Flags: review?(achronop) → review+
Pushed by achronop@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc1f697256cf
Allow OMT access to Android system audio properties. r=esawin
https://hg.mozilla.org/integration/autoland/rev/d84d0c956d48
use jni methods in place of removed cubeb methods. r=padenot
https://hg.mozilla.org/integration/autoland/rev/d462d18666be
remove an unused variable that produces a compile warning. r=padenot
https://hg.mozilla.org/integration/autoland/rev/ff486aaaddbb
remove get min latency implementation because makes use of dlopen. r=padenot
https://hg.mozilla.org/integration/autoland/rev/1123e0934d4d
remove get latency implementation because makes use of dlopen. r=padenot
https://hg.mozilla.org/integration/autoland/rev/bc670bbf9e2c
remove preferred sample rate implementation because makes use of dlopen. r=padenot
https://hg.mozilla.org/integration/autoland/rev/dcd12fd91dd0
get mixer latency from JNI instead of system library. r=jchen,padenot,snorp
https://hg.mozilla.org/integration/autoland/rev/96def2ab5e5e
Keep the old mechanism for the older versions that jni method is not available. r=padenot
https://hg.mozilla.org/integration/autoland/rev/11ca9fbd181f
Add new files in update.sh script. r=padenot
https://hg.mozilla.org/integration/autoland/rev/343e35827f30
Pass in cubeb JNIEnv instead of JavaVM. r=padenot
https://hg.mozilla.org/integration/autoland/rev/22c153aa809b
Update cubeb from upstream to eb3409e.r=achronop
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: