1316934 - Illegal access to system library libmedia.so

Status: RESOLVED DUPLICATE of bug 1410456

(Firefox for Android Graveyard :: Audio/Video, defect, P1)

Description

[Eugen Sawin [:esawin]]

We should use AudioManager where available instead of getting the device audio specifics directly from libmedia.so to avoid incompatibility pop-ups in Android M+ and potentially crashes in future Android version, see bug 1255628.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1316934-1.0-Use-float-for-playback-latency-for-i.patch

Currently, playback latency is stored as uint32_t in milliseconds and converted to frames when passed to Cubeb.

This results in highly imprecise latency frames which leads to audio jitter.

For example, the Pixel C device has 512 output frames at a preferred rate of 48kHz, which translates to a latency of 10.7ms. Currently, that would reflect in a latency of 10ms, translated back to frames we would initialize the Cubeb stream with a latency of 480 frames instead of 512.

Ideally, we should keep latency units consistent and handle everything in frames instead of mixing types (maybe in a follow-up bug).

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1316934-2.0-Refactor-PreferredSampleRate.-r-pade.patch

Combine InitPreferredSampleRate and PreferredSampleRate for simplification and to prepare for patch 4.

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1316934-3.0-Add-JNI-for-AudioManager-properties..patch

Add AudioManager JNI.

Comment 4

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1316934-4.0-Use-JNI-AudioManager-properties-to-s.patch

Use AudioManager JNI to set preferred sample rate and latency.

Comment 5

[Eugen Sawin [:esawin]]

Attachment: 0005-Bug-1316934-5.0-cubeb-Pass-device-specific-sample-ra.patch

Add preferred rate to the output parameters and use that.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0006-Bug-1316934-6.0-Pass-device-specific-sample-rate-to-.patch

Pass preferred rate to Cubeb.

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0007-Bug-1316934-7.0-cubeb-Remove-instances-of-direct-acc.patch

We don't need to know the Android version, Gingerbread is no longer supported.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 0008-Bug-1316934-8.0-cubeb-Remove-instances-of-direct-acc.patch

Remove all direct access to libmedia.

We can reduce some more if it's safe to set get_preferred_sample_rate and get_min_latency to NULL in the cubeb_ops struct.

Comment 9

[Paul Adenot (:padenot)]

Comment on attachment 8810517 [details] [diff] [review]
0007-Bug-1316934-7.0-cubeb-Remove-instances-of-direct-acc.patch

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

Stealing the review. Please upstream this and we'll land it for you.

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8810508 [details] [diff] [review]
0002-Bug-1316934-2.0-Refactor-PreferredSampleRate.-r-pade.patch

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

::: dom/media/CubebUtils.cpp
@@ +170,2 @@
>    }
> +  if (sPreferredSampleRate < 1000) {

Why `< 1000` here? It's kind of arbitrary. Either it's zero, or initialization failed and we know that, or it succeeded and it's not zero.

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8810505 [details] [diff] [review]
0001-Bug-1316934-1.0-Use-float-for-playback-latency-for-i.patch

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

As you've seen, we have two type of audio streams in Gecko: low latency and higher latency. Low latency is used for WebRTC, Web Audio API and when a stream is "captured" via the capture methods of an HTMLMediaElement. Higher latency stream are used when playing a media with an HTMLMediaElement (either via MSE, EME or regular HTTP streaming).

The assumption here is that we don't care about the exact latency of the "higher latency" stream, we prefer lower the CPU usage by working in larger chunks. Historically, this was in milliseconds because it's easier to reason about.

For low latency streams, we work in frames because we want to trigger some fast paths on some platforms, as you noted. This allow us to be more precise.

It looks like, down the line, we're always using small buffer size on Android to trigger the fast mixer path, so this does not matter, this number is not being used in cubeb anyways.

Comment 12

[Paul Adenot (:padenot)]

I think things have changed between proposing the solution to plumb the rate and buffer size from Gecko to cubeb:

- We now always resample in cubeb on Android if the media rate is different from the device rate: We (apparently) can't let audioflinger resample. I don't know why that changed, and I need to know why, possibly with code links from androidxref or something. Maybe it has to be with CPU throttling on apps that are in the background. I don't see code in other open-source programs that do the resampling explicitly (admittedly I haven't searched for long). We need to know why because it's often better to let the platform do the resampling, since the system mixer often better decisions as for when to mix and when to resample to save CPU.
- We've been using the smallest latency for any stream for some time now. It's unclear to me (1) whether we've looked at the implications that has in terms of battery and CPU (this provokes more wakeups, use a high-priority thread, adds another scheduling class...), (2) whether there is an alternative and this was simply the easiest solution to keep things running.
- We now have a way to resample the audio from `HTMLMediaElement` level. Jean-Yves implemented that recently. Maybe we could leverage that to always open cubeb streams at the preferred device rate ? This forces us to handle the resampling ourselves, but there are other reasons for Jean-Yves to have written this code, maybe he can comment on the matter. I don't know if those reasons apply on Android.
- WebRTC and Web Audio API (in other words, real-time audio where latency really matters) are always using the device sample-rate, there should be no resampling there.

Basically, I'm concern that this breaks the whole purpose of cubeb: allowing audio output and input in a cross platform fashion, without having to care about the specifics. Simply implementing the two functions we need in cubeb solves a number of problems.


All this basically boils down to the following:

How hard it is to write some JNI code in cubeb to get the two numbers we need? When I did the dlopen hack, it was easier to do than to use JNI, and it also had the benefit to work on B2G.

