800695 - [b2g-bluetooth] Audio route is set to bluetooth sco when disconnected to a bluetooth headset

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

We should check the connection status of a bluetooth headset before we open a Sco socket and route audio to it.

Comment 1

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

Attachment: v1 patch

Check the connection status before we create sco socket and handle volume change.

Comment 2

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

Attachment: Patch 1(v1): Set audio route to sco only when a device is connected

Comment 3

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

Attachment: Patch 2(v1): Cleanup for BluetoothHfpManager and BluetoothScoManager

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 670714 [details] [diff] [review]
Patch 1(v1): Set audio route to sco only when a device is connected

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

Connection status came in with bug 796176, disconnection status is landing in bug 800247 (which I'm putting in after I finish my reviews here). So you don't need to maintain status yourself, the socket will do it for you.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 670715 [details] [diff] [review]
Patch 2(v1): Cleanup for BluetoothHfpManager and BluetoothScoManager

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

Mostly looks good, but we still don't need the status updates in SCOManager because socket now has a status of its own that you can track.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +662,5 @@
> +      case CINDType::CALLSETUP:
> +        message.AppendInt(sCINDItems[CINDType::CALLSETUP].value);
> +        break;
> +      case CINDType::CALLHELD:
> +			  message.AppendInt(sCINDItems[CINDType::CALLHELD].value);

Nit: Indent

Comment 7

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

Attachment: v2 patch

Changes are listed as following:

- Get socket connection status directly from UnixSocketConsumer, instead of cache a local variable "mConnected" in Bluetooth*Manager. I'll also try to get socket address (bug 800249) directly from UnixSocketConsumer in the next version.

- Check the connection status before handling call status change and volume change. Also check it in connect/disconnect function.

- Notify AudioManager after we create/close a SCO socket successfully, (in OnConnectSuccess() and OnDisconnect()).

- Add enums for callstate, callsetupstate, and callheldstate.

- Add a general function for sending command to headset.

- Add function SetupCIND for both EnumerateCallState(aInitial = true) and CallStateChanged(aInitial = false). In the case of EnumerateCallState, we only initialize CIND value and don't send command to a headset. In addition, create SCO after headsfree socket has been connected successfully. While in the case of CallStateChanged, we change CIND value based on new and previous call state, send command to a headset.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 672182 [details] [diff] [review]
v2 patch

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

Looks good. Bug 800249 is r+'d, should land today.

Comment 9

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

Attachment: v3 patch

Modify two parts:

- Get device address via GetSocketAddr() and no cache in Bluetooth*Manager

- We should call get_bdaddr_as_string with only the char array in sockaddr rather than the whole struct object, resulting in a wrong address returned back.

Comment 10

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

Attachment: v3 patch

Comment 11

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 673126 [details] [diff] [review]
v3 patch

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

r+ with nits addressed. Untasking NotifyAudioManagerTask can be a followup, if it even needs to happen at all.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +559,5 @@
>  
>    nsCOMPtr<nsIRILContentHelper> ril =
>      do_GetService("@mozilla.org/ril/content-helper;1");
> +  if (!ril) {
> +    NS_WARNING("Failed to get RIL Content Helper");

nit: This should be a MOZ_ASSERT. Otherwise it's going to die after the if block anyways.

::: dom/bluetooth/BluetoothScoManager.cpp
@@ +246,5 @@
>  BluetoothScoManager::OnConnectSuccess()
>  {
> +  nsString address;
> +  GetSocketAddr(address);
> +  nsRefPtr<NotifyAudioManagerTask> task = new NotifyAudioManagerTask(address);

Does this really need to be a task anymore? We're guarenteed that On* events from UnixSocket happen on the main thread already. Not a big deal, really, just thinking we could consolidate.

::: dom/bluetooth/BluetoothUnixSocketConnector.cpp
@@ +190,5 @@
>  BluetoothUnixSocketConnector::GetSocketAddr(const sockaddr& aAddr,
>                                              nsAString& aAddrStr)
>  {
>    char addr[18];
> +  get_bdaddr_as_string((bdaddr_t*)aAddr.sa_data, addr);

Oops. Thanks for fixing that. :)

Comment 12

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

Attachment: Final version, r=qdot

Will push to try server after the downtime.

Comment 13

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

try server:
https://tbpl.mozilla.org/?tree=Try&rev=d6ab3613a1e3
https://tbpl.mozilla.org/?tree=Try&rev=3e338a99ae6b

Comment 14

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

try server:
https://tbpl.mozilla.org/?tree=Try&rev=6160b0602301

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c42c9ad0d5b4

Comment 16

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/c42c9ad0d5b4

Should this have a test?

Comment 17

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4508d82c92e1