815445 - Hook up telephony audio to audio channels backend

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

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

There's two things needed here:

* When telephony audio starts to play, i.e. when the user accepts an incoming phone call or places a phone call, all lower-priority audio channels should be automatically paused/muted.

* When a higher-priority audio channel is playing, the audio from the telephony should be silenced. This probably isn't really needed for v1 since the only higher-priority audio channels are "ringer", which will only be used by the telephony app, and "publicnotification" which is out for v1.

Comment 1

[Dietrich Ayala (:dietrich)]

Blocks a bunch of blockers -- should also block?

Comment 2

[Dietrich Ayala (:dietrich)]

Randy, once bug 805333 lands, what is your estimate for how long will this take to fix?

Comment 4

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

(In reply to Dietrich Ayala (:dietrich) from comment #2)
> Randy, once bug 805333 lands, what is your estimate for how long will this
> take to fix?
As discussed with kevin Hu, we may need 5 days to fix this & Bug 815452.

Comment 5

[Kai-Chih Hu [:khu]]

Hi, Dietrich, you can refer this document: 
https://docs.google.com/spreadsheet/ccc?key=0AjIcGNW1bFV5dEJxa09WeEhGT2tuVC1mTko0Sko3ZkE#gid=0

We've updated the status and ETA there.

Comment 6

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 7

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

Attachment: methodology review

try server:https://tbpl.mozilla.org/?tree=Try&rev=6ad981eb9573

Comment 8

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

Comment on attachment 688568 [details] [diff] [review]
methodology review

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

Andrea should really be reviewing this. The only problem I'm seeing is that this doesn't technically make the "ringer" channel take precedence over the "telephony" channel. I.e. if someone uses the "ringer" channel that won't silence any telephony sound.

It's unclear that we would want "ringer" to override "telephony" though. But that's the order the channels are in right now. Possibly we should switch that?

In any case this is a theoretical problem since I don't think anyone is planning on using the "ringer" channel when there's a phone call connected. So the patch looks good to me.

But you should get a real review from someone else.

Comment 9

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

You might want to assert that you are in the parent process though. Since I think this code won't work when run from a child process.

Comment 10

[Marco Chen [:mchen]]

Hi Randy, 

I think we can eliminate the consideration of ringer channel here.
Because ringer will be played as a media element with ringer channel type.

So the audio channel service itself will try to mute other low priorities channels.
This will be added by bug 815322. Thanks.

Comment 11

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

Marco: The problem is this.

As things are currently defined, the "ringer" channel has a higher priority than the "telephony" channel. That means that if the user is on a call when an app uses the "ringer" audio channel, the call should be muted.

The patch currently does not implement this behavior. I.e. even if another app used the "ringer" channel, the telephony audio would not be muted.

However I would say that that's ok for two reasons.

1. The "ringer" channel should probably be a lower priority than "telephony" anyway.

2. No built-in app is currently using the "ringer" channel, other than the telephony
   app. And it's not going to use the "ringer" channel when the user is on a call.
   And we likely won't grant 3rd party apps the permission to use the "ringer"
   channel.

I think that we should fix 1. I.e. we should make "ringer" have lower priority than "telephony".

Comment 12

[Marco Chen [:mchen]]

H(In reply to Jonas Sicking (:sicking) from comment #11)
> Marco: The problem is this.
> 

Yes, I agree with problem. And the main reason for my comment is want to highlight that the implementation here can eliminate the ring state from telephony. Because another media element will be associated with ringer channel and follow the rule on bug 805333. Just please just keep on in_call state only.

Comment 13

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

Comment on attachment 688568 [details] [diff] [review]
methodology review

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

Ah you are correct indeed.

And after talking with Randy it seems like we also need to make this callable from child processes, so there's more work needed here.

Comment 14

[Joe Cheng [:jcheng] (please needinfo)]

this bug blocks other C2 bugs, Change to C2 to be consistent

Comment 15

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

For telephony case, I make sure that audioManager run in chrome process, so I try to merge IPC part of Bug: 805333 and the music can be mute during the phone call, unmute after the phone call.

Comment 16

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

Attachment: patch 2

fix conflict

Comment 17

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

Comment on attachment 689065 [details] [diff] [review]
patch 2

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

I'm not sure I understand comment 15.

Are you saying that it's *already* the case that the AudioManager only runs in the parent process? Or are you saying that you will make sure that that is the case *after* landing this patch?

If Andrea is around when you have an updated patch, feel free to just ask him for review.

::: dom/audiochannel/AudioChannelService.cpp
@@ +282,5 @@
> +AudioChannelService::SetPhoneState(int32_t aState)
> +{
> +  //while ring tone and in-call mode, mute media element
> +  if (aState == nsIAudioManager::PHONE_STATE_RINGTONE ||
> +    aState == nsIAudioManager::PHONE_STATE_IN_CALL) {

As was pointed out by Marco in comment 10 and comment 12, you should remove the ringtone state from this test.

This means that you can probably simplify the SetPhoneState API to just be something like: SetPhoneInCall(bool aActive)

::: dom/system/gonk/AudioManager.cpp
@@ +303,5 @@
> +  nsRefPtr<AudioChannelService> audioChannelService = AudioChannelService::GetAudioChannelService();
> +  if (!audioChannelService) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  audioChannelService->SetPhoneState(aState);

Then change this to be SetPhoneInCall(aState == nsIAudioManager::PHONE_STATE_IN_CALL)

Comment 18

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

1. I have check the telephony part and that is running in chrome process.
2. OK, I will remove the ringtone check and change the API name.

Comment 19

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

Attachment: patch 3

1. API change
2. remove ringtone check

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 689136 [details] [diff] [review]
patch 3

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

::: dom/audiochannel/AudioChannelService.cpp
@@ +282,5 @@
> +AudioChannelService::SetPhoneInCall(bool aActive)
> +{
> +  //while ring tone and in-call mode, mute media element
> +  if (aActive) {
> +    mChannelCounters[AUDIO_CHANNEL_TELEPHONY] = 1;

Probably it's better to do:

if (aActive) {
  ++mChannelCounters[AUDIO_CHANNEL_TELEPHONY];
} else {
  --mChannelCounters[AUDIO_CHANNEL_TELEPHONY];
  MOZ_ASSERT(mChannelCounters[AUDIO_CHANNEL_TELEPHONY] >= 0);
}

Maybe we can have more than 1 audio element type=telephony playing at the same time.

Comment 21

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

Attachment: checkin-patch

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b21fbf188510

Comment 23

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/dc032b657836
https://hg.mozilla.org/releases/mozilla-beta/rev/f1da34a1b018

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

NB: marking RESO FIXED when these bugs hit beta because it's getting hard for b2g folks to track remaining work for upcoming milestones.

Comment 25

[Ed Morley [:emorley]]

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

Comment 26

[John Shih]

verified on 2012-12-12 unagi daily build
build info:
releases/gaia : db435724709181ffa3348c06cfcc80b71375e73d
releases/gecko : 8e66dde61aec02ba5a4a4e44583679aedc609a73