825149 - [b2g-bluetooth]HFP NREC commands required to bypass disable noise reduction/echo cancellation parameter to audio module

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

HFP AT command AT+NREC=0, AT+NREC=1, Bluetooth needs to inform audio sub-system to disable/enable noise reduction/echo cancellation. It is necessary for general Bluetooth in car system due to many Bluetooth car kit system would do NR/EC by itself. Usually Bluetooth car kit system sends AT+NREC=0 to turn off NR/EC, to avoid double effects. If audio system does not turn off NR/EC, the audio quality from Bluetooth car kit would decrease in general. Car kit on "Mercedes-BenZ" and "BMW" usually would do this request to phone.

Comment 1

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

Attachment: Add AT+NREC and add AudioManager setParameter API

Comment 2

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

Comment on attachment 696239 [details] [diff] [review]
Add AT+NREC and add AudioManager setParameter API

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

Several places needs to be revised, happy to review again once all have been done. :)

Also, not sure if it's suitable to expose nsIAudioManager::setParameter(). We need a DOM API peer to super-review this.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +743,5 @@
> +    nsCOMPtr<nsIAudioManager> am =
> +      do_GetService("@mozilla.org/telephony/audiomanager;1");
> +    if (!am) {
> +      NS_WARNING("Failed to get AudioManager service!");
> +      return;

Should we still respond with OK even if couldn't get audio service?

@@ +745,5 @@
> +    if (!am) {
> +      NS_WARNING("Failed to get AudioManager service!");
> +      return;
> +    }
> +    mNREC = (atCommandValues[0].EqualsLiteral("0"));

This is confusing. Other flags, such as mCLIP and mReceiveVgsFlag, are true if that function is enabled.

@@ +747,5 @@
> +      return;
> +    }
> +    mNREC = (atCommandValues[0].EqualsLiteral("0"));
> +    if (mNREC) {
> +      nsString audioSetting;

Nit: Using nsAutoString instead of nsString for local string which is less than 64 characters saves a runtime memory allocation.

@@ +755,5 @@
> +    } else {
> +      nsString audioSetting;
> +      audioSetting.AppendLiteral("bt_headset_name=Default Car-kit;");
> +      audioSetting.AppendLiteral("bt_headset_nrec=on");
> +      am->SetParameters(audioSetting);

I would suggest that we could move these outside the if-block except the second line of AppendLiteral.

nsString audioSetting;
audioSetting.AppendLiteral("bt_headset_name=Default Car-kit;");

if (mNREC) {
  audioSetting.AppendLiteral("bt_headset_nrec=off");
} else {
  audioSetting.AppendLiteral("bt_headset_nrec=on");
}

am->SetParameters(audioSetting);

::: dom/system/gonk/AudioManager.cpp
@@ +433,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +AudioManager::SetParameters(const nsAString &aParameter) {

Nit: we usually put the '&' at the end of 'nsAString'

::: dom/system/gonk/nsIAudioManager.idl
@@ +77,5 @@
>  
>    void setStreamVolumeIndex(in long stream, in long index);
>    long getStreamVolumeIndex(in long stream);
>    long getMaxStreamVolumeIndex(in long stream);
> +  void setParameters(in DOMString params);

I'm not sure if it's suitable to expose this. Please invite a DOM API team member for super-review.

Comment 3

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

After reviewing Bug 831176, audio team preferred to let Bluetooth call AudioSystem directly.

Comment 4

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

Comment on attachment 696239 [details] [diff] [review]
Add AT+NREC and add AudioManager setParameter API

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

We need to rebase the patch to the latest code base. Please send the review request again once the new patch is ready.

Comment 5

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

Hi Star,
ni you because I hope you can review this requirement for bluetooth audio parameter, if you need to study PulseAudio, please remember add this into your consideration!

Comment 6

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

This bug is my first bug, but seems that no progress anymore, which is sad. We shall nominate this feature to v.1.4.

Comment 7

[Preeti Raghunath(:Preeti)]

Marco,

Is this in scope for 1.4?

Comment 8

[Will Wang [:WillWang]]

Attachment: 0001-Bug-825149-support-HFP-NREC-feature.patch

Hi Shawn,

Could you help to review my patch?
Thanks!

Comment 9

[Will Wang [:WillWang]]

Examination for my patch:

1. Passed PTS test for the HFP NREC feature (TC_AG_ENO_BV_01_I) [1]
2. Tested with Nokia bluetooth speaker MD-12. [2]

[1]
Test case : TC_AG_ENO_BV_01_I started
	- SDP Service record for PTS: 'Handsfree HF' successfully registered
	- The IUT claims support for the following eSCO LMP packet types: EV3, 2-EV3, 
	- The IUT claims support for the following eSCO LMP packet types: EV3, 2-EV3, 
	- The IUT claims support for the following eSCO LMP packet types: EV3, 2-EV3, 
	- AT: SPP connect succeeded
	- AT: Service Level Connection established
	- AT: post SLC command sequence complete
	- MTC: AT+NREC=0
	- AT: RING
	- AT: Received +CLIP: "0973170512",129
	- HCI: Audio Connection enabled
	- AT: Incoming call established
	- HCI: Audio Connection disabled
	- AT: Call terminated
	- MTC: Call disabled by IUT
	- AT: Service Level Connection disabled
	- MTC: Test case ended
	 -Final Verdict: PASS
TC_AG_ENO_BV_01_I finished


[2]
Logcat log shows that NREC was set to on, once:
- RFCOMM connected(state 2,line 1342),
- and before a new SLC connection(state 3,line 1394).
Then NREC was set to off.

1342-07-30 03:52:42.079 I/GeckoBluetooth(  205): ConnectionStateNotification: state 2
1343:07-30 03:52:42.079 D/audio_hw_primary(  201): adev_set_parameters: enter: bt_headset_name=<unknown>;bt_headset_nrec=on
1344-07-30 03:52:42.079 D/audio_hw_extn(  201): audio_extn_set_anc_parameters: anc_enabled:0
1345-07-30 03:52:42.079 I/audio_a2dp_hw(  201): adev_set_parameters: state AUDIO_A2DP_STATE_STANDBY
1346-07-30 03:52:42.079 I/audio_a2dp_hw(  201): out_set_parameters: state AUDIO_A2DP_STATE_STANDBY
1347-07-30 03:52:42.079 I/str_params(  201): key: 'bt_headset_name' value: '<unknown>'
1348:07-30 03:52:42.079 I/str_params(  201): key: 'bt_headset_nrec' value: 'on'
........
1394 07-30 03:52:42.319 I/GeckoBluetooth(  205): ConnectionStateNotification: state 3
........
1458 07-30 03:52:42.579 D/audio_hw_primary(  201): adev_set_parameters: enter: bt_headset_name=<unknown>;bt_headset_nrec=off
1459 07-30 03:52:42.579 I/audio_a2dp_hw(  201): out_get_format: format 0x1           
1460 07-30 03:52:42.579 I/audio_a2dp_hw(  201): out_get_channels: channels 0x3       
1461 07-30 03:52:42.579 D/audio_hw_extn(  201): audio_extn_set_anc_parameters: anc_enabled:0
1462 07-30 03:52:42.579 I/audio_a2dp_hw(  201): adev_set_parameters: state AUDIO_A2DP_STATE_STANDBY
1463 07-30 03:52:42.579 I/audio_a2dp_hw(  201): out_set_parameters: state AUDIO_A2DP_STATE_STANDBY
1464 07-30 03:52:42.579 I/str_params(  201): key: 'bt_headset_name' value: '<unknown>'
1465 07-30 03:52:42.579 I/str_params(  201): key: 'bt_headset_nrec' value: 'off'

Comment 10

[Will Wang [:WillWang]]

Issues during my develop and examination:

1. At first, I tested patch on the device "flame-L", however 3 issues appeared:
- 1.1: flame-L cannot dial out. This is needed by PTS HFP testing.
- 1.2: flame-L cannot show correct bluetooth address due to the underlying trace_util service was not correctly set. Bluetooth address is needed by PTS testing.
- 1.3: flame-L was not set to build the bluetooth feature config file[1] of bluetooth stack, so a file inclusion need to be added in the file[2].

Therefore, flame-kk were used for test afterwards.

2. Sometimes log shows that an error occurred when setting audio parameter, like the followings:

72:07-29 11:15:26.604 D/audio_hw_primary(  228): adev_set_parameters: enter: bt_headset_name=<unknown>;bt_headset_nrec=on
73-07-29 11:15:26.604 D/audio_hw_extn(  228): audio_extn_set_anc_parameters: anc_enabled:0
74-07-29 11:15:26.604 E/audio_a2dp_hw(  228): adev_set_parameters: ERROR: set param called even when stream out is null

This is not directly related to HFP NREC feature implementation, however it need to be fixed.


[1] dom/bluetooth/bluedroid/b2g_bdroid_buildcfg.h
[2] device/t2m/flame/bluetooth/bdroid_buildcfg.h

Comment 11

[Will Wang [:WillWang]]

nit: s/develop/development in 1st line of above comment.

Comment 12

[Will Wang [:WillWang]]

Attachment: 0001-Bug-825149-add-NREC-feature-bit-in-bluetooth-stack.patch

Hi Shawn,

This is another needed patch for adding the NREC feature bit in the bluedroid,
could you help to review?

Thanks!

Comment 13

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

Comment on attachment 8640504 [details] [diff] [review]
0001-Bug-825149-support-HFP-NREC-feature.patch

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

Hi Will,
I'm glad to review again after you revise patch.

::: dom/bluetooth/BluetoothCommon.h
@@ +169,4 @@
>  #define BLUETOOTH_HID_STATUS_CHANGED_ID  "bluetooth-hid-status-changed"
>  #define BLUETOOTH_SCO_STATUS_CHANGED_ID  "bluetooth-sco-status-changed"
>  
> +#define BLUETOOTH_HFP_NREC_STATUS_CHANGED_ID  "bluetooth-hfp-nrec-status-changed"

nit: Move this line after BLUETOOTH_HFP_STATUS_CHANGED_ID.

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

Can you explain why you don't handle |aNrec|? You should set |mNrecEnabled| based on |aNrec|, right?

::: dom/system/gonk/AudioManager.cpp
@@ +344,5 @@
> +      } else {
> +          cmd.setTo("bt_headset_name=<unknown>;bt_headset_nrec=off");
> +          AudioSystem::setParameters(0, cmd);
> +      }
> +

nit: Remove this line.

Comment 14

[Will Wang [:WillWang]]

Attachment: 0001-Bug-825149-support-HFP-NREC-feature-v2.patch

Hi Shawn,

Thanks for your review!

The revised patch is as attached and passed the PTS test,
could you help to review?
Thanks!

Comment 15

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

Comment on attachment 8640929 [details] [diff] [review]
0001-Bug-825149-support-HFP-NREC-feature-v2.patch

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

Hi
Almost there, you still need to fix a few tiny things.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1201,5 @@
>    return (mConnectionState == HFP_CONNECTION_STATE_SLC_CONNECTED);
>  }
>  
> +bool
> +BluetoothHfpManager::IsNrecEnabled()

