1016277 - If two different apps try to use the 'telephony' channel at the same time both apps can play audio.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[José Antonio Olivera Ortega [:jaoo]]

STR

1.- Launch a WebRTC app for audio/video calls. Note this app uses the telephony channel for its video/audio elements.
2.- Answer any incoming call.

RESULT

The device plays audio from both calling parties the WebRTC one and the telephony one. Audio from both calls gets mixed.

EXPECTED RESULT

If two different apps try to use the 'telephony' channel at the same time only one them can play audio. The audio coming from the app in background must be paused.

Note: I'll be happy to share the WebRTC app used in the test. This app uses the telephony channel for the video/audio elements.

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

Marco, I noticed this while testing the WebRTC app we are developing in the context of the Loop project. Could you have a look a it a confirm what I noticed please? Thanks!

Comment 2

[Marco Chen [:mchen]]

I think we need UX input first to give the expected scenario.
Because telephony voice call is most important service on phone, I am not sure it is allowed to be muted by OS in any cases.

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Marco Chen [:mchen] from comment #2)

Thanks for your feedback Marco.

> I think we need UX input first to give the expected scenario.
> Because telephony voice call is most important service on phone, I am not
> sure it is allowed to be muted by OS in any cases.

Some background for UX team as we need its feedback. The FxOS Loop (WebRTC audio/video calls) app will be added for 2.0 release. While testing we have noticed that if there is a new incoming call (a telephony one) and the user answers it during the WebRTC call the device plays audio from both calling parties the WebRTC one and the telephony one. Audio from both calls gets mixed. Indeed this is an issue that needs to be fixed. We would need some input from you guys about how both apps dialer and Loop should behave. Should the telephony call pause the audio from the WebRTC one?

Comment 4

[Stephany Wilkes]

Flagging Maria on Loop UX, Carrie on Dialer/Comms generally, and Darrin on Loop.

Comment 5

[Darrin Henein [:darrin]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #3)
> input from you guys about how both apps dialer and Loop should behave.
> Should the telephony call pause the audio from the WebRTC one?

I believe this was the plan... cellular calls should take priority and place the WebRTC call 'on hold', until the cellular call is ended. We are planning on the desktop side to indicate to users if their calling party goes 'on hold' for this very scenario.

Comment 6

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

We need a UX decision here.

But generally speaking, interfering with a "normal" phone call seems scary. There might be regulatory concerns here too. What if the user is on a 911 call?

To me it seems sensible to tie it to "which app is visible" if multiple apps are using telephony audio channel. But we'd have to make sure that no other app could affect the visibility of the telephony attention screen. And we'd have to consider scenarios like:

1. User receives normal phone call
2. User "picks up" to answer call from step 1
3. While still on call, user switches from dialer attention screen to browser app
4. A loop call comes in

At this point, can loop (or other apps with the telephony-audio permission) interfere with the telephony audio from the normal phone call?

So definitely need to decide on a policy here.

Comment 7

[Carrie Wang [New account: [e-mail]]]

I'd suggest Dialer should always have highest priority on playing audio, no matter the incoming call came first or the loop call set first. In addition, we shall provide the information to users that the loop call is now being ""on hold. Thanks.

Comment 8

[Rafael Rebolleda]

I disagree with telephony voice being the most important service on a phone. I doubt that is the case for smartphone users.

If feasible, no call should take over any other call, no matter the service. The user should be notified of the incoming call and she'd have to chose whether to take it or not. Is she's talking to 911, just don't pick it up!

Nice services will be aware of this issue, and will handle being put on hold. Like Darrin mentioned, the last approach that I'm aware of for loop is that clients send that state to the other party so they can display "on hold".

In any case, this is tightly related with bug 988212, regarding attention screen management.

Comment 9

[Darrin Henein [:darrin]]

(In reply to Rafael Rebolleda [:rafaelrebolleda] from comment #8)
> If feasible, no call should take over any other call, no matter the service.
> The user should be notified of the incoming call and she'd have to chose
> whether to take it or not.

Sorry, to clarify... what Rafa proposes is what I meant. I was assuming the cellular call would prompt a dialer attention screen and when accepted, would place the loop call on hold (vs terminating it).

Comment 10

[Borja Salguero [:borjasalguero]]

Hi Andrea,
After talking with you and Vivien through IRC, we were talking about the possibility of changing the Audio policy so only *one* app could access to the resources of the telephony channel, and probably send an event to the rest of the apps trying to get the same resource letting them know that the resource is occupied by other app.

This would let us to 'hold' a phone call if a 'WebRTC Call' is answered while talking through a regular call, improving the User Experience.
Furthermore, this would avoid some issues with privacy, not mixing channels and conversations.

I think this is definitely an issue to fix for v.2.0, due to we are going to have 2 calls at the same time as a common scenario.

Do you think is feasible to create a patch for v2.0? We need to ensure, at least, that 2 conversations are not happening at the same time in our device.

Thanks!

Comment 11

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

Ok, so apparently people have been discussing using similar policies for the "telephony" channel as for the "content" channel. I.e. that if multiple apps try to use the "telephony" channel we'll only let the most recently visible one play audio. Any background ones are "paused". We could then fire onmozinterruptbegin and onmozinterruptend events on the telephony API when it is being "paused" the same way that we do for <audio> and <video> elements.

This sounds like a good idea to me, though there are a few problems that we need to address.

* How to "pause" telephony?

How do we "pause" telephony? Do we simply fire the onmozinterruptbegin event and then leave it up to application logic to put the call on hold? The downside with that is that once the call is on hold, it is no longer using the "telephony" channel. Will that mean that onmozinterruptend will fire immediately rather than when the loop call is hung up or the user switches back to the dialer?

One way to solve this would be to have the dialer app not only put the call on hold, but also do something like call <audio src="silent.ogg" loop mozaudiochannel=telephony onmozinterruptend>.play(). This way the dialer app will still compete for the telephony channel and will be notified when the channel is available again.

Another option is to not rely on application logic, and instead let the telephony API implementation put the call on hold automatically in order to pause the telephony audio. This way the API knows that we put the call on hold due to the audio policies and so it can still indicate to the audio manager that it's competing for the "telephony" audio channel. That way we will be notified when we can start playing audio again and can automatically resume the call. But creates some complexities in the telephony API as exposed to the app. Do we fire the normal "onhold" events? What happens if the app calls resume()?

I'm fine with any of the solutions here. As long as we test and make sure that things work as we expect. If we go with the last solution we might need to involve the RIL team. At least for reviews.

* How to "pause" webrtc microphone?

Say that the user is on a Loop call, and there's an incoming "normal" GSM call. The user answers the GSM call. Now we should "pause" the loop call.

However this doesn't just mean to pause any audio that the loop call is making. We should put the whole loop call on hold. This means that we should also stop listening to the microphone.

One solution to this is to simply have the loop app automatically microphone mute whenever it receives onmozinterruptbegin and then re-enable the microphone when it gets onmozinterruptend again.

This might be good enough for the 2.0 release.

However ideally we should have microphone policies similar to the audio policies. So that if multiple apps tries to use the microphone only the most recently visible one is actually getting access to it. This way the app that is getting chosen for "telephony" audio will also get chosen for microphone access. This sounds like something we should file a followup bug on.

* Verify that onmozinterruptbegin/onmozinterruptend works when using WebRTC

We should verify that when we play webrtc audio through an <audio> or video element, that the onmozinterruptbegin and onmozinterruptend events work as they should. Especially if we are relying on those events to solve any of the problems above.

Comment 12

[Maria Angeles Oteo (:oteo)]

Borja, regarding Jonas's comments about 

1) How to "pause" webrtc microphone?
2) "Verify that onmozinterruptbegin/onmozinterruptend works when using WebRTC

could you test if it works how Jonas suggests?

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jonas Sicking (:sicking) from comment #11)
> Ok, so apparently people have been discussing using similar policies for the
> "telephony" channel as for the "content" channel. I.e. that if multiple apps
> try to use the "telephony" channel we'll only let the most recently visible
> one play audio. Any background ones are "paused". We could then fire
> onmozinterruptbegin and onmozinterruptend events on the telephony API when
> it is being "paused" the same way that we do for <audio> and <video>
> elements.
> 
> This sounds like a good idea to me, though there are a few problems that we
> need to address.
> 
> * How to "pause" telephony?
> 
> How do we "pause" telephony? Do we simply fire the onmozinterruptbegin event
> and then leave it up to application logic to put the call on hold? The
> downside with that is that once the call is on hold, it is no longer using
> the "telephony" channel. Will that mean that onmozinterruptend will fire
> immediately rather than when the loop call is hung up or the user switches
> back to the dialer?
> 
> One way to solve this would be to have the dialer app not only put the call
> on hold, but also do something like call <audio src="silent.ogg" loop
> mozaudiochannel=telephony onmozinterruptend>.play(). This way the dialer app
> will still compete for the telephony channel and will be notified when the
> channel is available again.
> 
> Another option is to not rely on application logic, and instead let the
> telephony API implementation put the call on hold automatically in order to
> pause the telephony audio. This way the API knows that we put the call on
> hold due to the audio policies and so it can still indicate to the audio
> manager that it's competing for the "telephony" audio channel. That way we
> will be notified when we can start playing audio again and can automatically
> resume the call. But creates some complexities in the telephony API as
> exposed to the app. Do we fire the normal "onhold" events? What happens if
> the app calls resume()?
> 
> I'm fine with any of the solutions here. As long as we test and make sure
> that things work as we expect. If we go with the last solution we might need
> to involve the RIL team. At least for reviews.
> 
> * How to "pause" webrtc microphone?
> 
> Say that the user is on a Loop call, and there's an incoming "normal" GSM
> call. The user answers the GSM call. Now we should "pause" the loop call.
> 
> However this doesn't just mean to pause any audio that the loop call is
> making. We should put the whole loop call on hold. This means that we should
> also stop listening to the microphone.
> 
> One solution to this is to simply have the loop app automatically microphone
> mute whenever it receives onmozinterruptbegin and then re-enable the
> microphone when it gets onmozinterruptend again.
> 
> This might be good enough for the 2.0 release.
> 
> However ideally we should have microphone policies similar to the audio
> policies. So that if multiple apps tries to use the microphone only the most
> recently visible one is actually getting access to it. This way the app that
> is getting chosen for "telephony" audio will also get chosen for microphone
> access. This sounds like something we should file a followup bug on.
> 
> * Verify that onmozinterruptbegin/onmozinterruptend works when using WebRTC
> 
> We should verify that when we play webrtc audio through an <audio> or video
> element, that the onmozinterruptbegin and onmozinterruptend events work as
> they should. Especially if we are relying on those events to solve any of
> the problems above.

One other problem is:

* How to "resume" telephony.

 For example if you are on a phone call and the current loop call is on "hold". You may want to switch to the loop call and click on a button to "resume" the call. Then the WebRTC autio channel should unmute and the phone call audio channel should receive onmozinterruptbegin/onmozinterruptend.

 I was wondering if <audio>.play() can not unmute the telephony channel if it has been muted by an other telephony channel ?
This differs a bit from the regular policy but would let the app programmatically unmute the channel with a "resume" button, and let users switch between the 2 types of call.

Comment 14

[Omega Feng [:Omega] [:馮於懋]]

From UX side, we think it's better to let the foreground one to have the priority to use telephony channel, and mute the background one.
Notice that when it's in a phone call, and a WebRTC call is incoming, the WebRTC's attention screen will overlay the in-call screen, but the phone call still the telephony channel until WebRTC call is answered.

Comment 15

[Andrea Marchesini [:baku]]

> Do you think is feasible to create a patch for v2.0? We need to ensure, at
> least, that 2 conversations are not happening at the same time in our device.

Yes. This is something we can do. It will work for the audio channel, but not for the microphone.
I'll start working on this this week.

Comment 16

[Borja Salguero [:borjasalguero]]

After testing Vivien's patch and playing with 'telephony' audio channel and how to deal with several attention screens using it at the same time, we have defined the following 2 scenarios which must be working as expected:

SCENARIO 1
1/ There is an ongoing Loop call and a regular call is received.
2/ The Attention Screen for Dialer is shown on the top.
 2A/ If user rejects the call, everything continue as before
 2B/ If user accepts the regular call:
    Loop receives the onmozinterruptbegin event and puts the call on-hold. Telephony channel and microphone are granted to the dialer. Loop is going to stop publishing the audio stream and stop playing the incoming audio stream as well. Loop is *not* going to have the option to resume the call manually.
        The only option to resume the call is finishing the Dialer one, this will fire a onmozinterruptend event which means Loop can use the channel.

SCENARIO 2:
1/ There is an ongoing regular call and Loop call is received.
2/ The Attention Screen for Loop is shown on the top. 
 2A/ If user rejects the call, everything continue as before
 2B/ If user accepts the Loop call.
     Dialer receives the onmozinterruptbegin or equivalent event through 'telephony' and puts the call on-hold. Telephony channel is granted to the Loop app. 
     2B-I)  If user finishes the Loop call and wants to continue with the dialer one, he needs to go to the dialer and resume the call manually. If an equivalent event of 'onmozinterruptend' is available on the Telephony API, we could use it to resume automatically the call.
     2B-II) If user resumes the dialer call without finishing the Loop one, we will be in scenario 1.


What is required to make this happen?

1/ Two different apps using the telephony channel (through attention screen or not) can *not* play audio simultaneously.

