Closed Bug 842934 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: shawnjohnjr, Assigned: rlin)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(3 files, 8 obsolete files)

7.69 KB, patch
rlin
: feedback+
Details | Diff | Splinter Review
7.89 KB, patch
Details | Diff | Splinter Review
9.70 KB, patch
Details | Diff | Splinter Review
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
Assignee: nobody → rlin
Attached patch wip patch (obsolete) — Splinter Review
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.
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
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.
Tt sounds reasonable, thanks a lot.
Please help to confirm. Thanks.
Attachment #772004 - Flags: feedback?(rlin)
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?
Attachment #772004 - Flags: feedback?(rlin) → feedback+
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.
Attached patch patch v1 (obsolete) — Splinter Review
This is the AudioManager part, this patch also depends on Bluetooth part.
FM radio problem would have a patch on another bug.
Attachment #771624 - Attachment is obsolete: true
Attachment #773195 - Flags: review?(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.
Attachment #773195 - Flags: review?(mchen)
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #773195 - Attachment is obsolete: true
Attachment #775313 - Flags: review?(mchen)
Attachment #775313 - Flags: review?(mchen)
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.
Attachment #775313 - Flags: review?(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.
Attachment #775313 - Flags: review?(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.
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.
(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)
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.
Attachment #772004 - Attachment is obsolete: true
Attachment #776281 - Flags: review?(echou)
Attachment #776281 - Flags: feedback?(rlin)
FYI, Randy.

If you want to get current connection state, IsConnected()/IsScoConnected() can be used for different profiles. Please see patch for more details.
Attachment #776283 - Flags: feedback?(rlin)
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.
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 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;
Attachment #776281 - Flags: review?(echou) → review+
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?
Attached patch AudioManager patchv3 (obsolete) — Splinter Review
Attachment #775313 - Attachment is obsolete: true
(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.
Attached patch AudioManager change v4 (obsolete) — Splinter Review
Rebase to MC 7/19.
Attachment #777698 - Attachment is obsolete: true
Attachment #778380 - Flags: review?(mchen)
Attachment #778380 - Attachment description: bug.patch → AudioManager change v4
Attachment #776283 - Flags: feedback?(rlin) → feedback+
Attachment #776281 - Flags: feedback?(rlin) → feedback+
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.
Attachment #778380 - Flags: review?(mchen) → review+
Attached patch AudioManager check-in patch (obsolete) — Splinter Review
Fix nits, carry reviewer.
Hi Randy, Bug 842948 has been landed to birch. I've updated the patch. Please help to check it. Thanks.
Attachment #776281 - Attachment is obsolete: true
carry reviewer, re-base to current mc.
Attachment #778380 - Attachment is obsolete: true
Attachment #780282 - Attachment is obsolete: true
Depends on: 899769
blocking-b2g: --- → koi?
Unclear why this was nommed for koi, this landed long back based on comment #31 and should be there in 1.2.
blocking-b2g: koi? → ---
You need to log in before you can comment on or make changes to this bug.