Closed Bug 800695 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: gyeh, Assigned: gyeh)

References

Details

Attachments

(1 file, 6 obsolete files)

We should check the connection status of a bluetooth headset before we open a Sco socket and route audio to it.
Attached patch v1 patch (obsolete) — Splinter Review
Check the connection status before we create sco socket and handle volume change.
Attachment #670690 - Flags: review?(kyle)
Attachment #670690 - Attachment is obsolete: true
Attachment #670690 - Flags: review?(kyle)
Attachment #670714 - Flags: review?(kyle)
Duplicate of this bug: 799422
blocking-basecamp: ? → +
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.
Attachment #670714 - Flags: review?(kyle)
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
Attachment #670715 - Flags: review?(kyle)
blocking-basecamp: + → ?
Summary: [b2g-bluetooth] Audio route is set to sco before connecting to a bluetooth headset → [b2g-bluetooth] Audio route is set to bluetooth sco when disconnected to a bluetooth headset
Attached patch v2 patch (obsolete) — Splinter Review
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.
Attachment #670714 - Attachment is obsolete: true
Attachment #670715 - Attachment is obsolete: true
Attachment #672182 - Flags: feedback?(kyle)
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.
Attachment #672182 - Flags: feedback?(kyle) → feedback+
Attached patch v3 patch (obsolete) — Splinter Review
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.
Attachment #672182 - Attachment is obsolete: true
Attachment #673122 - Flags: review?(kyle)
Attached patch v3 patch (obsolete) — Splinter Review
Attachment #673122 - Attachment is obsolete: true
Attachment #673122 - Flags: review?(kyle)
Attachment #673126 - Flags: review?(kyle)
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. :)
Attachment #673126 - Flags: review?(kyle) → review+
Will push to try server after the downtime.
Attachment #673126 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c42c9ad0d5b4

Should this have a test?
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.