1092346 - At the end of a call the proper volume control channel is not restored in the dialer app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

Attachment: channel-change-bug.diff

While working on bug 834530 we noticed a problem that prevented us from using the `notification' channel in the dialer as our UX specs mandated: once a call ended the volume control channel used by the dialer (notification) would be set but then quickly overridden by the volume control channel used by the system app.

The STR is the following:

1. Have the changes from bug 834530 applied

2. Apply the attached patch, it will ensure that the dialer uses the `notification' channel to play sounds and sets it as its volume control channel (note that you might need to reset gaia after applying the patch as it changes the dialer's permissions)

3. Launch the dialer and adjust the volume, the `notification' channel is being used

4. Start a call, wait for the callscreen to come up then end it

5. Once you're back in the dialer adjust the volume again, the `content' channel is now being used

I've spent some time digging into why this is happening and I've found that the issue is triggered by the AudioChannelManager in response to visibility events. When closing the callscreen the dialer becomes visible again, this triggers the visibilitychange handler here:

http://hg.mozilla.org/mozilla-central/file/675913ddbb55/dom/system/gonk/AudioChannelManager.cpp#l148

This in turn will send an IPC message that is passed via the ContentParent to the main process:

http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/ipc/ContentParent.cpp#l2372

... and sets the current default volume control channel to the one used by the dialer, the `notification' channel.

Once this has happened the same visibilitychange handler is triggered once again but this time in the main process (i.e. on behalf of the system app I suppose). Since we're in the main process this adjusts the default control channel directly and sets it to the `content' channel because the system app doesn't have a default set and thus is on the `unknown' channel and triggers this path:

http://hg.mozilla.org/mozilla-central/file/675913ddbb55/dom/system/gonk/AudioChannelManager.cpp#l138

Unfortunately I'm unsure if it's the AudioChannelManager that is at fault (and should ignore the second change) or if we're sending the visibility events in the wrong order; so for now I'm filing this in the AudioChannel component but it might as well be a Gaia::System::Window Mgmt issue.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

This is something I've encountered when working on bug 834530. It prevents us from properly using the telephony channel for touch tones and might have other implications so I'd like to nominate it for 2.2. It's an issue of polish but it might be hiding some other underlying problems; see comment 0 for the detailed description and STR.

Comment 2

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

Hi Gabriele, 
If you use notification channel to playback the DTMF tone with plug-in the headset in the device, do you hear the sound from speaker? 
In this case, the sound should go through the headset only.

BTW, what's the main problem right now? 
When the call end, user try to adjust the volume but he adjust the "content" channel? Or else?

Comment 3

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Randy Lin [:rlin] from comment #2)
> If you use notification channel to playback the DTMF tone with plug-in the
> headset in the device, do you hear the sound from speaker? 
> In this case, the sound should go through the headset only.

Yes, that's not the issue here.

> BTW, what's the main problem right now? 
> When the call end, user try to adjust the volume but he adjust the "content"
> channel? Or else?

The main problem is that if I set the dialer app to use the `notification' channel, make a call (which brings me to the callscreen), then go back to the dialer what I get is the `content' channel instead. Basically the channel being used in the dialer is not restored correctly after a call.

Comment 4

[Alastor Wu [:alwu]]

Hi, Gabriele,
I can't understand very well about why the dialer would become a 'content' channel after the call.
Do you mean that the system app change the audio channel type of dialer app?
Thanks!

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Alastor Wu [:alwu] from comment #4)
> I can't understand very well about why the dialer would become a 'content'
> channel after the call.
> Do you mean that the system app change the audio channel type of dialer app?

Not directly. The channel is changed by the AudioChannelManager in response to visibility events; however visibility of the apps - and specifically of the callscreen - is set by the system app. So either the problem is with the AudioChannelManager that is not correctly interpreting the visibility status or with the system app that is not setting it correctly.

Comment 6

[Alastor Wu [:alwu]]

Attachment: Make default audio volume type consistent

[Root cause]
After the dialer change the default audio type, the system app also change it.
But the actual active app is the dialer app, we should avoid other apps to modify the default audio type.

[Solution]
Check the default window id to make sure the modification is allowed.

Comment 7

[Alastor Wu [:alwu]]

Attachment: Make default audio volume type consistent

After pushing this patch to the try-server, the result seems ok.

Hi, Baku,
Could you help me review this patch?
Very appreciate :)

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8558560 [details] [diff] [review]
Make default audio volume type consistent

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

::: dom/audiochannel/AudioChannelService.cpp
@@ +525,5 @@
>    // If this child is in the background and mDefChannelChildID is set to
>    // others then it means other child in the foreground already set it's
>    // own default channel already.
> +  if ((!aHidden && mDefChannelChildID != aChildID) ||
> +      (mDefChannelChildID != aChildID && 

extra space.

@@ +526,5 @@
>    // others then it means other child in the foreground already set it's
>    // own default channel already.
> +  if ((!aHidden && mDefChannelChildID != aChildID) ||
> +      (mDefChannelChildID != aChildID && 
> +      mDefChannelChildID != CONTENT_PROCESS_ID_UNKNOWN)) {

indent 1 space more this line.

Comment 9

[Gabriele Svelto [:gsvelto] (PTO)]

I'll give this a spin ASAP with both the regular and emergency dialer and report. BTW if the fix was so easy then I wholeheartedly suggest that we uplift it to 2.2. We might even go as far as uplifting to 2.1 so we'd have consistent audio channels there too.

Comment 10

[Alastor Wu [:alwu]]

Attachment: Make default audio volume type consistent. r=baku.

Comment 11

[Alastor Wu [:alwu]]

Hi, Gabriele,
The code of this function is the same as previous versions, so I think it could be uplifted easily.

Comment 12

[Alastor Wu [:alwu]]

Try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4eaf0055277

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6b2398b86cc3

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6b2398b86cc3

Comment 15

[Gabriele Svelto [:gsvelto] (PTO)]

Alastor, I'm experiencing a regression that I hadn't noticed before. If I open the SMS app, or the e-mail app both of which set the 'notification' default volume control channel I get instead the 'content' one. Reverting your patch fixes the problem. Could you have a look?

Comment 16

[Alastor Wu [:alwu]]

No problem, I will check it!
Keep ni for tracking.

Comment 17

[Alastor Wu [:alwu]]

The previous patches could fix this bug, but it's not a good solution.
I would file another bug to handle the issue. The new patch could both solve the issue of this bug and the regression errors of Comment15. 
See Bug 1133449.

Comment 18

[Alastor Wu [:alwu]]

Sorry for error changing the bug state.