Closed Bug 961986 Opened 6 years ago Closed 6 years ago

[fugu][buri] it took long time to resume music play after ending a phone call

Categories

(Firefox OS Graveyard :: Gaia::Music, defect, P1)

Other
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C3/1.4 S3(31jan)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: angelc04, Assigned: squib)

References

Details

(Keywords: perf, regression, Whiteboard: [caf priority: p1][CR 595623][c=progress p= s= u=1.3])

Attachments

(2 files)

Reproduce steps:
--------------------------------------------------
1. Launch Music and play a music
2. Launch Dialer, and dial some number
3. Music paused when call connection is estanblished
4. End the call 
   --> Music takes over 10s to resume. We should resume music play immediately after ending phone call.

Test Build: Buri v1.3
--------------------------------------------------
Gaia 50d6487d4d15efda942c934570e6fdfb91f6fe2e
Gecko http://hg.mozilla.org/releases/mozilla-aurora/rev/2e9d1f109655
BuildID 20140119004001
Version 28.0a2
Component: General → Gaia::Music
Keywords: perf
This is a regression from v1.1. V1.2 can also reproduce this bug.
blocking-b2g: --- → 1.3?
QA Contact: gbennett
10s to resume play is awfully long. Regression and Perf issue, marking this a blocker

Jim, could you please take a look at this. Also NI'ing Dominic
Assignee: nobody → squibblyflabbetydoo
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(dkuo)
Does this happen with other apps that play audio, or just music?
.:Last Working Build:.
Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20131106040203
Gaia: 3b5fe429f2414f2a9d7241b311b399033bb10612
Gecko: 9ba3faa35c96
Version: 28.0a1
Firmware Version: v1.2-device.cfg

.:First Broken Build:.
Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20131107040200
Gaia: 42bbe26a72e030faf07a6fc297f61a3a8ccda25b
Gecko: 70de5e24d79b
Version: 28.0a1
Firmware Version: v1.2-device.cfg

I tried the first and last Buri 1.2 builds using multiple devices, SIMs, SD cards, and songs without being able to repro.
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Whiteboard: [c=progress p= s= u=1.3]
Priority: -- → P1
If there's anyone who can help find the problem here, I encourage you to look into it as well. I've had very little luck finding out the problem, but the audio channel code is still unfamiliar to me, so I'm mostly fumbling around in the dark.

With any luck, I'll get some emulator builds running tomorrow and can actually reproduce this on a desktop.
Jim, can you enable the process priority debug by uncommenting the following line and then running through the test case?

  http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ProcessPriorityManager.cpp#46

Does it take the full 10 seconds for us to restore music to a high priority?

You could also try backing out the fix from bug 957346 to see if that helps.  I introduced an additional delay before downgrading the priority of an app using audio in order to allow music to change tracks while in the background.  Maybe this is keeping the dialer at an elevated priority too long.

This could also be related to bug 952893 where we are writing silent samples to the audio channel.  That bug is about preventing sleep, but it could also be keeping the dialer app at an elevated priority preventing/slowing music from starting again.

Just some ideas...
Flags: needinfo?(squibblyflabbetydoo)
Also, another delayed-music-playing problem for a different app was duped to bug 952893.  See bug 963220.
Ok, I finally managed to reproduce this (on an Inari). It doesn't appear to be a time-related thing at all, in fact. The issue seems to be that, after hanging up (or being hung up on), the music will remain paused until you hide the dialer app. This naturally means that when you *receive* a call, you're not going to have this issue; only when dialing out.

I'm not sure how/if this changes the importance of this bug, but this will hopefully narrow down the cause a bit...
Flags: needinfo?(squibblyflabbetydoo)
This does sound like bug 952893 then.  Dupe?
It's possible. Based on the regression range (early November 2013), I'm not so sure, though.

My current guess is that this was caused by bug 927852. It fits the time frame for the regression, and the comments in the code mention trashing the audio channel when hiding the dialer, which fits with the behavior I'm seeing. I'm going to try backing that out and seeing what happens.
Ok, this is almost certainly caused by bug 927852. I didn't see any improvement after applying bug 952893, but commenting out all the function bodies in tone_player.js (where most of the changes in bug 927852 occurred) fixes the issue.

