1196724 - Refactoring of AudioManager

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Sotaro Ikeda [:sotaro]]

Bug 1194442 cleans up AudioManager without changing how to control audio. This bug refactors AudioManager code as to reduce redundant code.

Comment 1

[Sotaro Ikeda [:sotaro]]

Bug 1170117 and Bug 1170117 added Separate volume control settings, but it seems not work properly with AudioPolicyService.

Comment 2

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> Bug 1170117 and Bug 1170117 added Separate volume control settings, but it
> seems not work properly with AudioPolicyService.

Android expects to use AudioSystem::getDevicesForStream() to get active devices for a stream type.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Fix nits.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Update nits.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Update nits.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Fix AudioManager::VolumeStreamState::SetVolumeIndexWithAliasDevices()

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Fixed VolumeStreamState::SetVolumeIndexWithAliasDevices()

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

reduce settings update.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Handle multiple audio devices out.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Update AudioManager::VolumeStreamState::IsDeviceChanged() to handle multiple out devices.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Update AudioManager::SendVolumeChangeNotification() to handle multiple devices.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Fix build error on KK.

Comment 18

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8659416 [details] [diff] [review]
patch - Refactoring of AudioManager

I am going to update the patch.

Comment 19

[Sotaro Ikeda [:sotaro]]

During testing, I recognized that sometimes audio out devices of AUDIO_STREAM_NOTIFICATION were not same when head set was plugged in.

From AudioManager's implementation, audio out devices of AUDIO_STREAM_NOTIFICATION get effect from AUDIO_STREAM_MUSIC activity. If AUDIO_STREAM_MUSIC is active within SONIFICATION_RESPECTFUL_AFTER_MUSIC_DELAY(5000ms), AUDIO_STREAM_NOTIFICATION is sent to same device to AUDIO_STREAM_MUSIC.

In AudioPolicyManager, AUDIO_STREAM_NOTIFICATION is mapped to STRATEGY_SONIFICATION_RESPECTFUL by AudioPolicyManager::getStrategy(). And devices for the strategy are choesn by AudioPolicyManager::getDeviceForStrategy().

http://androidxref.com/5.1.1_r6/xref/frameworks/av/services/audiopolicy/AudioPolicyManager.cpp#4352
http://androidxref.com/5.1.1_r6/xref/frameworks/av/services/audiopolicy/AudioPolicyManager.cpp#4515

Comment 20

[Sotaro Ikeda [:sotaro]]

Relationships between STRATEGY and audio stream type.

- STRATEGY_PHONE
    + AUDIO_STREAM_VOICE_CALL
    + AUDIO_STREAM_BLUETOOTH_SCO
    + out devices: one device is chosen by priority
- STRATEGY_SONIFICATION_RESPECTFUL
    + AUDIO_STREAM_NOTIFICATION
    + out devices: normally same to STRATEGY_SONIFICATION(normally 2 devices)
      except AUDIO_STREAM_MUSIC is active within 5000ms.
- STRATEGY_SONIFICATION
    + AUDIO_STREAM_RING
    + AUDIO_STREAM_ALARM
    + out devices: normally 2 devices(AUDIO_DEVICE_OUT_SPEAKER + 'STRATEGY_MEDIA selection') are chosen
      except phone call and hdmi audio
- STRATEGY_MEDIA
    + AUDIO_STREAM_MUSIC
    + AUDIO_STREAM_SYSTEM
    + out devices: normally one device is chosen by priority
      except hdmi audio 
- STRATEGY_ENFORCED_AUDIBLE
    + AUDIO_STREAM_ENFORCED_AUDIBLE
    + out devices: one device(speaker)

Comment 21

[Sotaro Ikeda [:sotaro]]

From comment 20, we could maintain device independent volume control to AUDIO_STREAM_MUSIC,AUDIO_STREAM_VOICE_CALL and AUDIO_STREAM_BLUETOOTH_SCO. But AUDIO_STREAM_NOTIFICATION, AUDIO_STREAM_RING and AUDIO_STREAM_ALARM almost always have multiple out devices.

Things seems to becomes simpler if AUDIO_STREAM_NOTIFICATION, AUDIO_STREAM_RING and AUDIO_STREAM_ALARM have single volume for all devices.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Add workaround of volume index of AUDIO_STREAM_NOTIFICATION, AUDIO_STREAM_ALARM and AUDIO_STREAM_RING.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Reduce binder ipc on lollipop.

Comment 24

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8660362 [details] [diff] [review]
patch - Refactoring of AudioManager

alwu, can you feedback to the patch?

Comment 25

[Alastor Wu [:alwu]]

Sure, but we have a workweek in this week. I'll feedback this patch later.

Comment 26

[Sotaro Ikeda [:sotaro]]

alwu, about when can you feed back to the patch?

Comment 27

[Alastor Wu [:alwu]]

Sorry about my delay feedback, I'll feedback it ASAP in recent day.

Comment 28

[Alastor Wu [:alwu]]

Hi, Sotaro,
Because this patch has lots changing, could you use the MozReview?
It can see the changing clearly, thanks.

Comment 29

[Sotaro Ikeda [:sotaro]]

I know there are still several places that need to be improved. But I wanted to know about feedback to basics.

Comment 30

[Alastor Wu [:alwu]]

Comment on attachment 8660362 [details] [diff] [review]
patch - Refactoring of AudioManager

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

Sorry for my late feedback, in general, it looks good.
Here are some questions.

::: dom/system/gonk/AudioManager.cpp
@@ +997,5 @@
>    }
> +
> +  if (streamAlias == AUDIO_STREAM_MUSIC && IsFmOutConnected()) {
> +    rv = mStreamStates[AUDIO_STREAM_FM]->
> +      SetVolumeIndex(aIndex, AUDIO_DEVICE_OUT_FM);

In Android version < 19, we have no "AUDIO_STREAM_FM", right? (you removed it)
So we don't have mStreamStates[AUDIO_STREAM_FM].

::: dom/system/gonk/AudioManager.h
@@ +93,5 @@
> +    uint32_t GetVolumeIndex(uint32_t aDevice);
> +    void ClearCurrentVolumeUpdated();
> +    nsresult SetVolumeIndexToDevices(uint32_t aIndex);
> +    nsresult SetVolumeIndexWithAliasStreams(uint32_t aIndex, uint32_t aDevice);
> +    nsresult SetVolumeIndexWithAliasDevices(uint32_t aIndex, uint32_t aDevice);

What's the difference between these functions? It's little confusion.
In addition, the first one is with alias streams, but the input parameter is device instead of stream, it's strange.

Comment 31

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu] from comment #30)
> Comment on attachment 8660362 [details] [diff] [review]
> patch - Refactoring of AudioManager
> 
> Review of attachment 8660362 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for my late feedback, in general, it looks good.
> Here are some questions.
> 
> ::: dom/system/gonk/AudioManager.cpp
> @@ +997,5 @@
> >    }
> > +
> > +  if (streamAlias == AUDIO_STREAM_MUSIC && IsFmOutConnected()) {
> > +    rv = mStreamStates[AUDIO_STREAM_FM]->
> > +      SetVolumeIndex(aIndex, AUDIO_DEVICE_OUT_FM);
> 
> In Android version < 19, we have no "AUDIO_STREAM_FM", right? (you removed
> it)
> So we don't have mStreamStates[AUDIO_STREAM_FM].

Bug 1206212 was not applied to the patch yet. On master, gecko does not use "AUDIO_STREAM_FM" if ANDROID_VERSION >= 19. AUDIO_STREAM_FM never exists on anroid AOSP, it is better to be removed if it is not used even on caf.

> 
> ::: dom/system/gonk/AudioManager.h
> @@ +93,5 @@
> > +    uint32_t GetVolumeIndex(uint32_t aDevice);
> > +    void ClearCurrentVolumeUpdated();
> > +    nsresult SetVolumeIndexToDevices(uint32_t aIndex);
> > +    nsresult SetVolumeIndexWithAliasStreams(uint32_t aIndex, uint32_t aDevice);
> > +    nsresult SetVolumeIndexWithAliasDevices(uint32_t aIndex, uint32_t aDevice);
> 
> What's the difference between these functions? It's little confusion.

Yea, it is confusing. I am going to remove SetVolumeIndexWithAliasDevices().

> In addition, the first one is with alias streams, but the input parameter is
> device instead of stream, it's strange.

Do you mention about "SetVolumeIndex***" functions? I assume your are asking about SetVolumeIndexWithAliasStreams(). It might better to change the name. Its implementation mimics part of android AudioService.VolumeStreamState.setIndex(int index, int device). It set volume index of specified stream and its alias streams. Argument of device just specify which device is going to be updated. Because volume could be different between devices in same stream type. gecko side could not know in advance which device will be actually chosen by AudioPolicyManager.

http://androidxref.com/5.1.1_r6/xref/frameworks/base/media/java/android/media/AudioService.java#3609

If you have questions, please let me know.

Comment 32

[Sotaro Ikeda [:sotaro]]

> > ::: dom/system/gonk/AudioManager.h
> > @@ +93,5 @@
> > > +    uint32_t GetVolumeIndex(uint32_t aDevice);
> > > +    void ClearCurrentVolumeUpdated();
> > > +    nsresult SetVolumeIndexToDevices(uint32_t aIndex);
> > > +    nsresult SetVolumeIndexWithAliasStreams(uint32_t aIndex, uint32_t aDevice);
> > > +    nsresult SetVolumeIndexWithAliasDevices(uint32_t aIndex, uint32_t aDevice);
> > 
> > What's the difference between these functions? It's little confusion.
> 
> Yea, it is confusing. I am going to remove SetVolumeIndexWithAliasDevices().

I changed my mind. For the time being, it seems better to keep it.

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

- rebase
- code clean up
- Changed some functions name to make more explicit
- Add comments to confusing functions

Comment 34

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8665577 [details] [diff] [review]
patch - Refactoring of AudioManager

alwu, is the code became easier to understand than before?

Comment 35

[Alastor Wu [:alwu]]

Comment on attachment 8665577 [details] [diff] [review]
patch - Refactoring of AudioManager

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

Here are some questions and comments.

I think the process of volume updating is quite complex.
Do you think could we simplify it more? 

Thanks!

::: dom/system/gonk/AudioManager.cpp
@@ +447,4 @@
>        String8 cmd;
>        cmd.appendFormat("bt_samplerate=%d", kBtSampleRate);
>        AudioSystem::setParameters(0, cmd);
>        SetForceForUse(nsIAudioManager::USE_COMMUNICATION, nsIAudioManager::FORCE_BT_SCO);

When we switch device, how do we change the profile and send the volume changing event to Gaia?

@@ +671,5 @@
> +  AudioSystem::setErrorCallback(BinderDeadCallback);
> +#if ANDROID_VERSION >= 21
> +  android::sp<GonkAudioPortCallback> callback = new GonkAudioPortCallback();
> +  AudioSystem::setAudioPortCallback(callback);
> +#endif

If ANDROID_VERSION < 21, how do we update the initial volume?

@@ +682,2 @@
>    }
> +  UpdateDevicesForStreams();

