Closed Bug 1226730 Opened 4 years ago Closed 4 years ago

Audio stutters when Fennec in background on Android M

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed
fennec 48+ ---

People

(Reporter: esawin, Assigned: esawin)

References

(Blocks 1 open bug)

Details

(Keywords: compat)

Attachments

(5 files, 5 obsolete files)

3.73 KB, text/plain
Details
849 bytes, patch
snorp
: review+
Details | Diff | Splinter Review
852 bytes, patch
kinetik
: review+
Details | Diff | Splinter Review
1.07 KB, patch
padenot
: review+
Details | Diff | Splinter Review
3.77 KB, patch
esawin
: review+
Details | Diff | Splinter Review
STR
1. Open any audio or video stream (e.g. https://m.soundcloud.com/lugo-music-3/miles-davis-the-jazz-giants)
2. Start playback
3. Background Fennec by pushing on the Android home button

Expected: Sound playback continues.

Actual: Sound consistently stutters during playback.
Attached file Logcat
It looks like audio decoding is getting throttled in [1] once Fennec is moved to background.

This is not reproducing on Chrome.

[1] http://androidxref.com/6.0.0_r1/xref/frameworks/av/services/audioflinger/Threads.cpp#2976
Assignee: nobody → esawin
According to [1], we're supposed to pass the sample rate here, not the bit depth.

However, this does not fix the stuttering.

[1] http://developer.android.com/reference/android/media/MediaFormat.html#createAudioFormat%28java.lang.String,%20int,%20int%29
Attachment #8690238 - Flags: review?(snorp)
Attachment #8690238 - Flags: review?(snorp) → review+
This is not reproducing on Android M on Nexus 6P. Here I get the following log message instead when backgrounding

12-01 16:17:31.861  3810 14787 I AudioFlinger: AudioFlinger's thread 0xf12c0000 ready to run

and the audio plays stutter-free.
It's reproducing on Nexus 5 and 7.
tracking-fennec: --- → +
(In reply to Eugen Sawin [:esawin] from comment #4)
> It's reproducing on Nexus 5 and 7.

And 5X
I have this problem on a Nexus 7 2013, Android 6.0.1. I'm using standard Firefox for Android which is currently up to date.

If I open a song on Youtube and move Firefox to the background before the video starts to play the audio will play as expected, otherwise it stutters bad. That is, the audio plays as expected until the device goes to sleep at which point the same stutter returns.
Duplicate of this bug: 1254112
See if you can repro with the patch[es] from bug 1249579
Flags: needinfo?(esawin)
tracking-fennec: + → 48+
Priority: -- → P1
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10)
> See if you can repro with the patch[es] from bug 1249579
Bug 1249579 is for audio focus, not for audio quality/performance improvement. Therefore it will not be helpful to this bug.
IMO, we should focus on music(audio only) case in this bug. 
AFAIK, Android apps including Chrome that are playing videos will be stopped if they are pushed to background. However, Firefox will not (tested it on my HTC M9+ with Android 5.0.2 and Firefox 45.0.1). So, we may need to check which UX we want. If we still want to keep playing it, then we need to stop video decoding (user cannot see it)and keep audio decoding as desktop browsers plan to have. Otherwise, CPU may not have enough resource to play audio smoothly.
Keywords: compat
No longer blocks: 1258259
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10)
> See if you can repro with the patch[es] from bug 1249579

Acquiring audio focus does not seem to prevent the AudioFlinger throttling.
Flags: needinfo?(esawin)
AudioFlinger provides normal and fast mixer threads. The non-blocking fast mixer realizes ~10ms latency at elevated scheduling priority. The fast mixer accepts only tracks encoded in the device's native sample rate.

When decoding samples with non-native rates, the normal mixer thread is used. Once Fennec is backgrounded, the normal mixer thread may be throttled, depending on the device's capability to run it in a low-power mode.

Tested on Nexus 7, which has a native sample rate of 48kHz, 48kHz playback runs jitter-free in background, while 44.1kHz playback produces the reported jitter.

We could try working around this issue by resampling the stream to the device's native rate before feeding it to AudioFlinger.
To avoid audio decoder throttling on the normal mixer thread, we need to move decoding to the fast mixer thread. For this we need to resample the audio to the native sample rate before passing it to AudioFlinger.

