Last Comment Bug 749794 - B2G telephony: need to reset audio state when phone call ends
: B2G telephony: need to reset audio state when phone call ends
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Hsin-Yi Tsai [:hsinyi]
:
Mentors:
Depends on:
Blocks: b2g-telephony
  Show dependency treegraph
 
Reported: 2012-04-27 14:12 PDT by Philipp von Weitershausen [:philikon]
Modified: 2012-07-18 19:31 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.63 KB, patch)
2012-05-02 00:30 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: review+
Details | Diff | Review

Description Philipp von Weitershausen [:philikon] 2012-04-27 14:12:36 PDT
Apparently enabling/disabling the loud speaker or muting a call continues to have an effect on the audio system after the phone call ends. See https://github.com/andreasgal/gaia/issues/1281#issuecomment-5371389 for the original report.

I suspect we need to reset gAudioManager.microphoneMuted as well as gAudioManager.setForceForUse(...) when the last call ends.
Comment 1 Philipp von Weitershausen [:philikon] 2012-04-27 14:14:14 PDT
Hey Hsinyi, would you mind taking a look at this? Thanks!
Comment 2 Hsin-Yi Tsai [:hsinyi] 2012-04-28 06:52:38 PDT
Sure, I would investigate this.
Comment 3 Hsin-Yi Tsai [:hsinyi] 2012-05-02 00:30:18 PDT
Created attachment 620218 [details] [diff] [review]
Patch

When there's no activeCall, we set 'gAudioManager.phoneState = nsIAudioManger.PHONE_STATE_NORMAL'. And no problem with the current setting when a call ends. 

However, when 'dialer' receives 'disconnected', the dialer will 'toggleMute()' to unmute or disable speaker if user has muted or enabled during the call. In |RadioInterfaceLayer.js| the setter of 'microphoneMuted' and 'speakerEnabled' set gAudioManager.phoneState as IN_CALL or IN_COMMUNICATION first to successfully mute or enable speaker. I.e, 'gAudioManager.phoneState = nsIAudioManger.PHONE_STATE_NORMAL' is overriden. So, this patch resets gAudioManager.phoneState before the setters end. Then video player works fine!
Comment 4 Hsin-Yi Tsai [:hsinyi] 2012-05-02 00:36:52 PDT
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> Created attachment 620218 [details] [diff] [review]
> Patch
> 
> When there's no activeCall, we set 'gAudioManager.phoneState =
> nsIAudioManger.PHONE_STATE_NORMAL'. And no problem with the current setting
> when a call ends. 
> 
> However, when 'dialer' receives 'disconnected', the dialer will
> 'toggleMute()' to unmute or disable speaker if user has muted or enabled
> during the call. In |RadioInterfaceLayer.js| the setter of 'microphoneMuted'
> and 'speakerEnabled' set gAudioManager.phoneState as IN_CALL or
> IN_COMMUNICATION first to successfully mute or enable speaker. I.e,
> 'gAudioManager.phoneState = nsIAudioManger.PHONE_STATE_NORMAL' is overriden.
> So, this patch resets gAudioManager.phoneState before the setters end. Then
> video player works fine!
B.t.w. I still don't know why XXX in the setters is needed. But without XXX, the setters don't work correctly. That's why I repeat the assignment for 'gAudioManager.phoneState' at the end of the setters.
Comment 5 Kan-Ru Chen [:kanru] (UTC+8) 2012-05-02 02:52:44 PDT
Android implementation:

http://androidxref.com/source/xref/frameworks/base/telephony/java/com/android/internal/telephony/CallManager.java#370

It seems audio mode needs to be synced with the phone state.

And how they do mute:

