Open fm,open music,answer a incoming call and hand up,fm will play for a few seconds

RESOLVED DUPLICATE of bug 1113086

Status

--
major
RESOLVED DUPLICATE of bug 1113086
4 years ago
4 years ago

People

(Reporter: jingmei.zhang, Assigned: justindarc)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.0 affected, b2g-v2.0M affected, b2g-v2.1 affected, b2g-v2.2 affected)

Details

(Whiteboard: [priority])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
[Blocking Requested - why for this release]:

1.Play FM.
2.Open Music.
3.Play songs.
4.Press home key
5.Recieve a in coming call.
6.FM start for a few seconds when disconnect the call.
(Reporter)

Comment 1

4 years ago
it is very strange that we can reproduce the issue in my locale but can not reproduce in flame.

The stack we the fm is started are as follows:

#0  android::AudioSystem::setDeviceConnectionState (device=device@entry=524288, state=state@entry=AUDIO_POLICY_DEVICE_STATE_AVAILABLE, device_address=0xb5df50f7 "") at frameworks/av/media/libmedia/AudioSystem.cpp:566
#1  0xb5441c00 in mozilla::dom::gonk::AudioManager::SetFmRadioAudioEnabled (this=0xb23aa8d0, aFmRadioAudioEnabled=<optimized out>) at ../../../../gecko/dom/system/gonk/AudioManager.cpp:607
#2  0xb53fa112 in mozilla::dom::FMRadioService::EnableAudio (this=this@entry=0xa947f080, aAudioEnabled=aAudioEnabled@entry=true) at ../../../gecko/dom/fmradio/FMRadioService.cpp:307
#3  0xb53fa60e in mozilla::dom::FMRadioService::Notify (this=0xa947f080, aInfo=...) at ../../../gecko/dom/fmradio/FMRadioService.cpp:743
#4  0xb4f5d808 in Broadcast (aParam=..., this=0xa98876d0) at ../dist/include/mozilla/Observer.h:67
#5  mozilla::hal::NotifyFMRadioStatus (aFMRadioState=...) at ../../gecko/hal/Hal.cpp:1005
#6  0xb4f604cc in mozilla::hal_impl::EnableFMRadio (aInfo=...) at ../../gecko/hal/gonk/GonkFMRadio.cpp:316
#7  0xb4f5cf20 in mozilla::hal::EnableFMRadio (aInfo=...) at ../../gecko/hal/Hal.cpp:1011
#8  0xb53fa094 in mozilla::dom::EnableRunnable::Run (this=0xb27cf980) at ../../../gecko/dom/fmradio/FMRadioService.cpp:125
#9  0xb4d8323e in ProcessNextEvent (result=0xbeedf6ff, mayWait=<optimized out>, this=0xb6a024e0) at ../../../gecko/xpcom/threads/nsThread.cpp:694
#10 nsThread::ProcessNextEvent (this=0xb6a024e0, mayWait=<optimized out>, result=0xbeedf6ff) at ../../../gecko/xpcom/threads/nsThread.cpp:618
#11 0xb4d56bea in NS_ProcessNextEvent (thread=<optimized out>, mayWait=mayWait@entry=false) at ../../../gecko/xpcom/glue/nsThreadUtils.cpp:263
#12 0xb4eb95b0 in mozilla::ipc::MessagePump::Run (this=0xb6a01df0, aDelegate=0xb6a651a0) at ../../../gecko/ipc/glue/MessagePump.cpp:95
#13 0xb4eadf76 in MessageLoop::RunInternal (this=this@entry=0xb6a651a0) at ../../../gecko/ipc/chromium/src/base/message_loop.cc:226
#14 0xb4eae028 in RunHandler (this=0xb6a651a0) at ../../../gecko/ipc/chromium/src/base/message_loop.cc:219
#15 MessageLoop::Run (this=0xb6a651a0) at ../../../gecko/ipc/chromium/src/base/message_loop.cc:193
#16 0xb5309252 in nsBaseAppShell::Run (this=0xb6a3be80) at ../../../gecko/widget/xpwidgets/nsBaseAppShell.cpp:164
#17 0xb58fe00e in nsAppStartup::Run (this=0xb2765580) at ../../../../gecko/toolkit/components/startup/nsAppStartup.cpp:276
#18 0xb58da2dc in XREMain::XRE_mainRun (this=this@entry=0xbeedf864) at ../../../gecko/toolkit/xre/nsAppRunner.cpp:4010
#19 0xb58db5e6 in XREMain::XRE_main (this=this@entry=0xbeedf864, argc=argc@entry=1, argv=argv@entry=0xbeee1a14, aAppData=aAppData@entry=0x24918 <sAppData>) at ../../../gecko/toolkit/xre/nsAppRunner.cpp:4079
#20 0xb58db73c in XRE_main (argc=1, argv=0xbeee1a14, aAppData=0x24918 <sAppData>, aFlags=<optimized out>) at ../../../gecko/toolkit/xre/nsAppRunner.cpp:4291
#21 0x0000a278 in do_main (argv=0xbeee1a14, argc=1) at ../../../gecko/b2g/app/nsBrowserApp.cpp:163
#22 main (argc=<optimized out>, argv=<optimized out>) at ../../../gecko/b2g/app/nsBrowserApp.cpp:256

