1006380 - Set phone in PHONE_STATE_IN_COMMUNICATION audio state when telephony audio channel is in use

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

To allow Loop to switch between the built-in earpiece and the speaker we need to expose the `telephonySpeaker` property in mozAudioChannelManager as suggested in https://wiki.mozilla.org/WebAPI/AudioChannels#Application_API

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Go for it.

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Ehsan, Andrea, do you think that this is the correct approach to switch between the earpiece and the speaker?

Comment 3

[(no longer active)]

Andrea and Marco know way more about this than myself, I'll defer to them.  :-)

Comment 4

[Marco Chen [:mchen]]

Hi Fernando,

Unfortunately telephonySpeaker in mozAudioChannelManager can't satisfy your requriement of switching audio routing path to earpiece. 

What it plans to acheive is just "force to enable speaker routing path" and it didn't care what orignal routing path is. That is to say you can not switch routing path to earpiece through it.

Currently there is no Web API to trigger "switching routing path to earpiece", and what it was achieved is magic inside Gecko & Audio HAL. Gecko told Audio HAL that the phone state is on "MODE_IN_CALL" then Audio HAL will decide to use earpiece (if there is no other high priority output device)

For use case of VOIP or A/V chat, Audio HAL expect that Gecko can set phone state to "MODE_IN_COMMUNICATION". Therefore 
  
  1. there should be a way from Web Content to notify user agent about VOIP or A/V chat is on going now.
  2. Or there is a Web API for Web Content to request earpiece routing path.

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Thanks Marco! The original idea was to switch from the earpiece to the speaker.

Anyway, after looking deeper into this I've found that we might be able to use the SpeakerManager API [1] to address Loop's use case. So if we provide Loop with audio-channel-telephony permissions as suggested in bug 990552, Loop calls will be done through the earpiece by default and we can then use the SpeakerManager to force the usage of the speaker.

Is my assumption correct? Jaoo, could you test that this is actually working, please?

[1] https://wiki.mozilla.org/WebAPI/SpeakerManager

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Didn't we create a speaker API a while back? I think exposing that to privileged apps might solve things here.

Comment 7