Also, it seems I was wrong about this not being a timing thing; it does work if you just wait around at the dialer screen, but it seems to take longer if you do that. Going to the homescreen seems to speed up matters (maybe by destroying the AudioContext).

Now, I still have no idea *why* this is happening, but one step at a time...
Blocks: 927852
This fixes the issue for me. Details on the PR.
Attachment #8367763 - Flags: review?(etienne)
Comment on attachment 8367763 [details] [review]
Reduce the priority of TonePlayer's audio channel when hanging up

Looks like we could use |TonePlayer.setChannel('normal')| directly.

We should also open and reference a gecko bug: since the call screen window is closed it shouldn't keep the channel priority for some time.
Attachment #8367763 - Flags: review?(etienne)
(In reply to Etienne Segonzac (:etienne) from comment #14)
> Comment on attachment 8367763 [details] [review]
> Reduce the priority of TonePlayer's audio channel when hanging up
> 
> Looks like we could use |TonePlayer.setChannel('normal')| directly.

My only concern with doing that is that it wouldn't reset KeypadManager._oncall to false. Maybe that happens somewhere else though...

> We should also open and reference a gecko bug: since the call screen window
> is closed it shouldn't keep the channel priority for some time.

I'm not 100% sure this is a Gecko issue. If we explicitly set the channel priority to "telephony" and forget to reset it to "normal", how would Gecko know that we really want the channel priority to be "normal"?
NI etienne back for comment 15
Flags: needinfo?(etienne)
Comment on attachment 8367763 [details] [review]
Reduce the priority of TonePlayer's audio channel when hanging up

I updated the PR with an alternate strategy. Let me know what you think.
Attachment #8367763 - Flags: review?(etienne)
Comment on attachment 8367763 [details] [review]
Reduce the priority of TonePlayer's audio channel when hanging up

All good!

Don't know what time it is for you, but if you want to sign off I can merge it once travis is green :)
Attachment #8367763 - Flags: review?(etienne) → review+
Flags: needinfo?(etienne)
(In reply to Etienne Segonzac (:etienne) from comment #18)
> Comment on attachment 8367763 [details] [review]
> Reduce the priority of TonePlayer's audio channel when hanging up
> 
> All good!
> 
> Don't know what time it is for you, but if you want to sign off I can merge
> it once travis is green :)

That'd be great, thanks! (It's 1:35 AM here now...)
https://github.com/mozilla-b2g/gaia/commit/8e239117769820a5bcbdf1d5a738a9fc926954d2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 8e239117769820a5bcbdf1d5a738a9fc926954d2
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(squibblyflabbetydoo)
This either needs bug 922006 uplifted or a new patch written specifically for 1.3. Anyone have a preference?
Flags: needinfo?(squibblyflabbetydoo)
Both are fine, the easier the better :)
Duplicate of this bug: 963220
Whiteboard: [c=progress p= s= u=1.3] → [c=progress p= s= u=1.3][CR 595623]
Attached file Patch for 1.3 branch
Here's a version for 1.3. It works for me, but I'm asking for review just to be safe. If someone else wants to r+ this so it lands faster, that's ok by me too!
Attachment #8370250 - Flags: review?(etienne)
Whiteboard: [c=progress p= s= u=1.3][CR 595623] → [c=progress p= s= u=1.3]
Comment on attachment 8370250 [details] [review]
Patch for 1.3 branch

Yep, that's the easiest/safest way to uplift it.
r=me!
Attachment #8370250 - Flags: review?(etienne) → review+
Duplicate of this bug: 968639
Duplicate of this bug: 963220
Flags: needinfo?(dkuo)
Whiteboard: [c=progress p= s= u=1.3] → [CR 595623][c=progress p= s= u=1.3]
Whiteboard: [CR 595623][c=progress p= s= u=1.3] → [caf priority: p1][CR 595623][c=progress p= s= u=1.3]
You need to log in before you can comment on or make changes to this bug.