842934 - [Bluetooth]A2DP sound disappear after SCO disconnected (call ended) and switching patch to A2DP

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

STR:
1. Connect HFP/A2DP Headset
2. Make an incoming call
3. Answer phone call then hang up
4. Music resumes but no music sound

Comment 2

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: wip patch

The problem caused by audio poicy object try to suspend/restore the audio mixer in audioflinger, and hit the counting problem. During the phone call end, if we let setForceUse use call before setphoneState, we may let the mSuspended can't become 0 and audioMixer won't wake up and write any data into a2dp output.

The FM also be affected, disable a2dp device before switch to fm audio path can avoid fm radio no sound issue.

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Take nexus s 4.04 as reference. 
BT connected. Playing music...
07-08 09:35:27.140 E/AudioSystem(  195): static android::status_t android::AudioSystem::setDeviceConnectionState(audio_devices_t, audio_policy_dev_state_t, const char*), 80, 1
07-08 09:35:27.367 E/AudioSystem(  195): static android::status_t android::AudioSystem::setDeviceConnectionState(audio_devices_t, audio_policy_dev_state_t, const char*), 20, 1

Phone call start...
07-08 09:36:01.156 E/AudioSystem(  195): static android::status_t android::AudioSystem::setPhoneState(int) 2

07-08 09:36:03.710 E/AudioSystem(  195): static android::status_t android::AudioSystem::setForceUse(audio_policy_force_use_t, audio_policy_forced_cfg_t), 0, 3
07-08 09:36:03.835 E/AudioSystem(  195): static android::status_t android::AudioSystem::setForceUse(audio_policy_force_use_t, audio_policy_forced_cfg_t), 2, 3

phone call end...
07-08 09:36:07.937 E/AudioSystem(  195): static android::status_t android::AudioSystem::setForceUse(audio_policy_force_use_t, audio_policy_forced_cfg_t), 0, 0
07-08 09:36:08.000 E/AudioSystem(  195): static android::status_t android::AudioSystem::setPhoneState(int) 0
07-08 09:36:08.351 E/AudioSystem(  195): static android::status_t android::AudioSystem::setForceUse(audio_policy_force_use_t, audio_policy_forced_cfg_t), 2, 0
07-08 09:36:08.406 E/AudioSystem(  195): static android::status_t android::AudioSystem::setForceUse(audio_policy_force_use_t, audio_policy_forced_cfg_t), 0, 0

Comment 4

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

Hi Randy,

I think we can separate this issue into two phases:
1. Connection status of Handsfree profile
2. Connection status of Sco link

For case one, we would notify observers of topic "bluetooth-hfp-status-changed".
For case two, observers of topic "bluetooth-sco-status-changed" would be notified.

Let me attach a patch later, and please let me know if it doesn't work. Thanks.

Comment 5

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Tt sounds reasonable, thanks a lot.

Comment 6

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

Attachment: Patch 1(v1): Add topic for hfp connection status changed

Please help to confirm. Thanks.

Comment 7

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 772004 [details] [diff] [review]
Patch 1(v1): Add topic for hfp connection status changed

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

Seems OK, BTW, I found if I disable the bluetooth from setting,
the AudioManager can't receive the disconnect a2dp notification, Can you help to add that?

Comment 8

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

Eric is working on this issue (bug 891257).

We should disconnect all profiles before turning off Bluetooth, and this is different from our current implementation. That's why you failed to receive the notification.

Can you try to disconnect connection manually in Settings App? Please let me know if you still don't receive the notification. Thanks.

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v1

This is the AudioManager part, this patch also depends on Bluetooth part.
FM radio problem would have a patch on another bug.

Comment 10

[Marco Chen [:mchen]]

Comment on attachment 773195 [details] [diff] [review]
patch v1

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