With this patch, we force the fallback to native sampling rate on Android 6+.

This puts the decoding on the fast track which fixes the audio jitter when Fennec is in background.

However, this introduces even worse audio jitter which accumulates over playback time even when Fennec is in foreground (buffering issue).

This also distorts the playback since the resampling is applied into the wrong direction (44.1kHz to 40.5kHz instead of 48kHz).
Attachment #8739017 - Flags: review?(kinetik)
With this patch, we override the latency from the default 100ms to advertised 10ms of AudioFlinger's fast track.

This fixes the accumulating jitter introduced with patch 2.
Attachment #8739018 - Flags: review?(kinetik)
With this patch, we fix the wrongly inverted resampling direction.

This fixes the playback rate issue introduced with patch 2.
Attachment #8739019 - Flags: review?(kinetik)
I'll review these here, but please submit them to the upstream repo at https://github.com/kinetiknz/cubeb and then import them using media/libcubeb/update.sh when landing the changes.
Comment on attachment 8739019 [details] [diff] [review]
0004-Bug-1226730-4.1-Fix-inverted-one-way-resampling-dire.patch

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

This looks right, but I'm confused because Paul made this change earlier and then reverted it:

In the upstream git repo:
144e8661 made this change
4524a677 reverted it

Paul, can you please clarify?
Attachment #8739019 - Flags: review?(kinetik) → review?(padenot)
Comment on attachment 8739017 [details] [diff] [review]
0002-Bug-1226730-2.1-Force-resampling-to-native-device-sa.patch

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

r- because of the __system_property_get issue, but the rest looks fine.

