Closed Bug 937434 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: AudioChannel, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
1.3 Sprint 5 - 11/22
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: scheng, Assigned: scheng)

References

Details

(Whiteboard: [u=devices c=media p=0])

Attachments

(2 files, 5 obsolete files)

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%
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.
Assignee: nobody → scheng
Attached patch Bug-937434.patch (obsolete) — Splinter Review
provide the A2DP device information to the SetStreamVolumeIndex() function.
Attachment #830674 - Flags: review?(mwu)
Attachment #830674 - Flags: review?(mchen)
This bug blocks Bluetooth A2DP on JB.
blocking-b2g: --- → 1.3?
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.
Attachment #830674 - Flags: review?(mwu)
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.
Attachment #830674 - Flags: review?(mchen)
triage: basic function failure. 1.3+
blocking-b2g: 1.3? → 1.3+
Attached patch Bug-937434-v2.patch (obsolete) — Splinter Review
1.update your commit message
2.change the return type from android specific to the types supported by Gecko
Attachment #830674 - Attachment is obsolete: true
Attachment #8334437 - Flags: review?(mwu)
Attachment #8334437 - Flags: review?(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.
Attachment #8334437 - Flags: review?(mchen)
Or nsresult is enough.
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
Attachment #8334437 - Flags: review?(mwu)
Attached patch Bug-937434-v3.patch (obsolete) — Splinter Review
1.remove redundant space
2.use nsresult as return type in AudioManager.cpp
3.remove the constant values in AudioManager.h
Attachment #8334437 - Attachment is obsolete: true
Attachment #8335038 - Flags: review?(mwu)
Attachment #8335038 - Flags: review?(mchen)
blocking-b2g: 1.3? → 1.3+
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
Attachment #8335038 - Flags: review?(mwu)
Attachment #8335038 - Flags: review?(mchen)
Attached patch Bug-937434-v4.patch (obsolete) — Splinter Review
1.move the declaration of variable to nearest place
2.change the return value as NS_ERROR_INVALID_ARG
3.indentation
Attachment #8335038 - Attachment is obsolete: true
Attachment #8335223 - Flags: review?(mchen)
Attached patch Bug-937434-v5.patch (obsolete) — Splinter Review
remove AUDIO_DEVICE_OUT_BLUETOOTH_A2DP_HEADPHONES & AUDIO_DEVICE_OUT_BLUETOOTH_A2DP_SPEAKER because they are not used in gekco
Attachment #8335223 - Attachment is obsolete: true
Attachment #8335223 - Flags: review?(mchen)
Attachment #8335825 - Flags: review?(mchen)
fix the compiling error (only in Nexus4) about nsresult usage:
1.+= operator error
2.return status ? NS_ERROR_FAILURE : NS_OK error
Attachment #8335825 - Attachment is obsolete: true
Attachment #8335825 - Flags: review?(mchen)
Attachment #8335973 - Flags: review?(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.
Attachment #8335973 - Flags: review?(mchen) → review+
Whiteboard: [u=devices c=media p=0]
Target Milestone: --- → 1.3 Sprint 5 - 11/22
the patch was reviewed by mchen.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a5585091e1e2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.