If ANDROID_VERSION < 21, we wouldn't update the devices for streams?

@@ +1114,5 @@
>  {
> +  if (!mIsVolumeInited) {
> +    return;
> +  }
> +

What is the situation we call this before volume initialized?
If we don't want it be called before initialization, maybe use MOZ_ASSERT(mIsVolumeInited);

@@ +1123,5 @@
>    }
>  
> +  // Synchronize volume index between devices if necessary
> +  for (uint32_t idx = 0; idx < MOZ_ARRAY_LENGTH(gVolumeData); ++idx) {
> +    int32_t  streamType = gVolumeData[idx].mStreamType;

Reduce one empty space after "int32_t".

@@ +1147,5 @@
>    mozilla::AutoSafeJSContext cx;
>    JS::Rooted<JS::Value> value(cx);
> +  uint32_t volume = 0;
> +  for (uint32_t idx = 0; idx < MOZ_ARRAY_LENGTH(gVolumeData); ++idx) {
> +    int32_t  streamType = gVolumeData[idx].mStreamType;

Ditto.

@@ +1200,5 @@
> +      lock->Set(AppendProfileToVolumeSetting(
> +                  gVolumeData[idx].mChannelName,
> +                  DEVICE_BLUETOOTH).get(),
> +                  value, nullptr, nullptr);
> +    }

This is wrong, think about this case,

Ex. Use BT first, then use headset. So in this case, our active device is headset.
However, the last event you dispatched to the DB is the BT volume setting.
That means the Gaia will get the wrong volume setting.

We should dispatch the present profile here.
It also means that mAudioOutProfileUpdated can't be the bitwise variable.

@@ +1226,4 @@
>  {
> +#if ANDROID_VERSION >= 21
> +  for (int32_t streamType = 0; streamType < AUDIO_STREAM_MAX; streamType++) {
> +    // Update deives for stream

s/deives/devices

@@ +1257,5 @@
> +      aStream == AUDIO_STREAM_ALARM ||
> +      aStream == AUDIO_STREAM_RING) {
> +    devices |= AUDIO_DEVICE_OUT_SPEAKER;
> +  }
> +

Could you give me an example for "unstable stream's volume index"?
In addition, if we're playing music, the "AUDIO_STREAM_NOTIFICATION" shouldn't be out from speaker. 
If no music and headset is connected, we can get AUDIO_DEVICE_OUT_SPEAKER from line#1248, right? If so, why we need this if-condition?

@@ +1381,5 @@
> +    if (exist && aIndex == oldVolumeIndex) {
> +      // No update
> +      continue;
> +    }
> +    nsresult rv = SetVolumeIndexToAliasDevices(aIndex, device);

Since you update the profile in SetVolumeIndexToAliasDevices(), this logic would cause a problem.
If the headset is connected, now we have a "alarm" stream, the audio outputs are both speaker and headset. Then you will set the speaker and headset to the profile. 
 
In this case, we just want the "headset" profile.
I'm not sure whether we need to change the speaker volume at that time, because that means you'll change the other volume setting.
 
When you're in the "headset" profile, you change the alarm's volume, then we change the speaker and headset volume.
After that, we switch back to the "primary" profile, is the speaker volume the still same as previous one? or it would be changed by above operation?

Comment 36

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu] from comment #35)
> Comment on attachment 8665577 [details] [diff] [review]
> patch - Refactoring of AudioManager
> 
> Review of attachment 8665577 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Here are some questions and comments.
> 
> I think the process of volume updating is quite complex.
> Do you think could we simplify it more? 
> 
> Thanks!
> 

The patch does not use audio output profiles to control audio volume. The profile is used just to store/read profile based volume. Instead active devices are used to actually control volume.

> ::: dom/system/gonk/AudioManager.cpp
> @@ +447,4 @@
> >        String8 cmd;
> >        cmd.appendFormat("bt_samplerate=%d", kBtSampleRate);
> >        AudioSystem::setParameters(0, cmd);
> >        SetForceForUse(nsIAudioManager::USE_COMMUNICATION, nsIAudioManager::FORCE_BT_SCO);
> 
> When we switch device, how do we change the profile and send the volume
> changing event to Gaia?

It does not notify to gaia depends on profile, but depends on active devices change. It is done by AudioManager::SendVolumeChangeNotification(bool aForce). Since L, active devices change is detected by onAudioPortListUpdate(). Before L, SendVolumeChangeNotification() is called manually to detect active devices change.

> 
> @@ +671,5 @@
> > +  AudioSystem::setErrorCallback(BinderDeadCallback);
> > +#if ANDROID_VERSION >= 21
> > +  android::sp<GonkAudioPortCallback> callback = new GonkAudioPortCallback();
> > +  AudioSystem::setAudioPortCallback(callback);
> > +#endif
> 
> If ANDROID_VERSION < 21, how do we update the initial volume?

The patch manually call SendVolumeChangeNotification() several places. In it, it checks if active devices are changed by calling VolumeStreamState::IsDevicesChanged().

> 
> @@ +682,2 @@
> >    }
> > +  UpdateDevicesForStreams();
> 
> If ANDROID_VERSION < 21, we wouldn't update the devices for streams?

Explanation seems lacking. I am going to add a comment here. It might better to change the function name.

If ANDROID_VERSION < 21, we do not need to update device status here. It is a optimization code for AudioManager::GetDevicesForStream() since L. On L, AudioManager can know when active devices are updated. Therefore, AudioManager need to get active devices only when they are updated. If there is not update to them, AudioManager could use cached values. UpdateDevicesForStreams() is a function to update the cache.

But if ANDROID_VERSION < 21, AudioManager could not know when active devices are changed, therefore, 
AudioManager::GetDevicesForStream() always get active devices from AudioPolicyManager in media server process. Therefore UpdateDevicesForStreams() is not necessary.

