Closed Bug 1059700 Opened 5 years ago Closed 5 years ago

AudioOffloadPlayback: How to release resources properly during pause?

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
blocking-b2g 2.0+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: vasanth, Assigned: vasanth)

References

Details

(Whiteboard: [caf priority: p2][CR 728037])

Attachments

(1 file, 2 obsolete files)

[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
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)
(clearing nom for now as there may be another way)
blocking-b2g: 2.0? → ---
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)
(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]
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)
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)
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.0?
Whiteboard: [POVB]
Whiteboard: [CR 728037]
Whiteboard: [CR 728037] → [caf priority: p2][CR 728037]
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.
Hi Bhavana - please see nomination request from codeaurora
Flags: needinfo?(bbajaj)
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)
Assignee: nobody → vasanth
blocking-b2g: 2.0? → 2.0+
Flags: needinfo?(bbajaj)
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 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+
Thanks Robert & Bruce. Addressed both the comments.
Will start try run
Attachment #8494365 - Flags: review+
Attachment #8493733 - Attachment is obsolete: true
(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
https://hg.mozilla.org/mozilla-central/rev/e2c24d2b6281
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Please nominate this for aurora and b2g32 approval when you get a chance.
Flags: needinfo?(vasanth)
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)
Attachment #8494365 - Flags: approval-mozilla-b2g32?
Attachment #8494365 - Flags: approval-mozilla-b2g32+
Attachment #8494365 - Flags: approval-mozilla-aurora?
Attachment #8494365 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.