AVAudioEngine for AuralProgressBar keeps running in background

NEW
Assigned to

Status

()

Firefox for iOS
General
--
major
2 years ago
a year ago

People

(Reporter: st3fan, Assigned: st3fan, NeedInfo)

Tracking

unspecified
Other
iOS

Firefox Tracking Flags

(fxios+)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
We have had multiple reports where Firefox is using a lot of background time. In the order of dozens of hours vs hours or minutes of Screen On use. This is causing battery drain.

I think I have tracked this down to the AuralProgressBar, and most likely to the recent change where we now use the audio background mode.

The use of the audio background mode is good, it gives us parity with Safari. You can start a podcast from a tab, lock your phone and keep listening.

The problem however is that the AuralProgressBar has an AudioEngine that, once started, keeps running in the background.

Due to some logic error, introduced with a recent patch to fix a crash, the AudioEngine, is always started when audio is resumed.

I have verified this with Instruments.app and the Energy Usage instrument. With the AuralProgressBar removed, the Audio CPU Activity stays at 0%. With the code enabled, and a audio suspend/resume triggered, the Audio CPU Activity will be a steady 0.8%, forever.

Steps to verify:

* Profile Firefox with the Energy Usage instrument
* Open a new tab, browse to a quiet site like news.ycombinator.com
* Look at the Audio CPU Activity, it will be 0%
* Long press the home button to trigger Siri. This causes audio to suspend/resume
* Cancel Siri, you are back in the application
* Look at the Audio CPU Activyty, it will be 0.8% (values may differ per device)

With the AuralProgressBar removed, the audio usage stays 0%

I have a decent idea how to fix this and will submit a fix.

Marking this as 1.2+, it is something we want to include as soon as possible.
(Assignee)

Comment 1

2 years ago
Created attachment 8682064 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/1213

This patch pulls the handling of the `AVAudioSessionInterruptionNotification` out of the `AuralProgressBar.UI` class and moves it into `AuralProgressBar`.

This lets us restart the audio engine only if there is still a load happening, when the progress is not `nil`.

This is not the best patch, but I tried to do a minimal change to keep the risk low. I think we should do some followup work to make the `AuralProgressBar` better integrated.
Attachment #8682064 - Flags: review?(etoop)
Attachment #8682064 - Flags: review?(bnicholson)
(Assignee)

Comment 2

2 years ago
Aaron, I tried my best to look at previous Audio regressions and test if those are still fixed after my changed. I think we are good, but maybe you can spend a little time on those too?
Flags: needinfo?(aaron.train)
Comment on attachment 8682064 [details] [review]
PR: https://github.com/mozilla/firefox-ios/pull/1213

Looks OK to me if it works. It seems like AVAudioEngine has been giving us a lot of trouble recently, so we might want to consider removing AuralProgressBar altogether if there are any more issues. It's a nice a11y feature, but it's not necessary and certainly not worth all the crashes/bugs it's been causing.
Attachment #8682064 - Flags: review?(bnicholson) → review+
Just throwing this out there but if we're not feeling 100% on the fix, could we remove it from the 1.2 build and then add a fix for 2.0 to give it additional 'bake' time?
(Assignee)

Comment 5

2 years ago
(In reply to Stephan Leroux [:sleroux] from comment #4)
> Just throwing this out there but if we're not feeling 100% on the fix, could
> we remove it from the 1.2 build and then add a fix for 2.0 to give it
> additional 'bake' time?

As an alternative, I would consider disabling AuralProgressBar completely until we have a better implementation.
Oops that what I meant - remove AuralProgressBar, not this patch.
(Assignee)

Comment 7

2 years ago
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1220842 lets decide during triage if we want to move this bug to 2.0
(Assignee)

Comment 8

2 years ago
Moving this to 2.0 or a next bugfix release.
tracking-fxios: 1.2+ → 2.0+
Attachment #8682064 - Flags: review?(etoop) → review+

Updated

2 years ago
Flags: needinfo?(aaron.train)
(Assignee)

Updated

2 years ago
tracking-fxios: 2.0+ → +
Looks like this has 2 r+'s and a PR with a comment on in. Should we land this?
Flags: needinfo?(sarentz)
You need to log in before you can comment on or make changes to this bug.