Illegal access to system library libmedia.so

NEW
Assigned to

Status

()

Firefox for Android
Audio/Video
P1
normal
a year ago
28 days ago

People

(Reporter: esawin, Assigned: esawin)

Tracking

(Blocks: 2 bugs, {leave-open})

51 Branch
All
Android
leave-open
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8810505 [details] [diff] [review]
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).
Attachment #8810505 - Flags: review?(padenot)
(Assignee)

Comment 2

a year ago
Created attachment 8810508 [details] [diff] [review]
0002-Bug-1316934-2.0-Refactor-PreferredSampleRate.-r-pade.patch

Combine InitPreferredSampleRate and PreferredSampleRate for simplification and to prepare for patch 4.
Attachment #8810508 - Flags: review?(padenot)
(Assignee)

Comment 3

a year ago
Created attachment 8810509 [details] [diff] [review]
0003-Bug-1316934-3.0-Add-JNI-for-AudioManager-properties..patch

Add AudioManager JNI.
Attachment #8810509 - Flags: review?(snorp)
(Assignee)

Comment 4

a year ago
Created attachment 8810510 [details] [diff] [review]
0004-Bug-1316934-4.0-Use-JNI-AudioManager-properties-to-s.patch

Use AudioManager JNI to set preferred sample rate and latency.
Attachment #8810510 - Flags: review?(padenot)
(Assignee)

Comment 5

a year ago
Created attachment 8810513 [details] [diff] [review]
0005-Bug-1316934-5.0-cubeb-Pass-device-specific-sample-ra.patch

Add preferred rate to the output parameters and use that.
Attachment #8810513 - Flags: review?(padenot)
(Assignee)

Comment 6

a year ago
Created attachment 8810514 [details] [diff] [review]
0006-Bug-1316934-6.0-Pass-device-specific-sample-rate-to-.patch

Pass preferred rate to Cubeb.
Attachment #8810514 - Flags: review?(padenot)
(Assignee)

Comment 7

a year ago
Created attachment 8810517 [details] [diff] [review]
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.
Attachment #8810517 - Flags: review?(achronop)
(Assignee)

Comment 8

a year ago
Created attachment 8810518 [details] [diff] [review]
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.
Attachment #8810518 - Flags: review?(achronop)
Attachment #8810509 - Flags: review?(snorp) → review+
Component: General → Audio/Video
Priority: -- → P2
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.
Attachment #8810517 - Flags: review?(achronop) → review+
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.
Attachment #8810508 - Flags: review?(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.
Attachment #8810505 - Flags: review?(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.
Flags: needinfo?(jyavenard)
Attachment #8810510 - Flags: review?(padenot)
Attachment #8810513 - Flags: review?(padenot)
Attachment #8810514 - Flags: review?(padenot)
Attachment #8810518 - Flags: review?(achronop)
(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.
Flags: needinfo?(jyavenard)
Duplicate of this bug: 1299246
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
Blocks: 1259098
Flags: needinfo?(padenot)
Flags: needinfo?(jyavenard)
I leave it to :padenot to answer, I'm not sure I can contribute to this discussion
Flags: needinfo?(jyavenard)
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.
Flags: needinfo?(padenot) → needinfo?(snorp)
(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...
Flags: needinfo?(snorp) → needinfo?(esawin)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
(Assignee)

Comment 20

2 months ago
(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?
Flags: needinfo?(esawin) → needinfo?(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).
Flags: needinfo?(padenot)
(Assignee)

Updated

2 months ago
Keywords: leave-open
(Assignee)

Comment 22

2 months ago
Created attachment 8914355 [details] [diff] [review]
0003-Bug-1316934-3.1-Add-JNI-for-AudioManager-properties..patch

Rebased.
Attachment #8810509 - Attachment is obsolete: true
Attachment #8914355 - Flags: review+

Comment 23

2 months ago
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/507feca45d0b
[3.1] Add JNI for AudioManager properties. r=snorp

Comment 24

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/507feca45d0b
Blocks: 1352015
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.
Flags: needinfo?(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.
Flags: needinfo?(padenot) → needinfo?(achronop)
In fact, I think that (modulo bitrot), Eugen has provided all the pieces here.
Any update here?
Flags: needinfo?(padenot)
Alex, any update ?
Flags: needinfo?(padenot)
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.
Flags: needinfo?(achronop)
Priority: P2 → P1
You need to log in before you can comment on or make changes to this bug.