> 
> @@ +1114,5 @@
> >  {
> > +  if (!mIsVolumeInited) {
> > +    return;
> > +  }
> > +
> 
> What is the situation we call this before volume initialized?
> If we don't want it be called before initialization, maybe use
> MOZ_ASSERT(mIsVolumeInited);

It is necessary especially during AudioFlinger restarting.

> 
> @@ +1123,5 @@
> >    }
> >  
> > +  // Synchronize volume index between devices if necessary
> > +  for (uint32_t idx = 0; idx < MOZ_ARRAY_LENGTH(gVolumeData); ++idx) {
> > +    int32_t  streamType = gVolumeData[idx].mStreamType;
> 
> Reduce one empty space after "int32_t".

Update in a next patch.

> 
> @@ +1147,5 @@
> >    mozilla::AutoSafeJSContext cx;
> >    JS::Rooted<JS::Value> value(cx);
> > +  uint32_t volume = 0;
> > +  for (uint32_t idx = 0; idx < MOZ_ARRAY_LENGTH(gVolumeData); ++idx) {
> > +    int32_t  streamType = gVolumeData[idx].mStreamType;
> 
> Ditto.

Update in a next patch.

> 
> @@ +1200,5 @@
> > +      lock->Set(AppendProfileToVolumeSetting(
> > +                  gVolumeData[idx].mChannelName,
> > +                  DEVICE_BLUETOOTH).get(),
> > +                  value, nullptr, nullptr);
> > +    }
> 
> This is wrong, think about this case,
> 
> Ex. Use BT first, then use headset. So in this case, our active device is
> headset.
> However, the last event you dispatched to the DB is the BT volume setting.
> That means the Gaia will get the wrong volume setting.

If AudioPolicyManager select headset instead of bluetooth, headset is chosen as active device also by AudioManager. Therefore in the above use case, BT volume setting seems not updated by AudioManager. Can you explain more about it?

I could not understand your comment well. Does gaia use audio out profile settings? Can you mention the code? SoundManager seems to care only to non-profile based setting(active device based volume setting).

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/sound_manager.js


> 
> We should dispatch the present profile here.
> It also means that mAudioOutProfileUpdated can't be the bitwise variable.

My patch does not use the profile for actual volume control, instead active devices in AudioPolicyManager is used. Can you explain why it does not work?

> 
> @@ +1226,4 @@
> >  {
> > +#if ANDROID_VERSION >= 21
> > +  for (int32_t streamType = 0; streamType < AUDIO_STREAM_MAX; streamType++) {
> > +    // Update deives for stream
> 
> s/deives/devices

Update in a next patch.

> 
> @@ +1257,5 @@
> > +      aStream == AUDIO_STREAM_ALARM ||
> > +      aStream == AUDIO_STREAM_RING) {
> > +    devices |= AUDIO_DEVICE_OUT_SPEAKER;
> > +  }
> > +
> 
> Could you give me an example for "unstable stream's volume index"?
> In addition, if we're playing music, the "AUDIO_STREAM_NOTIFICATION"
> shouldn't be out from speaker.
> If no music and headset is connected, we can get AUDIO_DEVICE_OUT_SPEAKER
> from line#1248, right? If so, why we need this if-condition?

This is a work around of AudioPolicyManager. The code does not control audio output as to sound from speaker. As in comment 20,  AudioPolicyManager choose "AUDIO_STREAM_NOTIFICATION"'s active devices depends on AUDIO_STREAM_MUSIC's activity. If AUDIO_STREAM_MUSIC is active(or active within 500ms) , AudioPolicyManager choose "AUDIO_STREAM_NOTIFICATION"'s active devices same as to AUDIO_STREAM_MUSIC. But AUDIO_STREAM_MUSIC is not active(or not active within 500ms), AudioPolicyManager choose "AUDIO_STREAM_NOTIFICATION"'s active devices as to "AUDIO_STREAM_MUSIC + speaker". Therefore, when "AUDIO_STREAM_NOTIFICATION" is changed, if we always change volume of "active device + speaker", we could change "AUDIO_STREAM_NOTIFICATION"'s volume independent from AUDIO_STREAM_MUSIC.

> 
> @@ +1381,5 @@
> > +    if (exist && aIndex == oldVolumeIndex) {
> > +      // No update
> > +      continue;
> > +    }
> > +    nsresult rv = SetVolumeIndexToAliasDevices(aIndex, device);
> 
> Since you update the profile in SetVolumeIndexToAliasDevices(), this logic
> would cause a problem.
> If the headset is connected, now we have a "alarm" stream, the audio outputs
> are both speaker and headset. Then you will set the speaker and headset to
> the profile. 

Can you explain about it more concretely? Does gaia uses profile's volume data? The patch control audio volume depends on active devices. In the patch, profile is not used to control audio volume.

>  
> In this case, we just want the "headset" profile.
> I'm not sure whether we need to change the speaker volume at that time,
> because that means you'll change the other volume setting.
>  
> When you're in the "headset" profile, you change the alarm's volume, then we
> change the speaker and headset volume.
> After that, we switch back to the "primary" profile, is the speaker volume
> the still same as previous one? or it would be changed by above operation?

The patch control audio volume depends on active devices. In the patch, profile is not used to control audio volume. Profile is used just to retain device dependent volume between system reboots.

Therefore, there is no "headset" profile mode. Instead, there is a case that "headset" is chosen as active device by AudioPolicyManager.

My patch change audio volume of each audio stream type independently. Your question's use case is not clear about the audio stream type. Can you make your use case more clear about it? By applying the patch, when audio volume of a audio stream type is requested to AudioManager, AudioManager changes volume of active devices of the audio stream type. The following is the exception.

In my patch, the following streams always change volume with speaker volume. Because if there is no AUDIO_STREAM_MUSIC activity(or not active within 500ms), AudioPolicyManager choose "speaker + AUDIO_STREAM_MUSIC" devices.
 - AUDIO_STREAM_NOTIFICATION
 - AUDIO_STREAM_RING
 - AUDIO_STREAM_ALARM

Comment 37

[Sotaro Ikeda [:sotaro]]

alwu, if you still have questions please ask me. The patch's implementation was chosen as more consistent to AudioPolicyManager.

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Rebase and apply comments.

Comment 39

[Alastor Wu [:alwu]]

Hi, Sotaro,
Thanks for your explanation!
Here still are some questions, thanks!

(In reply to Sotaro Ikeda [:sotaro] from comment #36)
> > ::: dom/system/gonk/AudioManager.cpp
> > @@ +447,4 @@
> > >        String8 cmd;
> > >        cmd.appendFormat("bt_samplerate=%d", kBtSampleRate);
> > >        AudioSystem::setParameters(0, cmd);
> > >        SetForceForUse(nsIAudioManager::USE_COMMUNICATION, nsIAudioManager::FORCE_BT_SCO);
> It does not notify to gaia depends on profile, but depends on active devices
> change. It is done by AudioManager::SendVolumeChangeNotification(bool
> aForce). Since L, active devices change is detected by
> onAudioPortListUpdate(). Before L, SendVolumeChangeNotification() is called
> manually to detect active devices change.

If my understanding is correct, in L, every time we change the audio output, this function would be triggered.
 
But if we aren't in L, when we switch the audio output, we call UpdateDeviceConnectionState(), and then update the volume setting in SendVolumeChangeNotification(), right?

> > @@ +1114,5 @@
> > >  {
> > > +  if (!mIsVolumeInited) {
> > > +    return;
> > > +  }
> > > +
> > 
> > What is the situation we call this before volume initialized?
> > If we don't want it be called before initialization, maybe use
> > MOZ_ASSERT(mIsVolumeInited);
> 
> It is necessary especially during AudioFlinger restarting.

If the volume has not been initialized yet, why we need to call SendVolumeChangeNotification()?
This function should be called after initialization in literal. 

If there are some extra things need to be done in this function, we should split them out.
So that we can make code more readable.
 
> > @@ +1200,5 @@
> > > +      lock->Set(AppendProfileToVolumeSetting(
> > > +                  gVolumeData[idx].mChannelName,
> > > +                  DEVICE_BLUETOOTH).get(),
> > > +                  value, nullptr, nullptr);
> > > +    }
> 
> If AudioPolicyManager select headset instead of bluetooth, headset is chosen
> as active device also by AudioManager. Therefore in the above use case, BT
> volume setting seems not updated by AudioManager. Can you explain more about
> it?
> 
> I could not understand your comment well. Does gaia use audio out profile
> settings? Can you mention the code? SoundManager seems to care only to
> non-profile based setting(active device based volume setting).
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/sound_manager.
> js
> 

Sorry, I didn't notice that the notification is with the device name. It wouldn't affect the Gaia behavior.

> My patch does not use the profile for actual volume control, instead active
> devices in AudioPolicyManager is used. Can you explain why it does not work?

Same, it's work.
 
> > @@ +1257,5 @@
> > > +      aStream == AUDIO_STREAM_ALARM ||
> > > +      aStream == AUDIO_STREAM_RING) {
> > > +    devices |= AUDIO_DEVICE_OUT_SPEAKER;
> > > +  }
> > > +
> 
> This is a work around of AudioPolicyManager. The code does not control audio
> output as to sound from speaker. As in comment 20,  AudioPolicyManager
> choose "AUDIO_STREAM_NOTIFICATION"'s active devices depends on
> AUDIO_STREAM_MUSIC's activity. If AUDIO_STREAM_MUSIC is active(or active
> within 500ms) , AudioPolicyManager choose "AUDIO_STREAM_NOTIFICATION"'s
> active devices same as to AUDIO_STREAM_MUSIC. But AUDIO_STREAM_MUSIC is not
> active(or not active within 500ms), AudioPolicyManager choose
> "AUDIO_STREAM_NOTIFICATION"'s active devices as to "AUDIO_STREAM_MUSIC +
> speaker". Therefore, when "AUDIO_STREAM_NOTIFICATION" is changed, if we
> always change volume of "active device + speaker", we could change
> "AUDIO_STREAM_NOTIFICATION"'s volume independent from AUDIO_STREAM_MUSIC.

The AUDIO_STREAM_NOTIFICATION includes ringer/notification/alarm.

That means we'll change the speaker volume even when we're using the headset, right?

Example.
If the user usually set the ringer volume to 5 in speaker, 10 in headset.
In this patch, if user is using headset and increase 1 index, the speaker volume would be changed to 11.
If so, I think that is not a good situation.

> > @@ +1381,5 @@
> Can you explain about it more concretely? Does gaia uses profile's volume
> data? The patch control audio volume depends on active devices. In the
> patch, profile is not used to control audio volume.

Same as above example.

Comment 40

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu][OOO from 10/3-10/11] from comment #39)
> Hi, Sotaro,
> Thanks for your explanation!
> Here still are some questions, thanks!
> 
> (In reply to Sotaro Ikeda [:sotaro] from comment #36)
> > > ::: dom/system/gonk/AudioManager.cpp
> > > @@ +447,4 @@
> > > >        String8 cmd;
> > > >        cmd.appendFormat("bt_samplerate=%d", kBtSampleRate);
> > > >        AudioSystem::setParameters(0, cmd);
> > > >        SetForceForUse(nsIAudioManager::USE_COMMUNICATION, nsIAudioManager::FORCE_BT_SCO);
> > It does not notify to gaia depends on profile, but depends on active devices
> > change. It is done by AudioManager::SendVolumeChangeNotification(bool
> > aForce). Since L, active devices change is detected by
> > onAudioPortListUpdate(). Before L, SendVolumeChangeNotification() is called
> > manually to detect active devices change.
> 
> If my understanding is correct, in L, every time we change the audio output,
> this function would be triggered.
>  
> But if we aren't in L, when we switch the audio output, we call
> UpdateDeviceConnectionState(), and then update the volume setting in
> SendVolumeChangeNotification(), right?

Yes. AudioManager manually call SendVolumeChangeNotification().

> > > What is the situation we call this before volume initialized?
> > > If we don't want it be called before initialization, maybe use
> > > MOZ_ASSERT(mIsVolumeInited);
> > 
> > It is necessary especially during AudioFlinger restarting.
> 
> If the volume has not been initialized yet, why we need to call
> SendVolumeChangeNotification()?
> This function should be called after initialization in literal. 

Since L, AudioManager automatically call SendVolumeChangeNotification() when AudioPolicyMnager's active devices are changed.

> If there are some extra things need to be done in this function, we should
> split them out.
> So that we can make code more readable.

Can you explain more concretely?

> > > @@ +1257,5 @@
> > > > +      aStream == AUDIO_STREAM_ALARM ||
> > > > +      aStream == AUDIO_STREAM_RING) {
> > > > +    devices |= AUDIO_DEVICE_OUT_SPEAKER;
> > > > +  }
> > > > +
> > 
> > This is a work around of AudioPolicyManager. The code does not control audio
> > output as to sound from speaker. As in comment 20,  AudioPolicyManager
> > choose "AUDIO_STREAM_NOTIFICATION"'s active devices depends on
> > AUDIO_STREAM_MUSIC's activity. If AUDIO_STREAM_MUSIC is active(or active
> > within 500ms) , AudioPolicyManager choose "AUDIO_STREAM_NOTIFICATION"'s
> > active devices same as to AUDIO_STREAM_MUSIC. But AUDIO_STREAM_MUSIC is not
> > active(or not active within 500ms), AudioPolicyManager choose
> > "AUDIO_STREAM_NOTIFICATION"'s active devices as to "AUDIO_STREAM_MUSIC +
> > speaker". Therefore, when "AUDIO_STREAM_NOTIFICATION" is changed, if we
> > always change volume of "active device + speaker", we could change
> > "AUDIO_STREAM_NOTIFICATION"'s volume independent from AUDIO_STREAM_MUSIC.
> 
> The AUDIO_STREAM_NOTIFICATION includes ringer/notification/alarm.
> 
> That means we'll change the speaker volume even when we're using the
> headset, right?
> 
> Example.
> If the user usually set the ringer volume to 5 in speaker, 10 in headset.