[Marco Chen [:mchen]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #5)
So if we provide
> Loop with audio-channel-telephony permissions as suggested in bug 990552,
> Loop calls will be done through the earpiece by default and we can then use
> the SpeakerManager to force the usage of the speaker.
> 
> Is my assumption correct?

As I know, earpiece will be switched if only the phone state is set to MODE_IN_CALL or MODE_IN_COMMUNICATION. That said to set audio as telephony channel can not switch audio to earpiece. But you try it I may be wrong.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Thanks Marco.

jaoo can you test this, please?(In reply to Jonas Sicking (:sicking) from comment #6)

> Didn't we create a speaker API a while back? I think exposing that to
> privileged apps might solve things here.

Yes, that would be the SpeakerManager API

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Marco Chen [:mchen] from comment #7)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #5)
> So if we provide
> > Loop with audio-channel-telephony permissions as suggested in bug 990552,
> > Loop calls will be done through the earpiece by default and we can then use
> > the SpeakerManager to force the usage of the speaker.
> > 
> > Is my assumption correct?
> 
> As I know, earpiece will be switched if only the phone state is set to
> MODE_IN_CALL or MODE_IN_COMMUNICATION. That said to set audio as telephony
> channel can not switch audio to earpiece. But you try it I may be wrong.

You're right Marco. It cannot switch audio to earpiece. Phone state must be either `PHONE_STATE_IN_CALL` or `PHONE_STATE_IN_COMMUNICATION` otherwise gecko forces using `FORCE_SPEAKER` (or `FORCE_NONE`) device category for `USE_MEDIA` usage. It seems gecko figures out the usage by checking the phone state (see [1]). Could be possible to force using `FORCE_SPEAKER` device category for `USE_COMMUNICATION` usage without being necessary set the phone state to either `PHONE_STATE_IN_CALL` or `PHONE_STATE_IN_COMMUNICATION` state?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/speakermanager/SpeakerManagerService.cpp#92

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Another option could be setting the phone state to PHONE_STATE_IN_COMMUNICATION or PHONE_STATE_IN_CALL when the telephony audio channel is being used.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ooh, I like that idea. That would make all audio from all channels go to the earpiece, right?

Comment 12

[Marco Chen [:mchen]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #10)
> Another option could be setting the phone state to
> PHONE_STATE_IN_COMMUNICATION or PHONE_STATE_IN_CALL when the telephony audio
> channel is being used.

Something like this

  telephony channel is fired -> is there a phone call? -> (yes) set PHONE_STATE_IN_CALL
                                                       -> (no) set PHONE_STATE_IN_COMMUNICATION

then it seems to be reasonable and common 9 will be fixed too.

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8425424 [details] [diff] [review]
WIP

Hey Marco, after having a look at the code and figure out what we need to add I've ended up with this WIP patch. At this point I should have some feedback more familiar than me with the audio channel management, so may I have your feedback please? Thanks.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8425424 [details] [diff] [review]
WIP

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

::: dom/audiochannel/AudioChannelService.cpp
@@ +143,5 @@
>  AudioChannelService::RegisterType(AudioChannel aChannel, uint64_t aChildID,
>                                    bool aWithVideo)
>  {
>    if (mDisabled) {
> +    return NS_OK;

You've made the return type be a bool, but here you are returning an nsresult.

@@ +244,5 @@
>                                      uint64_t aChildID,
>                                      bool aWithVideo)
>  {
>    if (mDisabled) {
> +    return NS_OK;

Same here.

Comment 16

[Marco Chen [:mchen]]

Comment on attachment 8425424 [details] [diff] [review]
WIP

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

Hi,

It is good to see your contribution here. Good work.

According to issue I mentioned below, I would suggest
  (offload decision of phone state from AudioChannelService to AudioManager who is the control center of routing information)
  a. AudioManager listens to event from AudioChannelService for notifying each change of AudioChannel. (currently only audio-channel-process-changed can fit this requirement but not it's original purpose)
  b. AudioManager check whether there are any telephony channels registered in AudioChannelService.
  c. Now AudioManager has two factors to decide final phone state: 
       one is from AudioManager::SetPhoneState by RIL.
       Second is from step b.


And comments for current patch are listed as below:

::: dom/audiochannel/AudioChannelService.cpp
@@ +261,5 @@
> +
> +    int32_t phoneState;
> +    audioManager->GetPhoneState(&phoneState);
> +    if (phoneState != nsIAudioManager::PHONE_STATE_IN_CALL) {
> +      audioManager->SetPhoneState(nsIAudioManager::PHONE_STATE_NORMAL);

Once there are multiple VOIP apps using telephony channel the first one called unregister() will directly set state to PHONE_STATE_NORMAL. Therefore the remaining VOIP apps will route audio to speaker not earpiece.

::: dom/audiochannel/AudioChannelService.h
@@ +123,5 @@
>    void SendAudioChannelChangedNotification(uint64_t aChildID);
>  
>    /* Register/Unregister IPC types: */
> +  bool RegisterType(AudioChannel aChannel, uint64_t aChildID, bool aWithVideo);
> +  bool UnregisterType(AudioChannel aChannel, bool aElementHidden,

I don't catch up the reason of changing return type from void to bool. May I know the reason?

Comment 17

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP

(In reply to Marco Chen [:mchen] from comment #16)
> Comment on attachment 8425424 [details] [diff] [review]
> WIP
> 
> Review of attachment 8425424 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for your review and comments Marco, they helped me so much!. Jonas, thanks for your review as well.

> According to issue I mentioned below, I would suggest
>   (offload decision of phone state from AudioChannelService to AudioManager
> who is the control center of routing information)
>   a. AudioManager listens to event from AudioChannelService for notifying
> each change of AudioChannel. (currently only audio-channel-process-changed
> can fit this requirement but not it's original purpose)
>   b. AudioManager check whether there are any telephony channels registered
> in AudioChannelService.
>   c. Now AudioManager has two factors to decide final phone state: 
>        one is from AudioManager::SetPhoneState by RIL.
>        Second is from step b.

This new WIP patch tries to implement what you detailed above, could you have a look and provide some feedback please? Thanks!

> And comments for current patch are listed as below:

> Once there are multiple VOIP apps using telephony channel the first one
> called unregister() will directly set state to PHONE_STATE_NORMAL. Therefore
> the remaining VOIP apps will route audio to speaker not earpiece.

I guess it won't happen with current patch.
 
> I don't catch up the reason of changing return type from void to bool. May I
> know the reason?

Please, forget about it. It wasn't needed and wasn't correct.

Comment 18

[Marco Chen [:mchen]]

Comment on attachment 8426244 [details] [diff] [review]
WIP

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

Thanks. And it looks good to me.

::: dom/system/gonk/AudioManager.cpp
@@ +282,5 @@
>  }
>  
> +void
> +AudioManager::HandleAudioChannelProcessChanged()
> +{

I think we can directly return first if mPhoneState is "IN_CALL" or "IN_RINGTONE".
Then we can check telephonyChannelIsActive variable only in the following codes.

This function will be called again once RIL set phone state from "IN_CALL" or "IN_RINGTONE" to "IN_NORMAL".
Then we get chance to set "IN_COMMUNICATION".

@@ +353,5 @@
>  
>      return NS_OK;
>    }
>  
> +  else if (!strcmp(aTopic, AUDIO_CHANNEL_PROCESS_CHANGED)) {

Could you help to move this code section prior to MOZ_SETTINGS_CHANGE_ID?
Just keep longest code section in the later.

Thanks for your help.

@@ +451,5 @@
>    }
>    if (NS_FAILED(obs->AddObserver(this, MOZ_SETTINGS_CHANGE_ID, false))) {
>      NS_WARNING("Failed to add mozsettings-changed observer!");
>    }
> +  if (NS_FAILED(obs->AddObserver(this, AUDIO_CHANNEL_PROCESS_CHANGED, false))) {

And don't forget to remve observer in de-constructor.

Comment 19

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

(In reply to Marco Chen [:mchen] from comment #18)
> Comment on attachment 8426244 [details] [diff] [review]
> WIP
> 
> Review of attachment 8426244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks. And it looks good to me.

Thanks for your feedback. I think the patch attached here is ready for the review. I pushed it to try server, see results at https://tbpl.mozilla.org/?tree=Try&rev=3648d863e21e please.

Comment 20

[Marco Chen [:mchen]]

Comment on attachment 8426990 [details] [diff] [review]
v1

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

It looks good to me. Thanks.
The only nit is a space only and need to have comment for HandleAudioChannelProcessChanged().

The first concern is the naming of audio-channel-XXX but it is not produced by this bug.
So I create a new one for it - https://bugzilla.mozilla.org/show_bug.cgi?id=1016918.

The second concer is there will be a transition time between PHONE_STATE_IN_NORMAL and PHONE_STATE_IN_COMMUNICATION.
The use case is what I raised as comment below.

::: dom/system/gonk/AudioManager.cpp
@@ +281,5 @@
>  #endif
>  }
>  
> +void
> +AudioManager::HandleAudioChannelProcessChanged()

User Scanrio:
  1. Take a phone call. (set to PHONE_STATE_IN_CALL)
  2. Take a VOIP call. (return directly because mPhoneState is PHONE_STATE_IN_CALL)
  3. hang up phone call. (set to PHONE_STATE_NORMAL)

If a developer looks into this call and think about the scanrio above,
he would think that phone state will be remained to PHONE_STATE_NORMAL made by AudioManager::SetPhoneState from RIL.

So it would be better to comment out why this is not an issue here.

@@ +283,5 @@
>  
> +void
> +AudioManager::HandleAudioChannelProcessChanged()
> +{
> +  if ((mPhoneState == PHONE_STATE_IN_CALL) || 

remove space.

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2

(In reply to Marco Chen [:mchen] from comment #20)
> Comment on attachment 8426990 [details] [diff] [review]
> v1
> 
> Review of attachment 8426990 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review!

> It looks good to me. Thanks.
> The only nit is a space only and need to have comment for
> HandleAudioChannelProcessChanged().

Review comments above addressed.

Comment 22

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v3

Update comment as we discuss on IRC.

Comment 23

[Marco Chen [:mchen]]

Comment on attachment 8430105 [details] [diff] [review]
v3

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

It looks good to me. Thanks for addressing the comment.

Comment 24

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Marco Chen [:mchen] from comment #23)
> Comment on attachment 8430105 [details] [diff] [review]
> v3
> 
> Review of attachment 8430105 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good to me. Thanks for addressing the comment.

Thanks for your reviews and for your help.

https://hg.mozilla.org/integration/b2g-inbound/rev/801fe17e5076

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/801fe17e5076