811204 - [Dialer] voice call can't switch between speaker and bt sco audio path

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

1. make voice call, make sure bt sco is connected
2. switch to speaker
3. switch back to bt sco, found the voice sound output to receiver

Comment 1

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

Attachment: fix-patch1

Comment 2

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

try server result: pass
https://tbpl.mozilla.org/?tree=Try&rev=b6435d19c8c3

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 680983 [details] [diff] [review]
fix-patch1

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

Approach looks ok for now, but it needs some cleanup.

::: dom/system/gonk/AudioManager.cpp
@@ +53,5 @@
>  static int sHeadsetState;
>  static int kBtSampleRate = 8000;
>  
>  static bool
> +IsBtScoOn()

Let's just generalize this (and IsFmRadioAudioOn) function to take a device and report whether it is available.

@@ +59,5 @@
> +  if (static_cast<
> +      audio_policy_dev_state_t (*) (audio_devices_t, const char *)
> +      >(AudioSystem::getDeviceConnectionState)) {
> +    return AudioSystem::getDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET, "") == 
> +           AUDIO_POLICY_DEVICE_STATE_AVAILABLE ? true : false;

While we're at it, remove ? true : false since the == comparison does that for us already.

@@ +61,5 @@
> +      >(AudioSystem::getDeviceConnectionState)) {
> +    return AudioSystem::getDeviceConnectionState(AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET, "") == 
> +           AUDIO_POLICY_DEVICE_STATE_AVAILABLE ? true : false;
> +  } else {
> +    return false;

Remove the else - we can just return false;

@@ +324,5 @@
>  AudioManager::SetForceForUse(int32_t aUsage, int32_t aForce)
>  {
>    status_t status = 0;
> +
> +  if (IsBtScoOn() == true && aUsage == nsIAudioManager::USE_COMMUNICATION 

Comparing against true is unnecessary, especially since that function returns a boolean.

Make sure you put && at the end of lines instead of the beginning.

Avoid any trailing whitespace when adding code.

Comment 4

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

Attachment: bug-806262-fix.patch2

Comment 5

[Michael Wu [:mwu]]

Comment on attachment 681815 [details] [diff] [review]
bug-806262-fix.patch2

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

::: dom/system/gonk/AudioManager.cpp
@@ +65,1 @@
>      return false;

The indentation is off here.

@@ +310,5 @@
>  AudioManager::SetForceForUse(int32_t aUsage, int32_t aForce)
>  {
>    status_t status = 0;
> +
> +  if (IsDeviceOn(AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET) && aUsage == nsIAudioManager::USE_COMMUNICATION &&

This line is a bit long. Put the aUsage == ... check on the next line and align everything with IsDeviceOn.

Comment 6

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

Attachment: patch3

Comment 7

[Michael Wu [:mwu]]

Comment on attachment 681823 [details] [diff] [review]
patch3

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

Looks good, thanks.

Note that the Mozilla coding style is {} for every if/while/for/etc. statement, even if it's not necessary. I don't care much for this but other reviewers do, and that appears to be the style in this file.

Comment 8

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

Attachment: checkin patch

add a=blocking-basecamp

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b766fb679bf

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7b766fb679bf

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5a8990dfed4a