859727 - [b2g-bluetooth] [Sco] Should set force to FORCE_NONE after Sco is disconnected

Status: VERIFIED FIXED

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

Description

[leo.bugzilla.gecko]

[Precondition]
 BT sco connect - incomming call - on call - press the speaker button - disconnect bluetooth
[symptoms]
 When bt is disconnect, listen by earpiece, speaker button is enabled.

I think geckolayer(call,bluetooth) misuse setForceForUse`s parameter.
When bt is disconnect, recevice AUDIO_POLICY_FORCE_BT_SCO from BT manager.

I`d like to suggest the below case.
---------------------------------------------------------
SetForceForUse(int32_t aUsage, int32_t aForce)
aForce
    AUDIO_POLICY_FORCE_NONE =0         ==> earpiece 
    AUDIO_POLICY_FORCE_SPEAKER = 1
    AUDIO_POLICY_FORCE_HEADPHONES =2 
    AUDIO_POLICY_FORCE_BT_SCO =3

1. BT sco connected
2. Recevied call/on call ==> send FORCE_BT_SCO
3. Press the speaker button ==> send FORCE_SPEAKER
4. disconnect BT sco & observer event in call manager ==> check speaker mode or not ==> Send FORCE_SPEAKER
5. call end ==> Send FORCE_NONE
------------------------------------------------------------
FORCE_NONE(headset/speaker/earpiece) automatically route from getDeviceForStrategy() in AudioPolicyManager.cpp

Comment 1

[Gina Yeh [:gyeh] [:ginayeh]]

I'd like to take it. Thanks for reporting.

Comment 2

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Set force to FORCE_NONE whenever Sco is disconnected

Comment 3

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

Comment on attachment 736648 [details] [diff] [review]
Patch 1(v1): Set force to FORCE_NONE whenever Sco is disconnected

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

::: dom/bluetooth/BluetoothScoManager.cpp
@@ +72,5 @@
>  
>    nsCOMPtr<nsIObserverService> obs =
>      do_GetService("@mozilla.org/observer-service;1");
>    NS_ENSURE_TRUE_VOID(obs);
> +  

nit: trailing whitespaces

@@ +83,5 @@
>      if (NS_FAILED(obs->NotifyObservers(nullptr, BLUETOOTH_SCO_STATUS_CHANGED, nullptr))) {
>        NS_WARNING("Failed to notify bluetooth-sco-status-changed observsers!");
>        return;
>      }
> +    force = am->FORCE_BT_SCO;

It seems that we should set FORCE_NONE here and FORCE_BT_SCO for the 'else' case, doesn't it?

@@ +91,5 @@
>        return;
>      }
>    }
>  
> +  am->SetForceForUse(am->USE_COMMUNICATION, force);

I would suggest that calling SetForceForUse() inside the if-block, so it would be like:

if (aAddress.IsEmpty()) {
  ...
  am->SetForceForUse(am->USE_COMMUNICATION, am->FORCE_BT_SCO);
} else {
  ...
  am->SetForceForUse(am->USE_COMMUNICATION, am->FORCE_NONE);
}

That looks cleaner :)

Comment 4

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v2): Set force to FORCE_NONE after disconnected

Comment 5

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

Comment on attachment 736701 [details] [diff] [review]
Patch 1(v2): Set force to FORCE_NONE after disconnected

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

Looks good!

Comment 6

