Closed Bug 825149 Opened 7 years ago Closed 4 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.2r+, feature-b2g:2.2r+, firefox43 fixed, b2g-v2.2 wontfix, b2g-v2.2r fixed, b2g-master fixed)

RESOLVED FIXED
FxOS-S5 (21Aug)
blocking-b2g 2.2r+
feature-b2g 2.2r+
Tracking Status
firefox43 --- fixed
b2g-v2.2 --- wontfix
b2g-v2.2r --- fixed
b2g-master --- fixed

People

(Reporter: shawnjohnjr, Assigned: wiwang)

References

Details

(Whiteboard: [red-tai])

Attachments

(3 files, 5 obsolete files)

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.
Assignee: nobody → shuang
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.
Attachment #696239 - Flags: review?(echou)
After reviewing Bug 831176, audio team preferred to let Bluetooth call AudioSystem directly.
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.
Attachment #696239 - Flags: review?(gyeh) → review?
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!
Flags: needinfo?(scheng)
This bug is my first bug, but seems that no progress anymore, which is sad. We shall nominate this feature to v.1.4.
blocking-b2g: --- → 1.4?
Marco,

Is this in scope for 1.4?
Flags: needinfo?(mchen)
Flags: needinfo?(scheng)
Flags: needinfo?(mchen)
blocking-b2g: 1.4? → ---
Assignee: shuang → wiwang
Blocks: 1181901
feature-b2g: --- → 2.2r+
Whiteboard: [red-tai] → [red-tai][ETA=8/31]
Hi Shawn,

Could you help to review my patch?
Thanks!
Attachment #696239 - Attachment is obsolete: true
Attachment #8640504 - Flags: review?(shuang)
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'
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
nit: s/develop/development in 1st line of above comment.
Hi Shawn,

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

Thanks!
Attachment #8640872 - Flags: review?(shuang)
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.
Attachment #8640504 - Flags: review?(shuang) → review-
Attachment #8640872 - Flags: review?(shuang) → review+
Hi Shawn,

Thanks for your review!

The revised patch is as attached and passed the PTS test,
could you help to review?
Thanks!
Attachment #8640504 - Attachment is obsolete: true
Attachment #8640929 - Flags: review?(shuang)
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)
Attachment #8640929 - Flags: review?(shuang) → review-
Hi Shawn

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

Thanks!
Attachment #8640929 - Attachment is obsolete: true
Attachment #8643509 - Flags: review?(shuang)
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.
Attachment #8643509 - Flags: review?(shuang) → review+
Comment on attachment 8640872 [details] [diff] [review]
0001-Bug-825149-add-NREC-feature-bit-in-bluetooth-stack.patch

># HG changeset patch
># User Will Wang <wiwang@mozilla.com>
>
>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)
>
Please ignore above comment 18 which was accidentally posted.
Add reviewer to commit message and carry r+ from previous patch(in comment 12).
Attachment #8640872 - Attachment is obsolete: true
Attachment #8644893 - Flags: review+
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
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!
Attachment #8643509 - Attachment is obsolete: true
Attachment #8645675 - Flags: review?(shuang)
Attachment #8645675 - Flags: review?(shuang) → review+
Component: General → Bluetooth
Keywords: checkin-needed
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)
Attachment #8646807 - Flags: review+
Attachment #8645675 - Attachment description: 0001-Bug-825149-support-HFP-NREC-feature(v4).patch → 0001-Bug-825149-support-HFP-NREC-feature(v4, for m-c).patch
Attachment #8644893 - Attachment description: 0001-Bug-825149-add-NREC-feature-bit-in-bluetooth-stack.patch → 0001-Bug-825149-add-NREC-feature-bit-in-bluetooth-stack(for both m-c and 2.2r).patch
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
blocking-b2g: --- → 2.2r?
Hi Josh

Per discussion, could you help to set blocking-b2g 2.2r+,
Thanks!
Flags: needinfo?(jocheng)
Done
blocking-b2g: 2.2r? → 2.2r+
Flags: needinfo?(jocheng)
QA Whiteboard: QAExclude
Flags: needinfo?(jmercado)
Flags: needinfo?(jmercado)
Depends on: 1203046
Blocks: b2g-hfp-16
You need to log in before you can comment on or make changes to this bug.