How can user set the volume of ringer like the above consistently? Can you explain about it?

As in Comment 20, AUDIO_STREAM_RING always rings speaker and headset(except phone call). But I did not check if gais's ring actually uses AUDIO_STREAM_RING.

> In this patch, if user is using headset and increase 1 index, the speaker
> volume would be changed to 11.
> If so, I think that is not a good situation.

Anyway, we need a compromise to control audio consistently.

AudioManager controls the following audio as same volume. If headset is connected to the phone, each stream sound like the following.
 - AUDIO_STREAM_SYSTEM(only headset)
 - AUDIO_STREAM_RING(majority of case, both speaker and headset(except phone call))
 - AUDIO_STREAM_NOTIFICATION(both speaker and headset if AUDIO_STREAM_MUSIC is not active
                             only headset if AUDIO_STREAM_MUSIC is active )

To control, audio consistently, always controlling speaker and headset to same volume makes consistent in any cases from volume controlling point of view.

If you have a better idea, can you explain about it?

> 
> > > @@ +1381,5 @@
> > Can you explain about it more concretely? Does gaia uses profile's volume
> > data? The patch control audio volume depends on active devices. In the
> > patch, profile is not used to control audio volume.
> 
> Same as above example.

Comment 41

[Sotaro Ikeda [:sotaro]]

Settings app uses AUDIO_STREAM_NOTIFICATION to change notification volume. If we want to change the following volumes as same value, it seems better to use AUDIO_STREAM_RING in settings app.
 - AUDIO_STREAM_SYSTEM
 - AUDIO_STREAM_RING
 - AUDIO_STREAM_NOTIFICATION

Comment 42

[Alastor Wu [:alwu]]

