Offload player not releasing resources when paused

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhargavg1, Assigned: vasanth)

Tracking

unspecified
mozilla31
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [CR 637866], )

Attachments

(2 attachments, 1 obsolete attachment)

With offload player for audio decode we acquire a session for DSP and when user does a pause the resources aren't released preventing the system going into low power modes.
Status: NEW → ASSIGNED
blocking-b2g: --- → 1.4?
Component: Gaia::Music → Video/Audio
Product: Firefox OS → Core
Whiteboard: [CR 637866]
Assignee: nobody → vasanth
What's the user impact here?
Flags: needinfo?(bhargavg1)
(In reply to Preeti Raghunath(:Preeti) from comment #1)
> What's the user impact here?

Would cause battery to drain
Flags: needinfo?(bhargavg1)
We can free the offloaded audio track from AudioOffloadPlayer.cpp in gecko if its paused for more than 60 seconds similar to how AwesomePlayer releases the offloaded AudioPlayer [1]
Will upload the fix in few days.

[1] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/av/tree/media/libstagefright/AwesomePlayer.cpp?h=kk_3.5#n1336
Andrew,

Please reassign
Flags: needinfo?(overholt)
No need to reassign, Vasanth is on it.
blocking-b2g: 1.4? → 1.4+
Flags: needinfo?(overholt)
Thank you, Michael. May I know if there has any update for this bug? Thanks!
(In reply to Kevin Hu [:khu] from comment #6)
> Thank you, Michael. May I know if there has any update for this bug? Thanks!

Hi Kevin, I'm working on it. Will upload a patch tomorrow. Thanks.
This change frees audio offload player resources if audio is paused for more than 60 seconds. It also handles if there is a play() after 60 secs (resources are freed)
It has changes in AudioOffloadPlayer and MediaOmxDecoder. 
Paul & Robert, could you guys please review this?
Attachment #8408942 - Flags: review?(roc)
Attachment #8408942 - Flags: review?(paul)
Comment on attachment 8408942 [details] [diff] [review]
Bug994881-V1.4-Release-audio-offload-player-resources.patch

Review of attachment 8408942 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/omx/AudioOffloadPlayer.h
@@ +71,5 @@
>    };
>  
> +  // Set when audio source is started and audioSink is initialized
> +  // Used only in main thread
> +  bool mStarted;

Instead of exposing this, just make Reset() check mStarted and return early if it's not started.

@@ +124,5 @@
>    // Used in main thread and offload callback thread, protected by Mutex
>    // mLock
>    bool mSeekDuringPause;
>  
> +  bool mPostSeekUpdate;

Please document this.

I don't think "post seek update" is a good name for this. Maybe we should call it "mDispatchSeekEvents"?

@@ +215,5 @@
>    // When decoding and playing happens separately, if there is a seek during
>    // pause, we can decode and keep data ready.
>    // In case of offload player, no way to seek during pause. So just fake that
>    // seek is done.
> +  status_t SeekTo(int64_t aTimeUs, bool aPostSeekUpdate = false);

Document this parameter
Attachment #8408942 - Flags: review?(roc) → review+
Attachment #8408942 - Attachment is obsolete: true
Attachment #8408942 - Flags: review?(paul)
Attachment #8409619 - Flags: review+
Addressed the nits. Thanks Robert!
Could one of you give "Try" run? If it's success we can land them in V1.4 and master.
Flags: needinfo?(roc)
Flags: needinfo?(paul)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> https://tbpl.mozilla.org/?tree=Try&rev=7d03c52a82c2

Thanks Robert. From try, run, starred 4 errors and other 1 looks completely unrelated. Adding checkin-needed
Flags: needinfo?(paul)
Whiteboard: [CR 637866] → [CR 637866][checkin-needed]
https://hg.mozilla.org/integration/b2g-inbound/rev/b7457024c448
Whiteboard: [CR 637866][checkin-needed] → [CR 637866]
https://hg.mozilla.org/mozilla-central/rev/b7457024c448
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Not enough information with current STR to write test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(rmitchell)
(In reply to RJ Mitchell from comment #18)
> Not enough information with current STR to write test case to address bug.

1. In MSM platforms (Ex: Flame, QRD8x10) play any .mp3 music file.
2. It would result in offload playback (From logcat, AuidoFlinger will show compress-offload-playback logs)
3. Pause the playback.
4. After 60 seconds, we should see AuidoFlinger compress-offload-playback *exit* logs from logcat, which means it has released adsp resources. Without that device won't go to lowest power mode.
5. Once user clicks play again, offload playback should resume from where it stopped. (step #2)

Let me know if you need more info.
Test case added in moztrap:

https://moztrap.mozilla.org/manage/case/14343/
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(rmitchell)
Flags: in-moztrap+
You need to log in before you can comment on or make changes to this bug.