[Gina Yeh [:gyeh] [:ginayeh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b32af5340c3

Comment 7

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: b2g18 patch, r=echou

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b32af5340c3

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ba1f58492daa

Comment 10

[leo.bugzilla.gecko]

Hi, 
I mistook this case as native code.
"FORCE_NONE(headset/speaker/earpiece) automatically route from getDeviceForStrategy() in AudioPolicyManager.cpp"

Can you modify this code, speaker mode is available by GetForceForUse() in BluetoothScoManager.cpp
This methrd is better than the process in android library 
------------------------------------
BluetoothScoManager::NotifyAudioManager(const nsAString& aAddress) {
.....
  if (aAddress.IsEmpty()) {
    if (NS_FAILED(obs->NotifyObservers(nullptr, BLUETOOTH_SCO_STATUS_CHANGED, nullptr))) {
      NS_WARNING("Failed to notify bluetooth-sco-status-changed observsers!");
	  LOGW("%s:%d [LGBT_gecko] Failed to notify bluetooth-sco-status-changed observsers!", __FUNCTION__, __LINE__);
      return;
    }
    int force;
    am->GetForceForUse(am->USE_COMMUNICATION, &force);

    if(force == am->FORCE_SPEAKER)
        am->SetForceForUse(am->USE_COMMUNICATION, force);
    else
        am->SetForceForUse(am->USE_COMMUNICATION, am->FORCE_NONE);
  } else {
--------------------------------------

Comment 11

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

>     int force;
>     am->GetForceForUse(am->USE_COMMUNICATION, &force);
> 
>     if(force == am->FORCE_SPEAKER)
>         am->SetForceForUse(am->USE_COMMUNICATION, force);
>     else
>         am->SetForceForUse(am->USE_COMMUNICATION, am->FORCE_NONE);
>   } else {
> --------------------------------------

Gina, does this make sense? I'm not familiar with audio settings and not sure if this would be better than current implementation. Please discuss with Shawn/Randy to make the final decision.

Comment 12

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

I suggest move the routing logic to audiomanager and let am to decide what to do, 
Please open another blocker bug for me. 
BTW, if a2dp is on, the setDeviceConnectionState should be turn-on when bt is paired.
once the real sco is established, then call the setForceUse again.

Comment 13

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to leo.bugzilla.gecko from comment #10)
> BluetoothScoManager::NotifyAudioManager(const nsAString& aAddress) {
> .....
>   if (aAddress.IsEmpty()) {
>     if (NS_FAILED(obs->NotifyObservers(nullptr,
> BLUETOOTH_SCO_STATUS_CHANGED, nullptr))) {
>       NS_WARNING("Failed to notify bluetooth-sco-status-changed
> observsers!");
> 	  LOGW("%s:%d [LGBT_gecko] Failed to notify bluetooth-sco-status-changed
> observsers!", __FUNCTION__, __LINE__);
>       return;
>     }
>     int force;
>     am->GetForceForUse(am->USE_COMMUNICATION, &force);
> 
>     if(force == am->FORCE_SPEAKER)
>         am->SetForceForUse(am->USE_COMMUNICATION, force);
>     else
>         am->SetForceForUse(am->USE_COMMUNICATION, am->FORCE_NONE);
>   } else {


I prefer to do that in AudioManager. It would be weird for BluetoothScoManager to set different force values based on the current force values.

Discussed with Randy and Shawn, we'd like to put these logics in AudioManager, which also makes sense to me.

Comment 14

[Gina Yeh [:gyeh] [:ginayeh]]

I'll generate new patch for BluetoothScoManager, and it will be landed after bug 862306 is resolved.

Comment 15

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

Attachment: move bt sco switching logic to audiomanager

Comment 16

[Gina Yeh [:gyeh] [:ginayeh]]

Hi Randy, it works. Please attach it on Bug 862306 and start review process. Thanks. :)

Comment 17

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v3): Remove routing logic from BluetoothScoManager

Comment 18

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

Comment on attachment 737960 [details] [diff] [review]
Patch 1(v3): Remove routing logic from BluetoothScoManager

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

The main idea of this patch is to move logics related to controlling audio to AudioManager. Although it makes sense to me, I would like to redirect this to rlin for another review to make sure this is what we want.

::: dom/bluetooth/BluetoothScoManager.cpp
@@ -88,5 @@
>      if (NS_FAILED(obs->NotifyObservers(nullptr, BLUETOOTH_SCO_STATUS_CHANGED, aAddress.BeginReading()))) {
>        NS_WARNING("Failed to notify bluetooth-sco-status-changed observsers!");
>        return;
>      }
> -    am->SetForceForUse(am->USE_COMMUNICATION, am->FORCE_BT_SCO);

Since we are going to move these operations into AudioManager, variable |am| would become unused. Please remove it as well.

Comment 19

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

this patch look for me. :)

Comment 20

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi Randy,
Could you link AudioManager change to related link?

Comment 21

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

I has put the patch in Bug 862306 and under reviewing.

Comment 22

[Gina Yeh [:gyeh] [:ginayeh]]

https://hg.mozilla.org/projects/birch/rev/996520044b84

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/996520044b84

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/3533e3017f32

Comment 25

[Parul Mathur]

Verified fixed on:
Device: Unagi phone 
Build Identifier: 20130610070206
Update channel: unagi/1.1.0/beta
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8e3f39363c54
Gaia: 43e1b73371c7e7e6b9fde90ea2d0d5e7745b97051370888635
Git commit info: 2013-06-10 18:23:55 
OS version: 1.1.0.0-prerelease

Prerequisites: 
2 phones and 1 bluetooth headset are needed. The B2G phone should have bluetooth turned on and paired with the bluetooth headset.

Steps to Reproduce:
1. Make a phone call from the second phone to the B2G phone.
2. When the B2G phone gets the incoming call, turn on speaker phone.
3. Disconnect the bluetooth headset from the B2G phone.

Expected Result:
The audio on the incoming call should still be routed through the speaker phone.

Actual Result:
Matches the expected result.

Existing test case in MozTrap: https://moztrap.mozilla.org/manage/case/6074/