(In reply to Sotaro Ikeda [:sotaro PTO 5/Oct - 14/Oct] from comment #40)
> > If there are some extra things need to be done in this function, we should
> > split them out.
> > So that we can make code more readable.
> 
> Can you explain more concretely?

In my opinion, we should know the explicit calling timing from the function name.
But now this function is about "updating", it's not very clear what situation we should call updating.

Since I don't have specific method to improve this now, if we still need to do that,
I think we can rename the function and also add the comment to describe where/when we should call this function.

> > > > @@ +1257,5 @@
> How can user set the volume of ringer like the above consistently? Can you
> explain about it?
> 
> As in Comment 20, AUDIO_STREAM_RING always rings speaker and headset(except
> phone call). But I did not check if gais's ring actually uses
> AUDIO_STREAM_RING.

We can do the separate volume controlling now, from bug 1170117.

User can have different volume setting in volume categories (media/notification/alarm/telephony/BT-SCO), and Gaia doesn't know about the stream type in volume controlling.

For alarm,
User have different volume settings in separate profile [1]. Think about that, user have volume 5 in the beginning, after a while, user starts to use the headset, and then adjust the volume to 10. So we have 5 in primary mode, 10 in headset mode.

In this patch, we'll set the volume to all active devices, when we plug the headset, the speaker and headset would all be set to 10. Unplug the headset, the speaker volume would be still keeping in 10, instead of changing back to 5. Is that right?

[1] See attachment, https://bugzilla.mozilla.org/show_bug.cgi?id=1172791

> Anyway, we need a compromise to control audio consistently.
> 
> AudioManager controls the following audio as same volume. If headset is
> connected to the phone, each stream sound like the following.
>  - AUDIO_STREAM_SYSTEM(only headset)
>  - AUDIO_STREAM_RING(majority of case, both speaker and headset(except phone
> call))
>  - AUDIO_STREAM_NOTIFICATION(both speaker and headset if AUDIO_STREAM_MUSIC
> is not active
>                              only headset if AUDIO_STREAM_MUSIC is active )
> 
> To control, audio consistently, always controlling speaker and headset to
> same volume makes consistent in any cases from volume controlling point of
> view.
> 
> If you have a better idea, can you explain about it?
> 

Yes, I agree with you to control the speaker and headset in the same time.

However, in user perspective, we should keep the profile volume consistently.

When we're using headset, changing the speaker/headset, it's ok and correct. 
But when we're back to speaker, the speaker volume should recover to its original volume, it shouldn't be changed by other profile's operation.

In my suggestion, we should add "profile" concept to control volume in this patch. 
Each profile have different stream volume settings, when user changes the audio output profile, we should get the correct profile volume setting, and set it to all devices.

In the reboot stage, we can get the profile data from the setting database, and it can make the volume setting consistently.

How do you think?

Comment 43

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu][OOO from 10/3-10/11] from comment #42)
> (In reply to Sotaro Ikeda [:sotaro PTO 5/Oct - 14/Oct] from comment #40)
> > > If there are some extra things need to be done in this function, we should
> > > split them out.
> > > So that we can make code more readable.
> > 
> > Can you explain more concretely?
> 
> In my opinion, we should know the explicit calling timing from the function
> name.
> But now this function is about "updating", it's not very clear what
> situation we should call updating.

If you want to clearer function name, it could be changed. 

> 
> > > > > @@ +1257,5 @@
> > How can user set the volume of ringer like the above consistently? Can you
> > explain about it?
> > 
> > As in Comment 20, AUDIO_STREAM_RING always rings speaker and headset(except
> > phone call). But I did not check if gais's ring actually uses
> > AUDIO_STREAM_RING.
> 
> We can do the separate volume controlling now, from bug 1170117.
> 
> User can have different volume setting in volume categories
> (media/notification/alarm/telephony/BT-SCO), and Gaia doesn't know about the
> stream type in volume controlling.

The patch also have different volumes between (media/notification/alarm/telephony/BT-SCO). I do not think there is a difference. The difference comes from how to handle a stream's volume that have multiple active devices from user point of view.

> For alarm,
> User have different volume settings in separate profile [1]. Think about
> that, user have volume 5 in the beginning, after a while, user starts to use
> the headset, and then adjust the volume to 10. So we have 5 in primary mode,
> 10 in headset mode.
> 
> In this patch, we'll set the volume to all active devices, when we plug the
> headset, the speaker and headset would all be set to 10. Unplug the headset,
> the speaker volume would be still keeping in 10, instead of changing back to
> 5. Is that right?

Yes. Could you explain why it needs to back to 5? AUDIO_STREAM_RING is audible even when headset is plugged.

On a stream that chooses one device seems to work same way on my patch and the profile concept. For example, AUDIO_STREAM_MUSIC, AUDIO_STREAM_VOICE_CALL always choose one stream. If headset is plugged, audio sound only on headset and volume change affect only to headset. If headset is un-plugged, speaker is chosen as out put and volume change only affect to headset.

From it, a problem exist only when a stream choose multiple devices. In my patch, it keeps its audio volume. In the profile concept, each profile preserve each volume set.

I could not understand the concept of profile well. Actual active device is chosen by AduioPolicyManager. if multiple audio out device is connected to the device. AudioPolicyManager select one device or some devices for output. If we want to have a profile kind of concept, it should be depend on it. In android, one active device selection is done by AudioService.getDeviceForStream(). One device selection logic is different from the profile concept. It prefer to choose 'speaker out'. By modifying the priority. We could choose active profile of a audio stream depends on AuidoPolicyManager's decision. Is there a problem about it?

http://androidxref.com/5.1.1_r6/xref/frameworks/base/media/java/android/media/AudioService.java#3404


> Yes, I agree with you to control the speaker and headset in the same time.
> 
> However, in user perspective, we should keep the profile volume consistently.

Consistency is different from user's perspective. If a audio stream chooses one audio out device there seems no confusion from user's point of view. But if a audio stream chooses multiple audio out devices, some kinds of compromise becomes necessary, because user does not control by choosing every stream type and every active devices.

Android control audio like the following. For me, it is more consistent than current AudioManager's profile.

- user's volume controlling becomes calling of AudioService's function call.
  audio stream type is detected depends on the context.
- When AudioService control audio stream type's audio,
  the following devices of volumes of all aliased streams are set to same volume index.
  + The adjusting stream's one active device 
  + Aliased stream's one active device
  
My patch control similarly to the android, but it adjusts all active devices of stream, instead of one device of stream.

> 
> When we're using headset, changing the speaker/headset, it's ok and correct. 
> But when we're back to speaker, the speaker volume should recover to its
> original volume, it shouldn't be changed by other profile's operation.
> 
> In my suggestion, we should add "profile" concept to control volume in this
> patch. 
> Each profile have different stream volume settings, when user changes the
> audio output profile, we should get the correct profile volume setting, and
> set it to all devices.
> 
> In the reboot stage, we can get the profile data from the setting database,
> and it can make the volume setting consistently.
> 
> How do you think?

I still do not understand why the profile concept is consistent to user. For example, android also have different volume control between different devices. But it does not have the profile concept. Android might choose it because it is consistent for their perspective.

The difference happens between the profile concept and the patch when a stream chooses multiple audio out devices. Except it, it seems to work same way from user's perspective.

For me, stream type and device combination is important to control audio. If the stream choose multiple devices, it control the devices and their volumes are kept independent from the device connection update. It looks more consistent from user point of view.

I could not convince that the profile is more consistent from user's point of view. Is there an actual system that works same way?

Comment 44

[Sotaro Ikeda [:sotaro]]

> 
> 
> > Yes, I agree with you to control the speaker and headset in the same time.
> > 
> > However, in user perspective, we should keep the profile volume consistently.
> 
> Consistency is different from user's perspective. 

Correction:

volume consistency could be different among users.

Comment 45

[Sotaro Ikeda [:sotaro]]

> 
> For me, stream type and device combination is important to control audio. If
> the stream choose multiple devices, it control the devices and their volumes
> are kept independent from the device connection update. It looks more
> consistent from user point of view.
> 
> I could not convince that the profile is more consistent from user's point
> of view. Is there an actual system that works same way?

Android is already widely used in the world and the patch's controlling is similar to android's one. Therefore from users point of view, the patch's controlling could be consistent. And users could have device dependent volumes on voice call and media playback. It seems to fulfill necessary use cases for users.

Therefore, I could not find actual benefits of the profile mode. And I do not know whether there is a system that is very similar to the profile mode. If we do not find big advantage of the profile, it seems better to control similar to android, because it is already commonly used in the world and it could provide device specific volume to phone call and media playback.

Can you explain the benefits of the profile more and whether there is similar system?

Comment 46

[Sotaro Ikeda [:sotaro]]

I checked how audio is controlled on Settings app on master flame-kk, when headset is plugged. Changing sliders of 'Ringtomes & Nofitications' and 'Alarm' did not change the volume of speaker. And Changing sliders of 'Ringtomes & Nofitications' and 'Alarm' seems not always update volume of headset.

Comment 47

[Alastor Wu [:alwu]]

(In reply to Sotaro Ikeda [:sotaro PTO 5/Oct - 14/Oct] from comment #43)
> > > > > > @@ +1257,5 @
> The patch also have different volumes between
> (media/notification/alarm/telephony/BT-SCO). I do not think there is a
> difference. The difference comes from how to handle a stream's volume that
> have multiple active devices from user point of view.

The problem is that we want the device can provide the separate volume setting. 
That is a feature which can let users enjoy the most comfortable volume when they change the device.

The different volumes categories (media/notification/alarm/telephony/BT-SCO) is basic, the advance options is we have multiple different volumes categories setting and it's depend on the device.

See bug1104972, bug1172791.

> Yes. Could you explain why it needs to back to 5? AUDIO_STREAM_RING is
> audible even when headset is plugged.
> 
> On a stream that chooses one device seems to work same way on my patch and
> the profile concept. For example, AUDIO_STREAM_MUSIC,
> AUDIO_STREAM_VOICE_CALL always choose one stream. If headset is plugged,
> audio sound only on headset and volume change affect only to headset. If
> headset is un-plugged, speaker is chosen as out put and volume change only
> affect to headset.
> 
> From it, a problem exist only when a stream choose multiple devices. In my
> patch, it keeps its audio volume. In the profile concept, each profile
> preserve each volume set.
> 

It's very important to back to 5.

Something need to be noticed,
* When user plug the headset, the volume would be automatically decrease. * [1] 

Let's start with this assumption, the ringtone volume is 5 in the beginning.

When without headset, I feel 5 is the most comfortable volume for me.
And then I use headset, and notice that the volume become lower, because of Android's mechanism [1] or device hardware constraint.
Therefore, I need to increase more volume (maybe to 10) that the ringtone volume can achieve the same level as previous one.

After that, I unplug the headset, and also expect that the ringtone volume is my expected volume. (5 is comfortable for me)
But, I found that the ringtone is too loud (because it changes from 5 to 10), so I need to decrease it again.

Above situation would happen again and again when we plug/unplug the headset.

That is the initial reason I want to implement the profile-based volume control.

We can also ask UX to confirm that, if you still think this scenario is incorrect.

[1] http://androidxref.com/5.1.1_r6/xref/frameworks/av/services/audiopolicy/AudioPolicyManager.cpp#5658
In my test, if we use maximum volume (UI volume), and check the actual stream volume (using ringtone/alarm),
Without headset, max volume = 15, actual stream volume = 1.0
With    headset, max volume = 15, actual stream volume = 0.5

> I could not understand the concept of profile well. Actual active device is
> chosen by AduioPolicyManager. if multiple audio out device is connected to
> the device. AudioPolicyManager select one device or some devices for output.
> If we want to have a profile kind of concept, it should be depend on it. In
> android, one active device selection is done by
> AudioService.getDeviceForStream(). One device selection logic is different
> from the profile concept. It prefer to choose 'speaker out'. By modifying
> the priority. We could choose active profile of a audio stream depends on
> AuidoPolicyManager's decision. Is there a problem about it?
> 
> http://androidxref.com/5.1.1_r6/xref/frameworks/base/media/java/android/
> media/AudioService.java#3404
> 

In my opinion, when we're using the common headset or BT headset, the mainly output device is headset instead of speaker.
I don't think choosing the "speaker out" is a good option.
Anyway, we can also ask UX feedback for that.

> > Yes, I agree with you to control the speaker and headset in the same time.
> > 
> > However, in user perspective, we should keep the profile volume consistently.
> 
> Consistency is different from user's perspective. If a audio stream chooses
> one audio out device there seems no confusion from user's point of view. But
> if a audio stream chooses multiple audio out devices, some kinds of
> compromise becomes necessary, because user does not control by choosing
> every stream type and every active devices.
> 
> Android control audio like the following. For me, it is more consistent than
> current AudioManager's profile.
> 
> - user's volume controlling becomes calling of AudioService's function call.
>   audio stream type is detected depends on the context.
> - When AudioService control audio stream type's audio,
>   the following devices of volumes of all aliased streams are set to same
> volume index.
>   + The adjusting stream's one active device 
>   + Aliased stream's one active device
>   
> My patch control similarly to the android, but it adjusts all active devices
> of stream, instead of one device of stream.

Your control flow doesn't have problem, I just think that we can preserve the profile concept as well.
With the profile concept doesn't mean we don't use this control flow.

Just add the extra condition when we change the audio output device.
- reset the volume settings depend on different profile

and then the following flow is the same as you mentioned.

> I still do not understand why the profile concept is consistent to user. For
> example, android also have different volume control between different
> devices. But it does not have the profile concept. Android might choose it
> because it is consistent for their perspective.
> 
> The difference happens between the profile concept and the patch when a
> stream chooses multiple audio out devices. Except it, it seems to work same
> way from user's perspective.

The most difference is that the device can't remember user's preference volume setting.

> For me, stream type and device combination is important to control audio. If
> the stream choose multiple devices, it control the devices and their volumes
> are kept independent from the device connection update. It looks more
> consistent from user point of view.
> 
> I could not convince that the profile is more consistent from user's point
> of view. Is there an actual system that works same way?

I don't familiar with other system, but I think we should find something we all think is correct. 
Same, ask UX for confirm the correct behavior.

Comment 48

[Alastor Wu [:alwu]]

Hi, Tori,
Sorry to bother you,
Here we have some audio UX related questions,
Could you help us to confirm that?
Very appreciate :)

In bug1104972, bug1172791, we discuss about the separate volume setting and it's implemented in bug 1170117.
Now we have some questions,

---

> Q1 - Is reasonable that keep different volume settings depend on profiles (primary/headset/BT-headset)?

First, see this attachment [1], it can let you know this feature easily.

We don't have the specific UX spec for this feature, so it causes us have different opinions for it.

Do you think that we change volume setting by different profiles is reasonable?

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8617123

---

> Q2 - What's the most proper way to handle volume settings? devices or profile?

Android has a special behavior is that some kinds of stream would be playback from multiple audio output devices. (ex. ringtone/alarm)

That means the incoming call would be heard from both speaker and headset when you're using the headset.

In my opinion, although we have two audio outputs, the headset is still the MAINLY output, the speaker is just used to avoid user missing the call if user doesn't wear the headset at that time.

Therefore, I want to create "profile" concept for users, it's like [1], user would have three different volume setting.

In Sotaro's opinion, he think user would care about both speaker and headset, so we shouldn't use the "profile" concept. We should control volume by each device. 

Since we can't have consensus, it cause the following question, Q3.

Could you provide some feedbacks from the UX perspective for this subject?

---

> Q3 - Whether the volume should be kept when we switch different audio output devices?

In this case, I use the ringtone as the example, and something you should know is that the ringtone would be playback from both speaker and headset when the headset is plugged (Android design).

In the beginning, I use the phone without any device, and I thinks the volume 5 is most comfortable volume for me.

Then I use headset, and the volume 10 is most comfortable for me, because of Android's mechanism [2] or device hardware constraint.

After that, when I unplug the headset, which behavior is correct?

Behavior A : Keep 10, following the headset volume.
Behavior B : Back to 5, the original setting.

[2] When user plug the headset, the volume would be automatically decrease.
    http://androidxref.com/5.1.1_r6/xref/frameworks/av/services/audiopolicy/AudioPolicyManager.cpp#5658

Comment 49

[Sotaro Ikeda [:sotaro]]

alwu, thanks for the detailed explanation. I am going to into PTO, but I am going to intermittently check bugzilia. After PTO, I move to Japan. Therefore, responding might become easier.

The volume control UI is very simple, but it has to control multiple devices and multiple use cases. Therefore any choice have use cases that users cannot control volume well as they want. Therefore, just showing a use case that does not fulfill some users could not be a reason to push an idea, I think.

The reason that I am not convinced about the profile is that I worked several TV and mobile media frameworks in the past. But I did not saw the system like that. And I could not find the enough reasons to push the ideas. And it seems not scale well to multiple device use cases and different AudioPolicyManager's choices.

Android like audio control could fulfill's normal user's use case(phone call and media playback) and it is already used widely in the world.

Comment 50

[Sotaro Ikeda [:sotaro]]

> And it seems not scale well to multiple device use cases and different
> AudioPolicyManager's choices.

For me, profile idea seems fit well if phone uses one audio out device at a time and it does not fit well if phone uses multiple audio out devices at a same time. How to handle the multiple out use case is a key to the discussion, because Android like audio control model and profile model looks same from use's point of view. It seems better to focus the discussion to this use cases.

Comment 51

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu][OOO from 10/3-10/11] from comment #47)
> 
> In my opinion, when we're using the common headset or BT headset, the mainly
> output device is headset instead of speaker.
> I don't think choosing the "speaker out" is a good option.
> Anyway, we can also ask UX feedback for that.

