Closed
Bug 1220378
Opened 9 years ago
Closed 6 years ago
AVAudioEngine for AuralProgressBar keeps running in background
Categories
(Firefox for iOS :: General, defect)
Tracking
()
RESOLVED
INVALID
Tracking | Status | |
---|---|---|
fxios | + | --- |
People
(Reporter: st3fan, Assigned: st3fan)
Details
Attachments
(1 file)
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•9 years ago
|
||
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•9 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 3•9 years ago
|
||
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+
Comment 4•9 years ago
|
||
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•9 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.
Comment 6•9 years ago
|
||
Oops that what I meant - remove AuralProgressBar, not this patch.
Assignee | ||
Comment 7•9 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•9 years ago
|
||
Moving this to 2.0 or a next bugfix release.
Updated•9 years ago
|
Attachment #8682064 -
Flags: review?(etoop) → review+
Updated•8 years ago
|
Flags: needinfo?(aaron.train)
Assignee | ||
Updated•8 years ago
|
Comment 9•8 years ago
|
||
Looks like this has 2 r+'s and a PR with a comment on in. Should we land this?
Flags: needinfo?(sarentz)
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(sarentz)
You need to log in
before you can comment on or make changes to this bug.
Description
•