812434 - support FM volume control for implement AudioChannel API

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

After change to use AudioChannel API design, 
The volume control wouldn't use setMasterVolume anymore.
b2g v1 version need to hook in music stream and apply fm volume also when fmRadio is on.

Comment 1

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

Attachment: patch1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 2

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

Attachment: patch2

remove try msg

Comment 3

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

tryserver result:
https://tbpl.mozilla.org/?tree=Try&rev=37306c3257c7

Comment 4

[Michael Wu [:mwu]]

So the plan is to have gaia control FM radio eventually? Is there a bug for that? Why don't we do that instead?

Comment 5

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

Hi Michael, 
This change comes from this bug 
Bug 795237 - Web API for setting audio stream type.
Gaia layer won't control the volume by call the set Master Volume anymore.

Comment 6

[Michael Wu [:mwu]]

(In reply to Randy Lin [:rlin] from comment #5)
> Hi Michael, 
> This change comes from this bug 
> Bug 795237 - Web API for setting audio stream type.
> Gaia layer won't control the volume by call the set Master Volume anymore.

My question was more about whether gaia can set the fm volume at the same time the music stream volume is set.

Comment 7

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

Dear Michael, 
The adjust FM volume API isn't published to gaia layer and there is no fm stream type for setting app to use(795237). As talk with alive, keeping the music/fm volume index in the same setting is better for UX. So we want to hook this FM volume control as MUSIC type in audioManager.cpp.

Comment 8

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

This can be done if we group the fm stream volume into content and control by settings/fm app.

Comment 9

[Michael Wu [:mwu]]

Comment on attachment 682364 [details] [diff] [review]
patch2

r=me on the dom/system/gonk/AudioManager.cpp part *only*. I'm assuming you accidentally mixed in the b2g/chrome/content/shell.js part.

Change the comment to:
// music volume also controls the fm volume

Also make the AUDIO_STREAM_MUSIC == aStream check go first, so we don't run IsFmRadioAudioOn() if we don't have to.

Comment 10

[Marco Chen [:mchen]]

What will be happened in the scenario as below?

1. FM & Music stream volume indexes are 15.
2. FM didn't be enabled and set music stream index to 3.
   (Due to FM not enabled so FM stream index didn't be set to 3).
3. Start to play FM, and in this time the FM stream index will be 15 not 3.

Comment 11

[Michael Wu [:mwu]]

Comment on attachment 682364 [details] [diff] [review]
patch2

Good point. Passing review to marco.

Comment 12

[Tony Chung [:tchung]]

Just to clarify, is this bug tracking that the Volume rocker buttons dont work when FM Radio app is running?   Because that's what i'm seeing.  

Daily unagi build 11-26-2012, 
20121126071306

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Tony Chung [:tchung] from comment #12)
> Just to clarify, is this bug tracking that the Volume rocker buttons dont
> work when FM Radio app is running?   Because that's what i'm seeing.  
> 
> Daily unagi build 11-26-2012, 
> 20121126071306

Yes, FM volume control is broken now.

Comment 14

[Marco Chen [:mchen]]

The FM stream volume will be set to FM chip and change output volume by FM chip too.
So a limitation here is we need to sync music stream volume into FM while FM is enabling but before playing.

Comment 15

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

Attachment: patch3

sync with music stream type and set volume when enable the fm radio.
also correct the volume step

Comment 16

[Marco Chen [:mchen]]

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

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

::: b2g/chrome/content/settings.js
@@ +79,5 @@
>      ['audio.volume.bt_sco', 15, nsIAudioManager.STREAM_TYPE_BLUETOOTH_SCO],
>      ['audio.volume.enforced_audible', 7, nsIAudioManager.STREAM_TYPE_ENFORCED_AUDIBLE],
>      ['audio.volume.dtmf', 15, nsIAudioManager.STREAM_TYPE_DTMF],
>      ['audio.volume.tts', 15, nsIAudioManager.STREAM_TYPE_TTS],
> +    ['audio.volume.fm', 15, nsIAudioManager.STREAM_TYPE_FM],

So do we already make sure that Gaia will hardcode the values for max volumes?

::: dom/fm/DOMFMRadioParent.jsm
@@ +361,5 @@
> +      if ("nsIAudioManager" in Ci && audioManager) {
> +        const nsIAudioManager = Ci.nsIAudioManager;
> +        let idx = audioManager.getMaxStreamVolumeIndex(nsIAudioManager.STREAM_TYPE_FM, idx);
> +        idx = audioManager.getStreamVolumeIndex(nsIAudioManager.STREAM_TYPE_MUSIC, idx);
> +        audioManager.setStreamVolumeIndex(nsIAudioManager.STREAM_TYPE_FM, idx);

The usage of audioManager.getXXXIndex() should just have one argument for stream type only and get index from return value. By the way if we put audio volume sync here, the user may listen the FM audio first then find volume be adjusted.

::: dom/system/gonk/AudioManager.cpp
@@ +378,5 @@
>  AudioManager::SetStreamVolumeIndex(int32_t aStream, int32_t aIndex) {
>    status_t status =
>      AudioSystem::setStreamVolumeIndex(static_cast<audio_stream_type_t>(aStream), aIndex);
> +    // sync the fm stream volume with music type
> +    if (IsDeviceOn(AUDIO_DEVICE_OUT_FM) && AUDIO_STREAM_MUSIC == aStream) {

Follow the Michael's comment, please move easy condition check to first one (AUDIO_STREAM_MUSIC == aStream) then we can avoid to call IsDeviceOn(AUDIO_DEVICE_OUT_FM).

Comment 17

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

Attachment: patch4

update patch 4 and move the fm power-on volume sync to AudioManager.

Comment 18

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

Follow by Jonas suggestion, we will request gaia layer hard-code the maximal volume at this stage.

Comment 19

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

Attachment: patch 5

Comment 20

[Marco Chen [:mchen]]

Comment on attachment 685544 [details] [diff] [review]
patch 5

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

::: dom/system/gonk/AudioManager.cpp
@@ +360,5 @@
>  
>  NS_IMETHODIMP
>  AudioManager::SetFmRadioAudioEnabled(bool aFmRadioAudioEnabled)
>  {
> +  int32_t  volIndex= 0;

Since we didn't use this variable on the case of "else", maybe we can move it into line 368.

@@ +370,5 @@
>        AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE, "");
>      InternalSetAudioRoutes(GetCurrentSwitchState(SWITCH_HEADPHONES));
> +    // sync volume with music after powering on fm radio
> +    AudioSystem::getStreamVolumeIndex(static_cast<audio_stream_type_t>(AUDIO_STREAM_MUSIC), &volIndex);
> +    AudioSystem::setStreamVolumeIndex(static_cast<audio_stream_type_t>(AUDIO_STREAM_FM), volIndex);

This timing here seems to be later then change of audio routing path.
We may need to move these codes before line 368.

Comment 21

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

Try to move those codes before line 368 but found it don't work.
That may cause by kernel driver only allow the volume-control during the enabled status.

Comment 22

[Marco Chen [:mchen]]

Comment on attachment 685544 [details] [diff] [review]
patch 5

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

Thanks for helping to confirm timing between routing path and FM volume setting.
As you mentioned, I think the reason is not about the timing of FM enabled or not.
But the ADSP need to change the routing path internally then ADSP just can accept the FM volume changing.

Comment 23

[Marco Chen [:mchen]]

Comment on attachment 685544 [details] [diff] [review]
patch 5

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

Please update the patch by comments then ask for review by Michanel Wu.

::: dom/system/gonk/AudioManager.cpp
@@ +370,5 @@
>        AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE, "");
>      InternalSetAudioRoutes(GetCurrentSwitchState(SWITCH_HEADPHONES));
> +    // sync volume with music after powering on fm radio
> +    AudioSystem::getStreamVolumeIndex(static_cast<audio_stream_type_t>(AUDIO_STREAM_MUSIC), &volIndex);
> +    AudioSystem::setStreamVolumeIndex(static_cast<audio_stream_type_t>(AUDIO_STREAM_FM), volIndex);

In addtional that, this function will be called in two conditions for eanbling and disabling FM audio. We should only do it on the case of enabling FM audio.

Comment 24

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

Attachment: patch 6

avoid sent volume control on disable fm radio

Comment 25

[Michael Wu [:mwu]]

Comment on attachment 685917 [details] [diff] [review]
patch 6

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

Please pay closer attention to style. The patch looks fine otherwise.

::: dom/system/gonk/AudioManager.cpp
@@ +369,5 @@
>        AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE, "");
>      InternalSetAudioRoutes(GetCurrentSwitchState(SWITCH_HEADPHONES));
> +    // sync volume with music after powering on fm radio
> +    if (aFmRadioAudioEnabled) {
> +      int32_t  volIndex= 0;

The whitespace on this line is wrong.

@@ +383,5 @@
>  NS_IMETHODIMP
>  AudioManager::SetStreamVolumeIndex(int32_t aStream, int32_t aIndex) {
>    status_t status =
>      AudioSystem::setStreamVolumeIndex(static_cast<audio_stream_type_t>(aStream), aIndex);
> +    // sync the fm stream volume with music type

This code is indented wrong.

Use this for the comment:
// sync the fm stream volume with music volume

@@ +384,5 @@
>  AudioManager::SetStreamVolumeIndex(int32_t aStream, int32_t aIndex) {
>    status_t status =
>      AudioSystem::setStreamVolumeIndex(static_cast<audio_stream_type_t>(aStream), aIndex);
> +    // sync the fm stream volume with music type
> +    if ((aStream == AUDIO_STREAM_MUSIC) && IsDeviceOn(AUDIO_DEVICE_OUT_FM)) {

Remove the parenthesis around the aStream check.

Comment 26

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

Attachment: patch7

correct the coding style

Comment 27

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

Attachment: patch 7

Comment 28

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

Attachment: patch 9

Comment 29

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

Comment on attachment 686378 [details] [diff] [review]
patch 9

Dear Michael, 
Please help to review my patch, thanks.

Comment 30

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

Attachment: check-in patch

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Ed Morley [:emorley]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8e2be08d3faa
https://hg.mozilla.org/releases/mozilla-beta/rev/e0dbc3236b64

Comment 34

[John Shih]

verified on Unagi
2012-12-03
gaia master : 5d150b2b10472478e8841730abd9ae9597595206
mozilla-beta : 1e56f66d7ee9