::: dom/system/gonk/AudioManager.cpp
@@ +193,5 @@
>  AudioManager::Observe(nsISupports* aSubject,
>                        const char* aTopic,
>                        const PRUnichar* aData)
>  {
> +  if ((strcmp(aTopic, BLUETOOTH_HFP_STATUS_CHANGED_ID) == 0) ||

It seems that "to monitor the HFP status change" is not related to this bug so can we move this part to another bug?

@@ +414,5 @@
>  {
>    if (mPhoneState == aState) {
>      return NS_OK;
>    }
> +  // This is a work-round for avoiding a2dp resume no sound.

s/work-round/workaround/
s/no sound/with no sound/

@@ +420,5 @@
> +  GetForceForUse(nsIAudioManager::USE_COMMUNICATION, &usage);
> +  if (aState == PHONE_STATE_NORMAL && usage == nsIAudioManager::FORCE_BT_SCO) {
> +    SetForceForUse(nsIAudioManager::USE_COMMUNICATION, nsIAudioManager::FORCE_NONE);
> +  }
> +  // end

Will discuss with Randy directly for detail.

::: dom/system/gonk/AudioManager.h
@@ +57,5 @@
>  private:
>    nsAutoPtr<mozilla::hal::SwitchObserver> mObserver;
>    nsCOMPtr<AudioChannelAgent>             mPhoneAudioAgent;
> +  nsAutoCString mBTsco_Address;
> +  nsAutoCString mA2dp_Address;

The member variables here used lower camel case.

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v2

Fix nits, also, I get the bt address and setDeviceConnectionState on b2_sco connect/disconnect changed event.
So the hfp notification can be ignored for this bug.

Comment 12

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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

After consulting with Gina, the a2dp address may disappear after a2dp disconnected. So I keep it for disconnect purpose.

Comment 13

[Marco Chen [:mchen]]

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

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

::: dom/system/gonk/AudioManager.cpp
@@ +223,5 @@
>          cmd.appendFormat("bt_samplerate=%d", kBtSampleRate);
>          AudioSystem::setParameters(0, cmd);
>          SetForceForUse(nsIAudioManager::USE_COMMUNICATION, nsIAudioManager::FORCE_BT_SCO);
>        } else {
> +        SetForceForUse(nsIAudioManager::USE_COMMUNICATION, nsIAudioManager::FORCE_NONE);

What will happen in the following flow?
1. Voice call is ongoing and SCO is connected.
2. User toggled the speaker icon in dialer app.
3. Step 2 triggered the setForceForUse to speaker and dialer app call Web API to disconnect SCO connection.
4. If sco disconnected is received at this time then speaker forcing will be clear here.
Could you help to check this?

@@ +230,2 @@
>        if (audioState == AUDIO_POLICY_DEVICE_STATE_AVAILABLE) {
> +        mA2dpAddress = NS_ConvertUTF16toUTF8(tmp_address);

The conversion from UTF16 to 8 is done for variable - address already so we can get a copy from it but not to do conversion again.

@@ +235,5 @@
>          AudioSystem::setParameters(0, cmd);
>          cmd.setTo("A2dpSuspended=false");
>          AudioSystem::setParameters(0, cmd);
> +      } else {
> +        AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_A2DP,

line 232 called the function which is exactly the same with here, so could you find a place to call it  once but for both of available/unavailable?

@@ +237,5 @@
>          AudioSystem::setParameters(0, cmd);
> +      } else {
> +        AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_A2DP,
> +                                              audioState, mA2dpAddress.get());
> +        mA2dpAddress = "";

If there is no code to check whether mA2dpAddress is empty or not, maybe we can just keep it's value and save this line.

@@ +405,5 @@
>  {
>    if (mPhoneState == aState) {
>      return NS_OK;
>    }
> +  // This is a workround for avoiding a2dp resume with no sound.

Since this is a workaround, could you add 
  1. ToDo keyword.
  2. Bug number.
  3. What limitation caused this workaround.

Thanks.

@@ -451,5 @@
> -  if (IsDeviceOn(AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET) &&
> -      aUsage == nsIAudioManager::USE_COMMUNICATION &&
> -      aForce == nsIAudioManager::FORCE_NONE) {
> -    aForce = nsIAudioManager::FORCE_BT_SCO;
> -  }

This is contained by Bug 876334 already.

Comment 14

[Marco Chen [:mchen]]

Comment on attachment 772004 [details] [diff] [review]
Patch 1(v1): Add topic for hfp connection status changed

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

Hi Gina,

It seems that it is worth to create a new member function for processing the observer from BT because there are already 3 events. Please help to do this on your next patch for AudioManager. Thanks.

Comment 15

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

1. Voice call is ongoing and SCO is connected.
2. User toggled the speaker icon in dialer app.
3. Step 2 triggered the setForceForUse to speaker and dialer app call Web API to disconnect SCO connection.
4. If sco disconnected is received at this time then speaker forcing will be clear here.
Could you help to check this?
----
For this, I think we should request dialer to use the right sequence to set the audio path.
The latest one wins. Or we need hack those things to for-fit those use cases.

Comment 16

[Marco Chen [:mchen]]

(In reply to Randy Lin [:rlin] from comment #15)
> For this, I think we should request dialer to use the right sequence to set
> the audio path.
> The latest one wins. Or we need hack those things to for-fit those use cases.

May I know the Web API for disconnecting SCO is a synchronous call or not?
If it is not, we can't guarantee that what timing of the SCO disconnected notification will go to AudioManager. (ex: it may go to AudioManager after forcing speaker on because the asynchronous call of SCO disconnection)

Comment 17

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

Attachment: Patch 1(v2): Add topic for hfp status changed and notify observers with device address

Whenever the profile connection status is changed, we notify observers with the device address. Thus, the address can be used to set device connection state.

Comment 18

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

Attachment: Patch 2(v1): Register/Unregister topic in AudioManager

FYI, Randy.

If you want to get current connection state, IsConnected()/IsScoConnected() can be used for different profiles. Please see patch for more details.

Comment 19

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

Comment on attachment 776281 [details] [diff] [review]
Patch 1(v2): Add topic for hfp status changed and notify observers with device address

This patch is pretty similar to bug 842948, patch 2. I'll rebase it when we're ready to land.

Comment 20

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

Hi Marco,

I've updated patch 2 and created a new member function in AudioManager. Please check it.

(In reply to Marco Chen [:mchen] from comment #14)
> Comment on attachment 772004 [details] [diff] [review]
> Patch 1(v1): Add topic for hfp connection status changed
> 
> Review of attachment 772004 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Gina,
> 
> It seems that it is worth to create a new member function for processing the
> observer from BT because there are already 3 events. Please help to do this
> on your next patch for AudioManager. Thanks.

Comment 21

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

Comment on attachment 776281 [details] [diff] [review]
Patch 1(v2): Add topic for hfp status changed and notify observers with device address

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

It looks good overall, but you need to revise BluetoothHfpManager.h or it can't be compiled.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2438,5 @@
> +  bool isConnected = false;
> +  if (profile) {
> +    isConnected = profile->IsConnected();
> +  }
> +  return isConnected;

To simplify, it can be written as

  return profile ? profile->IsConnected() : false;

Comment 22

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Gina, 
I found when device is playing the music and after ending the phone call, 
The audioManager.cpp would receive BLUETOOTH_A2DP_STATUS_CHANGED_ID disconnect notify.
Is it normal?

Comment 23

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: AudioManager patchv3

Comment 24

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

(In reply to Randy Lin [:rlin] from comment #22)
> Hi Gina, 
> I found when device is playing the music and after ending the phone call, 
> The audioManager.cpp would receive BLUETOOTH_A2DP_STATUS_CHANGED_ID
> disconnect notify.
> Is it normal?

Let me try it later and update here soon.

Comment 25

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: AudioManager change v4

Rebase to MC 7/19.

Comment 26

[Marco Chen [:mchen]]

Comment on attachment 778380 [details] [diff] [review]
AudioManager change v4

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

::: dom/system/gonk/AudioManager.cpp
@@ +174,5 @@
> +                                           const char* aTopic,
> +                                           const nsCString aAddress)
> +{
> +  bool status;
> +  if (strcmp(aTopic, BLUETOOTH_SCO_STATUS_CHANGED_ID) == 0) {

Normally we used "if (!strcmp(...))" and just like what you did on next strcmp.

@@ +363,5 @@
>  {
>    if (mPhoneState == aState) {
>      return NS_OK;
>    }
> +  // follow the switch audio path logic for android

Please add the bug number into comment, because there is more information about why do we need to add this section of code.

::: dom/system/gonk/AudioManager.h
@@ +63,5 @@
>  
>  private:
>    nsAutoPtr<mozilla::hal::SwitchObserver> mObserver;
>    nsCOMPtr<AudioChannelAgent>             mPhoneAudioAgent;
> +  void HandleBluetoothStatusChanged(nsISupports* aSubject,

nit: give a line between member variable and function.

Comment 27

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: AudioManager check-in patch

Fix nits, carry reviewer.

Comment 28

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

Attachment: [Final] Patch 1: Add topic for hfp status changed and notify observers with device address, r=echou

Hi Randy, Bug 842948 has been landed to birch. I've updated the patch. Please help to check it. Thanks.

Comment 29

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: AudioManager check-in patch

carry reviewer, re-base to current mc.

Comment 30

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

http://hg.mozilla.org/projects/birch/rev/33f5705cd0ba
http://hg.mozilla.org/projects/birch/rev/9f82a1e2f4eb

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f82a1e2f4eb
https://hg.mozilla.org/mozilla-central/rev/33f5705cd0ba

Comment 32

[bhavana bajaj [:bajaj]]

Unclear why this was nommed for koi, this landed long back based on comment #31 and should be there in 1.2.