Closed
Bug 1226730
Opened 9 years ago
Closed 9 years ago
Audio stutters when Fennec in background on Android M
Categories
(Firefox for Android Graveyard :: Audio/Video, defect, P1)
Tracking
(firefox48 fixed, fennec48+)
RESOLVED
FIXED
Firefox 48
People
(Reporter: esawin, Assigned: esawin)
References
Details
(Keywords: compat)
Attachments
(5 files, 5 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
padenot
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
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+
Assignee | ||
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
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
Comment 7•9 years ago
|
||
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.
I wonder if bug 1249579 helps this?
See if you can repro with the patch[es] from bug 1249579
Flags: needinfo?(esawin)
tracking-fennec: + → 48+
Updated•9 years ago
|
Priority: -- → P1
Comment 11•9 years ago
|
||
(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.
Comment 12•9 years ago
|
||
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.
Assignee | ||
Comment 13•9 years ago
|
||
(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)
Assignee | ||
Comment 14•9 years ago
|
||
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.
Assignee | ||
Comment 15•9 years ago
|
||
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)
Assignee | ||
Comment 16•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
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)
Comment 18•9 years ago
|
||
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 19•9 years ago
|
||
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 20•9 years ago
|
||
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 21•9 years ago
|
||
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)
Comment 22•9 years ago
|
||
(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.
Comment 23•9 years ago
|
||
(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.
Assignee | ||
Comment 24•9 years ago
|
||
Use opensl_get_min_latency to override fast track latency.
Attachment #8739018 -
Attachment is obsolete: true
Attachment #8739984 -
Flags: review?(kinetik)
Comment hidden (obsolete) |
Assignee | ||
Comment 26•9 years ago
|
||
Attachment #8739985 -
Attachment is obsolete: true
Attachment #8739985 -
Flags: review?(kinetik)
Attachment #8740094 -
Flags: review?(kinetik)
Updated•9 years ago
|
Attachment #8739984 -
Flags: review?(kinetik) → review+
Updated•9 years ago
|
Attachment #8740094 -
Flags: review?(kinetik) → review+
Comment 27•9 years ago
|
||
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)
Comment 28•9 years ago
|
||
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`.
Assignee | ||
Comment 29•9 years ago
|
||
Addressed comments.
Attachment #8739019 -
Attachment is obsolete: true
Attachment #8740472 -
Flags: review?(padenot)
Assignee | ||
Comment 30•9 years ago
|
||
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 31•9 years ago
|
||
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+
Updated•9 years ago
|
Flags: needinfo?(kinetik)
Comment 32•9 years ago
|
||
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+
Assignee | ||
Comment 33•9 years ago
|
||
Merged patch 2 and 5.
Attachment #8739017 -
Attachment is obsolete: true
Attachment #8740094 -
Attachment is obsolete: true
Attachment #8740938 -
Flags: review+
Assignee | ||
Comment 34•9 years ago
|
||
Pull request for patches 2-4: https://github.com/kinetiknz/cubeb/pull/97
Comment 35•9 years ago
|
||
Comment 36•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Comment 37•8 years ago
|
||
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.
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•