Closed
Bug 1059700
Opened 10 years ago
Closed 10 years ago
AudioOffloadPlayback: How to release resources properly during pause?
Categories
(Core :: Audio/Video, defect)
Tracking
()
People
(Reporter: vasanth, Assigned: vasanth)
References
Details
(Whiteboard: [caf priority: p2][CR 728037])
Attachments
(1 file, 2 obsolete files)
6.22 KB,
patch
|
vasanth
:
review+
bajaj
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-b2g32+
|
Details | Diff | Splinter Review |
[Blocking Requested - why for this release]: Customer requested Bug 994881 is not fixed completely. That fix uses nsITimer to trigger reset after 60 seconds of pause. Timer is not triggered when device is in suspend. (display off, usb disconnected) Analysis --------- 1. We can add wakelock before starting reset timer to fix this from FFOS AudioOffloadPlayer. 2. We release wakelock after AudioTrack’s destructor. 3. AudioFlinger gets wakelock before #2 (which doesn’t work in FFOS since power manager service is not available) 4. Async to AudioTrack’s destructor, AudioFlinger calls audio hal functions to destroy offload thread and releases wakelock 5. Because #3 not working, even with #1 and #2, this issue happens rarely. Need suggestions from Mozilla what is the best way to overcome #3?
Adding ni on few folks who might know the answer
Flags: needinfo?(roc)
Flags: needinfo?(paul)
(In reply to vasanth from comment #0) > Bug 994881 is not fixed completely. > That fix uses nsITimer to trigger reset after 60 seconds of pause. > Timer is not triggered when device is in suspend. (display off, usb > disconnected) > > Analysis > --------- > 1. We can add wakelock before starting reset timer to fix this from FFOS > AudioOffloadPlayer. > 2. We release wakelock after AudioTrack’s destructor. Do you mean that you wrote a patch to do these things? > 3. AudioFlinger gets wakelock before #2 (which doesn’t work in FFOS since > power manager service is not available) I don't understand exactly what goes wrong in this step.
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2) > (In reply to vasanth from comment #0) > > Bug 994881 is not fixed completely. > > That fix uses nsITimer to trigger reset after 60 seconds of pause. > > Timer is not triggered when device is in suspend. (display off, usb > > disconnected) > > > > Analysis > > --------- > > 1. We can add wakelock before starting reset timer to fix this from FFOS > > AudioOffloadPlayer. > > 2. We release wakelock after AudioTrack’s destructor. > > Do you mean that you wrote a patch to do these things? > A patch was tried with this change but didnt fix the issue completely. Though a wakelock is acquired since the release operation is async within the AudioTrack's destructor we end up in holding the resources
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2) > > 3. AudioFlinger gets wakelock before #2 (which doesn’t work in FFOS since > > power manager service is not available) > > I don't understand exactly what goes wrong in this step. In FFOS, AudioFlinger is unable to get wake lock. In Android, it gets wakelock using android power manager service [1]. In FFOS, that service is not available. During audio playback we usually see below logs which affects this case. W AudioFlinger: Thread AudioOut_D cannot connect to the power manager service E AudioFlinger: no wake lock to update! [1] https://android.googlesource.com/platform/frameworks/base/+/android-4.3_r2/services/java/com/android/server/power/PowerManagerService.java
Comment 5•10 years ago
|
||
Then this is not really an issue I'll be able to help on, we need to bring on people who know about this stuff. Kan-Ru, do you know what our options here are? The AudioFlinger thread can't get a wake lock, because it can't contact the power manager service, apparently. I'm not sure if you're the right person for this, but your name did come up when blaming the code, feel free to redirect.
Flags: needinfo?(paul) → needinfo?(kchen)
Comment 7•10 years ago
|
||
I think AudioFlinger is using the wake lock via libhardware_legacy directly that doesn't depend on PowerManagerService. It is the same way as how gecko are using the wake lock. (In reply to vasanth from comment #4) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2) > > > 3. AudioFlinger gets wakelock before #2 (which doesn’t work in FFOS since > > > power manager service is not available) > > > > I don't understand exactly what goes wrong in this step. > > In FFOS, AudioFlinger is unable to get wake lock. > In Android, it gets wakelock using android power manager service [1]. > In FFOS, that service is not available. > During audio playback we usually see below logs which affects this case. > > W AudioFlinger: Thread AudioOut_D cannot connect to the power manager service > E AudioFlinger: no wake lock to update! Where is this log from? I see the audioflinger doesn't try to use power manager service https://android.googlesource.com/platform/frameworks/av/+/android-4.3_r2/services/audioflinger/AudioPolicyService.cpp#902
Flags: needinfo?(kchen)
Comment 8•10 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] from comment #7) > Where is this log from? > > I see the audioflinger doesn't try to use power manager service > > https://android.googlesource.com/platform/frameworks/av/+/android-4.3_r2/ > services/audioflinger/AudioPolicyService.cpp#902 It came from [1]. I checked SensorService which acquires WakeLock from libhardware_legacy not from PowerService. Not sure AudioFlinger can do it from libhardware_legacy too or not. [1] https://android.googlesource.com/platform/frameworks/av/+/android-4.3_r2/services/audioflinger/Threads.cpp#488
(In reply to Marco Chen [:mchen] from comment #8) > (In reply to Kan-Ru Chen [:kanru] from comment #7) > > Where is this log from? > > > > I see the audioflinger doesn't try to use power manager service > > > > https://android.googlesource.com/platform/frameworks/av/+/android-4.3_r2/ > > services/audioflinger/AudioPolicyService.cpp#902 > > It came from [1]. > I checked SensorService which acquires WakeLock from libhardware_legacy not > from PowerService. > Not sure AudioFlinger can do it from libhardware_legacy too or not. > > [1] > https://android.googlesource.com/platform/frameworks/av/+/android-4.3_r2/ > services/audioflinger/Threads.cpp#488 Yes, that's how we plan to do and no change would be required in gecko. Marking this as POVB
Whiteboard: [POVB]
Assignee | ||
Comment 10•10 years ago
|
||
After discussing with Bhargav, We still need this change in gecko so that device won't go to suspend between pause and reset. Issue in AudioFlinger is after we reset, it took little time to actually free underlying resources. During that time, AudioFlinger has to hold wakelock which we can take care outside gecko. Paul, Could you please review this change?
Attachment #8492963 -
Flags: review?(padenot)
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8492963 [details] [diff] [review] AudioOffloadPlayer-Use-wakelock.patch Hi Robert and Bruce, Seems Paul is in PTO. Anyone of you please review this?
Attachment #8492963 -
Flags: review?(roc)
Attachment #8492963 -
Flags: review?(brsun)
Assignee | ||
Comment 12•10 years ago
|
||
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.0?
Whiteboard: [POVB]
Updated•10 years ago
|
Blocks: CAF-v2.0-CC-metabug
Updated•10 years ago
|
Whiteboard: [CR 728037]
Updated•10 years ago
|
Whiteboard: [CR 728037] → [caf priority: p2][CR 728037]
Comment 13•10 years ago
|
||
Comment on attachment 8492963 [details] [diff] [review] AudioOffloadPlayer-Use-wakelock.patch Review of attachment 8492963 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/AudioOffloadPlayer.cpp @@ +245,5 @@ > mResetTimer->InitWithFuncCallback(ResetCallback, > this, > OFFLOAD_PAUSE_MAX_MSECS, > nsITimer::TYPE_ONE_SHOT); > + WakeLockCreate(); Shouldn't |WakeLockCreate()| be triggered before |AudioTrack::stop()| or |AudioTrack::pause()| is called? ::: content/media/omx/AudioOffloadPlayer.h @@ +198,5 @@ > nsCOMPtr<nsITimer> mResetTimer; > > + // To avoid device suspend when mResetTimer is going to be triggered. > + // Used only from main thread so no lock is needed. > + nsRefPtr<mozilla::dom::WakeLock> mWakeLock; It would be good to check and assert |NS_IsMainThread()| before accessing |mWakeLock| if it is only accessible from main thread by its design.
Comment 14•10 years ago
|
||
Hi Bhavana - please see nomination request from codeaurora
Flags: needinfo?(bbajaj)
Assignee | ||
Comment 15•10 years ago
|
||
Thanks Bruce! Addressed the comments. Could you please review again?
Attachment #8492963 -
Attachment is obsolete: true
Attachment #8492963 -
Flags: review?(roc)
Attachment #8492963 -
Flags: review?(padenot)
Attachment #8492963 -
Flags: review?(brsun)
Attachment #8493733 -
Flags: review?(brsun)
Updated•10 years ago
|
Assignee: nobody → vasanth
blocking-b2g: 2.0? → 2.0+
Flags: needinfo?(bbajaj)
Comment 16•10 years ago
|
||
Comment on attachment 8493733 [details] [diff] [review] AudioOffloadPlayer-Use-wakelock.patch Review of attachment 8493733 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/omx/AudioOffloadPlayer.cpp @@ +233,1 @@ > if (mStarted) { I guess |WakeLockCreate()| might need to be protected by |if (mStarted) {| just like |AudioTrack::stop()| and |AudioTrack::pause()|.
Comment 17•10 years ago
|
||
Comment on attachment 8493733 [details] [diff] [review] AudioOffloadPlayer-Use-wakelock.patch BTW, I would like to suggest a better reviewer than me.
Attachment #8493733 -
Flags: review?(brsun) → review?(roc)
Comment on attachment 8493733 [details] [diff] [review] AudioOffloadPlayer-Use-wakelock.patch Review of attachment 8493733 [details] [diff] [review]: ----------------------------------------------------------------- What Bruce said ::: content/media/omx/AudioOffloadPlayer.cpp @@ +329,5 @@ > mStarted = false; > mPlaying = false; > mStartPosUs = 0; > + > + WakeLockRelease(); Add an NS_IsMainThread assert to Reset()
Attachment #8493733 -
Flags: review?(roc) → review+
Assignee | ||
Comment 19•10 years ago
|
||
Thanks Robert & Bruce. Addressed both the comments. Will start try run
Attachment #8494365 -
Flags: review+
Attachment #8493733 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=1daa2d7d780b
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to vasanth from comment #20) > Try run: https://tbpl.mozilla.org/?tree=Try&rev=1daa2d7d780b Try run looks good. Starred the failures. Adding checkin-needed
Keywords: checkin-needed
Comment 22•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2c24d2b6281
Keywords: checkin-needed
Comment 23•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e2c24d2b6281
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 24•10 years ago
|
||
Please nominate this for aurora and b2g32 approval when you get a chance.
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → fixed
status-firefox33:
--- → wontfix
status-firefox34:
--- → affected
status-firefox35:
--- → fixed
Flags: needinfo?(vasanth)
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 8494365 [details] [diff] [review] AudioOffloadPlayer-Use-wakelock-v3.patch Approval Request Comment [Feature/regressing bug #]: Use wakelock to avoid device suspend between audio offload playback pause and reset. [User impact if declined]: Power numbers will be high when user paused audio. Device will go to suspend, deep sleep mode won't happen since underlying DSP resources aren't released. [Describe test coverage new/current, TBPL]: 1. Play offloaded audio clip (MP3 & >60s) 2. Pause the playback, remove USB connection and switch off screen by pressing power key 3. Measure Current and observe it is equal to suspend mode current. 4. After 60 seconds, measure again and observer it is equal to deep sleep mode current (XO shutdown) [Risks and why]: None [String/UUID change made/needed]: None
Attachment #8494365 -
Flags: approval-mozilla-b2g32?
Attachment #8494365 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(vasanth)
Updated•10 years ago
|
Attachment #8494365 -
Flags: approval-mozilla-b2g32?
Attachment #8494365 -
Flags: approval-mozilla-b2g32+
Attachment #8494365 -
Flags: approval-mozilla-aurora?
Attachment #8494365 -
Flags: approval-mozilla-aurora+
Comment 26•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/76584e88ee70 https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/a179806720e4
Comment 27•10 years ago
|
||
http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/a179806720e4
status-b2g-v2.0M:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•