If outside caller call |IsNrecEnabled()|, what will you expect this value return, if you don't initialize |mNrecEnabled| in constructor. The default value is false.

@@ +1393,5 @@
>        NS_LITERAL_STRING(BLUETOOTH_HFP_STATUS_CHANGED_ID));
> +
> +  } else if (aState == HFP_CONNECTION_STATE_CONNECTED) {
> +    // Once RFCOMM is connected, enable NREC before each new SLC connection
> +    mNrecEnabled = true;

Please init |mNrecEnabled| in constructor and clean up |mNrecEnabled| in |cleanup|.

@@ +1483,5 @@
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  NS_ENSURE_TRUE_VOID(obs);
> +
> +  // Set NREC status once getting AT command
> +  mNrecEnabled = aNrec;

static_cast<bool>(aNrec)

Comment 16

[Will Wang [:WillWang]]

Attachment: 0001-Bug-825149-support-HFP-NREC-feature(v3).patch

Hi Shawn

Thanks for your advices!
Could you help to review again?
This patch is also passed the PTS test as well.

Thanks!

Comment 17

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

Comment on attachment 8643509 [details] [diff] [review]
0001-Bug-825149-support-HFP-NREC-feature(v3).patch

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

This looks good to me.

Comment 18

[Will Wang [:WillWang]]

Comment on attachment 8640872 [details] [diff] [review]
0001-Bug-825149-add-NREC-feature-bit-in-bluetooth-stack.patch

># HG changeset patch
># User Will Wang <[e-mail]>
>
>Bug 825149: add NREC feature bit in bluetooth stack. r=shuang.
>
>diff --git a/dom/bluetooth/bluedroid/b2g_bdroid_buildcfg.h b/dom/bluetooth/bluedroid/b2g_bdroid_buildcfg.h
>index c51b2f9..6cbfd73 100644
>--- a/dom/bluetooth/bluedroid/b2g_bdroid_buildcfg.h
>+++ b/dom/bluetooth/bluedroid/b2g_bdroid_buildcfg.h
>@@ -32,6 +32,7 @@
> ******************************************************************************/
> /* AG feature masks */
> #define BTIF_HF_FEATURES   ( BTA_AG_FEAT_3WAY | \
>+                             BTA_AG_FEAT_ECNR | \
>                              BTA_AG_FEAT_REJECT | \
>                              BTA_AG_FEAT_ECS    | \
>                              BTA_AG_FEAT_EXTERR)
>