2/ Fire the 'onmozinterruptbegin' and 'onmozinterruptend' events on <audio>/<video> elements (using the telephony audio channel) and on the telephony API. That way both Loop and dialer apps could be notified about other app using the telephony audio channel. Right not this is not happening, (e.g.) if there is a ongoing loop call and the user answers a regular call the <audio> or <video> loop elements are *not* notified with 'onmozinterruptbegin' event when the dialer app starts playing audio through the telephony audio channel.

3/ Modify the attention screen patch that Vivien has drafted to listen to the onmozinterruptbegin event to put the call on-hold in that case (and no when visibility is lost)

4/ Ensure that the resources are not shared in any case for more than one app using telephony channel. This means that microphone must be used for one app at a time.


How could we fix these bugs and merge all the changes needed?

1/ Merge https://bugzilla.mozilla.org/show_bug.cgi?id=988212, so attention screen would be enabled for privileged apps

2/ Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1016277 in order to fire the 'onmozinterruptbegin' and 'onmozinterruptend' events properly, and ensure the 'audio-channel' resource exclusivity.

3/ Open a follow up for  https://bugzilla.mozilla.org/show_bug.cgi?id=988212 and change 'visibility' to ''onmozinterruptbegin' for handling the hold of a regular call once 1016277 lands.


Wdyt?

Comment 17

[Marco Chen [:mchen]]

(In reply to Jonas Sicking (:sicking) from comment #11)
> Ok, so apparently people have been discussing using similar policies for the
> "telephony" channel as for the "content" channel. I.e. that if multiple apps
> try to use the "telephony" channel we'll only let the most recently visible
> one play audio. Any background ones are "paused". We could then fire
> onmozinterruptbegin and onmozinterruptend events on the telephony API when
> it is being "paused" the same way that we do for <audio> and <video>
> elements.
> 

In general, I don't think this is a good approach but in order to meet timeline of 2.0 maybe it is a choice.

Concern: Is it enough for audio competing policy depend on "Visibility"?

Consider to SCENARIO 1 on comment 16. And assume user goes through "1->2->2B".
Now user presses home key then call Loop client back to foreground. What will be happend?
Loop client will receive "onmozinterruptend" event because it's visibility is true.
Then Loop client restart to work without any user interaction (ex: UX expects user press a button to resume the VOIP call). And GSM call is muted immediately too.

Comment 18

[Jason Smith [:jsmith]]

This really belongs under the feature-b2g flag, not the blocking flag. Mainly because this is a core part of the feature work to get telephony to operate under the privileged permission.

Shell - Can you mark this with feature-b2g: 2.0?

Comment 19

[Maria Angeles Oteo (:oteo)]

(In reply to Jason Smith [:jsmith] from comment #18)
> This really belongs under the feature-b2g flag, not the blocking flag.
> Mainly because this is a core part of the feature work to get telephony to
> operate under the privileged permission.

Agree, seeting the flag

Comment 20

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

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #13)
> One other problem is:
> 
> * How to "resume" telephony.
> 
>  For example if you are on a phone call and the current loop call is on
> "hold". You may want to switch to the loop call and click on a button to
> "resume" the call. Then the WebRTC autio channel should unmute and the phone
> call audio channel should receive onmozinterruptbegin/onmozinterruptend.

The audio will be unmuted automatically, and we will fire onmozinterruptend, when the user switches to the loop app. The audio channel policies will take care of that.

But the loop app might need to manually un-microphone-mute when it gets onmozinterruptend.

Comment 21

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

(In reply to Borja Salguero (this week part-time in Gaia) [:borjasalguero] from comment #16)
> After testing Vivien's patch and playing with 'telephony' audio channel and
> how to deal with several attention screens using it at the same time, we
> have defined the following 2 scenarios which must be working as expected:
> 
> SCENARIO 1

This scenario is relatively easy to get to work. We just need to change the audio channel manager to use a similar policy for "telephony" channel as we do for "content" channel.

Sounds like Andrea is working on this part.

> SCENARIO 2:

This is one is trickier and needs one of the solutions for "How to "pause" telephony?" as described in comment 11.

> How could we fix these bugs and merge all the changes needed?
> 
> 1/ Merge https://bugzilla.mozilla.org/show_bug.cgi?id=988212, so attention
> screen would be enabled for privileged apps

Yes. We should definitely do this.

> 2/ Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1016277 in order to fire
> the 'onmozinterruptbegin' and 'onmozinterruptend' events properly, and
> ensure the 'audio-channel' resource exclusivity.
> 
> 3/ Open a follow up for  https://bugzilla.mozilla.org/show_bug.cgi?id=988212
> and change 'visibility' to ''onmozinterruptbegin' for handling the hold of a
> regular call once 1016277 lands.

Note that it's more complicated than this. Please see the "How to "pause" telephony?" section of comment 11.


Another way to solve the "How to "pause" telephony?" issue is to let the dialer app start playing a dummy
<audio src="silent.ogg" loop mozaudiochannel=telephony> whenever a call is in progress. This way the dialer app will get notified through onmozinterruptbegin on that element if the user answers a loop call. The dialer app can then put the call on hold.

This is much simpler from a platform level than adding onmozinterruptbegin to the telephony API. But it might be a big waste of CPU since we'll constantly decode the silent ogg file any time the user is on a call.


Really we need feedback from the RIL team and from baku which of the solutions to "How to "pause" telephony?" from comment 11 (or the solution above) that we can get done in the 2.0 timeframe.

Comment 22

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

Ok, Daniel pointed out an additional solution to the "How to "pause" telephony?" problem. So let me re-summarize the options:


A) Make the platform fire the onmozinterruptbegin somewhere on the telephony API event and then leave it up to application logic to put the call on hold.

Once the call is on hold it will no longer occupy the telephony channel, and so the dialer app won't get notified if the loop call is ended.

When the user switches to the dialer app the user will have to press the "resume" button in order to resume the call. Once the call is resumed the dialer app will be in the foreground and will occupy the "telephony" channel, which means that the loop app will be paused.

B) Like A, but we use the visibility event to automatically resume the call.

C) Like A, but we make Gecko treat a call on hold as still occupying the telephony channel. That means that the dialer app will receive onmozinterruptend once the loop call is hung up, or once the user switches to the dialer app. The dialer app can then automatically resume the call.

Though the dialer app should make sure to only automatically resume the call if it was automatically put on hold as a result of a onmozinterruptbegin event.

D) Any time a phone call is happening, start a <audio src="silent.ogg" loop mozaudiochannel=telephony>.

This way gecko will fire onmozinterruptbegin and onmozinterruptend on that <audio> if another app uses the telephony channel.

We can use these events to put the call on hold and to resume it automatically as appropriate.

E) Don't reply on application logic at all. Instead let the telephony API implementation put the call on hold automatically in order to pause the telephony audio. This way the API knows that we put the call on hold due to the audio policies and so it can still indicate to the audio manager that it's competing for the "telephony" audio channel. That way we will be notified when we can start playing audio again and can automatically resume the call. But creates some complexities in the telephony API as exposed to the app. Do we fire the normal "onhold" events? What happens if the app calls resume()?


I'm fine with any of the solutions here. As long as we test and make sure that things work as we expect. If we go with E, we need to involve the RIL team. At least for reviews.

I'll leave it to baku which solution he feels that he has time to do for 2.0.

Comment 23

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

(In reply to Marco Chen [:mchen] from comment #17)
> Concern: Is it enough for audio competing policy depend on "Visibility"?

Per comment 14, that sounds like what the UX team prefers. Seems good enough to me to at least try.

Comment 24

[Andrea Marchesini [:baku]]

> A) Make the platform fire the onmozinterruptbegin somewhere on the telephony
> API event and then leave it up to application logic to put the call on hold.

I like this option. But, this is mostly a UX issue.
What I would do for 2.0 is:

1. telephony channel, same behaviour of content channel: visible apps have priority.

2. second step can be to implement AudioChannelAgent in Telephony API so that this API receives mozinterruptbegin/mozinterruptend and it can propagate these events to the app.
Then the app can decide to put the call on hold or not.

If this second step requires to much time, we have the <audio src="silent.ogg" loop /> option or any WebAudio code using the telephony channel.

How does it sound?

Comment 25

[Daniel Coloma:dcoloma]

talked to :baku over IRC in order to discuss a minor change about the proposal he made in comment 24. It seems that the preferred approach is:

Step 1: 

 Update AudioChannel to avoid the current overlap: Telephony lifo in the Audio Channel API, ensuring only the last app can use telephony channel and progressing events to apps using <audio><video> element.

 Dialer app can implement the workaround Jonas described in comment 22 (D) as a temporary hack while step 2 is done.

 This seems to be feasible for 2.0 FL milestone.

Step 2:

 Plug-in the AudioChannel events to the Telephony API. When telephony API receives a mozinterruptbegin, it puts the call on hold, releasing the resources, and if a mozinterrumptend is received, the call might be resumed. We need to discuss this with the RIL team as Jonas suggested in comment 22 (E). If this is implemented in time we can remove the hack suggested for dialer in step 1.

Comment 26

[:shell escalante]

Adding that there's potential that the solution could have a late string change based on design.  I don't believe that's the plan right now though.

Comment 27

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Telephony Channel - LIFO

Marco, can you review this patch? Thanks!

Comment 28

[Andrea Marchesini [:baku]]

I need help to decide the correct policy for resuming/holding a call: unfortunately a call can be in several states:

  const unsigned short CALL_STATE_DIALING = 1;
  const unsigned short CALL_STATE_ALERTING = 2;
  const unsigned short CALL_STATE_CONNECTING = 3;
  const unsigned short CALL_STATE_CONNECTED = 4;
  const unsigned short CALL_STATE_HOLDING = 5;
  const unsigned short CALL_STATE_HELD = 6;
  const unsigned short CALL_STATE_RESUMING = 7;
  const unsigned short CALL_STATE_DISCONNECTING = 8;
  const unsigned short CALL_STATE_DISCONNECTED = 9;
  const unsigned short CALL_STATE_INCOMING = 10;

It means that we can receive a loop call when the call is "Connecting" or is "Disconnecting", or any state in between them. I would suggest this approach:

DIALING, ALERTING, CONNECTING, INCOMING -> Drop the call
CONNECTED, RESUMING -> Hold
HOLDING, HELD, DISCONNECTING, DISCONNECTED -> Nothing

How does it sound? This will be a separated patch.
The first patch will make Telephony API able to dispatch mozinterruptend/mozinterruptbegin.

Comment 29

[Andrea Marchesini [:baku]]

Attachment: patch 2 - Telephony API and mozinterruptbegin/end events

Comment 30

[José Antonio Olivera Ortega [:jaoo]]

Attachment: patch 2 - Alternative to the second step: play silence during the call

Rough patch that gives us an idea about the alternative to the second step Andrea commented about in comment 24. It still needs to be finish and would need some feedback from System/Dialer app peers.

Comment 31

