904025 - [B2G][WebRTC] media stream should stop when another high priority application request microphone resource

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

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

STR. 
1. gUM and start webRTC or recording. 
2. User receive a phone call dial in or user dial out a call, the remote side can't hear sounds. (Base on the patch that StevenLee used for b2g, seems the input device is holded by gUM)

For mobile device, we may need to stop the recoring/webRTC function and let phone call works well.

Comment 1

[Jason Smith [:jsmith]]

Seems like this is similar to bug 898965.

Comment 2

[u459114]

Sort of different. bug 898965 is focus on handling stop event from media stream. This bug is more upstream. MediaStream should register to audio channel service to know someone which has higher prioroty try to aqcuire microphone HW, release mircrophone immidiatly and sent a stop event toward media recorder(a media stream client).

Comment 3

[StevenLee[:slee]]

I think recording and webrtc should be different. 

* recording
** We should stop it when there is phone call coming.
** We may solve it by registering AudioChannelAgent with "content" level. When phone call is coming, ringer will stop the content level channel and callback. We can implement this mechanism in MediaRecorder. baku, do you think it is workable?

* webrtc
** When someone calls, we should pop up a dialog and users can decide whether they want to take that phone call or not.
** There are some interactions with RIL. I need some time to discuss with RIL guys.

We can divide this problem into 2 parts, recording and webrtc. We can solve the "recording" part first since it should be landed in 1.2. I will file another bug for webrtc.

Comment 4

[Timothy B. Terriberry (:derf)]

