http://developer.android.com/preview/behavior-changes.html#ndk I have seen this error in the N preview. Not crashing when I see the message. I tend to see this upon unlocking the phone.

I'm not sure we can avoid the private APIs we're currently using, so we might not have a good way forward here. Hopefully the full N release doesn't have these annoying toasts.

With these patches I think we get rid of a lot of the dlopen, but certainly not all of it.

Comment on attachment 8756509 [details] [diff] [review] Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents Review of attachment 8756509 [details] [diff] [review]: ----------------------------------------------------------------- Yay for removing code!

Comment on attachment 8756505 [details] [diff] [review] Build against NDK platform 15 Review of attachment 8756505 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/autoconf/android.m4 @@ +10,5 @@ > use the specified C++ STL (stlport, libstdc++, libc++)], > android_cxx_stl=$withval, > android_cxx_stl=libc++) > > +define([MIN_ANDROID_VERSION], [15]) Are we dropping support for Android < 4.0.3 ?

Bug 1155801 and Bug 1237342 dropped < API 15 support from Firefox for Android in 46.

(In reply to Richard Newman [:rnewman] from comment #8) > Bug 1155801 and Bug 1237342 dropped < API 15 support from Firefox for > Android in 46. O_o these bugs raise more questions... - Why are --with-android-min-sdk and --with-android-version separate? IOW, what is the point of building with different versions there? Something built with --with-android-min-sdk=15 won't run on < 4.0.3, right, so why care about making the native binaries compatible with older versions? - Why did https://reviewboard.mozilla.org/r/29721/diff/1#index_header *drop* the android version? Nothing in the context of those bugs explains it (or in the commit message).

(In reply to Mike Hommey [:glandium] from comment #9) > (In reply to Richard Newman [:rnewman] from comment #8) > > Bug 1155801 and Bug 1237342 dropped < API 15 support from Firefox for > > Android in 46. > > O_o these bugs raise more questions... > > - Why are --with-android-min-sdk and --with-android-version separate? IOW, > what is the point of building with different versions there? Something built > with --with-android-min-sdk=15 won't run on < 4.0.3, right, so why care > about making the native binaries compatible with older versions? I think there is some value to having these separate. You know better than I that newer NDK platform veresions sometimes break stuff (usually due to removed symbols). Pretty sure we don't build against 21 at all (or something close to that, I don't recall). > - Why did https://reviewboard.mozilla.org/r/29721/diff/1#index_header *drop* > the android version? Nothing in the context of those bugs explains it (or in > the commit message). That was only for b2gdroid. I think it had been incremented incorrectly, IIRC. At any rate, building against 15 seems like a good idea if we can remove a bunch of code.

Comment on attachment 8756505 [details] [diff] [review] Build against NDK platform 15 Review of attachment 8756505 [details] [diff] [review]: ----------------------------------------------------------------- > I think there is some value to having these separate. You know better than I > that newer NDK platform veresions sometimes break stuff (usually due to > removed symbols). Pretty sure we don't build against 21 at all (or something > close to that, I don't recall). OTOH, recent NDKs have broken *old* API versions (e.g. API version 9 in NDK 11 is not the same as API version 9 in NDK 10) Anyways, I don't care that much. r+

It looks like we could be pretty hosed when the final version of N rolls out according to this[0]. We use several libraries that are not in the temporarily approved list, so as soon as N final is out we will probably crash on startup. I'm going to land these patches and see where we stand, there's likely more stuff to fix up. [0] http://android-developers.blogspot.com/2016/06/improving-stability-with-private-cc.html

Renoming per shitstorm described in comment #12

We need to get this fixed before the final N release. Some folks are predicting that to be as early as mid-late August, so I think that means this needs fixed in 51.

Instead, use JNI in Gecko to obtain latency and preferred sample rate, and pass those to cubeb.

Comment on attachment 8775142 [details] [diff] [review] Don't dlopen libmedia in cubeb Review of attachment 8775142 [details] [diff] [review]: ----------------------------------------------------------------- You need to rebase this. Conveniently for you, cubeb now use frames for latency, so you can just pass the number directly.

(In reply to Paul Adenot (:padenot) from comment #16) > Comment on attachment 8775142 [details] [diff] [review] > Don't dlopen libmedia in cubeb > > Review of attachment 8775142 [details] [diff] [review]: > ----------------------------------------------------------------- > > You need to rebase this. Conveniently for you, cubeb now use frames for > latency, so you can just pass the number directly. I already did rebase it to use frames. Did I miss a spot?

Comment on attachment 8775142 [details] [diff] [review] Don't dlopen libmedia in cubeb Review of attachment 8775142 [details] [diff] [review]: ----------------------------------------------------------------- Can't vouch for the java and binding bits, but the C++ bits looks correct. Remember you need to uplift cubeb_opensl.c to https://github.com/kinetiknz/cubeb, and then use `./update.sh` in `media/libcube/` to get your patch. This might pull in a bit more that what you want. It's all been reviewed by either achronop or myself, feel free to land. ::: dom/media/CubebUtils.cpp @@ +106,5 @@ > + if (jni::IsAvailable()) { > + value = java::GeckoAppShell::GetAudioOutputLatencyFrames(); > + } else { > + // Need to have something here, go with 10 frames? > + value = 10; 10 frames is very very small. More like 256. ::: media/libcubeb/src/cubeb_opensl.c @@ +413,5 @@ > stm->lastCompensativePosition = -1; > > + // If no latency value is passed, use 10ms > + if (stm->latency <= 0) { > + stm->latency = 10; This is supposed to be in frame. Use 256, which I just invented, but should be alright. @@ +451,5 @@ > res = (*ctx->eng)->CreateAudioPlayer(ctx->eng, &stm->playerObj, &source, > &sink, NELEMS(ids), ids, req); > } > > + // Sample rate not supported? Try again with 44k? 44.1k. There are also very esoteric audio systems with 44k, and that caused us issues in the past. @@ +634,5 @@ > int64_t maximum_position = stm->written * (int64_t)stm->inputrate / stm->outputrate; > pthread_mutex_unlock(&stm->mutex); > assert(maximum_position >= 0); > > + if (msec > stm->latency) { stm->latency is in frames, your comparing it to milliseconds.

r+ with the the comments fixed.

Instead, use JNI in Gecko to obtain latency and preferred sample rate, and pass those to cubeb.

Paul I just want to make sure I didn't screw anything up with the review fixups. Thanks.

Comment on attachment 8776181 [details] [diff] [review] Don't dlopen libmedia in cubeb Review of attachment 8776181 [details] [diff] [review]: ----------------------------------------------------------------- snorp, remember to land the cubeb part via the upstream cubeb repo. achronop or kinetik can help you do that, I'm going on PTO in approximately 30 seconds.

Pushed by jwillcox@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7aa2512bf7 Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents r=jchen https://hg.mozilla.org/integration/mozilla-inbound/rev/4a3775a4a1ab Remove AndroidNativeWindow, as we can use the NDK functions directly now r=rbarker

https://hg.mozilla.org/mozilla-central/rev/7a7aa2512bf7 https://hg.mozilla.org/mozilla-central/rev/4a3775a4a1ab

Snorp, are you going to fill an uplift request for this to aurora & beta? Thanks

Comment on attachment 8756509 [details] [diff] [review] Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents Approval Request Comment [Feature/regressing bug #]: Android N release [User impact if declined]: *possibly* bad behavior on N (though non reported yet) [Describe test coverage new/current, TreeHerder]: Nightly [Risks and why]: low, just uses NDK calls for private calls we had before [String/UUID change made/needed]: none

Hey Kevin, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment on attachment 8756509 [details] [diff] [review] Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents Fixes needed for Android N compatibility, let's uplift to Aurora50.

I cannot. Google removed the alert dialogs from the last couple Android 7.0 preview releases. The alerts are not in the RTM Android 7.0 builds.

I see messages about libmedia.so when using my own (debuggable) build in Android 7. Not sure if the cubeb patch got upstreamed?

https://hg.mozilla.org/releases/mozilla-aurora/rev/72984b90a353 https://hg.mozilla.org/releases/mozilla-aurora/rev/3c133d122ea1

Correct, the cubeb one is still pending.

Actually, I'm going to file a different bug for the cubeb issue with libmedia.so

Comment on attachment 8756509 [details] [diff] [review] Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents Let's uplift this now and it should be in the beta 10 mobile build on Monday. We don't want Android N users to hit this issue which potentially could be a bad startup crash.

I'm hitting conflicts uplifting this to beta. Could we get a rebased patch?

I think we'll just let this one slide for 49 since it's so late now. Things seem to be working alright on N as it is anyway.

Comment on attachment 8756509 [details] [diff] [review] Replace and/or remove some graphics-related calls in AndroidBridge with NDK equivalents Clearing the beta approval since we are wontfixing for 49.