In my patch, it changes all active audio out devices' volume. In master gecko implementation, it change only headset.

Comment 52

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro PTO 5/Oct - 14/Oct] from comment #51)
> (In reply to Alastor Wu [:alwu][OOO from 10/3-10/11] from comment #47)
> > 
> > In my opinion, when we're using the common headset or BT headset, the mainly
> > output device is headset instead of speaker.
> > I don't think choosing the "speaker out" is a good option.
> > Anyway, we can also ask UX feedback for that.

My opinion seems different from you. Ringing and notification are not sounds that I need to listen long time and I do not care about the sound volume if they are comfortable on headphone. Is OK if it I could recognize it. Instead, I care about the volume of them from speaker, because it is heard in public.

And how to control volume on TV is also different. For me, speaker is always highest priority than headphone. And on android TV, all type of stream's volumes are set to same volume to AUDIO_STREAM_MUSIC.

http://androidxref.com/5.1.1_r6/xref/frameworks/base/media/java/android/media/AudioService.java#300

Comment 53

[Sotaro Ikeda [:sotaro]]

By the way, if hdmi_audio device is connected, volume of the device should be max volume. It is Bug 1204033.

Comment 54

[Sotaro Ikeda [:sotaro]]

> In this case, I use the ringtone as the example, and something you should
> know is that the ringtone would be playback from both speaker and headset
> when the headset is plugged (Android design).
> 
> In the beginning, I use the phone without any device, and I thinks the
> volume 5 is most comfortable volume for me.
> 
> Then I use headset, and the volume 10 is most comfortable for me, because of
> Android's mechanism [2] or device hardware constraint.
> 
> After that, when I unplug the headset, which behavior is correct?
> 
> Behavior A : Keep 10, following the headset volume.
> Behavior B : Back to 5, the original setting.
> 
> [2] When user plug the headset, the volume would be automatically decrease.
>    
> http://androidxref.com/5.1.1_r6/xref/frameworks/av/services/audiopolicy/
> AudioPolicyManager.cpp#5658

audio volume decrease by [2] is by AudioPolicyManager's design. [2] seems to say that for AudioPolicyManager, perception and ear guard are more important than comfort for AUDIO_STREAM_NOTIFICATION, AUDIO_STREAM_RING, AUDIO_STREAM_ALARM and AUDIO_STREAM_SYSTEM. And it says that the above streams volumes criteria are different from AUDIO_STREAM_VOICE_CALL, AUDIO_STREAM_BLUETOOTH_SCO and AUDIO_STREAM_MUSIC. They are main audio contents for users.

[2] does not decrease volumes of AUDIO_STREAM_VOICE_CALL, AUDIO_STREAM_BLUETOOTH_SCO and AUDIO_STREAM_MUSIC when headphone is plugged. The streams do not have the problem of 'speaker + headset' volume control.

Comment 55

[Alastor Wu [:alwu]]

Hi, Sotaro,
Very thanks for your detailed explanation.
As you said, maybe the profile concept doesn't fit in the case with multiple audio outputs.
Let's continue about your implementation.
In addition, if you have time, could you also add the tests for that?
Very appreciate!

Comment 56

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu] from comment #55)
> Hi, Sotaro,
> Very thanks for your detailed explanation.
> As you said, maybe the profile concept doesn't fit in the case with multiple
> audio outputs.
> Let's continue about your implementation.
> In addition, if you have time, could you also add the tests for that?
> Very appreciate!