[Andrea Marchesini [:baku]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #30)
> Created attachment 8435189 [details] [diff] [review]
> patch 2 - Alternative to the second step: play silence during the call

I wrote a patch for bug 1021084. When this lands, from a performance point of view, it's better to use WebAudio instead a silent.ogg file.

Comment 32

[Andrea Marchesini [:baku]]

Attachment: patch 1 - Telephony Channel - LIFO

Unfortunately the first implementation was "too" simple. In real life WebAudio can have multiple AudioDestinationNodes and each one can use the 'telephony' channel. This new patch keeps the number of instances per childID.

Comment 33

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #32)
> Created attachment 8435644 [details] [diff] [review]
> patch 1 - Telephony Channel - LIFO
> 

May I make sure what I see from the patch is exactly the same with what you want?

Consider the use case:
 1. Loop client accpeted a VOIP call.
 2. GSM call is accepted by dialer app 
   2-a silent.ogg is playing on telephony channel
   2-b telephony channel in Loop client is muted by gecko.
 3. User brings Loop client back to foreground.
   3-a GSM call is still on going.
   3-b Loop client is still muted by gecko.

And telephony channel in Loop client can't be unmuted later because dialer app is the last one registered telephony channel. This means that Loop client doesn't have a way to recover this VOIP call from on hold state.

Except that Loop client re-creates a audio/video element with telephony channel again.
(Is this really what we want? It seems to be really hacky.)

Comment 34

[Andrea Marchesini [:baku]]

> And telephony channel in Loop client can't be unmuted later because dialer
> app is the last one registered telephony channel. This means that Loop
> client doesn't have a way to recover this VOIP call from on hold state.

It seems to me that we don't want to use visibility for selecting which telephony has priority.
But, instead, we want to have a user interaction. I think this is the scenario:

1. loop client accepts a VOIP call
2. GSM receives a call
3. the user accepts the GSM call
4. GSM requires a telephony channel
5. loop receives mozinterruptbegin and it releases the resource (no telephony channel is used)
6. the user goes back to the loop app and presses some "resume call" button
7. a new telephony channel is created (or StartPlaying() in the AudioChannelAgent is called) and the loop call is restored
8. the GSM app receives mozinterruptbegin
9. Telephony API puts the GSM call on hold.

Comment 35

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

(In reply to Andrea Marchesini (:baku) from comment #34)
> It seems to me that we don't want to use visibility for selecting which
> telephony has priority.

I think using visibility is fine. That's the solution that was requested by the UX team.

Comment 36

[Daniel Coloma:dcoloma]

(In reply to Jonas Sicking (:sicking) from comment #35)
> (In reply to Andrea Marchesini (:baku) from comment #34)
> > It seems to me that we don't want to use visibility for selecting which
> > telephony has priority.
> 
> I think using visibility is fine. That's the solution that was requested by
> the UX team.

Humm, I think what UX suggested is not using visibility but allowing the user to decide, what comment 8 and 9 says are:

"... no call should take over any other call, no matter the service. The user should be notified of the incoming call and she'd have to chose whether to take it or not. Is she's talking to 911, just don't pick it up!"

Problem with using visibility is that an incoming call will put on hold the other call automatically, e.g. if I am talking to 911 over the Dialer, an incoming Loop call will pop up via attention screen and will put on hold the call to 911.

Comment 37

[Marco Chen [:mchen]]

(In reply to Daniel Coloma:dcoloma from comment #36)
> e.g. if I am talking to 911 over the Dialer, an
> incoming Loop call will pop up via attention screen and will put on hold the
> call to 911.

I think the attention screen of Loop call in this timing should be as ringer to notify user.
It should not contain telephony channel. Loop call should play telephony channel only if user confirms to accept this Loop call.

So the scenario here is not a problem.

Comment 38

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #34)
> 6. the user goes back to the loop app and presses some "resume call" button
> 7. a new telephony channel is created (or StartPlaying() in the
> AudioChannelAgent is called) and the loop call is restored
> 8. the GSM app receives mozinterruptbegin
> 9. Telephony API puts the GSM call on hold.

So each VOIP apps need to know this tricky RULE. Hmm...

Ok, I will continue to review based on this info first.
By the way, AudioManager (audiomanager.cpp) will create an audiochannelagent to represent ringer and telephony states. Not sure it will effect the LIFO policy or not.

Comment 39

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

(In reply to Daniel Coloma:dcoloma from comment #36)
> "... no call should take over any other call, no matter the service. The
> user should be notified of the incoming call and she'd have to chose whether
> to take it or not. Is she's talking to 911, just don't pick it up!"
> 
> Problem with using visibility is that an incoming call will put on hold the
> other call automatically, e.g. if I am talking to 911 over the Dialer, an
> incoming Loop call will pop up via attention screen and will put on hold the
> call to 911.

What I mean by "controlled by visibility" is that we'd only put a call on hold once there's another visible app that is trying to use the "telephony" channel.

So the alert screen from Loop for an incoming phone call would not put the call on hold because Loop would just use the "ringer" channel.

Not until the user answers the phone will the loop app be visible *and* use the "telephony" channel. So not until then will the GSM call be put on hold.


I think so far everyone is in agreement regarding behavior.

The question is what should happen if the user, while on the Loop call, switch to the dialer app. Should the mere fact that the dialer app is now visible be enough to switch to the GSM call? Or should the user have to manually press a "resume" button in order to mute the Loop call and resume the GSM call?

The same question of course applies in the other direction. If the user is on a GSM call and swith to the Loop app, should we automatically switch to the Loop call, or should the user have to press a "resume" button?

Comment 40

[Daniel Coloma:dcoloma]

(In reply to Jonas Sicking (:sicking) from comment #39)
> 
> I think so far everyone is in agreement regarding behavior.
>

Ops! sorry, I misunderstood you. Anyhow, that's great. 
 
> The question is what should happen if the user, while on the Loop call,
> switch to the dialer app. Should the mere fact that the dialer app is now
> visible be enough to switch to the GSM call? Or should the user have to
> manually press a "resume" button in order to mute the Loop call and resume
> the GSM call?
> 
> The same question of course applies in the other direction. If the user is
> on a GSM call and swith to the Loop app, should we automatically switch to
> the Loop call, or should the user have to press a "resume" button?

I tend to think users should be the ones taking decisions (i.e. they should press the resume button if they really want to re-activate the call). Furthermore, I think that simplify a bit the platform work for 2.0. In any case, asking for needinfo for Darrin and Rafa who were the ones describing targetted behaviour.

Comment 41

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Jonas Sicking (:sicking) from comment #22)
> Ok, Daniel pointed out an additional solution to the "How to "pause"
> telephony?" problem. So let me re-summarize the options:
> 
> 
> A) Make the platform fire the onmozinterruptbegin somewhere on the telephony
> API event and then leave it up to application logic to put the call on hold.
> 
> Once the call is on hold it will no longer occupy the telephony channel, and
> so the dialer app won't get notified if the loop call is ended.
> 
> When the user switches to the dialer app the user will have to press the
> "resume" button in order to resume the call. Once the call is resumed the
> dialer app will be in the foreground and will occupy the "telephony"
> channel, which means that the loop app will be paused.
> 
> B) Like A, but we use the visibility event to automatically resume the call.
> 
> C) Like A, but we make Gecko treat a call on hold as still occupying the
> telephony channel. That means that the dialer app will receive
> onmozinterruptend once the loop call is hung up, or once the user switches
> to the dialer app. The dialer app can then automatically resume the call.
> 
> Though the dialer app should make sure to only automatically resume the call
> if it was automatically put on hold as a result of a onmozinterruptbegin
> event.
> 
> D) Any time a phone call is happening, start a <audio src="silent.ogg" loop
> mozaudiochannel=telephony>.
> 
> This way gecko will fire onmozinterruptbegin and onmozinterruptend on that
> <audio> if another app uses the telephony channel.
> 
> We can use these events to put the call on hold and to resume it
> automatically as appropriate.
> 
> E) Don't reply on application logic at all. Instead let the telephony API
> implementation put the call on hold automatically in order to pause the
> telephony audio. This way the API knows that we put the call on hold due to
> the audio policies and so it can still indicate to the audio manager that
> it's competing for the "telephony" audio channel. That way we will be
> notified when we can start playing audio again and can automatically resume
> the call. But creates some complexities in the telephony API as exposed to
> the app. Do we fire the normal "onhold" events? What happens if the app
> calls resume()?
> 
> 
> I'm fine with any of the solutions here. As long as we test and make sure
> that things work as we expect. If we go with E, we need to involve the RIL
> team. At least for reviews.
> 

A big concern is there could be more than 1 cellular call, e.g. one connected and one held, or a more complicated one connected conference call and one held single call. If we put the connected one on hold, the other held one will be automatically resumed (due to gsm tech restriction). So, the telephony channel is still occupied. Then how should we handle this scenario? Enforce to release one call, and which one?


> I'll leave it to baku which solution he feels that he has time to do for 2.0.

Comment 42

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8435116 [details] [diff] [review]
patch 2 - Telephony API and mozinterruptbegin/end events

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

Though, according to comment 34, you are inclined to controlling the call-hold logic on gecko, I prefer letting gaia take care of the hold-call logic especially as we are going to dispatch |mozinterruptend| and |mozinterruptbegin| no matter how. It also looks better to me that no extra complexity and implicit behavior are exposed. 

Anyway, if you are going with the way you do, below is some suggestions for this patch and for the upcoming part - call-hold logic. Thank you.
1) As comment 41, we should consider multiple calls as well. I think we will need UX input here.
2) To automatically hold/hangup a call, be careful that not hold/hangup a single call several times. A better place to handle the logic seems TelephonyService.js. 
3) What if gaia resumes a call put on hold by gecko?

::: dom/telephony/Telephony.cpp
@@ +12,5 @@
>  #include "nsPIDOMWindow.h"
>  #include "nsIPermissionManager.h"
>  
> +#include "AudioChannelAgent.h"
> +#include "AudioChannelService.h"

No extra line here, please.

@@ +113,5 @@
>  };
>  
>  Telephony::Telephony(nsPIDOMWindow* aOwner)
>    : DOMEventTargetHelper(aOwner), mActiveCall(nullptr), mEnumerated(false)
> +  , mAudioChannelAgentPlaying(false)

nit: the style Telephony code uses is putting ',' in the end of the last line and aligning DOMEvent... and mAudioChannel...

@@ +515,5 @@
>        // statechange events is guaranteed: first on TelephonyCallGroup then on
>        // individual TelephonyCall objects.
>        bool fireEvent = !aIsConference;
>        modifiedCall->ChangeStateInternal(aCallState, fireEvent);
> +      CheckAudioChannelAgent();

We don't really need to do this here. ConferenceCallStateChanged() callback is called afterwards, let's check audio channel there.

@@ +719,5 @@
>    }
>  }
> +
> +void
> +Telephony::CheckAudioChannelAgent()

Please move it to be with other private functions.

@@ +729,5 @@
> +    activeCalls = true;
> +  }
> +
> +  for (uint32_t i = 0, len = mCalls.Length(); i < len && !activeCalls; ++i) {
> +    if (mCalls[i]->CallState() == nsITelephonyService::CALL_STATE_CONNECTED) {

This isn't right since PHONE_STATE_IN_CALL when there's a call in state of dialing, alerting, or, connected. You could simply do:

if (mActiveCall ||
    (mGroup &&
     mGroup->CallState() == nsITelephonyService::CALL_STATE_CONNECTED)) {
  activeCalls = true;
}

@@ +744,5 @@
> +    return;
> +  }
> +
> +  if (!mAudioChannelAgent) {
> +    mAudioChannelAgent = new AudioChannelAgent();

Please do:
mAudioChannelAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1");
MOZ_ASSERT(mPhoneAudioAgent);

@@ +749,5 @@
> +    mAudioChannelAgent->InitWithWeakCallback(
> +                                  GetOwner(),
> +                                  static_cast<int32_t>(AudioChannel::Telephony),
> +                                  this);
> +  }

Per my understanding we still need to call startPlaying() so that the agent is actually registered. Then use the return value of startPlaying() to update |mAudioChannelAgentPlaying| correctly.

@@ +751,5 @@
> +                                  static_cast<int32_t>(AudioChannel::Telephony),
> +                                  this);
> +  }
> +}
> +

Drop a comment here:
// nsIAudioChannelAgentCallback

@@ +763,5 @@
> +
> +  mAudioChannelAgentPlaying = canPlay;
> +
> +  return DispatchTrustedEvent(canPlay ? NS_LITERAL_STRING("mozinterruptend")
> +                                      : NS_LITERAL_STRING("mozinterruptbegin"));

Also need to revise dom/webidl/Telephony.webidl to add the new events.

::: dom/telephony/Telephony.h
@@ +119,5 @@
>    AddCall(TelephonyCall* aCall)
>    {
>      NS_ASSERTION(!mCalls.Contains(aCall), "Already know about this one!");
>      mCalls.AppendElement(aCall);
> +    CheckAudioChannelAgent();

Let's have CheckAudioChannelAgent() called in UpdateActiveCall() so that we don't need to have this everywhere.

@@ +129,5 @@
>    RemoveCall(TelephonyCall* aCall)
>    {
>      NS_ASSERTION(mCalls.Contains(aCall), "Didn't know about this one!");
>      mCalls.RemoveElement(aCall);
> +    CheckAudioChannelAgent();

ditto.

@@ +204,5 @@
>  
>    already_AddRefed<TelephonyCall>
>    GetCallFromEverywhere(uint32_t aServiceId, uint32_t aCallIndex);
> +
> +  void CheckAudioChannelAgent();

Have a consistent coding style, i.e.
void
CheckAudioChannelAgent();

@@ +207,5 @@
> +
> +  void CheckAudioChannelAgent();
> +
> +  nsCOMPtr<nsIAudioChannelAgent> mAudioChannelAgent;
> +  bool mAudioChannelAgentPlaying;

Move these two above to be with other private member variables.

Comment 43

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Andrea Marchesini (:baku) from comment #28)
> I need help to decide the correct policy for resuming/holding a call:
> unfortunately a call can be in several states:
> 
>   const unsigned short CALL_STATE_DIALING = 1;
>   const unsigned short CALL_STATE_ALERTING = 2;
>   const unsigned short CALL_STATE_CONNECTING = 3;
>   const unsigned short CALL_STATE_CONNECTED = 4;
>   const unsigned short CALL_STATE_HOLDING = 5;
>   const unsigned short CALL_STATE_HELD = 6;
>   const unsigned short CALL_STATE_RESUMING = 7;
>   const unsigned short CALL_STATE_DISCONNECTING = 8;
>   const unsigned short CALL_STATE_DISCONNECTED = 9;
>   const unsigned short CALL_STATE_INCOMING = 10;
> 
> It means that we can receive a loop call when the call is "Connecting" or is
> "Disconnecting", or any state in between them. I would suggest this approach:
> 
> DIALING, ALERTING, CONNECTING, INCOMING -> Drop the call

As incoming call is using 'ringer' channel, we don't need to do something special to it until the state changes to 'connected'.

Regarding 'connecting', strictly, when a call is in connecting, that means user just answers the incoming call but the operator hasn't connected it yet. At that time, the ringer channel is still in use, not telephony channel. It seems okay to leave it connecting. Once the call becomes connected, we hold it then.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #42)
> Comment on attachment 8435116 [details] [diff] [review]
> patch 2 - Telephony API and mozinterruptbegin/end events
> 
> Review of attachment 8435116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Though, according to comment 34, you are inclined to controlling the
> call-hold logic on gecko, I prefer letting gaia take care of the hold-call
> logic especially as we are going to dispatch |mozinterruptend| and
> |mozinterruptbegin| no matter how. It also looks better to me that no extra
> complexity and implicit behavior are exposed. 
> 
> Anyway, if you are going with the way you do, below is some suggestions for
> this patch and for the upcoming part - call-hold logic. Thank you.
> 1) As comment 41, we should consider multiple calls as well. I think we will
> need UX input here.

I just raised the issue to Omega and Jenny. They will be working on this. 

> 2) To automatically hold/hangup a call, be careful that not hold/hangup a
> single call several times. A better place to handle the logic seems
> TelephonyService.js. 

Hmmm... I re-thought about this. I'd like to suggest again handling the logic on gaia which looks a nicer place to control a specific ux policy.