see from the stack,we did send a message to open the fm.
(Reporter)

Comment 2

4 years ago
We guess that the open fm message is send from gaia.
In gaia,we found such codes in fm.js:
>// If the radio was previously enabled or was in the process
>// of becoming enabled, re-enable the radio.
>  if ((!!window._previousFMRadioState || !!window._previousEnablingState)) {
>     // Ensure the antenna is still available before re-starting
>     // the radio.
>     if (mozFMRadio.antennaAvailable) {
>        enableFMRadio(frequencyDialer.getFrequency());
>      }
>
>      // Re-enable the speaker if it was previously forced.
>       speakerManager.forcespeaker = !!window._previousSpeakerForcedState;
>    }

as the code design,the fm will hold its previous state(play) when it is interrupted.
when we open music,the fm stops,and when the call comes in and hand up later,code goes to run 'enableFMRadio',and then we could hear fm for a while.

I tried to make a change as:
>     if (mozFMRadio.antennaAvailable && document.hidden == false) {
>        enableFMRadio(frequencyDialer.getFrequency());
>      }

the issue does not exist.

But we have the same codes in this part as flame,I could not understand why flame doesn't have such issue.

hi Dominic,can you help to check the issue??
Flags: needinfo?(dkuo)
I can reproduce this in 2.0 KK Flame
Gaia-Rev        f0f22bb46c881e02524b3991c2587ff8c0a9fc37
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/ab2a88c05a4b
Build ID        20140919050548
Version         34.0a2
Device Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20140919.083111
FW-Date         Fri Sep 19 08:31:21 EDT 2014
Bootloader      L1TC10011800

Comment 4

4 years ago
Dominic, can you take a look?
Note that I think we also have a hack where the fm radio listens for attention screen state change notifications from the system app via the settings db. Is the FM app restarting itself when it gets the notification that the attention screen has gone away?  If it has already stopped itself because of the music app, it probably should not resume when the attention screen goes away.  

I'm just guessing here, I haven't looked at the code or tried to reproduce the bug.
(Reporter)

Comment 6

4 years ago
(In reply to David Flanagan [:djf] from comment #5)
> Note that I think we also have a hack where the fm radio listens for
> attention screen state change notifications from the system app via the
> settings db. Is the FM app res
hi David:

  We can find such codes in fm.js:

>  navigator.mozSettings.addObserver(
>   'private.broadcast.attention_screen_opening',
>    function(event) {……})

 it will listen for attention screen state change.
(Reporter)

Comment 7

4 years ago
if we change the Sequence,play music and then fm,when we hand up,the fm will restart witout music play.
So,there might be something wrong in fm.
as comment5,I don't think we need a listener to listen for attention screen to decide if we should stop or resume the fm.audiochannel will help to handle it.

Remove the code in fm.js can eliminate the issue,but I am not sure if it could cause other issue.

>  navigator.mozSettings.addObserver(
>   'private.broadcast.attention_screen_opening',
>    function(event) {
>     ……
>    })
Adding qawanted to see if the team can reproduce this issue on Flame.
Keywords: qawanted
This issue is reproducible on Flame v180 base itself, Flame 2.0, Flame 2.1, Flame 2.2 Master, and Open C 2.2 Master.

Observed behavior: Following STR, at step 6 after hanging up the call, audio goes back to playing from Music app for about 2 seconds, then it switches to playing FM Radio and does NOT switch back. User has to manually open Music app and music automatically resumes from when it was interrupted by phone call.

Tested on:
Device: Flame 2.0
BuildID: 20140922084144
Gaia: 8d7f2ac85f3154bdb149d67e5c2f9b035f5e4105
Gecko: 13913a159f18
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame 2.1
BuildID: 20140922103818
Gaia: 2c5f245929d40ee7b0227ef39e47a0220171d17b
Gecko: dfa9c38fecb6
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master
BuildID: 20140922125023
Gaia: e1fd99454b6cd5da4f2c58f928fc04c6d03f478f
Gecko: f4037194394e
Version: 35.0a1 (2.2 Master)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Device: Open_C 2.2 Master
BuildID: 20140922040649
Gaia: 3802009e1ab6c3ddfc3eb15522e3140a96b33336
Gecko: 5e704397529b
Version: 35.0a1 (2.2 Master)
Firmware: P821A10v1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
QA Whiteboard: [QAnalyst-Triage?]
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
Flags: needinfo?(jmitchell)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)

Comment 10

4 years ago
[Blocking Requested - why for this release]:According to comment 9, we got this issue with 1.4, 2.0, 2.0M and 2.1

--
Keven
blocking-b2g: 1.4? → 2.1?

Comment 11

