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

VERIFIED FIXED in Firefox 23

Status

Firefox OS
Bluetooth
P1
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: leo.bugzilla.gecko, Assigned: gyeh)

Tracking

unspecified
1.1 QE1 (5may)
x86
Windows 7
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:leo+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

(Whiteboard: [TD-9127] [fixed-in-birch])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
[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
(Reporter)

Updated

5 years ago
blocking-b2g: --- → leo?
(Assignee)

Updated

5 years ago
Component: General → Bluetooth
(Assignee)

Updated

5 years ago
Assignee: nobody → gyeh
(Assignee)

Comment 1

5 years ago
I'd like to take it. Thanks for reporting.
(Reporter)

Updated

5 years ago
Whiteboard: [TD-9127]

Updated

5 years ago
blocking-b2g: leo? → leo+
(Assignee)

Updated

5 years ago
Summary: [Call][Bluetooth] If press the speaker button, Should hear through speaker when BT sco is disconnect. → [b2g-bluetooth] [Sco] Should set force to FORCE_NONE after Sco is disconnected
(Assignee)

Comment 2

5 years ago
Created attachment 736648 [details] [diff] [review]
Patch 1(v1): Set force to FORCE_NONE whenever Sco is disconnected
Attachment #736648 - Flags: review?(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 :)
Attachment #736648 - Flags: review?(echou)
(Assignee)

Comment 4

5 years ago
Created attachment 736701 [details] [diff] [review]
Patch 1(v2): Set force to FORCE_NONE after disconnected
Attachment #736648 - Attachment is obsolete: true
Attachment #736701 - Flags: review?(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!
Attachment #736701 - Flags: review?(echou) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 736711 [details] [diff] [review]
b2g18 patch, r=echou
https://hg.mozilla.org/mozilla-central/rev/0b32af5340c3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ba1f58492daa
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → wontfix
status-firefox23: --- → fixed
(Reporter)

Comment 10

5 years ago
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 {
--------------------------------------
(Reporter)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
>     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.
Flags: needinfo?(gyeh)
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.
(Assignee)

Comment 13

5 years ago
(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.
Flags: needinfo?(gyeh)
(Assignee)

Updated

5 years ago
Depends on: 862306
(Assignee)

Comment 14

5 years ago
I'll generate new patch for BluetoothScoManager, and it will be landed after bug 862306 is resolved.
Created attachment 737945 [details] [diff] [review]
move bt sco switching logic to audiomanager
(Assignee)

Comment 16

5 years ago
Hi Randy, it works. Please attach it on Bug 862306 and start review process. Thanks. :)
(Assignee)

Comment 17

5 years ago
Created attachment 737960 [details] [diff] [review]
Patch 1(v3): Remove routing logic from BluetoothScoManager
Attachment #737960 - Flags: review?(echou)
Target Milestone: B2G C4 (2jan on) → Leo QE1 (5may)
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.
Attachment #737960 - Flags: review?(rlin)
Attachment #737960 - Flags: review?(echou)
Attachment #737960 - Flags: review+
this patch look for me. :)
Attachment #737960 - Flags: review?(rlin) → review+
Hi Randy,
Could you link AudioManager change to related link?
I has put the patch in Bug 862306 and under reviewing.
(Assignee)

Updated

5 years ago
Whiteboard: [TD-9127] → [TD-9127] [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/996520044b84
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Flags: in-moztrap?

Comment 25

5 years ago
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/
status-b2g18: fixed → verified
Flags: in-moztrap? → in-moztrap+

Updated

5 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.