How do you plan to determine which task a page is using the gUM stream for (and what if it's neither, or both)?

Comment 6

[Jason Smith [:jsmith]]

Note - this bug impacts gUM generally (not a media recording API bug specifically), so I've switched this bug to track against the microphone gUM API user story bug.

Comment 7

[Ivan Tsay (:ITsay)]

(In reply to StevenLee from comment #3)
> I think recording and webrtc should be different. 
> 
> * recording
> ** We should stop it when there is phone call coming.
> ** We may solve it by registering AudioChannelAgent with "content" level.
> When phone call is coming, ringer will stop the content level channel and
> callback. We can implement this mechanism in MediaRecorder. baku, do you
> think it is workable?
> 
> * webrtc
> ** When someone calls, we should pop up a dialog and users can decide
> whether they want to take that phone call or not.
> ** There are some interactions with RIL. I need some time to discuss with
> RIL guys.
> 
> We can divide this problem into 2 parts, recording and webrtc. We can solve
> the "recording" part first since it should be landed in 1.2. I will file
> another bug for webrtc.

I also think we should resolve recording part first for v1.2. We can just use this bug for recording part implementation. This bug should be nominated to koi+

Comment 8

[StevenLee[:slee]]

(In reply to Timothy B. Terriberry (:derf) from comment #4)
> How do you plan to determine which task a page is using the gUM stream for
> (and what if it's neither, or both)?
derf,

Please forget what I said. After discussing with JW, I am using other method to solve this problem. 

When there is an incoming phone call, there will be a dialog prompt to ask users whether they want to take that phone call or not. If users choose yes, RIL tells AudioManager and changes "phone state." When this happens, we should turn off all MediaStream that are using mic.

Comment 9

[StevenLee[:slee]]

Attachment: stop MediaStream when users decide to take a phone call

This is a WIP version.

1. When users take a phone call, it changes the phone state. Once the phone state is changed, ObserverService sends "phone-state-changed."
2. ContentParent gets the status change and notify ContentChild.
3. MediaManager gets informed and stops the MediaStream.

Comment 10

[Jason Smith [:jsmith]]

(In reply to StevenLee from comment #8)
> (In reply to Timothy B. Terriberry (:derf) from comment #4)
> > How do you plan to determine which task a page is using the gUM stream for
> > (and what if it's neither, or both)?
> derf,
> 
> Please forget what I said. After discussing with JW, I am using other method
> to solve this problem. 
> 
> When there is an incoming phone call, there will be a dialog prompt to ask
> users whether they want to take that phone call or not. If users choose yes,
> RIL tells AudioManager and changes "phone state." When this happens, we
> should turn off all MediaStream that are using mic.

Hmm...are we sure this is the right UX approach? What does Android do here? What does IPhone do here? I can see this as debatable as when a user wants to answer a call, they want to move quickly to answer it and not be blocked with an additional prompt. Then again, I understand the concern to give the user indication that the recording will be killed if we enter a call. Could we perhaps indicate the warning in the dialer attention screen instead if recording is present?

UX - Can we get some input here on the right approach?

Comment 11

[Stephany Wilkes]

Permissions really needs a larger, holistic UX pattern across the OS but, in the meantime, I am flagging Rob but also Francis, since the dialer attention screen is mentioned. 

Rob and Francis, Esther and Jacqueline have done extensive rounds of spec work here for 1.2 that will probably help.

Comment 12

[StevenLee[:slee]]

(In reply to Jason Smith [:jsmith] from comment #10)
> Hmm...are we sure this is the right UX approach? What does Android do here?
> What does IPhone do here? I can see this as debatable as when a user wants
I checked my iphone. Here are the scenarios.
Voice memos: V
Phone call: P
FaceTime: F

1. V -> P/F (recording and then there is incoming phone call/FaceTime)
   stop V directly
2. P/F -> V 
   Prompt dialog "To record voice memos, end call". User can only press OK button and the phone call keeps working.
3. P -> F or F -> P
   Prompt dialog and user can choose whether to take this phone call or not

Comment 13

[Timothy B. Terriberry (:derf)]

(In reply to StevenLee from comment #8)
> When there is an incoming phone call, there will be a dialog prompt to ask
> users whether they want to take that phone call or not. If users choose yes,
> RIL tells AudioManager and changes "phone state." When this happens, we
> should turn off all MediaStream that are using mic.

This sounds like a much better plan. Namely, I think this one could actually be implemented.

Comment 14

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

Comment on attachment 797097 [details] [diff] [review]
stop MediaStream when users decide to take a phone call

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

::: dom/media/MediaManager.cpp
@@ +59,5 @@
>  #define LOG(msg)
>  #endif
>  
>  
> +int64_t gWindow;

I'm assuming when this WIP patch is finished, this global will get replaced with something else.

Comment 15

[StevenLee[:slee]]

Attachment: part1: pass phone states from ContentParent to ContentChild

Hi Kyle,

Would you please review this patch?
Thanks.

Comment 16

[StevenLee[:slee]]

Attachment: part 2: stop MediaStream when phone state changes to PHONE_STATE_IN_CALL

Hi Randell,

Please help review this patch.
Thanks.

Comment 17

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

Comment on attachment 799369 [details] [diff] [review]
part 2: stop MediaStream when phone state changes to PHONE_STATE_IN_CALL

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

What happens when the call ends?  This handles only a single transition, and it stops the media streams but there's no other notification to the app so it can do something appropriate.  Unlike normal OnNavigation which stops media streams, the app will simply stop getting data but keep running.  This touches on the spec (in MediaCapture) as well, since I don't think this case has been considered.

This code (with the tweaks mentioned) should be ok, but I want to make sure we've thought through the implications, so r- for now

::: dom/media/MediaManager.cpp
@@ +1570,5 @@
>  
> +void
> +MediaManager::StopMediaStreams()
> +{
> +  nsISupportsArray* array;

nsCOMPtr<nsISupportsArray> array

@@ +1578,5 @@
> +  for (uint32_t i = 0; i < len; i++) {
> +    nsCOMPtr<nsISupports> window;
> +    array->GetElementAt(i, getter_AddRefs(window));
> +    nsCOMPtr<nsPIDOMWindow> win(do_QueryInterface(window));
> +    OnNavigation(win->WindowID());

null-check win

@@ +1580,5 @@
> +    array->GetElementAt(i, getter_AddRefs(window));
> +    nsCOMPtr<nsPIDOMWindow> win(do_QueryInterface(window));
> +    OnNavigation(win->WindowID());
> +  }
> +  NS_RELEASE(array);

Not needed if you convert to an NSCOMPtr array

Comment 18

[StevenLee[:slee]]

(In reply to Randell Jesup [:jesup] from comment #17)
> What happens when the call ends?  This handles only a single transition, and
> it stops the media streams but there's no other notification to the app so
> it can do something appropriate.  Unlike normal OnNavigation which stops
> media streams, the app will simply stop getting data but keep running.  This
> touches on the spec (in MediaCapture) as well, since I don't think this case
> has been considered.
In the current m-c, MediaRecorder receives TRACK_EVENT_ENDED, stops recording and can tells the apps what happens now.
I think we can use MediaStream::onended to replace the notification. Apps can get informed from this callback. And I think apps should also get "phone-state-changed" information since they are telephony applications, too. I can file another bug to implement onended callback.

Comment 19

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

(In reply to StevenLee from comment #18)
> (In reply to Randell Jesup [:jesup] from comment #17)
> > What happens when the call ends?  This handles only a single transition, and
> > it stops the media streams but there's no other notification to the app so
> > it can do something appropriate.  Unlike normal OnNavigation which stops
> > media streams, the app will simply stop getting data but keep running.  This
> > touches on the spec (in MediaCapture) as well, since I don't think this case
> > has been considered.
> In the current m-c, MediaRecorder receives TRACK_EVENT_ENDED, stops
> recording and can tells the apps what happens now.
> I think we can use MediaStream::onended to replace the notification. Apps
> can get informed from this callback. And I think apps should also get
> "phone-state-changed" information since they are telephony applications,
> too. I can file another bug to implement onended callback.

The working group is thinking of removing onended on MediaStreams.  Notifications will be available on tracks (MediaStreamTracks), though this can be more annoying to deal with.

Note that many web apps that use getUserMedia are not telephony applications, and would not expect asynchronous events to cut off their MediaStreams (though we can try to encourage them to watch for it with examples, I suspect most still will not do so - even desktop-focused video call/conference apps may not, and none currently do).  It does "fail safe" in that they just stop receiving audio and video, and the other end of the PeerConnection may see the call stay "alive" but stop receiving audio and video data (likely the app will time out and assume the connection is lost).

Comment 20

[StevenLee[:slee]]

(In reply to Randell Jesup [:jesup] from comment #19)
> The working group is thinking of removing onended on MediaStreams. 
> Notifications will be available on tracks (MediaStreamTracks), though this
> can be more annoying to deal with.
It sounds bad. :( They want to remove onended because of duplicate? If they really remove onended, how do apps get the status of MediaStream? Will they add some other callbacks?

> Note that many web apps that use getUserMedia are not telephony
> applications, and would not expect asynchronous events to cut off their
> MediaStreams (though we can try to encourage them to watch for it with
> examples, I suspect most still will not do so - even desktop-focused video
> call/conference apps may not, and none currently do).  It does "fail safe"
> in that they just stop receiving audio and video, and the other end of the
> PeerConnection may see the call stay "alive" but stop receiving audio and
> video data (likely the app will time out and assume the connection is lost).
On desktop applications, they won't encounter the problem. But if they want to run their applications on mobile devices, they should consider this situation. When there is an incoming call and the applications do not handle it, it should be the applications' bug. 
For PeerConnection, we should also handle the incoming call case, ex hang up this call? 

Could we land this patch first and we can discuss the scenario on another bug? Since media recording is on FFOS 1.2 and this bug makes users cannot talk on FFOS when they are recording audio(though I think it happens rarely but when it happens it should be very frustrated). 
Thanks.

Comment 21

[StevenLee[:slee]]

Attachment: part 2: stop MediaStream when phone state changes to PHONE_STATE_IN_CALL - v2

address comment 17

Comment 22

[Stephany Wilkes]

Flagging Jacqueline, just in case there is any UX outstanding that isn't already covered in the design etherpad and specs.

Comment 23

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

Comment on attachment 802053 [details] [diff] [review]
part 2: stop MediaStream when phone state changes to PHONE_STATE_IN_CALL - v2

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

With the one fix mentioned, r+

Discussion on what the implications are for apps can occur on bug 914494 (which I'll retitle).  We do need to consider this in the 1.2 timeframe, as it's not just for webrtc calls, but it impacts any website using getUserMedia() when a call comes in (a photo-strip site, etc).  However, this doesn't mean we have to have a solution to the problem in 1.2 (i.e. this isn't a blocking issue).

::: dom/media/MediaManager.cpp
@@ +1575,5 @@
> +  for (uint32_t i = 0; i < len; i++) {
> +    nsCOMPtr<nsISupports> window;
> +    array->GetElementAt(i, getter_AddRefs(window));
> +    nsCOMPtr<nsPIDOMWindow> win(do_QueryInterface(window));
> +    if (win->WindowID()) {

Null-check win, not windowID (which is not a pointer).

Comment 24

[StevenLee[:slee]]

Attachment: part1: pass phone states from ContentParent to ContentChild

Adding summary for the patch

Comment 25

[StevenLee[:slee]]

Attachment: part 2: stop MediaStream when phone state changes to PHONE_STATE_IN_CALL - v3

address comment 23

Comment 26

[StevenLee[:slee]]

Got building error on mac. I will fix it first then request checkin.

Comment 27

[StevenLee[:slee]]

Attachment: part 2: stop MediaStream when phone state changes to PHONE_STATE_IN_CALL - v4

Fix the compiling error on Mac and linux. Here is the try server log.
* try: -b o -p linux64,macosx64,win64,android,emulator,linux64_gecko -u none -t none 
** https://tbpl.mozilla.org/?tree=Try&rev=7d7cbc7fc0a7

Comment 28

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4a140c514375
https://hg.mozilla.org/integration/b2g-inbound/rev/ec5aab1c32e9

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4a140c514375
https://hg.mozilla.org/mozilla-central/rev/ec5aab1c32e9

Comment 30

[Jason Smith [:jsmith]]

Verified that the use case as specified in this bug works. There was a different use case found that didn't work with calls in bug 918054, but that's tracked in a separate bug.

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 804158 [details] [diff] [review]
part 2: stop MediaStream when phone state changes to PHONE_STATE_IN_CALL - v4

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

::: dom/media/MediaManager.cpp
@@ +1449,5 @@
>    }
> +#ifdef MOZ_WIDGET_GONK
> +  else if (!strcmp(aTopic, "phone-state-changed")) {
> +    nsString state(aData);
> +    if (atoi((const char*)state.get()) == nsIAudioManager::PHONE_STATE_IN_CALL) {

Please explain.

Comment 32

[StevenLee[:slee]]

Comment on attachment 804158 [details] [diff] [review]
part 2: stop MediaStream when phone state changes to PHONE_STATE_IN_CALL - v4

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

When someone is calling and user decides to take this call,
1. RIL calls AudioManager::SetPhoneState to change the phone status. 
2. AudioManager broadcast "phone-state-changed" message. 
3. MediaManager in the content process gets the "phone-state-changed" and finds that the users decide to take this phone(PHONE_STATE_IN_CALL)
4. stop the media streams

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I meant the part where you're casting a PRUnichar* (two bytes per character) to char* (one byte per character. Your code is broken on big-endian systems and as soon as your state gets more than one digit. This should have been caught in review, but apparently wasn't.

Comment 34

[StevenLee[:slee]]

(In reply to :Ms2ger from comment #33)
> I meant the part where you're casting a PRUnichar* (two bytes per character)
> to char* (one byte per character. Your code is broken on big-endian systems
> and as soon as your state gets more than one digit. This should have been
> caught in review, but apparently wasn't.
I will file another bug fixing this problem. Thanks for your reminding.