1079974 - [Loop] Ringer should be played through the main speaker

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Borja Salguero [:borjasalguero]]

Sometimes is not working as expected, so we need to force to route properly the sound.

Comment 1

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Comment 2

[Mark Banner (:standard8)]

Please file bugs about the FxOS client under the Firefox OS product in the Gaia:Loop component. Thanks.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8501857 [details] [review]
Pull Request

I'm not able to reproduce the issue. The ringer audio channel is supposed to play the ringtone through the speaker (*always*) and if this is not happening we have a gecko bug around. The patch tries to fix the issue through the MozSpeakerManager API which forces to route the audio through the speaker.
Actually it forces to route when the phone audio mode is IN_CALL or IN_COMMUNICATION which happens *only* when we are playing through the telephony audio channel (not the ringer one).

Massimo, are you able to reproduce this issue? Thanks!

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

Had a chat with Andrea [:baku] about this issue yesterday. The plan is to debug with his help a bit next Monday. The issue is not also happening in the Loop app but in the Callscreen app as well. I'll change the component then. Basically on GSM/CDMA calls the RIL plumbing plays the audio on the telephony channel through the audio manager. In parallel the Callscreen app plays silence through Web Audio on the telephony audio channel as well. There are chances in which the ringer tone of the next GSM/CDMA call plays on the built-in earpiece instead of playing it on the speaker. That happens in the Loop app as well so after a WebRTC call there are chances in which the ringer tone of the next WebRTC call plays on the built-in earpiece. Apparently something's fishy here. Andrea pointed out to bug 1073615 but it doesn't fix the issue (not for me at least). I'll double-check today again.

I'll add to CC list some folks that might help and I left a ni? at Andrea as a reminder for the next Monday.

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

Right after device receives an incoming webrtc call the Loop app plays silence through Web Audio on the telephony audio channel. Note it happens even before playing the ringtone. So if we start playing silence on the telephony channel and after that the app plays the ringtone on the ringer channel in parallel the ringtone goes into the built-in earpiece. I dropped the code for playing silence and the ringer goes into the speaker. Sadly we cannot remove that code as we need it to set the GSM call (if any) on hold. So the ringer doesn't go into the speaker if we play audio con the telephony audio channel through Web Audio. Andrea, how is that possible?

Comment 6

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Flash 2.0 10/20 PVT build on flame
On ringing mode on loop application and try to run 
#adb shell dumpsys media.audio_policy
===
AudioPolicyManager Dump: 0xb8be7f28
 Primary Output: 2
 A2DP device address: 
 SCO device address: 
 USB audio ALSA 
 Output devices: 00000003
 Input devices: 000001c4
 Phone state: 0  <--right
===
Outputs dump:
- Output 2 dump:
 Sampling rate: 48000
 Format: 00000001
 Channels: 00000003
 Latency: 80
 Flags 00000002
 Devices 00000001   <-- NG, it should be 2 normally.(speaker)

So the ringer would come out from earpiece.
The device choose logic is in android HAL and output device shouldn't keep in earpiece.

Comment 7

[Andrea Marchesini [:baku]]

Do we force the phone state when we are in a loop call?

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Andrea Marchesini (:baku) from comment #7)
> Do we force the phone state when we are in a loop call?

Yes, we do. Somehow actually. On bug 1006380 we added a few changes in the audio channel service and in the audio manager logic. The goal was to allow app using the telephony audio channel to switch between the earpiece and the speaker by using the MozSpeakerManager API. The MozSpeakerManager API allows to switch between them when the audio phone state is PHONE_STATE_IN_CALL or PHONE_STATE_IN_COMMUNICATION. The change added on bug 1006380 was to set the audio phone state to PHONE_STATE_IN_COMMUNICATION when the app is playing audio through the telephony audio channel.

Comment 9

[Andrea Marchesini [:baku]]

I suspect that bug 1006380 can be the origin of this issue. Look at this piece of code:

https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/AudioManager.cpp#332

Here we check if we have an active telephony channel. But we have a webAudio context active when the ringer plays for the AudioChannel competing policy. So, that check will be always true and that'll always make the phone in PHONE_STATE_IN_COMMUNICATION state also when it's not.