4 years ago
Also adding Pin Zhang/Tim Chien for their input on this issue.
Flags: needinfo?(timdream)
Flags: needinfo?(pzhang)
   (In reply to jingmei.zhang from comment #7)
> if we change the Sequence,play music and then fm,when we hand up,the fm will
> restart witout music play.
> So,there might be something wrong in fm.
> as comment5,I don't think we need a listener to listen for attention screen
> to decide if we should stop or resume the fm.audiochannel will help to
> handle it.
> 
> Remove the code in fm.js can eliminate the issue,but I am not sure if it
> could cause other issue.
> 
> >  navigator.mozSettings.addObserver(
> >   'private.broadcast.attention_screen_opening',
> >    function(event) {
> >     ……
> >    })

Hi jingmei, observing on the attention screen is introduced by bug 1037880, so i'm afraid we can't fix this bug by just removing the observer.
Flags: needinfo?(pzhang)
I think the right approach to fix both this bug and bug 1037880 in audio policy.

Hi, Randy, any thoughts?
Flags: needinfo?(rlin)
The bug 1037880 won't change the audio policy. I think this one need to find out why cause the delay of stopping fmradio and music application by audiochannel. 
Hi Star, any ideas?
Flags: needinfo?(rlin) → needinfo?(scheng)
(Reporter)

Comment 15

4 years ago
(In reply to Randy Lin [:rlin] from comment #14)
> The bug 1037880 won't change the audio policy. I think this one need to find
> out why cause the delay of stopping fmradio and music application by
> audiochannel. 

hi Randy,this imight not be a delay issue.we could hear fm because we enabled the fm in gaia fm.
Hi, Alive

Is this attention window issue? But I saw the bug 927862 is resolved. Could you help to comment it?
Flags: needinfo?(scheng) → needinfo?(alive)
Bug 927862 decrease the delay of put active app to background from 3000ms to 300ms (the animation time of callscreen).
We cannot improve anymore from gaia side because visibility is now tightly coupled to rendering so if we do not delay the background the user will see white screen for a while and this will become another bug.
The correct fix is https://bugzilla.mozilla.org/show_bug.cgi?id=1034001
A workaround is make *every app* observes the hack setting introduced by camera app.
Flags: needinfo?(alive)

Comment 18

4 years ago
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #17)
> Bug 927862 decrease the delay of put active app to background from 3000ms to
> 300ms (the animation time of callscreen).
> We cannot improve anymore from gaia side because visibility is now tightly
> coupled to rendering so if we do not delay the background the user will see
> white screen for a while and this will become another bug.
> The correct fix is https://bugzilla.mozilla.org/show_bug.cgi?id=1034001
> A workaround is make *every app* observes the hack setting introduced by
> camera app.


No workarounds/hacks please. We have been seeing this in the last few releases surfacing up in various apps. There was so much push in 2.0 to put the hack fix in just for camera as a workaround. Lets prioritize the correct fix for this in 2.2

Updated

4 years ago
Depends on: 1034001
(Assignee)

Updated

4 years ago
Assignee: nobody → jdarcangelo
Everyone agrees that the real fix for this bug depends on bug 1034001. But that bug won't land in 2.1, so we can't block on a real fix for this in 2.1.

It may still be worth investigating whether we can do something in the FM radio app.

In comment #7, Jingmei suggested removing the private.broadcast.attention_screen_opening hack. In comment #12 Pin says we can't do that without breaking another issue.

As the author of the private.broadcast.attention_screen_opening hack, I'll weigh in and say that that hack was created for the Camera app, so we could get it to stop recording before an incoming call started ringing, so that the ringtone would not be audible at the end of the video.

Separately, the FM radio app adopted the hack, but if I understand correctly, the FM radio app is using it for a slightly different reason. In this case, the issue isn't that the FM radio didn't stop in time. I think the issue is that the audio competing policy didn't apply to the FM radio. So it might be possible to remove the hack code from FM and fix the underlying bug in some other way.  

Or, maybe it is simpler than that. In this case, the radio app has already stopped playing when the call starts, right? In that case, it shouldn't do anything when the attention screen opens or when the attention screen closes.  It should just be a matter of setting a flag, or of registering the settings observer when we start playing and removing it when we stop playing, right? (I'm assuming that there is a way for the FM radio app to know when it has stopped playing because the music app starts.)
(In reply to David Flanagan [:djf] from comment #19)
> Separately, the FM radio app adopted the hack, but if I understand
> correctly, the FM radio app is using it for a slightly different reason. In
> this case, the issue isn't that the FM radio didn't stop in time. I think
> the issue is that the audio competing policy didn't apply to the FM radio.
> So it might be possible to remove the hack code from FM and fix the
> underlying bug in some other way.  
> 

I prefer removing the hack too, it's a quick dirty fix according to the comments[1]:

  // XXX We're abusing the settings API here to allow the system app
  // to broadcast a message to any certified apps that care. There
  // ought to be a better way, but this is a quick and easy way to
  // fix a last-minute release blocker.
  //

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/fm/js/fm.js#L848
(In reply to David Flanagan [:djf] from comment #19)
> Everyone agrees that the real fix for this bug depends on bug 1034001. But
> that bug won't land in 2.1, so we can't block on a real fix for this in 2.1.
> 
> It may still be worth investigating whether we can do something in the FM
> radio app.
> 
> In comment #7, Jingmei suggested removing the
> private.broadcast.attention_screen_opening hack. In comment #12 Pin says we
> can't do that without breaking another issue.
> 
> As the author of the private.broadcast.attention_screen_opening hack, I'll
> weigh in and say that that hack was created for the Camera app, so we could
> get it to stop recording before an incoming call started ringing, so that
> the ringtone would not be audible at the end of the video.
> 
> Separately, the FM radio app adopted the hack, but if I understand
> correctly, the FM radio app is using it for a slightly different reason. In
> this case, the issue isn't that the FM radio didn't stop in time. I think
> the issue is that the audio competing policy didn't apply to the FM radio.
> So it might be possible to remove the hack code from FM and fix the
> underlying bug in some other way.  
> 
> Or, maybe it is simpler than that. In this case, the radio app has already
> stopped playing when the call starts, right? In that case, it shouldn't do
> anything when the attention screen opens or when the attention screen
> closes.  It should just be a matter of setting a flag, or of registering the
> settings observer when we start playing and removing it when we stop
> playing, right? (I'm assuming that there is a way for the FM radio app to
> know when it has stopped playing because the music app starts.)

David is correct, I am misled by the needinfo.
Re-read the bug I tend to think this is fmRadio platform issue or a audio channel service issue.

Need platform help.
Flags: needinfo?(scheng)
From the title, it looks like sound leakage(In reply to jingmei.zhang from comment #15)
> (In reply to Randy Lin [:rlin] from comment #14)
> > The bug 1037880 won't change the audio policy. I think this one need to find
> > out why cause the delay of stopping fmradio and music application by
> > audiochannel. 
> 
> hi Randy,this imight not be a delay issue.we could hear fm because we
> enabled the fm in gaia fm.
Does fmradio application receive canPlay callback from AudioChannel?
And music application receive the same event and resume playback? 
I guess AudioChannel may not design for this kind of scenario...

Comment 23

4 years ago
Making it a priority but not a blocker. It is not a regression from previous release. Justin will investigate after his blockers are fixed.
Whiteboard: [priority]

Updated

4 years ago
blocking-b2g: 2.1? → ---

Updated

4 years ago
Flags: needinfo?(dkuo)
Looks like the help is here already.
Flags: needinfo?(timdream)
(Assignee)

Comment 25

4 years ago
Created attachment 8498357 [details] [review]
pull-request (master)
Attachment #8498357 - Flags: review?(pzhang)
Attachment #8498357 - Flags: feedback?(jingmei.zhang)

Updated

4 years ago
Flags: needinfo?(scheng)

Comment 26

4 years ago
(In reply to Justin D'Arcangelo [:justindarc] from comment #25)
> Created attachment 8498357 [details] [review]
> pull-request (master)

The patch works fine,and FmRadio does not play any more after incoming call.
Thank you for your work!
(Assignee)

Comment 27

4 years ago
Comment on attachment 8498357 [details] [review]
pull-request (master)

Also flagging Tim for review as my "R?" flag for Pin has gone unanswered for several days now.
Attachment #8498357 - Flags: review?(timdream)
(In reply to Justin D'Arcangelo [:justindarc] from comment #27)
> Comment on attachment 8498357 [details] [review]
> pull-request (master)
> 
> Also flagging Tim for review as my "R?" flag for Pin has gone unanswered for
> several days now.

I just get back from China's National Holiday, sorry for my late reply, please also check my comments in PR, thanks.
Attachment #8498357 - Flags: review?(timdream)
Attachment #8498357 - Flags: review?(pzhang)

Comment 29

4 years ago
Dear Justin D'Arcangelo,

Your patch may lead to a new problem. 

Step:
1.Open FM page->play FM
2.Open Music->play music
3.Open FM page->Need to click on play button to listen FM.
Expected Result: Play the FM automatic when open FM page.

I add 'mozinterruptend' event based on your patch.

 mozinterruptListener.addEventListener('mozinterruptend', function(evt) {
    restoreState();
  });

Is it OK? Thanks!
Flags: needinfo?(jdarcangelo)
(In reply to Chaochao Huang from comment #29)
> Dear Justin D'Arcangelo,
> 
> Your patch may lead to a new problem. 
> 
> Step:
> 1.Open FM page->play FM
> 2.Open Music->play music
> 3.Open FM page->Need to click on play button to listen FM.
> Expected Result: Play the FM automatic when open FM page.
> 
> I add 'mozinterruptend' event based on your patch.
> 
>  mozinterruptListener.addEventListener('mozinterruptend', function(evt) {
>     restoreState();
>   });
> 
> Is it OK? Thanks!

That's exactly what i commented in the PR, :)
(Assignee)

Comment 31

4 years ago
Comment on attachment 8498357 [details] [review]
pull-request (master)

Pin: Addressed the regression you noted in the PR. Now, when returning to the FM Radio app, the radio will resume playing if it was previously playing.

Also note, the `attention_screen_opening` hack is still necessary to prevent the radio audio from overlapping with the incoming call screen.
Attachment #8498357 - Flags: review?(pzhang)
Flags: needinfo?(jdarcangelo)
Hi Justin, i don't see the updates, :(
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 33

4 years ago
(In reply to Pin Zhang [:pzhang] from comment #32)
> Hi Justin, i don't see the updates, :(

They're there... Starting at line 882:

https://github.com/mozilla-b2g/gaia/pull/24624/files
Flags: needinfo?(jdarcangelo) → needinfo?(pzhang)
(In reply to Justin D'Arcangelo [:justindarc] from comment #33)
> (In reply to Pin Zhang [:pzhang] from comment #32)
> > Hi Justin, i don't see the updates, :(
> 
> They're there... Starting at line 882:
> 
> https://github.com/mozilla-b2g/gaia/pull/24624/files

Embarrassed ... sorry for my sloppy.
Flags: needinfo?(pzhang)
(Assignee)

Comment 35

4 years ago
No worries! :-)
Comment on attachment 8498357 [details] [review]
pull-request (master)

Hi Justin, another regression is introduced unfortunately, :(
Attachment #8498357 - Flags: review?(pzhang)

Comment 37

4 years ago
Hi Justin, could adding 'mozinterruptend' event deal with the issue as Comment 29?
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 38

4 years ago
(In reply to Pin Zhang [:pzhang] from comment #36)
> Comment on attachment 8498357 [details] [review]
> pull-request (master)
> 
> Hi Justin, another regression is introduced unfortunately, :(

Pin: Are you able to reproduce the regression you noted in the PR? I just followed your STR listed in the PR and was not able to reproduce any regression.

STR:
1. Open FM Radio app and press "Stop" button to stop playing the FM radio
2. Open Music app and play music
3. Open FM Radio app again

  Expected:
  - Music app continues playing music

  Actual:
  - Music app continues playing music

Note that by calling `play()` on our `mozinterruptListener` <audio> element when we return to FM Radio, I too would expect that it would cause the Music app to stop playing music. However, it does not cause that to happen and I'm assuming that's because there is no `src` content for the <audio> element. The purpose of that <audio> element is *only* for listening for the mozinterrupt events and since it has no `src` content, it by itself does not cause other apps' audio to be interrupted. This is just a hack until a better audio competing policy can be implemented in FxOS.

If you are unable to reproduce this regression, could you please finish reviewing this patch so we can get it landed? Thank you.

Also, I would like to file a follow-up bug to this for modifying the mozFMRadio API to make it behave like an ordinary <audio> element so that it can emit/listen for mozinterrupt events by itself. Currently, navigator.mozFMRadio seems to behave like a special case which is causing an additional level of difficulty in cooperating with the audio competing policy.
Flags: needinfo?(jdarcangelo) → needinfo?(pzhang)
(Assignee)

Comment 39

4 years ago
(In reply to Chaochao Huang from comment #37)
> Hi Justin, could adding 'mozinterruptend' event deal with the issue as
> Comment 29?

Chaochao: I don't think that will work because, presumably, you wouldn't want the FM radio to resume playing while the FM Radio app is in the background. Prior to this patch, if you play the FM radio, then play music in the Music app and tap the "Pause" button in the Music app, the FM radio does NOT resume playing. It is not until you re-enter the FM Radio app that the FM radio begins playing again. By adding your `mozinterruptend` handler from Comment 29, the FM radio would begin playing *immediately* when you tap "Pause" in the Music app which would be a regression.
(In reply to Justin D'Arcangelo [:justindarc] from comment #38)
> (In reply to Pin Zhang [:pzhang] from comment #36)
> > Comment on attachment 8498357 [details] [review]
> > pull-request (master)
> > 
> > Hi Justin, another regression is introduced unfortunately, :(
> 
> Pin: Are you able to reproduce the regression you noted in the PR? 

Yes, i tested your patch before commenting the PR, :)

I also checked it again and add some logs in the |visibilitychange| event listener, the sound of the Music app stopped just right after the FM app is switched back to foreground, so i'm pretty sure the regression is valid.

I tested it on Flame but not with the latest Gecko and Gaia, i don't think the regression would disappear in the latest codebase, but if you really want me to do the testing again with the latest code, please let me know.
Flags: needinfo?(pzhang) → needinfo?(jdarcangelo)
(Assignee)

Comment 41

4 years ago
(In reply to Pin Zhang [:pzhang] from comment #40)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #38)
> > (In reply to Pin Zhang [:pzhang] from comment #36)
> > > Comment on attachment 8498357 [details] [review]
> > > pull-request (master)
> > > 
> > > Hi Justin, another regression is introduced unfortunately, :(
> > 
> > Pin: Are you able to reproduce the regression you noted in the PR? 
> 
> Yes, i tested your patch before commenting the PR, :)
> 
> I also checked it again and add some logs in the |visibilitychange| event
> listener, the sound of the Music app stopped just right after the FM app is
> switched back to foreground, so i'm pretty sure the regression is valid.
> 
> I tested it on Flame but not with the latest Gecko and Gaia, i don't think
> the regression would disappear in the latest codebase, but if you really
> want me to do the testing again with the latest code, please let me know.

Pin: I believe it does matter that you use the latest Gecko/Gaia. A patch landed recently that impacts visibilitychange with regards to audio competing.
Flags: needinfo?(jdarcangelo) → needinfo?(pzhang)
(In reply to Justin D'Arcangelo [:justindarc] from comment #41)
> Pin: I believe it does matter that you use the latest Gecko/Gaia. A patch
> landed recently that impacts visibilitychange with regards to audio
> competing.

Hi Justin, I tried on the lastest Gecko and Gaia, the regression still exists, here is the commits I used:
   Gecko:
     https://git.mozilla.org/?p=releases/gecko.git;a=commit;h=0dadefde36b76d53c3b8c885e8e8c75f276d31db
   Gaia:
     https://github.com/mozilla-b2g/gaia/commit/cc448fe
Flags: needinfo?(pzhang) → needinfo?(jdarcangelo)
(Assignee)

Comment 43

4 years ago
(In reply to Pin Zhang [:pzhang] from comment #42)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #41)
> > Pin: I believe it does matter that you use the latest Gecko/Gaia. A patch
> > landed recently that impacts visibilitychange with regards to audio
> > competing.
> 
> Hi Justin, I tried on the lastest Gecko and Gaia, the regression still
> exists, here is the commits I used:
>    Gecko:
>     
> https://git.mozilla.org/?p=releases/gecko.git;a=commit;
> h=0dadefde36b76d53c3b8c885e8e8c75f276d31db
>    Gaia:
>      https://github.com/mozilla-b2g/gaia/commit/cc448fe

This is so bizarre, I tried everything just now to reproduce this regression with my patch and I'm still unable to. I'm running Flame-KK with firmware v184. Here's the versions I'm using:

Gaia-Rev        3b81896f04a02697e615fa5390086bd5ecfed84f
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/f547cf19d104
Build-ID        20141013040202
Version         35.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141009.071519
FW-Date         Thu Oct  9 07:15:28 EDT 2014
Bootloader      L1TC00011840

I have an idea that might resolve the issue you're describing. But, since I'm unable to reproduce it in the first place, its going to be impossible for me to verify.
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 44

4 years ago
(In reply to Pin Zhang [:pzhang] from comment #42)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #41)
> > Pin: I believe it does matter that you use the latest Gecko/Gaia. A patch
> > landed recently that impacts visibilitychange with regards to audio
> > competing.
> 
> Hi Justin, I tried on the lastest Gecko and Gaia, the regression still
> exists, here is the commits I used:
>    Gecko:
>     
> https://git.mozilla.org/?p=releases/gecko.git;a=commit;
> h=0dadefde36b76d53c3b8c885e8e8c75f276d31db
>    Gaia:
>      https://github.com/mozilla-b2g/gaia/commit/cc448fe

UPDATE: I was able to reproduce! I forgot to do a `make reset-gaia` after flashing the patch to my device. Since this patch adds the `audio-channel-content` permission, it will not take effect unless a full reset is done.
(Assignee)

Comment 45

4 years ago
Comment on attachment 8498357 [details] [review]
pull-request (master)

Pin: I've updated the patch to fix the regression you noted. In order to maintain a consistency, I've added a `disableFMRadio()` function to the app to parallel the existing `enableFMRadio()` function we already had. The reason I did this is so we have a clean way to enable/disable the `mozinterrupt` event listener according to whether the FM radio is enabled or disabled. This prevents the `mozinterrupt` event listener from "stealing" the audio content focus when the user re-enters the FM Radio app.
Attachment #8498357 - Flags: review?(pzhang)
Comment on attachment 8498357 [details] [review]
pull-request (master)

Hi Justin, here is how i test the new patch:
  - Open FM app, it's turned on automatically
  - Open Music app, play a song
  - Switch back to the FM app

Expected result:
  The audio is switch back to the FM radio immediately.

Actual result:
  The FM radio is already been disabled, ant it's enabled again automatically.
Attachment #8498357 - Flags: review?(pzhang)
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 47

4 years ago
(In reply to Pin Zhang [:pzhang] from comment #46)
> Comment on attachment 8498357 [details] [review]
> pull-request (master)
> 
> Hi Justin, here is how i test the new patch:
>   - Open FM app, it's turned on automatically
>   - Open Music app, play a song
>   - Switch back to the FM app
> 
> Expected result:
>   The audio is switch back to the FM radio immediately.
> 
> Actual result:
>   The FM radio is already been disabled, ant it's enabled again
> automatically.

Yes, this is the same result I get as well. We may want UX input here, but I'm thinking this behavior might lead to some power savings since the FM radio is temporarily disabled when another audio source begins playing. Previously, the FM radio remained enabled in the background even though it was not audible. I would assume that there is some additional battery power being consumed in that case, but I haven't finished measuring it yet.

Regardless, I believe we can still get the old behavior with a slight modification to this patch. But I first wanted to see if there were any power savings by disabling the radio when another audio source interrupts the FM Radio app.
Flags: needinfo?(jdarcangelo)
(Assignee)

Comment 48

4 years ago
Ok, so I measured with my ammeter on a Flame device running KK-v184 and here's my results (+/- 5mA):

master
======
Baseline (No running apps): 290mA

FM Radio (Radio Playing):   310mA (+20mA)
FM Radio (Radio Stopped):   290mA (+0mA)

Music (Music Playing):      340mA (+50mA)
Music (Music Paused):       300mA (+10mA)

FM Radio + Music (Radio Playing, Music Playing): 360mA
FM Radio + Music (Radio Stopped, Music Playing): 340mA
FM Radio + Music (Radio Playing, Music Paused):  320mA
FM Radio + Music (Radio Stopped, Music Paused):  300mA

master + 1069887
================
Baseline (No running apps): 290mA

FM Radio (Radio Playing):   310mA (+20mA)
FM Radio (Radio Stopped):   290mA (+0mA)

Music (Music Playing):      340mA (+50mA)
Music (Music Paused):       300mA (+10mA)

FM Radio + Music (Radio Playing, Music Playing): 340mA
FM Radio + Music (Radio Stopped, Music Playing): 340mA
FM Radio + Music (Radio Playing, Music Paused):  320mA
FM Radio + Music (Radio Stopped, Music Paused):  300mA

These results are expected. By themselves, the FM Radio and Music apps have no difference in power consumption. However, when you play the FM radio, then go into Music and play music, there is a 20mA power savings as a result of the FM radio hardware being powered off since it is no longer audible anyways.

So, there is a trade-off here:

* Slight delay in resuming radio audio when switching back to FM Radio from Music, but no additional power is consumed when the user switches away from FM Radio to Music (master + this patch)

-or-

* NO delay in resuming radio audio when switching back to FM Radio from Music, but an additional 20mA power is consumed when the user switches away from FM Radio to Music (master)

Flagging UX for more info to confirm that the delay of re-enabling the FM radio in this patch is an acceptable trade-off for the power savings.
Flags: needinfo?(jsavory)
(Assignee)

Comment 49

4 years ago
Comment on attachment 8498357 [details] [review]
pull-request (master)

Also adding UX for ui-review for this patch. Use cases to check:

Use Case 1
----------

1. Play FM Radio
2. Play song in Music (FM Radio should stop)
3. Receive phone call (Music should pause)
4. End phone call

Expected:
- Music should resume playing
- FM Radio should not be heard

Use Case 2
----------

1. Play FM Radio
2. Play song in Music (FM Radio should stop)
3. Return to FM Radio

Expected:
- Music should pause playing
- FM Radio should resume playing
Attachment #8498357 - Flags: ui-review?(jsavory)
Comment on attachment 8498357 [details] [review]
pull-request (master)

This is looking good and working as expected. I think that the slight pause is not an issue and the benefit of preserving power is great.
Attachment #8498357 - Flags: ui-review?(jsavory) → ui-review+
Flags: needinfo?(jsavory)
(Assignee)

Comment 51

4 years ago
Created attachment 8505703 [details] [review]
pull-request (master) [ALT]

Alternative pull request with "static" sound when waiting for radio to power up.
Attachment #8505703 - Flags: ui-review?(jsavory)
(Assignee)

Comment 52

4 years ago
Comment on attachment 8498357 [details] [review]
pull-request (master)

Pin: As UX has noted in Comment 50, the delay to power on the radio that is introduced by this patch is acceptable. I have also confirmed in Comment 48 that we are saving power consumption by shutting down the radio when it is not audible. Can you please give a code review for this patch when you get a chance? Thanks!

Also, you may have noticed I have an alternative PR attached to this bug as well. After talking to jsavory about the radio startup delay, I had an idea to play an FM static sound while the radio is powering up. I'm not sure if this is something UX will want or not, but I've attached the "alt" PR here for her to take a look at as an experiment.
Attachment #8498357 - Flags: review?(pzhang)
(In reply to Justin D'Arcangelo [:justindarc] from comment #52)
> Comment on attachment 8498357 [details] [review]
> pull-request (master)
> 
> Pin: As UX has noted in Comment 50, the delay to power on the radio that is
> introduced by this patch is acceptable. I have also confirmed in Comment 48
> that we are saving power consumption by shutting down the radio when it is
> not audible. Can you please give a code review for this patch when you get a
> chance? Thanks!
> 
I'm OK with the patch, but before give r+ flag, i just want to confirm if other similar cases are acceptable, say case A:
  - Open FM radio and play.
  - Open Phone app, dial a number.
  - Hang up the phone

  Current result:
    The FM radio continue to play.

  Result after applying this patch:
    FM radio is stopped, user have to switch back to the FM app.

the similar case is a call is incoming when user is listening to the FM radio, 

case B:
  - Open FM radio and play
  - Open Settings app to change the ringtone or alerts sound.

  Current result:
    The FM radio is not affected.

  Result after applying this patch:
    FM radio is stopped, user have to switch back to the FM app.

I think there are some other cases that user might not expect the FM app would be stopped.
Flags: needinfo?(jsavory)
Flags: needinfo?(jdarcangelo)
(Reporter)

Comment 54

4 years ago
hi Justin,aonther issue caused by the patch:

precondition:without connecting head phone in the devices
1.play music
2.open fm
music is paused.
it should be better for the music to go on playing in this suiation.

can you help to check the issue?
(Reporter)

Updated

4 years ago
Attachment #8505703 - Flags: feedback-
(Reporter)

Updated

4 years ago
Attachment #8498357 - Flags: feedback?(jingmei.zhang) → feedback-
(Reporter)

Updated

4 years ago
Attachment #8505703 - Flags: feedback-
Pin: For the questions you have outlined in case A, I believe that FM radio should follow music, which means the current behaviour is correct and the result after the patch should be changed. 

For case B: Again, FM radio should follow music's lead in behaviour. For this one I think the current behaviour is also corrent, once the ringtone is set the FM radio should start playing again. 

ni? on Mike to confirm or correct my assumptions for these audio competing cases.
Flags: needinfo?(jsavory) → needinfo?(mtsai)
Comment on attachment 8505703 [details] [review]
pull-request (master) [ALT]

Hi Justin, sorry for the delay on this one. I tried it out today and while I liked the sound when you reopen the radio application, I'm not sure it'll work without the context of the radio. It might be easy to mistake the sound for the phone call not ending properly. 

I think overall I prefer the version without the static sound, but it was a good try!
Attachment #8505703 - Flags: ui-review?(jsavory) → ui-review-

Comment 57

4 years ago
In reply to Comment 55.
Confirming Jacqueline's Case A and Case B audio competing assumptions.
Flags: needinfo?(mtsai)
Flags: needinfo?(jdarcangelo)
Comment on attachment 8498357 [details] [review]
pull-request (master)

Let me clear r? flag here because of uir-.
Attachment #8498357 - Flags: review?(pzhang)
Duplicate of this bug: 1111906

Updated

4 years ago
Blocks: 1054172, 1107999
blocking-b2g: --- → 2.0M+
status-b2g-v2.0M: --- → affected
Hi Justin, are you still working on this?
Flags: needinfo?(jdarcangelo)

Comment 61

4 years ago
I have a solution:
1.remove the whole function in fm.js :
navigator.mozSettings.addObserver(
'private.broadcast.attention_screen_opening',) ...
we don't need this in gaia

2.make some changes in "audioChannelService.cpp" :
AudioChannelService::GetStateInternal() { 
....
  // Let play any visible audio channel.
  if (!aElementHidden) {
    if (CheckVolumeFadedCondition(newType, aElementHidden)) {
      return AUDIO_CHANNEL_STATE_FADED;
    }
   //add begin
    if ( (aChannel == AudioChannel::Content) && 
      ( mCurrentHigherChannel >= static_cast<int32_t>(AudioChannel::Notification) && mCurrentHigherChannel <= static_cast<int32_t>(AudioChannel::Ringer) ) ) {
      return AUDIO_CHANNEL_STATE_MUTED;
    }
  //add end
    return CheckTelephonyPolicy(aChannel, aChildID);
  }
....

but I am not sure if it will cause some other issue??

Comment 62

4 years ago
Hi Pin,
Can you help to review partner's solution per comment 61?
Thank you!
Flags: needinfo?(pzhang)
(In reply to Josh Cheng [:josh] from comment #62)
> Hi Pin,
> Can you help to review partner's solution per comment 61?
> Thank you!

Hi Josh, i think it's the right way to fix this issue from Gecko layer as comment 61 suggested, but i'm not familiar with audio channel things, let's ni? Randy for some input.
Flags: needinfo?(pzhang) → needinfo?(rlin)
I think this would be the latency problem of audio channel services. 
i.e ringtone play first and this action would trigger fm to stop later.
On bug 1113086 (v 3.0) , we would like to change the control logic of audio channel.
This problem has also been considered.
Flags: needinfo?(rlin)

Comment 65

4 years ago
Hi Pin,
Should we dup this to bug 1113086?
Thanks!
blocking-b2g: 2.0M+ → ---
Flags: needinfo?(pzhang)
(In reply to Josh Cheng [:josh] from comment #65)
> Hi Pin,
> Should we dup this to bug 1113086?
> Thanks!

As Randy said in comment 64, this problem has been handled in bug 1113086, let's close this bug as a dup of bug 1113086.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(pzhang)
Flags: needinfo?(jdarcangelo)
Resolution: --- → DUPLICATE
Duplicate of bug: 1113086
You need to log in before you can comment on or make changes to this bug.