937434 - [Audio] Can not change voice volume through vol key when A2DP is connected

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Star Cheng [:scheng]]

Description:
When a user connect a BT device which supports A2DP profile. And then enter Music App to play a mp3 song. The voice volume is not changed by pressing vol key.


Repro Steps:
1) Connect a bluetooth(BT) headset to your phone
2) Enter Music App to play a mp3 song.
3) Press the volume key in order to change audio volume


Actual:
The audio volume is not changed.

Expected:
The audio volume can be changed.

Environmental Variables
Device: Flatfish
Gecko: https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/e4d673ad84b0
Gaia: b4d59df5c3f974d9815e8f618e85266fc10a6c70
Platform Version: 26.0

Notes:
Repro frequency: 100%

Comment 1

[Star Cheng [:scheng]]

The ANDROID_VERSION is 17 in Flatfish. So the AudioSystem::setStreamVolumeIndex() API is different, need to provide a argument - "device" to the function API. In this case, lack providing A2DP device information so that user can't change the volume through pressing vol key.

Comment 2

[Star Cheng [:scheng]]

Attachment: Bug-937434.patch

provide the A2DP device information to the SetStreamVolumeIndex() function.

Comment 3

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

This bug blocks Bluetooth A2DP on JB.

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 830674 [details] [diff] [review]
Bug-937434.patch

Deferring main review to Marco.

Adding status codes here is wrong, though that's not your fault. Might be a good time to fix it if you have any ideas.

Comment 5

[Marco Chen [:mchen]]

Comment on attachment 830674 [details] [diff] [review]
Bug-937434.patch

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

1. Please update your commit message for more precise description of this bug.

2. AudioManager::Get/SetStreamVolumeIndex(...) return a android::status_t type and this will break Gecko's portability.
   We should change the return type from android specific to the types supported by Gecko.

Please help to fix them. Thanks.
And thanks for mwu's suggestion too.

Comment 6

[Joe Cheng [:jcheng] (please needinfo)]

triage: basic function failure. 1.3+

Comment 7

[Star Cheng [:scheng]]

Attachment: Bug-937434-v2.patch

1.update your commit message
2.change the return type from android specific to the types supported by Gecko

Comment 8

[Marco Chen [:mchen]]

Comment on attachment 8334437 [details] [diff] [review]
Bug-937434-v2.patch

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

::: dom/system/gonk/AudioManager.cpp
@@ +573,5 @@
>  }
>  
>  NS_IMETHODIMP
>  AudioManager::SetAudioChannelVolume(int32_t aChannel, int32_t aIndex) {
> +  int32_t status;

Why do we need to change the type here?
AudioManager used Android functions and types and what we tried to avoid is to leak these android types into gecko.

@@ +663,5 @@
>    *aMaxIndex = sMaxStreamVolumeTbl[stream];
>     return NS_OK;
>  }
>  
> +int32_t

I think we can just return boolean and it is enough for the use case now. (only RecoverTask used this function)
So we can remove the constant values in AudioManager.h too.

@@ +668,3 @@
>  AudioManager::SetStreamVolumeIndex(int32_t aStream, int32_t aIndex) {
> +  status_t status;
> +  

redundant space.

@@ +705,5 @@
> +              AUDIO_DEVICE_OUT_BLUETOOTH_A2DP_HEADPHONES);
> +  status += AudioSystem::setStreamVolumeIndex(
> +              static_cast<audio_stream_type_t>(aStream),
> +              aIndex,
> +              AUDIO_DEVICE_OUT_BLUETOOTH_A2DP_SPEAKER);  

redundant space.

@@ +722,5 @@
>    status += AudioSystem::setStreamVolumeIndex(
>                static_cast<audio_stream_type_t>(aStream),
>                aIndex,
>                AUDIO_DEVICE_OUT_EARPIECE);
> +  

redundant space.

Comment 9

[Marco Chen [:mchen]]

Or nsresult is enough.

Comment 10