http://androidxref.com/source/xref/frameworks/base/telephony/java/com/android/internal/telephony/CallManager.java#871
Comment 6 Hsin-Yi Tsai [:hsinyi] 2012-05-02 03:21:38 PDT
Kan-Ru, thanks for sharing. Yes, audio system should be synced with phone/call state. And our design considers this as well.
Comment 7 Philipp von Weitershausen [:philikon] 2012-05-03 15:54:00 PDT
Comment on attachment 620218 [details] [diff] [review]
Patch

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +740,5 @@
>      gAudioManager.microphoneMuted = value;
> +
> +    if (!this._activeCall) {
> +      gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_NORMAL;
> +    }

Why set `phoneState` twice? Wouldn't an if/else clause make more sense.

Also, this seems to be missing the actual fixes to `updateCallAudioState`. That said, https://github.com/andreasgal/gaia/issues/1281#issuecomment-5425901 reports the issue to be gone. Can you still reproduce it?
Comment 8 Hsin-Yi Tsai [:hsinyi] 2012-05-03 20:09:06 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #7)
> Comment on attachment 620218 [details] [diff] [review]
> Patch
> 
> Review of attachment 620218 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +740,5 @@
> >      gAudioManager.microphoneMuted = value;
> > +
> > +    if (!this._activeCall) {
> > +      gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_NORMAL;
> > +    }
> 
> Why set `phoneState` twice? Wouldn't an if/else clause make more sense.
> 
We need to set 'phoneState' IN_CALL or IN_COMMUNICATION right before setting 'microphoneMuted', not matter which phoneState and call state we are in. Otherwise, we cannot set 'microphoneMuted' as expected.

So, after we mute the microphone and when there's no active call, we need to reset the phoneState to release the audio system for other functionalities, like video. That's why I need to set 'phoneState' twice.

> Also, this seems to be missing the actual fixes to `updateCallAudioState`.
> That said,
> https://github.com/andreasgal/gaia/issues/1281#issuecomment-5425901 reports
> the issue to be gone. Can you still reproduce it?
Yes, I can. I added a comment there, 
https://github.com/andreasgal/gaia/issues/1281#issuecomment-5425901
Comment 9 Hsin-Yi Tsai [:hsinyi] 2012-05-03 20:12:25 PDT
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> So, after we mute the microphone and when there's no active call, we need to

Sorry, should be 'after we *unmute* the microphone and ....

> reset the phoneState to release the audio system for other functionalities,
> like video. That's why I need to set 'phoneState' twice.
Comment 10 Philipp von Weitershausen [:philikon] 2012-05-14 12:27:40 PDT
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +740,5 @@
> > >      gAudioManager.microphoneMuted = value;
> > > +
> > > +    if (!this._activeCall) {
> > > +      gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_NORMAL;
> > > +    }
> > 
> > Why set `phoneState` twice? Wouldn't an if/else clause make more sense.
> > 
> We need to set 'phoneState' IN_CALL or IN_COMMUNICATION right before setting
> 'microphoneMuted', not matter which phoneState and call state we are in.
> Otherwise, we cannot set 'microphoneMuted' as expected.
> 
> So, after we mute the microphone and when there's no active call, we need to
> reset the phoneState to release the audio system for other functionalities,
> like video. That's why I need to set 'phoneState' twice.

Oh, good point!
Comment 11 Philipp von Weitershausen [:philikon] 2012-05-14 21:14:56 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b351bb34089f
Comment 12 Ed Morley [:emorley] 2012-05-15 06:30:17 PDT
https://hg.mozilla.org/mozilla-central/rev/b351bb34089f
Comment 13 John Hammink 2012-07-18 18:29:20 PDT
https://github.com/mozilla-b2g/gaia/issues/1281 - tested on Otoro build from tip from 7/18/2012 and it works.
Comment 14 Philipp von Weitershausen [:philikon] 2012-07-18 19:31:40 PDT
John, please don't change the resolution to WORKSFORME when there was a patch that landed. Resolutions have a *very specific* meaning (INVALID = not a valid bug, WORKSFORME = can't reproduce it, FIXED = there was a bug and we fixed it, WONTFIX = there is a bug but we won't fix it).

I think you mean VERIFIED FIXED, so I'm changing it to that.

Note You need to log in before you can comment on or make changes to this bug.