::: media/libcubeb/src/cubeb_opensl.c
@@ +209,4 @@
>  {
>    cubeb * ctx;
>  
> +#if defined(__ANDROID__)

I don't think we can do this - c7f87382 upstream added the GINGERBREAD_MR1 check because __system_property_get isn't available on some later NDKs.
Attachment #8739017 - Flags: review?(kinetik) → review-
Comment on attachment 8739018 [details] [diff] [review]
0003-Bug-1226730-3.1-Override-latency-for-fast-track-Audi.patch

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

::: media/libcubeb/src/cubeb_opensl.c
@@ +566,5 @@
>      // Reset preferred samping rate to trigger fallback to native sampling rate.
>      preferred_sampling_rate = 0;
> +    // AudioFlinger's fast track latency is advertised as 10ms.
> +    latency = 10;
> +    stm->latency = latency;

Can you explain why we need to do this? Does using a larger latency throw us off the fast path?

If we have to do this, can we get the latency via opensl_get_min_latency rather than hard coding a value?
Attachment #8739018 - Flags: review?(kinetik)
(In reply to Matthew Gregan [:kinetik] from comment #19)
> Comment on attachment 8739019 [details] [diff] [review]
> 0004-Bug-1226730-4.1-Fix-inverted-one-way-resampling-dire.patch
> 
> Review of attachment 8739019 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks right, but I'm confused because Paul made this change earlier and
> then reverted it:
> 
> In the upstream git repo:
> 144e8661 made this change
> 4524a677 reverted it
> 
> Paul, can you please clarify?

Indeed this has changed and I did not put in any documentation, and forgot to update Android, sorry about that.

From the point of view of cubeb, the `target_rate` is the rate that the _client_ prefers. The source rate is the rate at which the buffer is provided by the system. This change was necessary because of input/duplex handling: the preferred rate of the input and output can be different, but the `target_rate` (i.e. the rate of the buffer presented in the callback) is always the same. See this use of the resampler: https://dxr.mozilla.org/mozilla-central/source/media/libcubeb/src/cubeb_wasapi.cpp?case=true&from=cubeb_wasapi.cpp#1534

It used to be that the target rate was the rate of the system (see https://hg.mozilla.org/mozilla-central/file/9b568fb8d447/media/libcubeb/src/cubeb_wasapi.cpp#l1116), because we were only resampling one direction.

Regarding the two opposite commits, I had it right in the first place, then I managed to confused myself very hard, put in a commit that swapped destination and source rates, while having the two also inverted somewhere else in the code, so it sounded alright. Only later by re-runnning more tests on Windows I found my error and put in another commit so that everything was set properly.

I'm writing documentations about this upstream so that it does not happen again.
(In reply to Matthew Gregan [:kinetik] from comment #21)
> Comment on attachment 8739018 [details] [diff] [review]
> 0003-Bug-1226730-3.1-Override-latency-for-fast-track-Audi.patch
> 
> Review of attachment 8739018 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libcubeb/src/cubeb_opensl.c
> @@ +566,5 @@
> >      // Reset preferred samping rate to trigger fallback to native sampling rate.
> >      preferred_sampling_rate = 0;
> > +    // AudioFlinger's fast track latency is advertised as 10ms.
> > +    latency = 10;
> > +    stm->latency = latency;
> 
> Can you explain why we need to do this? Does using a larger latency throw us
> off the fast path?
> 
> If we have to do this, can we get the latency via opensl_get_min_latency
> rather than hard coding a value?

If the goal is to use fast mixer, the heuristic that determines whether we'll get it or not is here:

http://androidxref.com/6.0.1_r10/xref/frameworks/av/services/audioflinger/Threads.cpp#1636

To summarize and at the AudioTrack level, we need have:
- A mono or stereo stream
- A sample-rate equal to the system sample rate (this is something you can get using cubeb_get_preferred_rate)
- A buffer size of 0 (that means the system picks for us) or larger than the preferred frame count of the HAL

That said, we're using OpenSL ES. Depending on the Android version, the value can be of importance or not: http://androidxref.com/6.0.1_r10/xref/frameworks/wilhelm/src/itf/IEngine.c#329. Using opensl_get_min_latency should give us a good value.
Use opensl_get_min_latency to override fast track latency.
Attachment #8739018 - Attachment is obsolete: true
Attachment #8739984 - Flags: review?(kinetik)
Attachment #8739985 - Attachment is obsolete: true
Attachment #8739985 - Flags: review?(kinetik)
Attachment #8740094 - Flags: review?(kinetik)
Attachment #8739984 - Flags: review?(kinetik) → review+
Attachment #8740094 - Flags: review?(kinetik) → review+
Comment on attachment 8739019 [details] [diff] [review]
0004-Bug-1226730-4.1-Fix-inverted-one-way-resampling-dire.patch

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

The change needs to happen in `cubeb_opensl.c`, something like this:

> cubeb_stream_params params = output_stream_params;
> params.rate = preferred_sampling_rate;
> 
> stm->resampler = cubeb_resampler_create(stm, NULL, params,
>                                         output_stream_params,
>                                         data_callback,
>                                         user_ptr,
>                                         CUBEB_RESAMPLER_QUALITY_DEFAULT);
Attachment #8739019 - Flags: review?(padenot)
Sorry that was meant to be:

> cubeb_stream_params params = output_stream_params;
> params.rate = preferred_sampling_rate;
> 
> stm->resampler = cubeb_resampler_create(stm, NULL, params,
>                                         output_stream_params.rate,
>                                         data_callback,
>                                         user_ptr,
>                                         CUBEB_RESAMPLER_QUALITY_DEFAULT);

note the `output_stream_params.rate`.
Addressed comments.
Attachment #8739019 - Attachment is obsolete: true
Attachment #8740472 - Flags: review?(padenot)
Matthew, do you want me to merge patch 2 and 5 or do you want to update the review for patch 2 as it is, given the fix in patch 5?
Flags: needinfo?(kinetik)
Comment on attachment 8739017 [details] [diff] [review]
0002-Bug-1226730-2.1-Force-resampling-to-native-device-sa.patch

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

r+ with patch 5, probably makes sense to merge these two patches
Attachment #8739017 - Flags: review- → review+
Flags: needinfo?(kinetik)
Comment on attachment 8740472 [details] [diff] [review]
0004-Bug-1226730-4.2-Fix-inverted-one-way-resampling-dire.patch

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

Thanks !
Attachment #8740472 - Flags: review?(padenot) → review+
Merged patch 2 and 5.
Attachment #8739017 - Attachment is obsolete: true
Attachment #8740094 - Attachment is obsolete: true
Attachment #8740938 - Flags: review+
Depends on: 1264594
https://hg.mozilla.org/mozilla-central/rev/4623410a2303
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Thanks a lot for fixing this issue - it was affecting me for quite some time.
Background playback is a really valueable feature of Fennec, I use it every day.
You need to log in before you can comment on or make changes to this bug.