[Michael Wu [:mwu]]

Comment on attachment 8334437 [details] [diff] [review]
Bug-937434-v2.patch

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

As Marco mentioned, nsresult should be enough. The default set of error codes should be able to cover what you need - http://hg.mozilla.org/mozilla-central/file/c335eaa4494a/xpcom/base/ErrorList.h

Comment 11

[Star Cheng [:scheng]]

Attachment: Bug-937434-v3.patch

1.remove redundant space
2.use nsresult as return type in AudioManager.cpp
3.remove the constant values in AudioManager.h

Comment 12

[Marco Chen [:mchen]]

Comment on attachment 8335038 [details] [diff] [review]
Bug-937434-v3.patch

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

Hi Star,

Thanks.
It almost did the whole things here but please help to fix some nits and one question.

::: dom/system/gonk/AudioManager.cpp
@@ +573,5 @@
>  }
>  
>  NS_IMETHODIMP
>  AudioManager::SetAudioChannelVolume(int32_t aChannel, int32_t aIndex) {
> +  nsresult status;

Does this intend to change?
It seems function already did the transform from status_t to nsresult.

@@ +668,2 @@
>  AudioManager::SetStreamVolumeIndex(int32_t aStream, int32_t aIndex) {
> +  status_t status;

Please move the declaration of variable to nearest place using it. ex: before Line 675

@@ +670,2 @@
>    if (aIndex < 0 || aIndex > sMaxStreamVolumeTbl[aStream]) {
> +    return NS_ERROR_FAILURE;

NS_ERROR_INVALID_ARG

@@ +677,2 @@
>             static_cast<audio_stream_type_t>(aStream),
>             aIndex);

nits: indentation.

@@ +691,3 @@
>               static_cast<audio_stream_type_t>(aStream),
>               aIndex,
>               device);

nits: indentation.

@@ +730,3 @@
>  AudioManager::GetStreamVolumeIndex(int32_t aStream, int32_t *aIndex) {
>    if (!aIndex) {
> +    return NS_ERROR_FAILURE;

NS_ERROR_INVALID_ARG

@@ +733,4 @@
>    }
>  
>    if (aStream <= AUDIO_STREAM_DEFAULT || aStream >= AUDIO_STREAM_MAX) {
> +    return NS_ERROR_FAILURE;

NS_ERROR_INVALID_ARG

Comment 13

[Star Cheng [:scheng]]

Attachment: Bug-937434-v4.patch

1.move the declaration of variable to nearest place
2.change the return value as NS_ERROR_INVALID_ARG
3.indentation

Comment 14

[Star Cheng [:scheng]]

Attachment: Bug-937434-v5.patch

remove AUDIO_DEVICE_OUT_BLUETOOTH_A2DP_HEADPHONES & AUDIO_DEVICE_OUT_BLUETOOTH_A2DP_SPEAKER because they are not used in gekco

Comment 15

[Star Cheng [:scheng]]

Attachment: Bug-937434-v6.patch

fix the compiling error (only in Nexus4) about nsresult usage:
1.+= operator error
2.return status ? NS_ERROR_FAILURE : NS_OK error

Comment 16

[Marco Chen [:mchen]]

Comment on attachment 8335973 [details] [diff] [review]
Bug-937434-v6.patch

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

Thanks for your help on changing return type to nsresult.

r=me after fixing the nit.

::: dom/system/gonk/AudioManager.cpp
@@ +699,5 @@
>  
> +  status = AudioSystem::setStreamVolumeIndex(
> +              static_cast<audio_stream_type_t>(aStream),
> +              aIndex,
> +              AUDIO_DEVICE_OUT_BLUETOOTH_A2DP);

indention: 2 not 3 spaces.

Comment 17

[Star Cheng [:scheng]]

Attachment: Bug-937434.patch-reviewed

the patch was reviewed by mchen.

Comment 18

[Marco Chen [:mchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5585091e1e2

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a5585091e1e2