Closed
Bug 842934
Opened 11 years ago
Closed 10 years ago
[Bluetooth]A2DP sound disappear after SCO disconnected (call ended) and switching patch to A2DP
Categories
(Firefox OS Graveyard :: General, defect)
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
Reporter | ||
Updated•11 years ago
|
Blocks: b2g-bluetooth-a2dp
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → rlin
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
Tt sounds reasonable, thanks a lot.
Comment 6•10 years ago
|
||
Please help to confirm. Thanks.
Attachment #772004 -
Flags: feedback?(rlin)
Assignee | ||
Comment 7•10 years ago
|
||
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+
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #775313 -
Flags: review?(mchen)
Assignee | ||
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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 14•10 years ago
|
||
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.
Assignee | ||
Comment 15•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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)
Comment 18•10 years ago
|
||
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 19•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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+
Assignee | ||
Comment 22•10 years ago
|
||
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?
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #775313 -
Attachment is obsolete: true
Comment 24•10 years ago
|
||
(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.
Assignee | ||
Comment 25•10 years ago
|
||
Rebase to MC 7/19.
Attachment #777698 -
Attachment is obsolete: true
Attachment #778380 -
Flags: review?(mchen)
Assignee | ||
Updated•10 years ago
|
Attachment #778380 -
Attachment description: bug.patch → AudioManager change v4
Assignee | ||
Updated•10 years ago
|
Attachment #776283 -
Flags: feedback?(rlin) → feedback+
Assignee | ||
Updated•10 years ago
|
Attachment #776281 -
Flags: feedback?(rlin) → feedback+
Comment 26•10 years ago
|
||
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+
Assignee | ||
Comment 27•10 years ago
|
||
Fix nits, carry reviewer.
Comment 28•10 years ago
|
||
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
Assignee | ||
Comment 29•10 years ago
|
||
carry reviewer, re-base to current mc.
Attachment #778380 -
Attachment is obsolete: true
Attachment #780282 -
Attachment is obsolete: true
Comment 30•10 years ago
|
||
http://hg.mozilla.org/projects/birch/rev/33f5705cd0ba http://hg.mozilla.org/projects/birch/rev/9f82a1e2f4eb
Whiteboard: [fixed-in-birch]
Comment 31•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9f82a1e2f4eb https://hg.mozilla.org/mozilla-central/rev/33f5705cd0ba
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
blocking-b2g: --- → koi?
Comment 32•10 years ago
|
||
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.
Description
•