Should we implement audio on android differently so we don't run into underruns ? Can we use larger buffers for backgrounded apps ?

I'm cancelling the reviews in the mean time.

Comment 13

[Jean-Yves Avenard [:jya]]

(In reply to Paul Adenot (:padenot) from comment #12)
> - We now have a way to resample the audio from `HTMLMediaElement` level.
> Jean-Yves implemented that recently. Maybe we could leverage that to always
> open cubeb streams at the preferred device rate ? This forces us to handle
> the resampling ourselves, but there are other reasons for Jean-Yves to have
> written this code, maybe he can comment on the matter. I don't know if those
> reasons apply on Android.

Sorry for the late reply... somehow this skipped my radar.

Currently, except for 44.1kHz and 48kHz, the DecodedAudioDataSink will always resample everything coming from the media decoder to cubeb's preferred sampling rate.

This allows us not to have to care about what sampling rate is coming from media content where the sampling rate could be changing part-way (like is the case with MSE and adaptative streaming).

So the media decoding stack is almost already always opening a cubeb stream using cubeb's preferred rate, be it on desktop or android.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

We need to get some activity on this again. The Fennec team wants to target API 26 to take work better on newer versions of Android, and this is going to cause the OS to start denying the libmedia.so dlopen(). https://developer.android.com/about/versions/nougat/android-7.0-changes.html#ndk

Comment 16

[Jean-Yves Avenard [:jya]]

I leave it to :padenot to answer, I'm not sure I can contribute to this discussion

Comment 17

[Paul Adenot (:padenot)]

snorp, if you can get me a way to get the native sample-rate and the native buffer size (only available through Java/JNI) in Gecko, we can solve this now.

I need to have the value of `rate` and `size` in this java code:

> AudioManager audioManager = (AudioManager) this.getSystemService(Context.AUDIO_SERVICE);
> String rate = audioManager.getProperty(AudioManager.PROPERTY_OUTPUT_SAMPLE_RATE);
> String size = audioManager.getProperty(AudioManager.PROPERTY_OUTPUT_FRAMES_PER_BUFFER);

https://github.com/kinetiknz/cubeb/pull/141 will have to be rebased, but it's not hard.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Paul Adenot (:padenot) from comment #17)
> snorp, if you can get me a way to get the native sample-rate and the native
> buffer size (only available through Java/JNI) in Gecko, we can solve this
> now.

In Gecko, or in cubeb? It's easy in Gecko because we have a bindings system for JNI. If we don't have that facility it's a giant PITA, but doable. I think Eugen will handle this...

Comment 19

[Paul Adenot (:padenot)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #18)
> (In reply to Paul Adenot (:padenot) from comment #17)
> > snorp, if you can get me a way to get the native sample-rate and the native
> > buffer size (only available through Java/JNI) in Gecko, we can solve this
> > now.
> 
> In Gecko, or in cubeb? It's easy in Gecko because we have a bindings system
> for JNI. If we don't have that facility it's a giant PITA, but doable. I
> think Eugen will handle this...

In Gecko yeah. It would be better to have it in cubeb, but if it's too hard we can do it in Gecko.

Comment 20

[Eugen Sawin [:esawin]]

(In reply to Paul Adenot (:padenot) from comment #17)
> snorp, if you can get me a way to get the native sample-rate and the native
> buffer size (only available through Java/JNI) in Gecko, we can solve this
> now.
> 
> I need to have the value of `rate` and `size` in this java code:
> 
> > AudioManager audioManager = (AudioManager) this.getSystemService(Context.AUDIO_SERVICE);
> > String rate = audioManager.getProperty(AudioManager.PROPERTY_OUTPUT_SAMPLE_RATE);
> > String size = audioManager.getProperty(AudioManager.PROPERTY_OUTPUT_FRAMES_PER_BUFFER);
> 
> https://github.com/kinetiknz/cubeb/pull/141 will have to be rebased, but
> it's not hard.

That's patch 3, want me to land it?

Comment 21

[Paul Adenot (:padenot)]

Yes please. Once we have that in, we can do the audio part (rebase the cubeb part, call the new functions you exposed when calling into cubeb).

Comment 22

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1316934-3.1-Add-JNI-for-AudioManager-properties..patch

Rebased.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/507feca45d0b
[3.1] Add JNI for AudioManager properties. r=snorp

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/507feca45d0b

Comment 25

[James Willcox (:snorp) ([e-mail]) (he/him)]

Paul, Eugen landed the patch to allow you to get the sample rate via JNI in Gecko, are you going to take care of the rest? Moving Fennec to SDK 26 is a P1 bug for 58, but we'll have no audio if this bug is not fixed.

Comment 26

[Paul Adenot (:padenot)]

snorp, yes, one of us will take care of the cubeb side. You've already provided a patch, it's upstream. I expect this to be done in the 58 cycles.

Alex, do you have cycles to rebase https://github.com/kinetiknz/cubeb/pull/141 and land it? The patch landed in comment 23 allows querying the correct rate and buffer size, we should modify GraphDriver.cpp to use those values, on android.

Comment 27

[Paul Adenot (:padenot)]

In fact, I think that (modulo bitrot), Eugen has provided all the pieces here.

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

Any update here?

Comment 29

[Paul Adenot (:padenot)]

Alex, any update ?

Comment 30

[Alex Chronopoulos [:achronop]]

Yeah, I mentioned to jib during our 1-1 and we agreed that it is more important than the other tasks. I'm starting it tomorrow morning.

Comment 32

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.