Thanks. Which kind of tests are you thinking? Is there already similar tests? I am going to create a test in a different bug.

Comment 57

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Rebased

Comment 58

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Fix build failure on L.

Comment 59

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Apply comments and some code clean up.

Comment 60

[Sotaro Ikeda [:sotaro]]

I misunderstood about how android controls audio volume when a stream has multiple audio out devices.

AudioService chooses one device by getDeviceForStream(int stream) and AudioService seems to control only the device's volume. But it was wrong.
 http://androidxref.com/5.1.1_r6/xref/frameworks/base/media/java/android/media/AudioService.java#3404

AudioPolicyManager::setStreamVolumeIndex() controls volumes of "a device in argument + all active devices for the stream". Therefore, android actually controls volumes of all active devices for a stream.
 http://androidxref.com/5.1.1_r6/xref/frameworks/av/services/audiopolicy/AudioPolicyManager.cpp#1809

attachment 8677222 [details] [diff] [review] already fixed it.

Comment 61

[Alastor Wu [:alwu]]

Hi, Satoro,
Since this patch changes a lots of code, could you help me to use the reviewboard for easily reviewing?

In addtition, I haven't written the test for AudioManager before. 
Is possible to write a test which can identify the volume consistency between switching different audio outputs, and also check whether the volume of the multiple audio output is changed in the same time?

Thanks!

Comment 62

[Tori Chen [:tori]]

forward the ni info to Mark(UX)

Comment 63

[Sotaro Ikeda [:sotaro]]

Attachment: MozReview Request: Bug 1196724 - Refactoring of AudioManager

Bug 1196724 - Refactoring of AudioManager

Comment 64

[Alastor Wu [:alwu]]

https://reviewboard.mozilla.org/r/23077/#review20703

Hi, Sotaro,
The patch is good! 
But here I still have some questions and little nits, 
Thanks :)