Comment 19

[Will Wang [:WillWang]]

Please ignore above comment 18 which was accidentally posted.

Comment 20

[Will Wang [:WillWang]]

Attachment: 0001-Bug-825149-add-NREC-feature-bit-in-bluetooth-stack(for both m-c and 2.2r).patch

Add reviewer to commit message and carry r+ from previous patch(in comment 12).

Comment 21

[Will Wang [:WillWang]]

The try result[1] shows "busted" in B2G ICS Emulator, I am making a modified patch to solve this.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b42811e42553

Comment 22

[Will Wang [:WillWang]]

Attachment: 0001-Bug-825149-support-HFP-NREC-feature(v4, for m-c).patch

Hi Shawn

Here is the patch which was modified to avoid the build break in B2G ICS Emulator, could you help to review again?

I can built it in my local ICS Emulator repo without build break, and will send to try server once the server is back (refer bug 1192758)

Thanks!

Comment 23

[Will Wang [:WillWang]]

try server is back:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f91af77ce7f3

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/d8813047ca09
https://hg.mozilla.org/integration/b2g-inbound/rev/863a1606a3a8

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d8813047ca09
https://hg.mozilla.org/mozilla-central/rev/863a1606a3a8

Comment 26

[Will Wang [:WillWang]]

Attachment: 0001-Bug-825149-support-HFP-NREC-feature(v5, for 2.2r).patch

Attached patch is modified[1] from v4 for branch 2.2r. 
Carry r+ from previous patch.

[1] change c++11 "override" back to "MOZ_OVERRIDE" to unify coding style,
and re-generate a patch with 2.2r context(MOZ_OVERRIDE) to have patch applied without rejection in 2.2r (refer to bug 1145631)

Comment 27

[Will Wang [:WillWang]]

Hi Sheriff,

Could you help to uplift two patches[1] to 2.2r once "blocking-b2g 2.2r+" set,
Thanks!

[1]
1st patch: 0001-Bug-825149-add-NREC-feature-bit-in-bluetooth-stack(for both m-c and 2.2r).patch in comment 20
2nd patch: 0001-Bug-825149-support-HFP-NREC-feature(v5, for 2.2r).patch in comment 26

Comment 28

[Will Wang [:WillWang]]

Hi Josh

Per discussion, could you help to set blocking-b2g 2.2r+,
Thanks!

Comment 29

[Josh Cheng [:josh]]

Done

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/7f698cda882d
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/421cc0c9dabc