That said, if we want to do it on TelephonyService, TelephonyService.js should listen to audio channel callback as well as Telephony.cpp. So, TelephonyService could put a certain call on hold at the right time. However, that makes more Telephony modules involved into this specific interactive behaviour and we sort of break the modularization. I'd like to avoid that as much as we can... Another option is handling the logic in Telephony.cpp. But there could be multiple Telephony instances within a single process, and Telephony API hasn't well supported the case of multiple users trying to hold the same call. We'd be careful that only one instance is taking the role. 

So, after reviewing possible gecko solutions, I am more inclined relying on gaia due to
1) make API behaviour explicit and clear
2) hopefully simplify implementation

> 3) What if gaia resumes a call put on hold by gecko?

Comment 44

[Andrea Marchesini [:baku]]

Attachment: patch 2 - Telephony API and mozinterruptbegin/end events

Comment 45

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #44)
> Created attachment 8437556 [details] [diff] [review]
> patch 2 - Telephony API and mozinterruptbegin/end events

Hi,

I want to input one thing here.

After landing this patch we will have two set of AudioChannelAgents for Telephony. One is in content process (telephony.cpp) and the other in parent process (AudioManager.cpp). I think we can let one in telephony.cpp to cover the other in AudioManager.cpp. So finally we have only telephony AudioChannelAgents in content process not b2g process.

If we don't do it, I affraid that telphony AudioChannelAgent in parent process will impact LIFO policy.

The reason for one in AudioManager.cpp is in order to represent ringer and in_call state into AudioChannelService. We don't expect app give the correct states by audio tag.