Let me know if I'm wrong.
But, if this is correct I see 2 options:

1. we cannot use the telephony active state to change the PhoneState and we have to find a different approach.

2. we have to find a different solution for the audiochannel competing policy GSM vs WebRTC.

Comment 10

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Logs

Andrea, the logs you requested me. I added the |adb shell dumpsys media.audio_policy| command output I ran during the test.

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

It looks like when b2g creates track as telephony and keep it alive cause this problem.
I try to modify the cubeb_stream_type ConvertChannelToCubebType() to avoid convert to telephony stream type and it works well..

I did a compare test on nexus 5 with skype, the result is:
=====Ringing: 
 Phone state: 1  <--- MODE_RINGTONE,
----------
- Output 2 dump:
 Sampling rate: 48000
 Format: 00000001
 Channels: 00000003
 Latency: 50
 Flags 00000002
 Devices 00000002
 Stream volume refCount muteCount
 00     0.063     00       00
 01     0.032     00       00
 02     0.026     01       00 <---STREAM_TYPE=RING

==== connected.
Phone state: 3 <--- MODE_IN_COMMUNICATION,
Outputs dump:
- Output 2 dump:
 Sampling rate: 48000
 Format: 00000001
 Channels: 00000003
 Latency: 50
 Flags 00000002
 Devices 00000001
 Stream volume refCount muteCount
 00     1.000     01       00 <---STREAM_TYPE= VOICE
 01     0.084     00       00
 02     0.007     00       00

Comment 12

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

In AudioPolicyManagerBase.cpp, 
<https://android.googlesource.com/platform/hardware/libhardware_legacy/+/android-4.4.3_r1.1.0.1/audio/AudioPolicyManagerBase.cpp>

If the voice stream type is active, this module would prefer to select earpiece as the default output device.

ex: when stream voice call is active. (telephony), this function call would return STRATEGY_PHONE. 
  AudioPolicyManagerBase::routing_strategy AudioPolicyManagerBase::getStrategy()  (L2277)
   case AudioSystem::VOICE_CALL:
   case AudioSystem::BLUETOOTH_SCO:
    return STRATEGY_PHONE; then this function would like to choose earpiece.
  AudioPolicyManagerBase::getDeviceForStrategy() (L2315)
  ...
  would goto this case:default:    // FORCE_NONE case, then fail into this line. 
    device = mAvailableOutputDevices & AUDIO_DEVICE_OUT_EARPIECE;

So if possible, I think we should avoid to create the CUBEB_STREAM_TYPE_VOICE_CALL stream type if device has not entered the Voip usecase.

Comment 13

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Baku, 
Any ideas for that?

Comment 14

[Andrea Marchesini [:baku]]

