944262 - [B2G]On conference call,we can't switch the speaker between off and on

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Ming]

preconditions: on a conference call

then follows:
1.turn the speaker on : it'll be ok

2.turn the speaker off: it'll fail to turn off the speaker.

with some logs，i found that in Telephonyprovider.js ，on a conference call ,when set speakerEnabled ,it'll set the gAudioManager.phoneState to normal .
As a result , the next time we call gAudioManager.setForceForUse ,it fails. 

  set speakerEnabled(aEnabled) {
    if (aEnabled == this.speakerEnabled) {
      return;
    }
    let force = aEnabled ? nsIAudioManager.FORCE_SPEAKER :
                           nsIAudioManager.FORCE_NONE;
    gAudioManager.setForceForUse(nsIAudioManager.USE_COMMUNICATION, force);

    if (!this._activeCall) {  
      gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_NORMAL;
    }
  },

Comment 1

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

(In reply to Ming from comment #0)
> preconditions: on a conference call
> 
> then follows:
> 1.turn the speaker on : it'll be ok
> 
> 2.turn the speaker off: it'll fail to turn off the speaker.
> 
> with some logs，i found that in Telephonyprovider.js ，on a conference call
> ,when set speakerEnabled ,it'll set the gAudioManager.phoneState to normal .
> As a result , the next time we call gAudioManager.setForceForUse ,it fails. 
> 
>   set speakerEnabled(aEnabled) {
>     if (aEnabled == this.speakerEnabled) {
>       return;
>     }
>     let force = aEnabled ? nsIAudioManager.FORCE_SPEAKER :
>                            nsIAudioManager.FORCE_NONE;
>     gAudioManager.setForceForUse(nsIAudioManager.USE_COMMUNICATION, force);
> 
>     if (!this._activeCall) {  
>       gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_NORMAL;
>     }
>   },

Thanks for reporting the issue.
It looks we miss conference state check before resetting phone state.

We probably should do 

if (!this._activeCall &&
    conferenceState !== nsITelephonyProvider.CALL_STATE_CONNECTED) {
  gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_NORMAL;
}

Comment 2

[Steven Yang [:styang]]

Hsinyi, wanna take it?

Comment 3

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

Taken :)

Comment 4

[James Zhang (Spreadtrum)]

fugu and hamachi also have this issue, can I set koi flag?

Comment 5

[Ken Chang[:kenkai]]

Hsinyi, Any update...:)

Comment 6

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

(In reply to Ken Chang[:ken] from comment #5)
> Hsinyi, Any update...:)

I will try to provide a WIP tomorrow.

Comment 7

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

Change the bug description because this is a general issue, not specific to any device.

Comment 8

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

Attachment: 944262.patch - track the active conference call

Comment 9

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

Comment on attachment 8343613 [details] [diff] [review]
944262.patch - track the active conference call

This issue happens because we don't track 'active' conference call. Instead, we keep only an 'active' 'single' call. Thus, we always hit |if(!this._activeCall)| in [1] that leads to the problem with setting speaker.

This patch resolves the issue by tracking not only active 'single' call but also active 'conference' call.


[1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyProvider.js?from=TelephonyProvider.js#472

Comment 10

[Preeti Raghunath(:Preeti)]

QA,

Please test on Buri device and see if this is reproducible.

Comment 11

[Sarah Parsons]

I was not able to repro this issue on the Buri 1.2 Build ID 20131210004008

Gaia   3bede56043379283cb0f6673730f91be88018d13
SourceStamp e535d93d88ad
BuildID 20131210004008
Version 26.0

I also could not repro on Buri 1.3 Build ID: 20131210004003

Gaia   3452fbdb5e1bed0cd27cc6173136537a03e8072f
SourceStamp e0c328d99742
BuildID 20131210004003
Version 28.0a2

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8343613 [details] [diff] [review]
944262.patch - track the active conference call

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

Thank you! I'm going to attach a test script for this.  Hold on a minute.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Attachment: 944262.patch-2 - test cases

Most of the functions in this test script are copied from "test_conference.js".  Since we have other plan for moving these duplicates into "head.js", I will not touch others now.

Try: https://tbpl.mozilla.org/?tree=Try&rev=965e6f3bc47b

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Attachment: 944262.patch-2 - test cases

Sorry, remove unused bits only.  No change to effective lines.

Comment 15

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

Comment on attachment 8345800 [details] [diff] [review]
944262.patch-2 - test cases

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

Looks great, thanks for the test.

Comment 16

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

   https://hg.mozilla.org/integration/b2g-inbound/rev/f93d32e252e9
   https://hg.mozilla.org/integration/b2g-inbound/rev/d65561224919

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f93d32e252e9
https://hg.mozilla.org/mozilla-central/rev/d65561224919

Comment 18

[Ming]

Hi styang ,pls help to merge this to v1.3

Comment 19

[Steven Yang [:styang]]

It's in v1.3.

Comment 20

[ying.xu]

Hi,steven.

We found this patch on master
not on v1.3
Please check it again

(In reply to Steven Yang [:styang] from comment #19)
> It's in v1.3.

Comment 21

[Steven Yang [:styang]]

1.3+ to make sure the fix be put on 1.3.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e98e973fad6
https://hg.mozilla.org/releases/mozilla-aurora/rev/995f9a8971a5