Comment 46

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Marco Chen [:mchen] from comment #45)
> (In reply to Andrea Marchesini (:baku) from comment #44)
> > Created attachment 8437556 [details] [diff] [review]
> > patch 2 - Telephony API and mozinterruptbegin/end events
> 
> Hi,
> 
> I want to input one thing here.
> 
> After landing this patch we will have two set of AudioChannelAgents for
> Telephony. One is in content process (telephony.cpp) and the other in parent
> process (AudioManager.cpp). I think we can let one in telephony.cpp to cover
> the other in AudioManager.cpp. So finally we have only telephony
> AudioChannelAgents in content process not b2g process.
> 
> If we don't do it, I affraid that telphony AudioChannelAgent in parent
> process will impact LIFO policy.
> 
> The reason for one in AudioManager.cpp is in order to represent ringer and
> in_call state into AudioChannelService. We don't expect app give the correct
> states by audio tag.

Thanks for the valuable input, Marco.

Sadly, even we remove the AudioChannelAgent in AudioManager.cpp, the issue remains, because now more than one app (including system app, callscreen app, dialer app...) initiate Telephony instances. More than one app compete for the telephony channel resource, and there's no way to guarantee it's dialer/callscreen app to win.

Just discussing with Marco, a very ugly solution could be we check hard-coded app manifest in Telephony.cpp. Only the approved one could join competition, i.e. call startPlaying(). But this solution makes our framework tightly bound to applications. If application manifest changes or if the specified app no longer exists, the gecko framework should change accordingly... I am not even sure if this is what we want.

Maybe we should revisit Jonas' suggestion(D), which looks a more feasible one?

Comment 47

[Marco Chen [:mchen]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #46)
> Maybe we should revisit Jonas' suggestion(D), which looks a more feasible
> one?

I think the main issue of current patches is that there will be multiple AudioChannelAgents with telephony type be initiated by different apps which acquire Web Telepony API. Then

  1. in current patches, we can not gaurantee which app's telphony channel will be the last one registerred into AudioChannelService. Because telephony.cpp in each apps will fire a request.

  2. In Jonas' suggestion (foreground app always wins), once user brings a app acquired telepony api to foreground then gsm call or loop call will be muted. (ex: when user is hearing a gsm call on callscreen app, user brings dialer app to foreground again. Then before user really dial another call, the first call will be put on hold because telephony channel in dialer app wins)

Comment 48

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Marco Chen [:mchen] from comment #47)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #46)
> > Maybe we should revisit Jonas' suggestion(D), which looks a more feasible
> > one?
> 
> I think the main issue of current patches is that there will be multiple
> AudioChannelAgents with telephony type be initiated by different apps which
> acquire Web Telepony API. Then
> 
>   1. in current patches, we can not gaurantee which app's telphony channel
> will be the last one registerred into AudioChannelService. Because
> telephony.cpp in each apps will fire a request.
> 
>   2. In Jonas' suggestion (foreground app always wins), once user brings a
> app acquired telepony api to foreground then gsm call or loop call will be
> muted. (ex: when user is hearing a gsm call on callscreen app, user brings
> dialer app to foreground again. Then before user really dial another call,
> the first call will be put on hold because telephony channel in dialer app
> wins)

Yup, considering these competition situation, we don't fire mozinterruptbegin/mozinterruptend on telephony API (unless we accept hard-coded app manifest in that API) , instead we adopt the suggestion D) 

  |Any time a phone call is happening, start a <audio src="silent.ogg" loop mozaudiochannel=telephony>.
   This way gecko will fire onmozinterruptbegin and onmozinterruptend on that <audio> 
   if another app uses   telephony channel.
 
   We can use these events to put the call on hold and to resume it automatically as appropriate.|

Is that feasible?

Comment 49

[Andrea Marchesini [:baku]]

>   |Any time a phone call is happening, start a <audio src="silent.ogg" loop
> mozaudiochannel=telephony>.
>    This way gecko will fire onmozinterruptbegin and onmozinterruptend on
> that <audio> 

Let me repeat it: WebAudio instead an <audio> HTML element :)
With WebAudio we don't have to decode any silent.ogg

Comment 50

[Marco Chen [:mchen]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #48)
> Yup, considering these competition situation, we don't fire
> mozinterruptbegin/mozinterruptend on telephony API (unless we accept
> hard-coded app manifest in that API) , instead we adopt the suggestion D) 
> 
>   |Any time a phone call is happening, start a <audio src="silent.ogg" loop
> mozaudiochannel=telephony>.
>    This way gecko will fire onmozinterruptbegin and onmozinterruptend on
> that <audio> 
>    if another app uses   telephony channel.
>  
>    We can use these events to put the call on hold and to resume it
> automatically as appropriate.|
> 
> Is that feasible?

If we accept this solution then patch 1 is not enough?

Comment 51

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Marco and :baku,

Sorry that I am not that familiar with the audio things, and sorry for trivial questions :( But I am trying to ask for a nicer solution than having hard-coded information in Telephony API.

(In reply to Andrea Marchesini (:baku) from comment #49)
> >   |Any time a phone call is happening, start a <audio src="silent.ogg" loop
> > mozaudiochannel=telephony>.
> >    This way gecko will fire onmozinterruptbegin and onmozinterruptend on
> > that <audio> 
> 
> Let me repeat it: WebAudio instead an <audio> HTML element :)
> With WebAudio we don't have to decode any silent.ogg

Oh, how about Dialer (or Callscreen) app has its AudioContex and listens to mozinterruptbegin/mozinterruptend fired on AudioContex? Is this a better solution to comment 46 and comment 47?

(In reply to Marco Chen [:mchen] from comment #50)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #48)
> > Yup, considering these competition situation, we don't fire
> > mozinterruptbegin/mozinterruptend on telephony API (unless we accept
> > hard-coded app manifest in that API) , instead we adopt the suggestion D) 
> > 
> >   |Any time a phone call is happening, start a <audio src="silent.ogg" loop
> > mozaudiochannel=telephony>.
> >    This way gecko will fire onmozinterruptbegin and onmozinterruptend on
> > that <audio> 
> >    if another app uses   telephony channel.
> >  
> >    We can use these events to put the call on hold and to resume it
> > automatically as appropriate.|
> > 
> > Is that feasible?
> 
> If we accept this solution then patch 1 is not enough?

Marco, I agree with your comment 47 which is indeed a valid problem, but I have no idea how we could do these audio channel control better. I will buy your and :baku's proposal as you are audio experts! 

I do hope to avoid hard-coded info in API, but if we cannot find a better way than firing mozinterruptbegin/end events on Telephony API, then considering the 2.0 time frame, I'll take it. However, we need to make sure only one and the right app (even there are more acquiring telephony API) could join competition per your concern. The current patch part2 isn't enough then. Also, I think this is a short-term solution. We shall figure out a long-term method.

Comment 52

[José Antonio Olivera Ortega [:jaoo]]

Andrea, I did a quick test by aplying the patch 1 (I applied also patch in bug 1023175) and set the listerner for mozinterrup* events on the callscreen app. The test was something like this: answer a GSM call and answer a Loop one next. AFAIK the callscreen should received the mozinterrup* events on the AudioContext instance I added. Sadly It doesn't happen and once the Loop call was accepted audio from both call plays at the same time.

Am I missing something? Please, reach me on IRC if you want to discuss about it. I'd like to know how to make this test work.

Comment 53

[Marco Chen [:mchen]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #51)
> I do hope to avoid hard-coded info in API, but if we cannot find a better
> way than firing mozinterruptbegin/end events on Telephony API, then
> considering the 2.0 time frame, I'll take it. However, we need to make sure
> only one and the right app (even there are more acquiring telephony API)
> could join competition per your concern. The current patch part2 isn't
> enough then. Also, I think this is a short-term solution. We shall figure
> out a long-term method.

From my point of view, it may be a good way of letting app decide to join telephony competing or not.
But using slient WebAudio or audio tag by app seems to be too hacky. 

What I can propose is we revisit to [1].
That one gives apps a Web API to join audio compting policy without real playing any kind of audio, so callscreen and VOIP apps can explicitly call this API to participate telephony competing.

[1] Bug 829409 - [Web API - AudioChannelManager] Give a Way for Joining AudioChannelService by Application Context not MediaElement

Comment 54

[Andrea Marchesini [:baku]]

> What I can propose is we revisit to [1].
> That one gives apps a Web API to join audio compting policy without real
> playing any kind of audio, so callscreen and VOIP apps can explicitly call
> this API to participate telephony competing.
> 
> [1] Bug 829409 - [Web API - AudioChannelManager] Give a Way for Joining
> AudioChannelService by Application Context not MediaElement

We already discuss about that bug. I don't think this GSM/Loop issue is a valid reason to reopen that bug. But I want to see what Jonas thinks about it.

I agree with you that the current proposal is too hacky. But I think telephony API, HTMLMediaElement and WebAudio should work at the same way when muted by AudioChannelService: mozinterruptbegin/end.

> I think the main issue of current patches is that there will be multiple
> AudioChannelAgents with telephony type be initiated by different apps which
> acquire Web Telepony API. Then

Telephony API is for certified app only (I'm not 100% sure about this). I think this scenario is a very remote corner-case.

>   1. in current patches, we can not gaurantee which app's telphony channel
> will be the last one registerred into AudioChannelService. Because
> telephony.cpp in each apps will fire a request.

Maybe I'm completely wrong, but I think this is a different kind of problem. If you have more than 2 apps using telephony APIs and you receive a GSM call, which app wins? This is not about AudioChannel, I guess.

>   2. In Jonas' suggestion (foreground app always wins), once user brings a
> app acquired telepony api to foreground then gsm call or loop call will be
> muted. (ex: when user is hearing a gsm call on callscreen app, user brings
> dialer app to foreground again. Then before user really dial another call,
> the first call will be put on hold because telephony channel in dialer app
> wins)

But I think UX doesn't want this. Comment 36 is about this. I would like to receive feedback from some gaia and UX team as well :)

So, my point of view is still that Telephony API should dispatch mozinterruptbegin/end and the app should put the call on hold if needed.

Comment 55

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Andrea Marchesini (:baku) from comment #54)
> > What I can propose is we revisit to [1].
> > That one gives apps a Web API to join audio compting policy without real
> > playing any kind of audio, so callscreen and VOIP apps can explicitly call
> > this API to participate telephony competing.
> > 
> > [1] Bug 829409 - [Web API - AudioChannelManager] Give a Way for Joining
> > AudioChannelService by Application Context not MediaElement
> 
> We already discuss about that bug. I don't think this GSM/Loop issue is a
> valid reason to reopen that bug. But I want to see what Jonas thinks about
> it.
> 
> I agree with you that the current proposal is too hacky. But I think
> telephony API, HTMLMediaElement and WebAudio should work at the same way
> when muted by AudioChannelService: mozinterruptbegin/end.
> 
> > I think the main issue of current patches is that there will be multiple
> > AudioChannelAgents with telephony type be initiated by different apps which
> > acquire Web Telepony API. Then
> 
> Telephony API is for certified app only (I'm not 100% sure about this). 

Yes, you are right!

> I think this scenario is a very remote corner-case.

Currently system app, callscreen app and dialer app acquire Telephony API.

Comment 56

[José Antonio Olivera Ortega [:jaoo]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #52)
> Andrea, I did a quick test by aplying the patch 1 (I applied also patch in
> bug 1023175) and set the listerner for mozinterrup* events on the callscreen
> app. The test was something like this: answer a GSM call and answer a Loop
> one next. AFAIK the callscreen should received the mozinterrup* events on
> the AudioContext instance I added. Sadly It doesn't happen and once the Loop
> call was accepted audio from both call plays at the same time.

Good news. Well Andrea and I were debugging into this issue and managed (finally) to see this working. I mean both apps callscreen and Loop were competing for the telephony audio channel and everything was working how this is supposed to work. The LIFO policy worked and the app being interrupted is silenced correctly (plus mozinterrup* events correctly fired). We added an audio element in the callcreen playing 'silence' while the GSM is happening (need to give a try with Web Audio API - an AudioContext instance) so once the callscreen is interrupted it gets silenced and mozinterrup* events are fired. On the down side we noticed both apps callscreen and loop compete correctly for audio telephony channel when the Loop app also plays silence while the WebRTC app is happening. The Loop app plays the audio from the WebRTC call by using a video element and it seems the app doesn't competes correctly unless the app also plays silence at the same time. So there is something fishy here and we need to figure out why the Loop app doesn't compete correctly. Baku and I think it might be a WebRTC issue but we need to continue debugging here.

Andrea, you might want to comment something about that. Feel free please.

Well having said that let say the apps are correctly interrupted and silenced and the rest would be letting the app handle the calls once it gets silenced. The Loop case is more or less easy to handle, if the Loop app is interrupted and silenced we just need to put the call on hold or something like that. For the callscreen case is a bit more complicated as there might be conferences calls, held calls and so on. Anyway the question is are the apps responsible for handling the calls once the app is silenced? Are we going to handle it in gaia side? If so we might involve the owner and/or peers of the call screen app.

Comment 57

[José Antonio Olivera Ortega [:jaoo]]

Attachment: patch 2 - Alternative to the second step: play silence during the call

Second version of this patch. Needs more work if we finally decide to handle GSM/CDMA calls in the callscreen app when the callscreen is interrupted due the audio competing policy. This patch just adds some debug traces to see that the app is correctly interrupted.

Comment 58

[Andrea Marchesini [:baku]]

> Andrea, you might want to comment something about that. Feel free please.

Yes. Today I'll test it locally and, in case, I'll file a bug: WebRTC audio/video element should behave as any other audio/video elements with mozinterruptbegin/end events.

Comment 59

[:shell escalante]

This bug was flagged as potentially introducing UI changes involving strings (ex: Resume). An extended deadline for FxOS string changes (so they can be localized) was requested until 6/20.  It sounds like this is on track for review and landing before end of week.  please correct me if there (1) aren't string changes or (2) reviewers cannot review by end of week and we need to find other reviewers.

Comment 60

[Randell Jesup [:jesup] (needinfo me)]

So, there are no such thing as "webrtc audio/video elements" per se.

We simply assign a mediastream that comes from a peerconnection to an <video> (or <audio>) element's mozSrcObject attribute.  The element has no real idea if it's playing realtime media, if the media is local (getUserMedia), coming from WebAudio, coming from another MediaStream source (stream = element.mozCaptureStream), from a canvas (soon), or a PeerConnection.  And for added fun, some of these could be either remote-realtime (WebAudio processing a Mediastream from a PeerConnection) or local.

On the other hand, PeerConnections know what they're pushing into the MediaStreamGraph; hooking those in may make more sense.  (In theory, a PeerConnection with media might not be assigned to a visible/audible media element - it might be going to WebAudio and MediaRecorder (taking a message, acting as an answering machine, etc)

So, please in this bug you file describe in detail what you need, not just how you think it should be solved.  Thanks!

Comment 61

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

(In reply to Andrea Marchesini (:baku) from comment #54)
> > [1] Bug 829409 - [Web API - AudioChannelManager] Give a Way for Joining
> > AudioChannelService by Application Context not MediaElement
> 
> We already discuss about that bug. I don't think this GSM/Loop issue is a
> valid reason to reopen that bug. But I want to see what Jonas thinks about
> it.

It sounds like we can use the WebAudio API to simulate joining the AudioChannelService, and that WebAudio does not need to spend a lot of CPU decoding an silent audio file. If that's correct, then I think that's good enough for the 2.0 release.

It's definitely a hack though, so I agree that we should at some point create a proper API instead.

However the fact that multiple apps already use the Telephony API sounds like a problem. Since all of them would be using the WebAudio and all of them would try to mute the call at once.

But do multiple apps really manage telephonyCall objects while the user is on a call? And do they all do so at *at once*? I know that we have multiple apps that use the API, but if they just start calls, or never run at the same time, then I think we might be ok.

Anyhow, comment 56 make it sound like we have a plan forward, is that the case?

Did we get answer from UX about what to do for all the various edge cases? Is this documented anywhere?

Comment 62

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Removing the needinfo as the solution is ongoing.

Comment 63

[Rafael Rebolleda]

Hey guys,

Since there seem to be some requests for another round of UX input, here's mine FWIW:

The main rule should be that ultimately, the user chooses whether to pick a call or not, wherever it comes from. No automatic switching or jumping from one service to the other unless explicitly done by the user.

If there is one incoming call to be handled, it should work the same way for Loop or gsm. Say we're on a gsm call and a Loop call comes in. We present the user with incoming call information, and she chooses whether to pick it up or not. If she does, the ongoing call is put on hold. If she doesn't, the incoming call screen is dismissed and she's returned to wherever she was (might not necessarily be the previous call screen). Same way if Loop was the ongoing call. The ongoing conversation is never put on hold automatically.

A more complex scenario would be to have an ongoing gsm call and another gsm call on hold, since apparently gsm works in a way whereby calls on hold are automatically resumed as the channel becomes available. There's no way (apparently, again) to have all gsm calls on hold at the same time. In this [I'd say very] edge case, and as MVP-to-iterate approach, I'd just bounce an incoming Loop call.


How to switch between ongoing calls amongst different services is a different debate; at the bare minimum I guess we could have no switching at all and the user is forced to hang up the second call to return to the first one.

Comment 64

[Maria Sandberg [:mushi]]

I'm no longer on this project - unflagging myself :)

Comment 65

[José Antonio Olivera Ortega [:jaoo]]

Attachment: audiocontext-logs.txt

I'm attaching some logs we captured to show how this works with Web Audio API instead of having an <audio> element playing silence. Bug 1027172 have fixed some issues we had to see this working correctly. Thanks Andrea!

Well having said that the plan would be to use it Web Audio in the callscreen app. That way the callscreen competes for audio in the telephony audio channel and it is interrupted correctly due the LIFO policy implemented in patch 1.

Need to test now all the corner case we could have with webrtc and gsm calls. I'm referring to what happens if there is an outgoing gsm call and another incoming webrtc one at the same time, what happens if a webrtc call interrupts a conference call, etc.

Comment 66

[Maria Angeles Oteo (:oteo)]

(In reply to sescalante from comment #59)
> This bug was flagged as potentially introducing UI changes involving strings
> (ex: Resume). An extended deadline for FxOS string changes (so they can be
> localized) was requested until 6/20.  It sounds like this is on track for
> review and landing before end of week.  please correct me if there (1)
> aren't string changes or (2) reviewers cannot review by end of week and we
> need to find other reviewers.

The bad news is that we believe it’s not going to be feasible to land this on Friday as there are many edge-cases we would like to test before landing everything. 

The good news are that:

 1/ We believe all the platform work is already identified either in this bug or in bugs blocking this one and all seem to be completed.
 2/ The hold/resume screen is already implemented in the dialer (see https://bugzilla.mozilla.org/show_bug.cgi?id=1019090#c6) and hence all the required strings are already available in the dialer [1]

Jose Antonio is now working on testing that the solution works flawlessly.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/locales/dialer.en-US.properties#L83

Comment 67

[bhavana bajaj [:bajaj]]

Shell, is the late-10n relevant here  ? None of the patches under review seem to have string changes, so I am confused :-/

Comment 68

[bhavana bajaj [:bajaj]]

I am removing the late-10n keyword as based on the patches under review, I do not see any string changes.

Comment 69

[José Antonio Olivera Ortega [:jaoo]]

Attachment: patch 2 - Alternative to the second step: play silence during the call

Still WIP patch

Comment 70

[Marco Chen [:mchen]]

(In reply to Jonas Sicking (:sicking) from comment #61)
> It sounds like we can use the WebAudio API to simulate joining the
> AudioChannelService, and that WebAudio does not need to spend a lot of CPU
> decoding an silent audio file. If that's correct, then I think that's good
> enough for the 2.0 release.
> 
> It's definitely a hack though, so I agree that we should at some point
> create a proper API instead.
> 

I found [1] ring tone app used the same hack way (WebAudio) to stop background music and it forget to release ringer channel while in background. Finally it breaks audio competing and volume adjustment.

[1] Bug 1020771 - When ringtone application is in background, music volume cant be changed

And one concern for silent audio file - some platforms will lock Wakelock actively when it detects any PCM stream. It means that device can't go into suspend mode (acpu power collpa) and effect the power consumption during phone call.

I would suggest it can be verified by people taking care power.

Comment 71

[José Antonio Olivera Ortega [:jaoo]]

Attachment: patch 2 - Alternative to the second step: play silence during the call

v1 version

Set gsm call on hold when the app is interrupted and resume call when the app can play audio again. Still need to take care of multiple (>1 and < CALLS_LIMIT) handled calls in the callscreen app.

I'd like to provide some flow diagrams to explain the changes added in the callscreen app in this bug so that might help to understand what is going on here. I'll do it asap.

Comment 72

[Jason Smith [:jsmith]]

This is not a bug. Moving this back to backlog again for the same reasons why we made this feature-b2g.

Comment 73

[Andrea Marchesini [:baku]]

Wrong dependences.

Comment 74

[José Antonio Olivera Ortega [:jaoo]]

Attachment: patch 2 - Alternative to the second step: play silence during the call

This version take care of handling multiple calls in the callscreen.

Comment 75

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8446718 [details] [diff] [review]
patch 2 - Alternative to the second step: play silence during the call

Well, this patch implements a first version of the solution we agreed in the latest comment. The idea is that the use of telephony audio channel will be granted on a LIFO basic (this is implemented by patch 'patch 1 - Telephony Channel - LIFO') and the app being interrupted will receive the mozinterrupt* events. For the callscreen app we need to take care of handling the calls might be connected, held, whatever, by the time the app is interrupted. This patch takes care of these situations (e.g. the callscreen app is interrupted while a gsm call is connected, there is a couple of gsm calls one connected and another one held, etc). I've testes all possible scenarios and it seems to work and it doesn't broke anything. My plan is to attach some flow diagrams to explain what and how is implemented here. In the mean time it would be great to have some feedback from you guys.

Comment 76

[Etienne Segonzac (:etienne)]

Comment on attachment 8446718 [details] [diff] [review]
patch 2 - Alternative to the second step: play silence during the call

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

I might be missing part of the magic, but I think the AudioCompetingHelper should have some sort of ref-counting logic where we only create 1 silence buffer even if multiple handledCalls call compete() but release it only once every one of those handledCalls called leaveCompetition().
Means we'll probably also need a dedicated method for the cases where we want to "force-win" by stoping/restarting the silence buffer.

Other than that it looks like a sane, safe and easily unit-testable approach :)

::: apps/callscreen/js/audiocompeting_helper.js
@@ +82,5 @@
> +    leaveCompetition: function ach_leaveCompetition() {
> +      if (!_silenceBufferSource) {
> +        return;
> +      }
> +      _silenceBufferSource.stop(0);

does stopping here implicitly removes the "mozinterrupt*" event listeners?

::: apps/callscreen/js/calls_handler.js
@@ +754,5 @@
> +  function onMozInterrupBegin() {
> +    // If there are multiple calls handled by the callscreen app and it is
> +    // interrupted by another app which uses the telephony audio channel the
> +    // callscreen wins.
> +    if (handledCalls.length > 1) {

If we have an ongoing conference call, |handledCalls.length > 1| will be true, but we could actually just hold the conference group.

I think you want to count the number of open "telephony lines" here, if you search |openLines| in the code you should fine examples (basically number of calls outside of a group + 1 if a conference group is ongoing)

@@ +792,5 @@
>      mergeConferenceGroupWithActiveCall: mergeConferenceGroupWithActiveCall,
>      updateAllPhoneNumberDisplays: updateAllPhoneNumberDisplays,
>  
> +    onMozInterrupBegin: onMozInterrupBegin,
> +    onMozInterrupEnd: onMozInterrupEnd,

not sure we need to expose those

::: apps/callscreen/js/handled_call.js
@@ +95,1 @@
>        CallScreen.render('connected');

so if I get a first call, then a second one (without the first one being disconnected), don't we leave a silent audio node in the wild or something?

Comment 77

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8446718 [details] [diff] [review]
patch 2 - Alternative to the second step: play silence during the call

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

Sounds a good approach to me.

Comment 78

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Diagrams for "patch 2 - Alternative to the second step: play silence during the call"

I'm attaching here a few diagrams of the use cases we might have. These diagrams try to explain a bit what we are implementing and how.

Next is to address Etienne's comments and provide some tests.

Comment 79

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Etienne Segonzac (:etienne) from comment #76)
> Comment on attachment 8446718 [details] [diff] [review]
> patch 2 - Alternative to the second step: play silence during the call
> 
> Review of attachment 8446718 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for providing some feedback on this.

> I might be missing part of the magic, but I think the AudioCompetingHelper
> should have some sort of ref-counting logic where we only create 1 silence
> buffer even if multiple handledCalls call compete() but release it only once
> every one of those handledCalls called leaveCompetition().

Well that is what I had in earlier versions of the patch. That approach doesn't
seem valid as 'once a source node has finished playing back, it can’t play back
more. To play back the underlying buffer again, you should create a new 
AudioBufferSourceNode'. The AudioCompetingHelper singleton object needs more
work as e.g. we should include one AudioContext per page. I'll take care of
these improvements in the next version and request feedback at Andrea (:baku) as
he is the expert on it.

> Means we'll probably also need a dedicated method for the cases where we
> want to "force-win" by stoping/restarting the silence buffer.

Agree, sounds good to me.

> Other than that it looks like a sane, safe and easily unit-testable approach
> :)

Sure, we will have tests for all this.
 
> ::: apps/callscreen/js/audiocompeting_helper.js
> @@ +82,5 @@
> > +    leaveCompetition: function ach_leaveCompetition() {
> > +      if (!_silenceBufferSource) {
> > +        return;
> > +      }
> > +      _silenceBufferSource.stop(0);
> 
> does stopping here implicitly removes the "mozinterrupt*" event listeners?

No, it doesn't but the events are not fired when the app doesn't compete. I'll
remove them explicity when we call stop as it seems safer.
 
> @@ +792,5 @@
> >      mergeConferenceGroupWithActiveCall: mergeConferenceGroupWithActiveCall,
> >      updateAllPhoneNumberDisplays: updateAllPhoneNumberDisplays,
> >  
> > +    onMozInterrupBegin: onMozInterrupBegin,
> > +    onMozInterrupEnd: onMozInterrupEnd,
> 
> not sure we need to expose those

Nope, I left those exposed I guess in a previous version of the patch.

> ::: apps/callscreen/js/handled_call.js
> @@ +95,1 @@
> >        CallScreen.render('connected');
> 
> so if I get a first call, then a second one (without the first one being
> disconnected), don't we leave a silent audio node in the wild or something?

No, we don't. I think that to be honest. I'll try to assure nothing left in the
wild. Thanks for pointing that.

Comment 80

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8437556 [details] [diff] [review]
patch 2 - Telephony API and mozinterruptbegin/end events

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

Looks like another approaching is on-going :)

Comment 81

[Marco Chen [:mchen]]

(In reply to Marco Chen [:mchen] (Workshop with Partner 6/16 ~ 6/20) from comment #70)
> And one concern for silent audio file - some platforms will lock Wakelock
> actively when it detects any PCM stream. It means that device can't go into
> suspend mode (acpu power collpase) and affect the power consumption during
> phone call.
> 
> I would suggest it can be verified by people taking care power.

May I know whether this side effect is acceptable or not? thanks.

Comment 82

[Lucas Adamski [:ladamski]]

Adding Jon to get his feedback on the power issue

Comment 83

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Does this patch take into account that other privileged apps, other than loop, might request access to the audio-channel-telephony permission? (Or do we need to restrict access to this permission via bug 1024396)?

IIUC any app that is in foreground and has the audio-channel-telephony permission will be able to interrupt loop and dialer calls. Is that an accurate summary?

Comment 84

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Actually Jonas has indicated in bug 1024396#15 that audio-channel-telephony should be loop only, at least initially. But either way the scenario should be considered.

Comment 85

[José Antonio Olivera Ortega [:jaoo]]

I'll comment here with Andrea(:baku)'s permission.

(In reply to Paul Theriault [:pauljt] from comment #83)
> Does this patch take into account that other privileged apps, other than
> loop, might request access to the audio-channel-telephony permission? (Or do
> we need to restrict access to this permission via bug 1024396)?

Nope, the patch 'patch 1 - Telephony Channel - LIFO' provided by Andrea (:baku) mutes the apps using the telephony audio channel on a LIFO basis and those apps must include an AudioContext object for playing 'silence' (through the telephony audio channel too) for being muted and interrupted correctly. Patch 'patch 2 - Alternative to the second step: play silence during the call' takes care of adding the AudioConext object for both the callscreen and the Loop apps.

Requesting ni? at Antonio at he might provide some feedback on the restrict access thing.

Comment 86

[José Antonio Olivera Ortega [:jaoo]]

Attachment: patch 2 - Alternative to the second step: play silence during the call

Andrea, may I have your feedback of the AudioCompetingHelper object added to this patch please? It basically joins the callscreen and loop apps to the audio competition and allows those apps to receive the mozinterrupt* events. I really need to have your feedback about the force-win mechanism implemented here which is used by the callscreen app to re-compete for the telephony audio channel when the loop app interrupts the callscreen one when there are two gsm/cdma calls in progress. Thanks!

PS. The flow diagrams I added might help you to understand what I'm talking about.

Comment 87

[Maire Reavy [:mreavy]]

Hi Marco -- I need review and feedback for the patches on this bug ASAP.  Can you take over ownership this bug please?  Andrea is unavailable, per his manager.  Thanks.

Comment 88

[:shell escalante]

renominating for blocking consideration - as this bug is not something we can ship with.

Comment 89

[Jason Smith [:jsmith]]

(In reply to sescalante from comment #88)
> renominating for blocking consideration - as this bug is not something we
> can ship with.

This is feature work as part of having audio channels work under privileged, so the feature-b2g flag is what will be used to track this. See https://bugzilla.mozilla.org/show_bug.cgi?id=1016277#c19. I clarified the reasoning generally why tracking is done this way here - https://bugzilla.mozilla.org/show_bug.cgi?id=1021595#c19.

Comment 90

[Marco Chen [:mchen]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #87)
> Hi Marco -- I need review and feedback for the patches on this bug ASAP. 
> Can you take over ownership this bug please?  Andrea is unavailable, per his
> manager.  Thanks.

I can not take over this bug because I am stuck in TV project belonged to Stream 3.
Which is planed to ship first version on Setemper.

After discussing with Maire, I can do my best to give review of patch 1 and feedback of patch 2 once someone can confirm the solution right now is enough for V2.0 already and power issue can be clarified or compromized.

Comment 91

[Maire Reavy [:mreavy]]

Jon -- Do you have any thoughts or feedback about the power issue Marco raised (see Comment 70, Comment 81, Comment 82, and Comment 90)?  Thanks.

Comment 92

[Maire Reavy [:mreavy]]

Lucas -- In Comment 90, Marco is really asking for blessing from Jonas as architect that the current design (in patches 1 and 2) is ok for v2.0.  With Jonas on PTO, who can give that blessing?   

(FYI: I just talked with PaulT on irc, and he's reviewing the patches to make sure they are good from a security perspective.)  Thanks!

Comment 93

[Daniel Coloma:dcoloma]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #92)
> Lucas -- In Comment 90, Marco is really asking for blessing from Jonas as
> architect that the current design (in patches 1 and 2) is ok for v2.0.  With
> Jonas on PTO, who can give that blessing?   
> 

I've talked to Jonas extensively about this patch during the previous weeks and he was happy with the approach as many of the comments in this bug show. The only caveat is that Jonas wanted us to consider corner-cases such as dialer handling multiple calls or interrumpting an incoming call. The patch is already covering those corner cases and the diagrams Jose Antonio provided document how are they handled: https://bug1016277.bugzilla.mozilla.org/attachment.cgi?id=8447724. 

We have been working weeks on this solution based on that assumption so I don't think we need anything special apart from the requested reviews (including Paul's review on the security).

> (FYI: I just talked with PaulT on irc, and he's reviewing the patches to
> make sure they are good from a security perspective.)  Thanks!

Comment 94

[Paul Theriault [:pauljt] (no longer reading bugmail)]

So I have been through these patches and they look OK to me from a security perspective.

Comment 95

[Maire Reavy [:mreavy]]

José Antonio - Marco will provide feedback on your patch from an audio channels perspective, but he reminded me that the patch needs review from a dialer app peer (gaia).  I believe Fernando is a dialer app peer.  If you have any trouble finding a reviewer, please let me know.  Andrea is on PTO until 25 July.   (Marco will review patch 1 today.)

Comment 96

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #95)

Thanks for your help.

> José Antonio - Marco will provide feedback on your patch from an audio
> channels perspective, but he reminded me that the patch needs review from a
> dialer app peer (gaia).

Yes, that's right. I'd like to remember that I would need feedback from Marco on the gaia part as well (see the request I made at comment 86 please).

> I believe Fernando is a dialer app peer.  If you
> have any trouble finding a reviewer, please let me know.

I'll request review at Etienne as we already gave us some feedback.

Comment 97

[Maire Reavy [:mreavy]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #96)
> (In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #95)
> 
> Thanks for your help.
> 
> > José Antonio - Marco will provide feedback on your patch from an audio
> > channels perspective, but he reminded me that the patch needs review from a
> > dialer app peer (gaia).
> 
> Yes, that's right. I'd like to remember that I would need feedback from
> Marco on the gaia part as well (see the request I made at comment 86 please).

I talked with Marco on irc about Daniel's Comment 92 (Thank you, Daniel!).  Marco will review patch 1 today and provide feedback on patch 2 by tomorrow.  

> 
> > I believe Fernando is a dialer app peer.  If you
> > have any trouble finding a reviewer, please let me know.
> 
> I'll request review at Etienne as we already gave us some feedback.

That sounds perfect.  Thank you!

Per our irc conversation, I will assign this bug to you to finish off.

With all this information, I believe I can clear the needinfo to Lucas.

Comment 98

[Maire Reavy [:mreavy]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #97)
> I talked with Marco on irc about Daniel's Comment 92 (Thank you, Daniel!). 

I meant Comment 93.  (And again: thank you Daniel and José Antonio!)

Comment 99

[Antonio Manuel Amaya Calvo (:amac)]

> Does this patch take into account that other privileged apps, other than
> loop, might request access to the audio-channel-telephony permission? (Or do
> we need to restrict access to this permission via bug 1024396)?

Ok, concerning this, I think that at this point we need to either restrict this to loop, or ensure that the marketplace reviewers need what to look here. The problem here is that what this patch implements is a way to allow app developers to know when the telephony channel is busy so they can mute/pause their reproduction. But there's no *enforcement* of this on the system (that is, the system just tells the apps: the channel is busy now, please act in consequence, but doesn't force one of the apps to pause). So a malicious (or just incorrect) app can keep using the telephony channel even after receiving the 'Hey, this channel is busy now' notification, and both contents would come out mixed.

Comment 100

[Maire Reavy [:mreavy]]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #99)
> > Does this patch take into account that other privileged apps, other than
> > loop, might request access to the audio-channel-telephony permission? (Or do
> > we need to restrict access to this permission via bug 1024396)?
> 
> Ok, concerning this, I think that at this point we need to either restrict
> this to loop, or ensure that the marketplace reviewers need what to look
> here. The problem here is that what this patch implements is a way to allow
> app developers to know when the telephony channel is busy so they can
> mute/pause their reproduction. But there's no *enforcement* of this on the
> system (that is, the system just tells the apps: the channel is busy now,
> please act in consequence, but doesn't force one of the apps to pause). So a
> malicious (or just incorrect) app can keep using the telephony channel even
> after receiving the 'Hey, this channel is busy now' notification, and both
> contents would come out mixed.

Fernando --Is the solution for Bug 1024396 restricting this to Loop?  I believe it is, but just want to confirm.  Thanks.

Comment 101

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #100)
> Fernando --Is the solution for Bug 1024396 restricting this to Loop?  I
> believe it is, but just want to confirm.  Thanks.

Yes, it was added to bug 1024396

Comment 102

[Marco Chen [:mchen]]

Comment on attachment 8435644 [details] [diff] [review]
patch 1 - Telephony Channel - LIFO

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

This review is based on comment 93 which said B2G tech lead - Jonas can accept this solution for V2.0.
Then it looks good to me except for one nit.

By the way, as I know UX hopes user can always resume the call back by he/her self's action.
Didn't want to have a automatic resuming.

But in this patch, once last ChildID is finished all it's telephony channels.
This patch will remove this childID from array then last second one will become last one.
It means the paused VOIP or telephony call on hold will be resumed automatically because app receives mozinterruptend.

Is this accepted by UX? or maybe app will handle this situation.

::: dom/audiochannel/AudioChannelService.cpp
@@ +418,5 @@
> +  bool found = false;
> +  for (uint32_t i = 0, len = mTelephonyChildren.Length(); i < len; ++i) {
> +    if (mTelephonyChildren[i].mChildID == aChildID) {
> +      found = true;
> +      break;;

nit: double semi-colon

Comment 103

[Marco Chen [:mchen]]

> Is this accepted by UX? or maybe app will handle this situation.
needinfo to :jaoo for comment 102

Comment 104

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Marco Chen [:mchen] (Workshop with Partner 6/16 ~ 6/20) from comment #102)
> Comment on attachment 8435644 [details] [diff] [review]
> patch 1 - Telephony Channel - LIFO
> 
> Review of attachment 8435644 [details] [diff] [review]:

Thanks for reviewing this Marco.

> By the way, as I know UX hopes user can always resume the call back by
> he/her self's action.
> Didn't want to have a automatic resuming.
> 
> But in this patch, once last ChildID is finished all it's telephony channels.
> This patch will remove this childID from array then last second one will
> become last one.
> It means the paused VOIP or telephony call on hold will be resumed
> automatically because app receives mozinterruptend.

The patch for the Loop and callscreen apps will set the calls on hold automatically once the apps receive the onmozinterrupbegin events. Users can resume the call wherever they want. Once the apps receive the onmozinterrupend events the patch for the apps resume the calls automatically.

Comment 105

[Daniel Coloma:dcoloma]

> By the way, as I know UX hopes user can always resume the call back by
> he/her self's action.
> Didn't want to have a automatic resuming.
> 
> But in this patch, once last ChildID is finished all it's telephony channels.
> This patch will remove this childID from array then last second one will
> become last one.
> It means the paused VOIP or telephony call on hold will be resumed
> automatically because app receives mozinterruptend.
> 
> Is this accepted by UX? or maybe app will handle this situation.

Indeed automatic resuming was the first option that was suggested and as we did not realice it was feasible we suggested the manual resume (see comment 22) so I assume this is even better.

Comment 106

[Maire Reavy [:mreavy]]

To clarify Marco's power consumption concerns based on my irc conversation with him: if the user switches to a telephony call during a Loop call, the phone may not go into suspend mode because we are sending silence.

However, we must send silence or the Loop call will drop. We need to continue to process network code (sending and receiving network packets).  So, I believe we are good in terms of power concerns.

Based on this clarification, I'll remove the needinfo to jhylands (Jon).

Comment 107

[Marco Chen [:mchen]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #106)
I would like to clarify again and give more detail user story and impact on what I mentioned about power consumption. Then ask jhylands to feedback.

In normal telephony call
  1. User make or pick up a call.
  2. Device will go to suspend mode after screen time out.
     Or by actions cooresponding to proximity sensor.
  3. Application CPU is on Power Collapse mode and only Modem subsystem is working for reducing power.

In telephony call with this patch
  Step 1 and 2 are the same with "normal telephony call"
  3. Application CPU "CAN NOT" go into Power Collapse mode because there is a silient PCM stream from Web Audio inide dialer app.
  4. Then this will impact the standy time during telephony call.

(Note: Some devices can have proximity sensor to wake up device by GPIO defined as wakeup source so there is no necceary for locking a wakelock by dialer app for detecting proximity sensor)

Comment 108

[Marco Chen [:mchen]]

Comment on attachment 8449538 [details] [diff] [review]
patch 2 - Alternative to the second step: play silence during the call

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

Basically I think the usage of audio channel is ok.
I also leave some comments for others issue based on my little understanding of JS.

::: apps/callscreen/js/audiocompeting_helper.js
@@ +43,5 @@
> +      return;
> +    }
> +
> +    for (var i = 0; i <_listeners[type].length; i++) {
> +      if(_listeners[type][i] && (typeof _listeners[type][i] === 'function')) {

The only point to push callback into listeners is at line 123 and line 119 already filter the non-function case already.
I think we don't need to check again.

@@ +76,5 @@
> +      _silenceBufferSource.buffer = _ac.createBuffer(1, 2048, _ac.sampleRate);
> +      _silenceBufferSource.connect(_ac.destination);
> +      _silenceBufferSource.loop = true;
> +
> +      if (_addListenersBeforeCompeting) {

Since everytime you create a new AudioContext(), event handlers should be attached as well.
Maybe the effort you want to save is only for binding _onmozinterruptbegin/end only but do _ac.addEventListener everytime.

@@ +93,5 @@
> +     * Request the helper to leave the competition for the use of the telephony
> +     * audio channel.
> +     */
> +    leaveCompetition: function ach_leaveCompetition() {
> +      if (!_silenceBufferSource) {

It seems that _silenceBufferSource didn't drop it's refernce so bad app can call this function and pass this verification always.

::: apps/callscreen/js/calls_handler.js
@@ +811,5 @@
> +    // If there are multiple calls handled by the callscreen app and it is
> +    // interrupted by another app which uses the telephony audio channel the
> +    // callscreen wins.
> +    if (openLines !== 1) {
> +     forceAnAudioCompetitionWin();

line alignment

Comment 109

[José Antonio Olivera Ortega [:jaoo]]

Attachment: patch 2 - Alternative to the second step: play silence during the call (v1)

Latest version of the patch to be added in the callscreen app. I'll attach the patch with some test later. Etienne, could you have a look at this patch in the mean time? Thanks.

Marco, I still need some feedback on the AudioCompetingHelper singleton object added here. Please, read comment 86 with the request I made a few days ago. Thanks!

Comment 110

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8450875 [details] [diff] [review]
patch 2 - Alternative to the second step: play silence during the call (v1)

Cancelling feedback request as Marco provided that feedback at comment 108. Thanks Marco!

Comment 111

[Etienne Segonzac (:etienne)]

Comment on attachment 8450875 [details] [diff] [review]
patch 2 - Alternative to the second step: play silence during the call (v1)

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

I'll do the full review with the tests.
f+, all the comments are addressed, but I have a new one below.

::: apps/callscreen/js/handled_call.js
@@ +100,5 @@
> +      // stops then a mozinterrupbegin event is fired. This is a race condition
> +      // we could easily avoid with a 1-second-timeout fix.
> +      window.setTimeout(function onTimeout() {
> +        AudioCompetingHelper.compete();
> +      }, 1000);

:/ I'd really like to avoid this.
I'm not sure I understand why we have this issue since the DialerAgent is on the ringer channel and we're on telephony...

Can we try using the 'connected' event for this, I think it's always triggered after the statechange event.

Comment 112

[Maria Angeles Oteo (:oteo)]

In bug 1034460, we have included the User Story corresponding to the development implemented in this bug.

Comment 113

[José Antonio Olivera Ortega [:jaoo]]

Attachment: Diagrams for "patch 2 - Alternative to the second step: play silence during the call"

(In reply to Maria Angeles Oteo (:oteo) from comment #112)
> In bug 1034460, we have included the User Story corresponding to the
> development implemented in this bug.

Diagrams updated.

Comment 114

[José Antonio Olivera Ortega [:jaoo]]

Attachment: audiocontext-logs.txt

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #65)
> Created attachment 8442759 [details]
> audiocontext-logs.txt

> Well having said that the plan would be to use it Web Audio in the
> callscreen app. That way the callscreen competes for audio in the telephony
> audio channel and it is interrupted correctly due the LIFO policy
> implemented in patch 1.

Someone might want to have some logs to see how this works.

Comment 115

[José Antonio Olivera Ortega [:jaoo]]

Attachment: 1016277-logs-reply-comment111.txt

(In reply to Etienne Segonzac (:etienne) from comment #111)
> Comment on attachment 8450875 [details] [diff] [review]
> patch 2 - Alternative to the second step: play silence during the call (v1)
> 
> Review of attachment 8450875 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll do the full review with the tests.

Sure, thanks for the feedback!

> I'm not sure I understand why we have this issue since the DialerAgent is on
> the ringer channel and we're on telephony...

Let me explain a bit more. The priority of the ringer audio channel is higher
that the telephony one. Due the current audio channel policy the audio channel
service mutes any audio element playing on the telephony audio channel when
audio channel ringer plays. Moreover the onmozinterrupbegin events are fired on
the audio elements being muted. Having said that let me explain why it needs
this timeout-based fix. Once we call compete() the mozinterrupbegin event
listeners are set on the AudioContext object and it seems that at that point the
audio channel service is still allowing the dialer agent to play the ringtone
that causes a mozinterrupbegin event is fired on the AudioContext object.

See the logs I attached please.

> Can we try using the 'connected' event for this, I think it's always
> triggered after the statechange event.

We already use the 'connected' event.

Comment 116

[Etienne Segonzac (:etienne)]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #115)
> Let me explain a bit more. The priority of the ringer audio channel is higher
> that the telephony one. Due the current audio channel policy the audio
> channel
> service mutes any audio element playing on the telephony audio channel when
> audio channel ringer plays. Moreover the onmozinterrupbegin events are fired
> on
> the audio elements being muted. Having said that let me explain why it needs
> this timeout-based fix. Once we call compete() the mozinterrupbegin event
> listeners are set on the AudioContext object and it seems that at that point
> the
> audio channel service is still allowing the dialer agent to play the ringtone
> that causes a mozinterrupbegin event is fired on the AudioContext object.
> 

Thanks it's (a bit :)) clearer.

> 
> We already use the 'connected' event.

We use the onstatechange event in both places (dialer agent and handled call).
I wonder if using onstatechange in the dialer agent and onconnected in handled call could help us enforce the order.

Comment 117

[José Antonio Olivera Ortega [:jaoo]]

Attachment: patch 2 - Alternative to the second step: play silence during the call (Pull request)

(In reply to Etienne Segonzac (:etienne) from comment #116)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #115)

> > We already use the 'connected' event.
> 
> We use the onstatechange event in both places (dialer agent and handled
> call).
> I wonder if using onstatechange in the dialer agent and onconnected in
> handled call could help us enforce the order.

Sadly It didn't enforce the order. I've tested this and It seems we get the same result.

There you go the PR, I've also tests as you requested. Could you have a look at the patch please? Thanks!

Comment 118

[Etienne Segonzac (:etienne)]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #117) 
> Sadly It didn't enforce the order. I've tested this and It seems we get the
> same result.
> 

Thanks for trying it :)
I'll have a look at the patch tomorrow.

Comment 119

[Etienne Segonzac (:etienne)]

Comment on attachment 8451134 [details] [review]
patch 2 - Alternative to the second step: play silence during the call (Pull request)

Awesome (tiny comment on github), can we get bare minimum sanity unit tests for the AudiocompetingHelper too? Just the event listeners stuff, I don't think we'll be able to cover the audio part easily.

Comment 120

[Jon Hylands [:jhylands]]

Sorry, I was on PTO for a week - do you still need some input from me on this bug?

Comment 121

[Maire Reavy [:mreavy]]

(In reply to Jon Hylands [:jhylands] from comment #120)
> Sorry, I was on PTO for a week - do you still need some input from me on
> this bug?

Hi Jon -- If you could review Comment 107 (from Marco) and provide feedback, that would be ideal.  Comment 107 captures Marco's current concerns.  Thanks.

Comment 122

[Jon Hylands [:jhylands]]

I'll have to try this with and without the patch. There appear to be three patches in this bug, which one(s) do I need? Is it okay to test this on a Flame, or would some other device be better? The bug is filed as a 2.0 blocker - should I test on that, or use latest master?

Comment 123

[Maire Reavy [:mreavy]]

(In reply to Jon Hylands [:jhylands] from comment #122)
> I'll have to try this with and without the patch. There appear to be three
> patches in this bug, which one(s) do I need? Is it okay to test this on a
> Flame, or would some other device be better? The bug is filed as a 2.0
> blocker - should I test on that, or use latest master?

Patch 2 - Alternative (which :jaoo wrote) is the one we want evaluated/tested;  it plays a "silent PCM stream from Web Audio inside dialer app, which will impact the standby time during telephony call" per Marco.   Flame is the device we care about, and you should be able to test this on the latest master or 2.0.  We need to ship this in v2.0, but the code for this on master and the 2.0 should be the same at this point.

Comment 124

[Maria Angeles Oteo (:oteo)]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #123)
> (In reply to Jon Hylands [:jhylands] from comment #122)
> > I'll have to try this with and without the patch. There appear to be three
> > patches in this bug, which one(s) do I need? Is it okay to test this on a
> > Flame, or would some other device be better? The bug is filed as a 2.0
> > blocker - should I test on that, or use latest master?
> 
> Patch 2 - Alternative (which :jaoo wrote) is the one we want
> evaluated/tested;  it plays a "silent PCM stream from Web Audio inside
> dialer app, which will impact the standby time during telephony call" per
> Marco.   Flame is the device we care about, and you should be able to test
> this on the latest master or 2.0.  We need to ship this in v2.0, but the
> code for this on master and the 2.0 should be the same at this point.

Apart of that, just to clarify, we need patch 1 applied first and after that compare the battery consumption with and without the Patch 2 - Alternative (As Maire has pointed out)

Comment 125

[Jon Hylands [:jhylands]]

I have some power numbers for comparison between having the patch and not having the patch. Before I post them, I know there were issues with putting current draw numbers in public bugs for some phones (like the Hamachi). Do we have the same restrictions on Flame power numbers?

Comment 126

[Maire Reavy [:mreavy]]

(In reply to Jon Hylands [:jhylands] from comment #125)
> I have some power numbers for comparison between having the patch and not
> having the patch. Before I post them, I know there were issues with putting
> current draw numbers in public bugs for some phones (like the Hamachi). Do
> we have the same restrictions on Flame power numbers?

I believe we have restrictions on the Flame, but let's ask Lucas.

Lucas -- Can Jon post the power numbers he has here (in a public bug) for the Flame?   Thanks.

Comment 127

[Maire Reavy [:mreavy]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #126)
> (In reply to Jon Hylands [:jhylands] from comment #125)
> > I have some power numbers for comparison between having the patch and not
> > having the patch. Before I post them, I know there were issues with putting
> > current draw numbers in public bugs for some phones (like the Hamachi). Do
> > we have the same restrictions on Flame power numbers?
> 
> I believe we have restrictions on the Flame, but let's ask Lucas.
> 
> Lucas -- Can Jon post the power numbers he has here (in a public bug) for
> the Flame?   Thanks.

Sorry, I can't type today.  I believe we do *NOT* have restrictions on the Flame, but I defer to Lucas.

Comment 128

[Lucas Adamski [:ladamski]]

I haven't heard of any, please post until someone squawks.

Comment 129

[Jon Hylands [:jhylands]]

Thanks Lucas.

So, without the patch (Patch 2 - Alternate) I get 184.6 mA current usage in a call, with the screen blanked (putting my thumb over the proximity sensor).

With the patch, I get 197.9 mA current usage in a call, with the screen blanked the same way.

These are both averaged over ~16 seconds, taking samples at ~125 Hz.

The phone is flashed from a build pulled from master today, with the first patch applied in both cases. I'm using a Virgin Mobile (Canada) SIM card in the phone, calling it from a second mobile phone (Android). Wifi is off in both cases.

Comment 130

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 8451134 [details] [review]
patch 2 - Alternative to the second step: play silence during the call (Pull request)

(In reply to Etienne Segonzac (:etienne) from comment #119)
> Comment on attachment 8451134 [details] [review]
> patch 2 - Alternative to the second step: play silence during the call (Pull
> request)

Thanks for the review!

> Awesome (tiny comment on github), can we get bare minimum sanity unit tests
> for the AudiocompetingHelper too? Just the event listeners stuff, I don't
> think we'll be able to cover the audio part easily.

Comments addressed and AudiocompetingHelper tests added. Could you have a look at the PR again please? Thanks!

Comment 131

[Andrew Overholt [:overholt]]

Andrea is on PTO for a few weeks so clearing the ni? on him since this seems time-sensitive.

Comment 132

[Etienne Segonzac (:etienne)]

Comment on attachment 8451134 [details] [review]
patch 2 - Alternative to the second step: play silence during the call (Pull request)

One last (easy) comment on github then we're good to go.
(please remove the debug statement before landing.)

Thanks for the hard work!

Comment 133

[Marco Chen [:mchen]]

(In reply to Jon Hylands [:jhylands] from comment #129)
> Thanks Lucas.
> 
> So, without the patch (Patch 2 - Alternate) I get 184.6 mA current usage in
> a call, with the screen blanked (putting my thumb over the proximity sensor).
> 
> With the patch, I get 197.9 mA current usage in a call, with the screen
> blanked the same way.
> 

Hi Jon, thanks for your help to measure the current. I think the power regression here is caused by silent PCM stream prevented CPU from getting into Idle state.

I can image another user scenario will cause more difference of power consumption between with (maybe 190mA) and without patch (maybe 90mA). Consider the steps as below;
  1. user dials a call to remote side.
  2. user enables the speaker mode. (optional)
  3. user actively presses the power key then device goes into screen off.
  4. then user continue to talk.

Jon, could you help to verify this scenario again? Thanks.

Comment 134

[Jon Hylands [:jhylands]]

Sure, I can test that this afternoon and report back here.

Comment 135

[Jon Hylands [:jhylands]]

So without the patch, using the instructions in command 133, we're getting 165 mA.

With the patch, we're getting 163 mA, which is pretty much identical.

As an aside, at idle (screen off, no call), the phone is consuming ~22 mA.

Comment 136

[Jon Hylands [:jhylands]]

Note that I tested this multiple times both with and without the 2nd patch, and it was consistently between 163 and 165 mA every time.

Comment 137

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Etienne Segonzac (:etienne) from comment #132)
> Comment on attachment 8451134 [details] [review]
> patch 2 - Alternative to the second step: play silence during the call (Pull
> request)
> 
> One last (easy) comment on github then we're good to go.
> (please remove the debug statement before landing.)
> 
> Thanks for the hard work!

Thanks for the review Etienne! I'll comment here about the last comment you added in the PR.

Well Etienne has requested me (per bug 997701) to change the way we set the audio channel when using the AudioContext object (see [1]). Andrea (:baku) didn't mention that need and AFAIK there is no issue with setting the audio channel through the mozAudioChannelType property before playing anything and everything works fine. Did the change and any higher audio channel (ringer, publicnotification) doesn't play anymore after accepting a either a GSM or a WebRTC call.

Marco, i) do we really need to set the audio channel when creating the AudioContext object? (e.g. I don't see tests from bug 1023175 or bug 1027172 creating AudioConext object that way) ii) all this audio stuff are new for me so why is this happening? (any higher audio channel doesn't play anymore after using an AudioContext created that way)

[1] https://github.com/mozilla-b2g/gaia/pull/21399#discussion_r14660054

Comment 138

[Marco Chen [:mchen]]

(In reply to Jon Hylands [:jhylands] from comment #135)
> So without the patch, using the instructions in command 133, we're getting
> 165 mA.
> 
> With the patch, we're getting 163 mA, which is pretty much identical.
> 
> As an aside, at idle (screen off, no call), the phone is consuming ~22 mA.

Hi Jon,

Thanks for the help although It is weird that phone can't go to suspend during voice call without wakelock by proximity sensor.

Comment 139

[(no longer active)]

If the question is about whether you should pass the channel type to the AudioContext constructor or set the mozAudioChannelType attribute, ideally they should both do the exact same thing, but reading through the code, it seems like setting the mozAudioChannelType attribute doesn't set the AudioChannel on the stream and I think that is a bug.  I filed it as bug 1036431, but please pass the channel to the constructor for now.

Comment 140

[Marco Chen [:mchen]]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #137)
> Marco, i) do we really need to set the audio channel when creating the
> AudioContext object? (e.g. I don't see tests from bug 1023175 or bug 1027172
> creating AudioConext object that way) ii) all this audio stuff are new for
> me so why is this happening? (any higher audio channel doesn't play anymore
> after using an AudioContext created that way)
> 
> [1] https://github.com/mozilla-b2g/gaia/pull/21399#discussion_r14660054

I took a quick look at it and found 
  way 1. carry audio channel type in constructor will create an AudioChannelAgent and set audio channel type into MediaStream (mStream) too. http://dxr.mozilla.org/mozilla-central/source/content/media/webaudio/AudioDestinationNode.cpp#300
  way 2. Set audio channel type by member function will create AudioChannelAgent too but doesn't set audio channel type into MediaStream.
So I suspect the issue is related to set type into MediaStream.

Basically I think way 2 should be workable to set audio channel type as the same result with way 1.

Comment 141

[(no longer active)]

(In reply to Marco Chen [:mchen] (Workshop with Partner 6/16 ~ 6/20) from comment #140)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #137)
> > Marco, i) do we really need to set the audio channel when creating the
> > AudioContext object? (e.g. I don't see tests from bug 1023175 or bug 1027172
> > creating AudioConext object that way) ii) all this audio stuff are new for
> > me so why is this happening? (any higher audio channel doesn't play anymore
> > after using an AudioContext created that way)
> > 
> > [1] https://github.com/mozilla-b2g/gaia/pull/21399#discussion_r14660054
> 
> I took a quick look at it and found 
>   way 1. carry audio channel type in constructor will create an
> AudioChannelAgent and set audio channel type into MediaStream (mStream) too.
> http://dxr.mozilla.org/mozilla-central/source/content/media/webaudio/
> AudioDestinationNode.cpp#300
>   way 2. Set audio channel type by member function will create
> AudioChannelAgent too but doesn't set audio channel type into MediaStream.
> So I suspect the issue is related to set type into MediaStream.
> 
> Basically I think way 2 should be workable to set audio channel type as the
> same result with way 1.

Yep, that is exactly what I found out.  That's bug 1036431.

Comment 142

[:shell escalante]

update from email conversations:
That sounds good to me!  Please let me know if this doesn't work.

Thanks
--
Ehsan

On Wed, Jul 9, 2014 at 12:47 PM, Jose Antonio Olivera Ortega <[e-mail]> wrote:

    Hi there,

    thank you all for the help on this.

    I just gave a try and both ways of setting the audio channel work.
    Yesterday It was not happening, as I commented at [1]. I did the
    change Etienne suggested and any higher audio channel (ringer,
    publicnotification) doesn't play anymore after accepting a either a
    GSM or a WebRTC call. I have tested right now with a new build and it
    works fine. Maybe the reason It wasn't working was the build I tested
    on?...whatever. I'm building a new fresh 2.1 build and I give a try
    again to double check and if it works I'll go ahead and start landing
    patches. If it doesn't work I'll ping you (Marco or Ehsan) on IRC.

    Is everybody here happy with this plan?

Comment 143

[Jason Smith [:jsmith]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1016277#c89.

Comment 144

[José Antonio Olivera Ortega [:jaoo]]

(In reply to sescalante from comment #142)
> update from email conversations:
> That sounds good to me!  Please let me know if this doesn't work.

>     I'm building a new fresh 2.1 build and I give a try
>     again to double check and if it works I'll go ahead and start landing
>     patches.

Well It seems it works so let's go ahead and start landing patches as we planned.

Comment 145

[José Antonio Olivera Ortega [:jaoo]]

Let's see the try server results at https://tbpl.mozilla.org/?tree=Try&rev=f1d2704b5ee7

Comment 146

[[:fabrice] Fabrice Desré]

I retriggered the M1 failures, but they look real.

Comment 147

[[:fabrice] Fabrice Desré]

I fixed the issue locally, pushing to try with the one-liner folded to be extra sure:

https://tbpl.mozilla.org/?tree=Try&rev=0e62d808c089

Comment 148

[Marco Chen [:mchen]]

(In reply to Jon Hylands [:jhylands] from comment #129)
> Thanks Lucas.
> 
> So, without the patch (Patch 2 - Alternate) I get 184.6 mA current usage in
> a call, with the screen blanked (putting my thumb over the proximity sensor).
> 
> With the patch, I get 197.9 mA current usage in a call, with the screen
> blanked the same way.
> 
> These are both averaged over ~16 seconds, taking samples at ~125 Hz.

Note here: There is a power increase about 13mA introduced by Patch 2.

Comment 149

[[:fabrice] Fabrice Desré]

New try run with network.disable.ipc.security=true at https://tbpl.mozilla.org/?tree=Try&rev=8d72fb8747fa

Comment 150

[[:fabrice] Fabrice Desré]

No dice :(

I need to get an emulator build to investigate more, because there is absolutely nothing in the logs.

Comment 151

[Maire Reavy [:mreavy]]

Update: Fabrice is actively debugging the one remaining test failure.  It's an emulator bug.  Once that is resolved we can land.

FYI: Padenot believes he has resolved the intermittent (bug 1032266) that we were occasionally seeing on try pushes, and we'll ask for aurora uplift on that.

Comment 152

[[:fabrice] Fabrice Desré]

I know why the emulator test is failing, but I'm not 100% sure what the right fix is. The issue is that the AudioChannelService uses a 'childID' to register channels in the LIFO queue, and thus consider that all audio players in a process are equals. Unfortunately the test runner loads the mochitest iframe OOP, so the sub-iframe ends up in the same process, and we never interrupt the first audio player.

Do we actually want this per-process segregation, or should we be more granular? If we want to keep the process boundary, the simplest thing to do is to disable this test on emulator (it passes on desktop b2g because the setup is different there). If we want to allow several channels per process, some refactoring is needed.

Comment 153

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

At the airport so don't have time to read whole backlog. However generally we try to not separate things per process but per app. That should go for audio policies too.

Comment 154

[[:fabrice] Fabrice Desré]

Right. Let's try to hook up the ipc to TabChild/TabParent instead of ContentChild/ContentParent then.

Comment 155

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Fabrice Desré [:fabrice] from comment #154)
> Right. Let's try to hook up the ipc to TabChild/TabParent instead of
> ContentChild/ContentParent then.

I'm wondering if once the change gets done everything in Gaia side will work as expected. I'll test everything again once the patch being refactored gets attached. Thanks Fabrice.

Comment 156

[Maire Reavy [:mreavy]]

(In reply to Fabrice Desré [:fabrice] from comment #154)
> Right. Let's try to hook up the ipc to TabChild/TabParent instead of
> ContentChild/ContentParent then.

Fabrice -- Just to verify: Can you make this change, or do I need to find someone else?  Or do you have someone in mind?

Andrea, who wrote patch 1, is on PTO until July 25.  (Obviously we're trying to get all the work on this bug done and checked in ASAP.  So if you can make this change, that would help a lot.)  Thanks for identifying the cause of the test failure!

Comment 157

[[:fabrice] Fabrice Desré]

Attachment: wip audio-tabparent.patch

wip patch to hook up to tabparent instead to contentparent. Still a bunch of things to fix (see https://tbpl.mozilla.org/?tree=Try&rev=1a1ff5927963) and I get the feeling that will be too risky for 2.0

I'm also not really familiar with the audio channel management code, so if someone that actually knows it could step up to help that would be great.

Comment 158

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

Andrea and Marco knows this code.

Comment 159

[[:fabrice] Fabrice Desré]

Green try at https://tbpl.mozilla.org/?tree=Try&rev=fcfbf33d8a35

Pushed with the known-to-fail tests disabled:
https://hg.mozilla.org/integration/b2g-inbound/rev/e92cb91b07b4

Comment 160

[Maire Reavy [:mreavy]]

José Antonio -- We (myself, Fabrice, and Jonas) decided the safest/best path forward for gecko in v2.0 was to disable the emulator test and land patch 1.  Can you land patch 2 to resolve this bug?  Thanks!   (And thanks, Fabrice!!)

Comment 161

[Maire Reavy [:mreavy]]

José Antonio -- One last question: can you ask for aurora & "2.0?" approval for this bug?  If you don't feel comfortable doing so or if you need help, let me know.  We want to get this fully landed and uplifted on Monday (tomorrow).

Comment 162

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #160)
> José Antonio -- We (myself, Fabrice, and Jonas) decided the safest/best path
> forward for gecko in v2.0 was to disable the emulator test and land patch 1.
> Can you land patch 2 to resolve this bug?  Thanks!   (And thanks, Fabrice!!)

Everything tested and It continues working fine. I talked to Etienne and we are passing Travis after rebasing the patch. So, once It finishes today we will land.

Comment 163

[José Antonio Olivera Ortega [:jaoo]]

Travis and gaia-try went all green. Landed at:

https://github.com/mozilla-b2g/gaia/commit/ee645d7d0306d972cb3e1f9482278ffbf5e49c46

Comment 164

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #161)
> José Antonio -- One last question: can you ask for aurora & "2.0?" approval
> for this bug?  If you don't feel comfortable doing so or if you need help,
> let me know.  We want to get this fully landed and uplifted on Monday
> (tomorrow).

Still need the gecko patch reaches m-c. Once it gets there, the sheriff will resolve the bug. As Fabrice set this bug as 2.0+ I guess there is no need for approval request, am I right? Thanks!

Comment 165

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e92cb91b07b4

Comment 166

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2055e28772b0

Comment 167

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/4336bac82b7b145a447c9327e1710658f6e1f6a5

Comment 168

[Marco Chen [:mchen]]

[1] show that there might be an app want to fire beep for notifying user something happened during the phone call. But the policy and mechanism here blocks this possibility.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=976678#c22