1135614 - Use audio channel interruptions instead of a Settings flag, to turn off the radio when the user have an incoming call

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::FMRadio, defect)

Description

[Manuel Casas Barrado [:mancas]]

We should change the way of turn off the radio when an incoming call is received. This will help us to separate the access to settings from the radio, and will clean up our code in order to make it more readable and maintainable.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] mancas:bug1135614 > mozilla-b2g:master

Comment 2

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8567862 [details] [review]
[gaia] mancas:bug1135614 > mozilla-b2g:master

Hey Tim, I think this improvement is good for the app. Could you review it when you get a chance? Feel free to ask me any change that we should make or whatever.

Thanks!

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8567862 [details] [review]
[gaia] mancas:bug1135614 > mozilla-b2g:master

I am not convinced this is the correct fix. 

Checking the AudioCompetingHelper it looks like it simply manually competes the telephony channel -- can such interruption event happen on FM API itself instead?

Why do we need AudioCompetingHelper at the first place?

Dominic, could you comment based on your work on audio channel management?

Comment 4

[Dominic Kuo [:dkuo]]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #3)
> Dominic, could you comment based on your work on audio channel management?

I think the reason why we use this mozSettings hack is to disable the fm radio at the right timing(the comment explained it), not because we need this hack to disable/enable the fm radio, that means, without the current mozSettings hack, the fm radio should still pause/resume by itself, or I should say by gecko. So, to use the AudioCompetingHelper is actually replacing the mozSettings hack from one to another, additionally, it will introduce an audio competing regression.

Comment 5

[Dominic Kuo [:dkuo]]

Comment on attachment 8567862 [details] [review]
[gaia] mancas:bug1135614 > mozilla-b2g:master

mancas, thanks for working on this, but as I mentioned in comment 4, this patch is just an alternative hack and should cause regressions, let's not to use this hack.

Actually several gecko and gaia devs are implementing the new audio channel api and a sub-module in system app, to solve the audio competing issues completely.

After the new audio channel api and the audio channel service enabled in system app, theoretically all the hacks for audio channel should be removed, including this one and we don't have to count on audio_competing_helper.js.

And if you are interested in the new audio channel api, please see:
- Spec: bug 961967
- Gecko: bug 1113086
- Gaia: bug 1100822

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

What's your recommend on this bug? WONTFIX? Remove the settings hack entirely once bug 1100822 lands?

Comment 7

[Borja Salguero [:borjasalguero]]

Hi Dominic and Tim,
I've been tracking your work based on the new AudioChannelManager and looks great (actually I've added some comments to te code in [1] and [2]), but the goal of this bug it's getting rid of the hacks based on settings, in order to have a method based on AudioChannel, which will be easy to replace with the new model of AudioChannelManager when ready.

Actually this approach it's working in other apps as ringtone [3] or callscreen[4].

In the case of Radio FM and Telephony, the problem we have in Gaia is that there is no <audio> element or similar, so interruptions can not be controlled directly without accesing to the API, due to these APIs are routing the audio directly to the speaker at low level. In these cases, currently we need to create an 'AudioContext' with the same level of priority in order to detect when other channel is interrupting us, and behave properly.

On the other hand, for example Music App, we don't need this. Music App can listen 'mozinterruptbegin' and 'mozinterruptend' directly, and this must be working currently in master.

Our main goal with this bug is just clean all hacks related with Settings, and have the code ready for the AudioChannelManager which is coming in the near future. We would like to have a clear picture of the apps which really need access to Settings, and the ones which are using Settings API for workarounds or functionalities that can be solved without Settings API, which now is overused.
Wdyt?


[1] https://github.com/mozilla-b2g/gaia/pull/26512
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1126224
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/tone_player.js#L143
[4] https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/audio_competing_helper.js

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Borja Salguero [:borjasalguero] from comment #7)
> Our main goal with this bug is just clean all hacks related with Settings,
> and have the code ready for the AudioChannelManager which is coming in the
> near future. We would like to have a clear picture of the apps which really
> need access to Settings, and the ones which are using Settings API for
> workarounds or functionalities that can be solved without Settings API,
> which now is overused.
> Wdyt?

Making incremental improvements before AudioChannelManager is indeed a valid point, but I will leave Dominic to comment on this.

Comment 9

[Dominic Kuo [:dkuo]]

Before feedbacking more on the questions, does the AudioCompetingHelper hack cause no regression? the mozSettngs hack was first introduced in bug 1037880, it did solved the fm lacking problem and we should keep it.

Comment 10

[Borja Salguero [:borjasalguero]]

Attachment: WIP

Comment 11

[Borja Salguero [:borjasalguero]]

Hi Dominic,
I've tested in my Flame with the code you have in [1] (I've attached as WIP patch to this bug), and the STR described in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1037880#c6 is working as expected.
When receiving an incoming call or when dialing a phone number, Radio FM is stopped properly, and it recovers the previous status once the AudioChannel is ready (when the phone call is over).

Actually, when testing in master, if when playing a Radio FM channel through the speakers (you need to have your headphones plugged), if an incoming call is received, once this call is over Radio FM start playing again *through the headphones*, so it's not working as expected. This scenario is fixed when applying the WIP patch attached. I hope it helps! If the patch is ok for you I'll start adding tests, and I'll ask you for a r? when ready. Thanks a lot!


[1]https://github.com/mozilla-b2g/gaia/pull/28597

Comment 12

[Dominic Kuo [:dkuo]]

(In reply to Borja Salguero [:borjasalguero] from comment #11)
> Hi Dominic,
> I've tested in my Flame with the code you have in [1] (I've attached as WIP
> patch to this bug), and the STR described in bug
> https://bugzilla.mozilla.org/show_bug.cgi?id=1037880#c6 is working as
> expected.
> When receiving an incoming call or when dialing a phone number, Radio FM is
> stopped properly, and it recovers the previous status once the AudioChannel
> is ready (when the phone call is over).

If you read bug 1037880 comment 0(the Actual Result), you will know that issue was not about the fm radio does not stop when ringtone rings, it did stop but it stopped after the ringtone rang 2 seconds. 

That's because at the time fm playing in foreground then callscreen rings, both fm and callscreen are viable(visibilities are true) and gecko allows content channel(fm is content) to play in foreground, no matter there is a higher channel or not, so that's why the current hack listens to |private.broadcast.attention_screen_opening| because system app dispatch this event before the callscreen actually launches.

And as I mentioned earlier in comment 4, fm app does not count on the mozSettings hack to pause/resume itself when audio channel competes, the mozFMRadio api has already used the media element way to produce sounds, which means, mozFMRadio also follow the audio channel policy, and using an audio context, listeing to |mozinterruptbegin| then doing "mozFMRadio.disable()" are not necessary for mozFMRadio. You can try to remove the current mozSettings hack without your WIP, the fm app should also work as your expectation.

> Actually, when testing in master, if when playing a Radio FM channel through
> the speakers (you need to have your headphones plugged), if an incoming call
> is received, once this call is over Radio FM start playing again *through
> the headphones*, so it's not working as expected. This scenario is fixed
> when applying the WIP patch attached. I hope it helps! If the patch is ok
> for you I'll start adding tests, and I'll ask you for a r? when ready.
> Thanks a lot!

It sounds like an audio routing issue but I think the audio routing is managed by gecko, your patch might work but a correct fix should use gecko patch I guess.

Comment 13

[Dominic Kuo [:dkuo]]

Comment on attachment 8571943 [details] [review]
WIP

Cancelling the feedback because of comment 12.

Comment 14

[BMO Automation]

Firefox OS is not being worked on