::: dom/system/gonk/AudioManager.cpp:101
(Diff revision 1)
> +static const int32_t sStreamVolumeAliasTbl[AUDIO_STREAM_CNT] = {

Do we still need this table?
This variable is used in the functions Set/Get/GetMaxAudioChannelVolume.
But these functions don't be used by anyone.

::: dom/system/gonk/AudioManager.cpp:261
(Diff revision 1)
> +  //Enable volume change notification

Add one space.

::: dom/system/gonk/AudioManager.cpp:993
(Diff revision 1)
> -    case AudioChannel::Content:
> +  switch (aProfile) {

Because we don't use the profile concept in this patch, I think we should use the new format to storage the volume data to the DB. (Also remove profile related codes)

Original format is designed for "profile", so I can init the streams of each different profile. 

If we still use this format, it means we only store three device data into the DB, but in your design, every stream has lots of device data. 

Ex. HDMI's volume data doesn't be save in DB. (do we care about that?)

::: dom/system/gonk/AudioManager.cpp:1040
(Diff revision 1)
> +  if (aNotify) {

What's the use case not to update the volume data to the database?
Because I don't see any place to call this method with aNotify=false.

::: dom/system/gonk/AudioManager.cpp:1176
(Diff revision 1)
> +

Follow the above question, since we're not using the profile concept, do we still need this kind of format to store the volume data? 

This kind format can't store all device volume info to the DB.

::: dom/system/gonk/AudioManager.cpp:1265
(Diff revision 1)
> +  audio_devices_t device = AudioManager::SelectDeviceFromDevices(devices);

Just SelectDeviceFromDevices, no need to add the prefix (AudioManager::).

::: dom/system/gonk/AudioManager.cpp:1280
(Diff revision 1)
> +    } else if ((device & AUDIO_DEVICE_OUT_HDMI_ARC) != 0) {

Have we already support these outputs in Gonk?

::: dom/system/gonk/AudioManager.cpp:1288
(Diff revision 1)
> +       device &= AUDIO_DEVICE_OUT_ALL_A2DP;

Why we want to choose A2DP here?

::: dom/system/gonk/AudioManager.cpp:1293
(Diff revision 1)
> +AudioManager::VolumeStreamState::VolumeStreamState(AudioManager& aManager,

Add an empty line.

Comment 65

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu] from comment #64)
> https://reviewboard.mozilla.org/r/23077/#review20703
> 
> Hi, Sotaro,
> The patch is good! 
> But here I still have some questions and little nits, 
> Thanks :)
> 
> ::: dom/system/gonk/AudioManager.cpp:101
> (Diff revision 1)
> > +static const int32_t sStreamVolumeAliasTbl[AUDIO_STREAM_CNT] = {
> 
> Do we still need this table?
> This variable is used in the functions Set/Get/GetMaxAudioChannelVolume.
> But these functions don't be used by anyone.

Yes, it is necessary. You can find 'sStreamVolumeAliasTbl' usages in several places in the patch.
We could remove GetMaxAudioChannelVolume(). Instead AudioManager::VolumeStreamState::GetMaxIndex() exists.

Just GetMaxAudioChannelVolume() is not used in the patch. And there is no SetMaxAudioChannelVolume.
Can you clarify it more?

> ::: dom/system/gonk/AudioManager.cpp:261
> (Diff revision 1)
> > +  //Enable volume change notification
> 
> Add one space.

Update in a next patch.

> ::: dom/system/gonk/AudioManager.cpp:993
> (Diff revision 1)
> > -    case AudioChannel::Content:
> > +  switch (aProfile) {
> 
> Because we don't use the profile concept in this patch, I think we should
> use the new format to storage the volume data to the DB. (Also remove
> profile related codes)

This patch does not update interaction between Setting API. It is intentional.
If we add it, the change becomes too big. I do not want to make change bigger than current patch.
Even without change, devices keeps same amount of volumes.

Created Bug 1218629 to fix it.

> 
> Original format is designed for "profile", so I can init the streams of each
> different profile. 
> 
> If we still use this format, it means we only store three device data into
> the DB, but in your design, every stream has lots of device data. 
> 
> Ex. HDMI's volume data doesn't be save in DB. (do we care about that?)

As the above, this patch does not update how to store volume in db. Bug 1218629 is for it.

> 
> ::: dom/system/gonk/AudioManager.cpp:1040
> (Diff revision 1)
> > +  if (aNotify) {
> 
> What's the use case not to update the volume data to the database?
> Because I don't see any place to call this method with aNotify=false.

Sorry, it is not used anymore. I am going to remove it in a next patch.

> 
> ::: dom/system/gonk/AudioManager.cpp:1176
> (Diff revision 1)
> > +
> 
> Follow the above question, since we're not using the profile concept, do we
> still need this kind of format to store the volume data? 
> 
> This kind format can't store all device volume info to the DB.

As the above, I put off to update setting db interaction to another bug.
 Bug 1218629 is going to handle it.

> 
> ::: dom/system/gonk/AudioManager.cpp:1265
> (Diff revision 1)
> > +  audio_devices_t device = AudioManager::SelectDeviceFromDevices(devices);
> 
> Just SelectDeviceFromDevices, no need to add the prefix (AudioManager::).

Remove it in a next patch.

> 
> ::: dom/system/gonk/AudioManager.cpp:1280
> (Diff revision 1)
> > +    } else if ((device & AUDIO_DEVICE_OUT_HDMI_ARC) != 0) {
> 
> Have we already support these outputs in Gonk?

It is not yet supported on gonk. But I assume that we are going to add support soon to support Firefox OS TV.

Its implementation just reflect AudioService.getDeviceForStream(). It is necessary for AudioPolicyManager works correctly.

The following function needs to be synchronized.
- AudioService.getDeviceForStream()
  http://androidxref.com/5.1.1_r6/xref/frameworks/base/media/java/android/media/AudioService.java#getDeviceForStream
- AudioPolicyManager::getDeviceForVolume()
  http://androidxref.com/5.1.1_r6/xref/frameworks/av/services/audiopolicy/AudioPolicyManager.cpp#getDeviceForVolume

It might better to update the comment in a next patch.

> 
> ::: dom/system/gonk/AudioManager.cpp:1288
> (Diff revision 1)
> > +       device &= AUDIO_DEVICE_OUT_ALL_A2DP;
> 
> Why we want to choose A2DP here?

As the above, AudioPolicyManager expects it.

> 
> ::: dom/system/gonk/AudioManager.cpp:1293
> (Diff revision 1)
> > +AudioManager::VolumeStreamState::VolumeStreamState(AudioManager& aManager,
> 
> Add an empty line.

Update in a next patch.

Comment 66

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #65)
> (In reply to Alastor Wu [:alwu] from comment #64)
> > https://reviewboard.mozilla.org/r/23077/#review20703
> > 
> > Hi, Sotaro,
> > The patch is good! 
> > But here I still have some questions and little nits, 
> > Thanks :)
> > 
> > ::: dom/system/gonk/AudioManager.cpp:101
> > (Diff revision 1)
> > > +static const int32_t sStreamVolumeAliasTbl[AUDIO_STREAM_CNT] = {
> > 
> > Do we still need this table?
> > This variable is used in the functions Set/Get/GetMaxAudioChannelVolume.
> > But these functions don't be used by anyone.
> 
> Yes, it is necessary. You can find 'sStreamVolumeAliasTbl' usages in several
> places in the patch.
> We could remove GetMaxAudioChannelVolume(). Instead
> AudioManager::VolumeStreamState::GetMaxIndex() exists.
> 

Sorry, we can not remove it, because it is defined in nsIAudioManager.idl

Comment 67

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8678009 [details]
MozReview Request: Bug 1196724 - Refactoring of AudioManager

Bug 1196724 - Refactoring of AudioManager

Comment 68

[Alastor Wu [:alwu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #65)
> > ::: dom/system/gonk/AudioManager.cpp:993 
> This patch does not update interaction between Setting API. It is
> intentional.
> If we add it, the change becomes too big. I do not want to make change
> bigger than current patch.
> Even without change, devices keeps same amount of volumes.
> 
> Created Bug 1218629 to fix it.

OK, Thanks :)

> > ::: dom/system/gonk/AudioManager.cpp:1288
> As the above, AudioPolicyManager expects it.

I have a question,
First, we get device(s) by GetDevicesForStream(), so here the devices are already the active device.
After that, we call SelectDeviceFromDevices() to choose ONE device, and use this device to get/set volume.

But if the devices we get in first step are already active device, why I still need to choose one from them?
The active devices should all have the same volume, right?

Comment 69

[Alastor Wu [:alwu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #66) 
> Sorry, we can not remove it, because it is defined in nsIAudioManager.idl

After searching in Gaia repo, no one uses these function.
Maybe we can discuss whether discard these functions in another bug.

Comment 70

[Alastor Wu [:alwu]]

Sorry, correct comment69.
I searched both Gecko and Gaia, no one uses these functions. (Set/Get/GetMaxAudioChannelVolume)

Comment 71

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu] from comment #70)
> Sorry, correct comment69.
> I searched both Gecko and Gaia, no one uses these functions.
> (Set/Get/GetMaxAudioChannelVolume)

I have one question why you write as '(Set/Get/GetMaxAudioChannelVolume)'. GetMaxAudioChannelVolume is the only one function that has "MaxAudioChannelVolume" in function name.

And I do not think there is necessity to remove it in this bug.

Comment 72

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu] from comment #68)
> 
> I have a question,
> First, we get device(s) by GetDevicesForStream(), so here the devices are
> already the active device.
> After that, we call SelectDeviceFromDevices() to choose ONE device, and use
> this device to get/set volume.
> 
> But if the devices we get in first step are already active device, why I
> still need to choose one from them?
> The active devices should all have the same volume, right?

As comment 60, it is actually controlled by AudioPolicyManager::setStreamVolumeIndex(). But the function still needs device as argument. The device specific volume is used in AudioPolicyManager::StreamDescriptor::getVolumeIndex().

Further, old platform specific AudioPolicyManager use the device to control as all devices having same volume.
 http://androidxref.com/4.4.4_r1/xref/hardware/libhardware_legacy/audio/AudioPolicyManagerBase.cpp#1091

Comment 73

[Alastor Wu [:alwu]]

Oh, I mean SetAudioChannelVolume/GetAudioChannelVolume/GetMaxAudioChannelVolume.
But, yes, this can be discussed in another bug.

Comment 74

[Alastor Wu [:alwu]]

https://reviewboard.mozilla.org/r/23077/#review20859

LGTM, Thanks!

Comment 75

[Alastor Wu [:alwu]]

Comment on attachment 8678009 [details]
MozReview Request: Bug 1196724 - Refactoring of AudioManager

https://reviewboard.mozilla.org/r/23079/#review20861

Comment 76

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu] from comment #73)
> Oh, I mean
> SetAudioChannelVolume/GetAudioChannelVolume/GetMaxAudioChannelVolume.
> But, yes, this can be discussed in another bug.

Yea, after test bug is addressed, we can discuss about it.

Comment 77

[Sotaro Ikeda [:sotaro]]

[Blocking Requested - why for this release]: This bug is dup of Bug 1195227. Bug 1195227 is 2.5+.

Comment 78

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b467ef64d604

Comment 79

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8678009 [details]
MozReview Request: Bug 1196724 - Refactoring of AudioManager

Bug 1196724 - Refactoring of AudioManager

Comment 80

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c84c8bf4300

Comment 81

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Refactoring of AudioManager

Comment 82

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29c9e01ef7a2

Comment 83

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/29c9e01ef7a2