Closed Bug 911635 Opened 7 years ago Closed 4 years ago

HFP 1.6 optional item: Wideband Speech

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.2r+, b2g-v2.2r fixed)

RESOLVED FIXED
feature-b2g 2.2r+
Tracking Status
b2g-v2.2r --- fixed

People

(Reporter: echou, Assigned: wiwang)

References

Details

(Whiteboard: [red-tai][ETA=8/31])

Attachments

(1 file, 3 obsolete files)

We have implemented mandatory items for HFP 1.6 (bug 879635), and now we can start to see if it's possible to support Wideband Speech in the next version.
Blocks: b2g-hfp-16
No longer blocks: b2g-hfp-16
Blocks: b2g-hfp-16
Hi Will,
Please make sure WBS can correctly work on 8909 platform.
Blocks: 1181901
feature-b2g: --- → 2.2r+
Whiteboard: [red-tai] → [red-tai][ETA=8/31]
Depends on: 1189315
Attached patch 0001-WBS-git-patch.patch (obsolete) — Splinter Review
Hi Shawn

Could you help to review this patch?
Thanks!
Attachment #8655370 - Flags: review?(shuang)
Comment on attachment 8655370 [details] [diff] [review]
0001-WBS-git-patch.patch

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

You missed the change for fallback folder.

::: dom/bluetooth/bluedroid/b2g_bdroid_buildcfg.h
@@ +35,4 @@
>                               BTA_AG_FEAT_ECNR | \
>                               BTA_AG_FEAT_REJECT | \
>                               BTA_AG_FEAT_ECS    | \
> +                             BTA_AG_FEAT_CODEC  | \

This file will be removed. You can consult with tzimmermann to land the configuration into another repository.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1516,5 @@
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  NS_ENSURE_TRUE_VOID(obs);
> +
> +  if (HFP_WBS_YES == aWbs) {
> +  mWbsEnabled = true;

nit: Fix the coding style.

@@ +1518,5 @@
> +
> +  if (HFP_WBS_YES == aWbs) {
> +  mWbsEnabled = true;
> +  } else {
> +  mWbsEnabled = false;

Ditto.

::: dom/system/gonk/AudioManager.cpp
@@ +392,5 @@
> +          AudioSystem::setParameters(0, cmd);
> +      } else {
> +          cmd.setTo("bt_wbs=off");
> +          AudioSystem::setParameters(0, cmd);
> +      }

nit: Fix the indentation.
Attachment #8655370 - Flags: review?(shuang) → review-
Comment on attachment 8655370 [details] [diff] [review]
0001-WBS-git-patch.patch

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

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1515,5 @@
> +  // Notify Gecko observers
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  NS_ENSURE_TRUE_VOID(obs);
> +
> +  if (HFP_WBS_YES == aWbs) {

Simplify as

  mWbsEnabled = (aWbs == HFP_WBS_YES);

@@ +1527,5 @@
> +                                     BLUETOOTH_HFP_WBS_STATUS_CHANGED_ID,
> +                                     mDeviceAddress.get()))) {
> +    BT_WARNING("Failed to notify bluetooth-hfp-wbs-status-changed observsers!");
> +  }
> +

nit: remove this extra newline.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.h
@@ +131,5 @@
>                          const nsAString& aBdAddress) override;
>    void NRECNotification(BluetoothHandsfreeNRECState aNrec,
>                          const nsAString& aBdAddr) override;
> +  void WbsNotification(BluetoothHandsfreeWbsConfig aWbs,
> +                  const nsAString& aBdAddr) override;

nit: alignment.
Thank you Shawn, Ben.

Hi Shawn,

Could you help to review again this patch?
I rebased it to branch 2.2r, and ran the PTS test TC_AG_WBS_BV_01_I,
Thanks!
Attachment #8655370 - Attachment is obsolete: true
Attachment #8656523 - Flags: review?(shuang)
Note:

To enable WBS, a define of stack bluedroid named BTIF_HF_WBS_PREFERRED[1] must be TRUE,
then the preferred codec will be overridden as following:
09-03 07:26:44.202 I/bt-btif ( 1458): btif_hf_upstreams_evt btif_hf override-Preferred Codec to MSBC

and codec status will be set to MSBC instead of original CVSD:
09-03 07:26:44.202 D/bt-btif ( 1458): BTA_AG_WBS_EVT Set codec status 0 codec 2 1=CVSD 2=MSBC

This will be reflected in my incoming gecko patch.

Besides,
another define BTM_WBS_INCLUDED[2] of bluedroid must be TRUE as well.


[1] file path: external/bluetooth/bluedroid/btif/src/btif_hf.c
[2] file path: external/bluetooth/bluedroid/include/bt_target.h
Comment on attachment 8656523 [details] [diff] [review]
Patch: Add WBS support for branch 2.2r

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

Remove PATCH 2/2 from subject, also you should add hfp fallback. Do you plan to add it in this patch or another bug?
Attachment #8656523 - Flags: review?(shuang) → review+
Revised patch based on Shawn's review.
- add support for hfp-fallback.
- rebase for another HFP NREC patch in Bug 1197815.


Hi Shawn,

Could you help to review?
Thanks!
Attachment #8656523 - Attachment is obsolete: true
Attachment #8658113 - Flags: review?(shuang)
Also, above patch move declaration into BT_DECL_HFP_MGR_BASE as Bug 1197815 did.
Comment on attachment 8658113 [details] [diff] [review]
Patch: Add HFP Wideband Speech(WBS) support. (for branch 2.2r)

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

::: dom/system/gonk/AudioManager.cpp
@@ +367,5 @@
> +    String8 cmd;
> +    BluetoothHfpManagerBase* hfp =
> +      static_cast<BluetoothHfpManagerBase*>(aSubject);
> +    if (hfp->IsNrecEnabled()) {
> +      cmd.setTo("bt_headset_name=<unknown>;bt_headset_nrec=on");

nit: Please add "TODO" note, to fix <unknown> name.
Attachment #8658113 - Flags: review?(shuang) → review+
Hi Shawn

Thanks for your review :)
Revised patch is as attached.
- Add TODO comment. (ref: Bug 880785)
- Add a newline \n to end of file for POSIX standard.

Carry r+ from previous patch.
Attachment #8658113 - Attachment is obsolete: true
Attachment #8658642 - Flags: review+
Hi Wesley

Could you help to handle this for branch 2.2r,
thanks!
blocking-b2g: --- → 2.2r?
Flags: needinfo?(whuang)
Keywords: checkin-needed
it has been flagged with feature-b2g:2.2r and check-in needed. 
Sheriff will uplift it.
blocking-b2g: 2.2r? → ---
Flags: needinfo?(whuang)
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/86954a963254
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.