(In reply to Randy Lin [:rlin] from comment #13)
> Hi Baku, 
> Any ideas for that?

Unfortunately yes. We create a AudioContext('telephony') also when the call is not active.
This is done to know when the app is muted by the AudioChannelService but it seems this introduces this issue.

What I would propose is:

can we revert the AudioContext('telephony') hack and introduce a new API just for this priority "loop vs gsm" issue?

I'm thinking about something extremely simple like;

[Constructor(Window window, USVString name), Exposed=Window]
interface TelephonyAgent : EventTarget {
  attribute EventHandler oninterruptbegin;
  attribute EventHandler oninterruptend;
};

If any 'telephony' app creates this object with a different name, this can be used to count them, and send events when the visibility of the window changes (if we have more than 1 agent active).

Of course when oninterruptbegin, the app has to interrupt the call, or put it in hold state.
(probably oninterruptbeginneeded would be a better name...)

This can be an approach. It would take a couple of days to be implemented.

Another approach can be to introduce mozinterruptbegin/end in the telephony API. But there are some issues, exposed by mchen in bug 1016277

Comment 15

[Andrea Marchesini [:baku]]

I had a call with Jonas about this issue. And here a better proposal:

At the moment we create a AudioContext('telephony') when the loop and gsm apps start. Instead doing this what about if:

1. we receive a GSM or loop call. The GSM or loop app goes in foreground and it just creates a MediaElement for the ringer using 'ringer' AudioChannel. We use onmozinterruptbegin/end on this ringer mediaElement to know if we had to drop the call.
 
2. The user answers the call, and we create a AudioContext('telephony') for the audio competing policy.
We use onmozinterruptbegin/end to know if we have to put the call in hold.

3. At this point we are still in a call. But we receive another call using a different protocol (if we were in loop, now it's a GSM or viceversa). The other app goes in foreground and it creates a MediaElement for the ringer, using the 'ringer' audio Channel. - At this point the ringer has to be 'softer' and this can be done using some existing API, or with some new API. We need to know if this API already exists... probably it doesn't.

4. The other call is still active because we didn't create any 'telephony' audio channel, so this is still the highest active audioChannel.

5. the user answers the new call. The app creates a AudioContext('telephony') and this will mute the other app (the active one). Then the previous call is set to hold and the new call will be active one.

I think that the removing of AudioContext('telephony') when the call is not actually active will fix this issue.
But I want to know if this is doable and I have covered all the possible corner-cases.

Comment 16

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Andrea Marchesini (:baku) from comment #15)
> I had a call with Jonas about this issue. And here a better proposal:

Thank you guys for providing the output of that call here.

> At the moment we create a AudioContext('telephony') when the loop and gsm
> apps start. 

We only do that for the Loop app. The callscreen one doesn't create it until the GSM call is connected. Actually we added this hack recently due a issue we had for the Loop app (see [1] please).

> Instead doing this what about if:

What you guys propose bellow is actually what we have implemented more or less.

> 1. we receive a GSM or loop call. The GSM or loop app goes in foreground and
> it just creates a MediaElement for the ringer using 'ringer' AudioChannel.
> We use onmozinterruptbegin/end on this ringer mediaElement to know if we had
> to drop the call.
>  
> 2. The user answers the call, and we create a AudioContext('telephony') for
> the audio competing policy.
> We use onmozinterruptbegin/end to know if we have to put the call in hold.
> 
> 3. At this point we are still in a call. But we receive another call using a
> different protocol (if we were in loop, now it's a GSM or viceversa). The
> other app goes in foreground and it creates a MediaElement for the ringer,
> using the 'ringer' audio Channel. - At this point the ringer has to be
> 'softer' and this can be done using some existing API, or with some new API.
> We need to know if this API already exists... probably it doesn't.
> 
> 4. The other call is still active because we didn't create any 'telephony'
> audio channel, so this is still the highest active audioChannel.
> 
> 5. the user answers the new call. The app creates a
> AudioContext('telephony') and this will mute the other app (the active one).
> Then the previous call is set to hold and the new call will be active one.
> 
> I think that the removing of AudioContext('telephony') when the call is not
> actually active will fix this issue.

Yes, I thought I'd shared this findings with you. The issue gets fixed. Read bellow please.

> But I want to know if this is doable and I have covered all the possible
> corner-cases.

It's doable for sure but we need to find a fix for the issue we reported on bug 1069793. On bug 1069793 the issue was basically a race condition. We needed a way to force setting the GSM call on hold as soon as possible because the gUM request we made for showing a local video preview was always failing. I really prefer to revert the patch from bug 1069793 and try to find a different hack for it. Hacking on the FxOS audio plumbing seems more complicated.

Lets meet tomorrow and discuss about it. Thanks!
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1069793#c12

Comment 17

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

Do you really want to put the GSM call on hold automatically as soon as there's an incoming loop call? Wouldn't we want the user to have to answer the loop call before the GSM call is put on hold. As a user it seems like I'd want to be able to keep talking on the GSM call and just ignore the loop call if I choose to.

That's also more similar to what happens if I'm on a GSM call and there's another incoming GSM call. All I hear is a small "do-do-do" sound, which I can ignore if my current call is more important.

Comment 18

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Jonas Sicking (:sicking) from comment #17)
> Do you really want to put the GSM call on hold automatically as soon as
> there's an incoming loop call?

No we don't but sadly we rely on mozinterrupt{begin, end} events. Let me explain this. While the GSM call is connected the Loop app listens for them. On a incoming WebRTC call the Loop app plays the ringtone. As the priority of the ringer channel is higher than the telephony one the events are fired and the GSM call is set on hold. The channel of the audio interrupting is not known so there is no way to differentiate if we really need/want to set the GSM call on hold when the ringtone plays.

Comment 19

[Andrea Marchesini [:baku]]

Jonas, any particular reason why 'ringer' is higher than 'telephony'?
If 'ringer' is lower than telephony, we can have the loop app ringing, while a GSM call is ongoing.
We just need to inform the app that we have an ongoing call to play volume at a low level (the famous "doo-doo-doo" issue).

Comment 20

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Pointer to Github PR https://github.com/mozilla-b2g/firefoxos-loop-client/pull/242

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8511519 [details] [review]
Pointer to Github PR https://github.com/mozilla-b2g/firefoxos-loop-client/pull/242

Cristian, this PR is about re-ordering a bit the calls to the audio competing helper. Lets pair up for the review when you have time. Thanks!

Comment 22

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8511519 [details] [review]
Pointer to Github PR https://github.com/mozilla-b2g/firefoxos-loop-client/pull/242

LGTM Jose, I left a couple of comments but minor things, thanks

Comment 23

[José Antonio Olivera Ortega [:jaoo]]

Landed on master branch at:

https://github.com/mozilla-b2g/firefoxos-loop-client/commit/1b6a1f04ea428271b4a6b6afa06eab79a1102cc7

Comment 24

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

I always thought that telephony was higher than ringer, so no, i have no idea why it isn't.

That said, I'm totally fine with adding a TelephonyAgent API. I could see something like that being used in order to play the "doo doo doo" sound.

Comment 25

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Jonas Sicking (:sicking) from comment #24)

> That said, I'm totally fine with adding a TelephonyAgent API. I could see
> something like that being used in order to play the "doo doo doo" sound.

When we defined the telephony audio channel usage policy we were in a hurry because we needed it for v2.0 release. IMHO the current policy needs some improvements so we should discuss about these improvements and how to support them. I'll file a bug for that. Thanks!

Comment 26

[Andrea Marchesini [:baku]]

(In reply to Jonas Sicking (:sicking) from comment #24)
> I always thought that telephony was higher than ringer, so no, i have no
> idea why it isn't.

We don't need the telephony Agent, and jaoo just fixed this bug in gaia.
But yes, we should file a bug to have ringer < telephony and discuss  the implementation of this (small - in gecko) change. Jonas, can you file the bug and CC anyone who should know about this change?

For this 'doo doo doo' sound, instead implementing a Telephony Agent, I would like to extend  AudioChannelManager adding this:

readonly attribute telephonyChannelActive;
attribute EventHandler ontelephonyChannelChanged;

This should be enough to select the correct ringer sound. But not for v2.0 I guess.

Comment 27

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Andrea Marchesini (:baku) from comment #26)

> For this 'doo doo doo' sound, instead implementing a Telephony Agent, I
> would like to extend  AudioChannelManager adding this:
> 
> readonly attribute telephonyChannelActive;
> attribute EventHandler ontelephonyChannelChanged;
> 
> This should be enough to select the correct ringer sound. But not for v2.0 I
> guess.

Another approach could be to pass the audio channel interrupting the current audio channel in use on the onmozinterruptbegin event. That way once the ringer channel interrupts the telephony one we Loop/callscreen app could check whether either it should play the 'do-do-do' tone or set the WebRTC/GSM call on hold. Right now only the system app knows about the current audio channel in use.

Comment 28

[Maria Angeles Oteo (:oteo)]

After some testing I've not been able to reproduce the bug, and as this is a blocker issue for 1.1 Loop mobile version, let's land it in 1.1 branch.

Jose Antonio, could you please do the uplifting?
Thanks a lot!

Comment 29

[José Antonio Olivera Ortega [:jaoo]]

Landed on 1.1 branch at:

https://github.com/mozilla-b2g/firefoxos-loop-client/commit/f16828c0e14f2e7fe008af7737e9f51ca2397baa

Comment 30

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

(In reply to Andrea Marchesini (:baku) from comment #26)
> But yes, we should file a bug to have ringer < telephony and discuss the
> implementation of this (small - in gecko) change. Jonas, can you file the
> bug and CC anyone who should know about this change?

Bug 1089742