1218629 - Save audio volume for each device to setting db

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #1196724 +++

Bug 1196724 does not update how to hold volume. This bug should update it as to hold audio volume for each device to setting db.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Save audio volume of each device to setting db

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Save audio volume of each device to setting db

Fix build failure on gonk KK.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Save audio volume of each device to setting db

Fix build failure on ICS.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Save audio volume of each device to setting db

Fixed db save problems.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Save audio volume of each device to setting db

Fix nits.

Comment 6

[Sotaro Ikeda [:sotaro]]

attachment 8694031 [details] [diff] [review] seems to work well on nexus-5-l and aries. But it caused the problem on ics hamachi. AudioPolicyManager's different seems to affect to it.

Comment 7

[Sotaro Ikeda [:sotaro]]

On hamachi, AudioSystem::getDevicesForStream() is not provided yet. Instead AudioManager uses AUDIO_DEVICE_OUT_DEFAULT.

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> On hamachi, AudioSystem::getDevicesForStream() is not provided yet. Instead
> AudioManager uses AUDIO_DEVICE_OUT_DEFAULT.

And there is not way to specify a device to out put in AudioSystem::setStreamVolumeIndex().

In ics, it seems reasonable to handle as if it supports only one audio device out.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Save audio volume of each device to setting db

fix ics gonk problem.

Comment 10

[Alastor Wu [:alwu]]

Comment on attachment 8694077 [details] [diff] [review]
patch - Save audio volume of each device to setting db

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

LGTM, but here are still some questions.
Thanks!

::: dom/system/gonk/AudioManager.cpp
@@ +125,5 @@
>    AUDIO_STREAM_SYSTEM,          // AudioChannel::System
>  };
>  
> +
> +struct AudioDeviceTable {

I think "AudioDeviceInfo" would be better.

@@ +138,5 @@
> +  { "earpiece",      AUDIO_DEVICE_OUT_EARPIECE },
> +  { "speaker",       AUDIO_DEVICE_OUT_SPEAKER },
> +  { "wired_headset", AUDIO_DEVICE_OUT_WIRED_HEADSET },
> +  { "wired_headphone", AUDIO_DEVICE_OUT_WIRED_HEADPHONE },
> +  { "bt_scoheadser", AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET },

s/bt_scoheadser/bt_scoheadset

@@ +1007,5 @@
>    }
>  
>    int32_t streamAlias = sStreamVolumeAliasTbl[aStream];
> +  VolumeStreamState* streamState = mStreamStates[streamAlias].get();
> +  return streamState->SetVolumeIndexToAliasStreams(aIndex, aDevice);

We already found the alias stream in InitVolumeForDevice(), why we need to set alias streams again?

I think we can call SetVolumeIndex() here, instead of InitVolumeForDevice()?

@@ +1116,2 @@
>  {
> +  // Default volume of AUDIO_DEVICE_OUT_DEFAULT is already set.

Why remove initialization here?
Here is to set the default volume to all devices, not just for AUDIO_DEVICE_OUT_DEFAULT.

Here is case, 
In the first time we enable this patch, the new device info (audio.[volume_type].[device]) doesn't be stored in the DB yet. Therefore, we might get the init-error (value undefined), then we need to set the default volume for all devices.

@@ +1190,4 @@
>  }
>  
>  void
> +AudioManager::InitVolumeForDevice(int32_t aStreamType,

Change function name to "InitVolumeForAliasStreams".

@@ +1440,5 @@
>           aDevice);
>    return rv ? NS_ERROR_FAILURE : NS_OK;
>  #else
>    if (aUpdateCache) {
> +    mVolumeIndexes.Put(AUDIO_DEVICE_OUT_SPEAKER, aIndex);

why AUDIO_DEVICE_OUT_SPEAKER here?

@@ +1445,2 @@
>      mIsVolumeIndexesChanged = true;
> +    mManager.AudioOutDeviceUpdated(AUDIO_DEVICE_OUT_SPEAKER);

ditto.

::: dom/system/gonk/AudioManager.h
@@ +103,5 @@
>    int32_t mPhoneState;
>  
>    bool mIsVolumeInited;
>  
> +  // A bitwise variable for volume update of audio output devices

nit, 
// A bitwise variable for volume update of audio output devices, clear it after store the value into database.

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu] from comment #10)
> Comment on attachment 8694077 [details] [diff] [review]
> patch - Save audio volume of each device to setting db
> 
> Review of attachment 8694077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, but here are still some questions.
> Thanks!
> 
> ::: dom/system/gonk/AudioManager.cpp
> @@ +125,5 @@
> >    AUDIO_STREAM_SYSTEM,          // AudioChannel::System
> >  };
> >  
> > +
> > +struct AudioDeviceTable {
> 
> I think "AudioDeviceInfo" would be better.

I'll update it in a next patch.

> 
> @@ +138,5 @@
> > +  { "earpiece",      AUDIO_DEVICE_OUT_EARPIECE },
> > +  { "speaker",       AUDIO_DEVICE_OUT_SPEAKER },
> > +  { "wired_headset", AUDIO_DEVICE_OUT_WIRED_HEADSET },
> > +  { "wired_headphone", AUDIO_DEVICE_OUT_WIRED_HEADPHONE },
> > +  { "bt_scoheadser", AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET },
> 
> s/bt_scoheadser/bt_scoheadset

I'll update it in a next patch


> 
> @@ +1007,5 @@
> >    }
> >  
> >    int32_t streamAlias = sStreamVolumeAliasTbl[aStream];
> > +  VolumeStreamState* streamState = mStreamStates[streamAlias].get();
> > +  return streamState->SetVolumeIndexToAliasStreams(aIndex, aDevice);
> 
> We already found the alias stream in InitVolumeForDevice(), why we need to
> set alias streams again?

InitVolumeForDevice() became unnecessary. I'll remove it in a next patch.

> 
> I think we can call SetVolumeIndex() here, instead of InitVolumeForDevice()?

InitVolumeForDevice() became unnecessary. I'll remove it in a next patch.

 
> @@ +1116,2 @@
> >  {
> > +  // Default volume of AUDIO_DEVICE_OUT_DEFAULT is already set.
> 
> Why remove initialization here?
> Here is to set the default volume to all devices, not just for
> AUDIO_DEVICE_OUT_DEFAULT.

Initialization only to AUDIO_DEVICE_OUT_DEFAULT is enough for default volume setting.

> Here is case, 
> In the first time we enable this patch, the new device info
> (audio.[volume_type].[device]) doesn't be stored in the DB yet. Therefore,
> we might get the init-error (value undefined), then we need to set the
> default volume for all devices.

We do not need to set default volume for all devices since each device does not have different volume setting by default.

> 
> @@ +1190,4 @@
> >  }
> >  
> >  void
> > +AudioManager::InitVolumeForDevice(int32_t aStreamType,
> 
> Change function name to "InitVolumeForAliasStreams".

InitVolumeForDevice is going to be removed in a next patch.

> 
> @@ +1440,5 @@
> >           aDevice);
> >    return rv ? NS_ERROR_FAILURE : NS_OK;
> >  #else
> >    if (aUpdateCache) {
> > +    mVolumeIndexes.Put(AUDIO_DEVICE_OUT_SPEAKER, aIndex);
> 
> why AUDIO_DEVICE_OUT_SPEAKER here?

ICS does not support per device volume setting and AudioManager does not store AUDIO_DEVICE_OUT_DEFAULT. Just use AUDIO_DEVICE_OUT_SPEAKER for storing volume to DB. I am going to add a comment in a next patch.

> @@ +1445,2 @@
> >      mIsVolumeIndexesChanged = true;
> > +    mManager.AudioOutDeviceUpdated(AUDIO_DEVICE_OUT_SPEAKER);
> 
> ditto.

ditto.

> 
> ::: dom/system/gonk/AudioManager.h
> @@ +103,5 @@
> >    int32_t mPhoneState;
> >  
> >    bool mIsVolumeInited;
> >  
> > +  // A bitwise variable for volume update of audio output devices
> 
> nit, 
> // A bitwise variable for volume update of audio output devices, clear it
> after store the value into database.

I will update the comment in a next patch.

Comment 12

[Sotaro Ikeda [:sotaro]]

> > Here is case, 
> > In the first time we enable this patch, the new device info
> > (audio.[volume_type].[device]) doesn't be stored in the DB yet. Therefore,
> > we might get the init-error (value undefined), then we need to set the
> > default volume for all devices.
> 
> We do not need to set default volume for all devices since each device does
> not have different volume setting by default.

Per device volume is necessary only when the volume is different from the default volume.

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu] from comment #10)
> 
> Here is case, 
> In the first time we enable this patch, the new device info
> (audio.[volume_type].[device]) doesn't be stored in the DB yet. Therefore,
> we might get the init-error (value undefined), then we need to set the
> default volume for all devices.

I did not see error. It just failed "if (aResult.isInt32())".

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Save audio volume of each device to setting db

Apply the comment.

Comment 15

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #13)
> (In reply to Alastor Wu [:alwu] from comment #10)
> > 
> > Here is case, 
> > In the first time we enable this patch, the new device info
> > (audio.[volume_type].[device]) doesn't be stored in the DB yet. Therefore,
> > we might get the init-error (value undefined), then we need to set the
> > default volume for all devices.
> 
> I did not see error. It just failed "if (aResult.isInt32())".

In settings api implementation, undefined seems also a valid value.
  https://dxr.mozilla.org/mozilla-central/source/dom/settings/SettingsDB.jsm#203

Comment 16

[Alastor Wu [:alwu]]

Comment on attachment 8700556 [details] [diff] [review]
patch - Save audio volume of each device to setting db

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

LGTM, little nits and one question.
Thanks!

::: dom/system/gonk/AudioManager.cpp
@@ +140,5 @@
> +  { "speaker",       AUDIO_DEVICE_OUT_SPEAKER },
> +  { "wired_headset", AUDIO_DEVICE_OUT_WIRED_HEADSET },
> +  { "wired_headphone", AUDIO_DEVICE_OUT_WIRED_HEADPHONE },
> +  { "bt_scoheadset", AUDIO_DEVICE_OUT_BLUETOOTH_SCO_HEADSET },
> +  { "bt_a2dp",       AUDIO_DEVICE_OUT_BLUETOOTH_A2DP },

nit : align all AUDIO_DEVICE_OUT_XXX.

@@ +1390,5 @@
>    bool exist = mVolumeIndexes.Get(aDevice, &oldVolumeIndex);
>    if (exist && aIndex == oldVolumeIndex) {
>      // No update
>      return NS_OK;
>    }

nit : add one space line.

@@ +1401,5 @@
>      if ((streamType != mStreamType) &&
>           sStreamVolumeAliasTbl[streamType] == mStreamType) {
>        // Rescaling of index is not necessary.
>        rv = mManager.mStreamStates[streamType]->
>          SetVolumeIndexToAliasStreams(aIndex, aDevice);

One question, although it's not include in this changeset.

When we achieve this line, means that we already find the alias stream, so why we don't just call "SetVolumeIndex()"?

---

One more idea, if above suggestion is workable, maybe we can move line#1389~1394 into SetVolumeIndex().
IMMO, these codes are more appropriate within that function.

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Alastor Wu [:alwu] from comment #16)
> 
> @@ +1401,5 @@
> >      if ((streamType != mStreamType) &&
> >           sStreamVolumeAliasTbl[streamType] == mStreamType) {
> >        // Rescaling of index is not necessary.
> >        rv = mManager.mStreamStates[streamType]->
> >          SetVolumeIndexToAliasStreams(aIndex, aDevice);
> 
> One question, although it's not include in this changeset.
> 
> When we achieve this line, means that we already find the alias stream, so
> why we don't just call "SetVolumeIndex()"?

Yes, it could be reasonable clean up. The function originally mimics android's VolumeStreamState.setIndex().
http://androidxref.com/6.0.0_r1/xref/frameworks/base/services/core/java/com/android/server/audio/AudioService.java#3809

But the current VolumeStreamState::SetVolumeIndexToAliasStreams()'s role is a bit different from VolumeStreamState.setIndex().

> 
> ---
> 
> One more idea, if above suggestion is workable, maybe we can move
> line#1389~1394 into SetVolumeIndex().
> IMMO, these codes are more appropriate within that function.

Yea, it is reasonable change. Since by the patch, VolumeStreamState::SetVolumeIndexToAliasDevices() is removed.

Comment 18

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro PTO 28/Dec - 4/Jan] from comment #17)
> > 
> > ---
> > 
> > One more idea, if above suggestion is workable, maybe we can move
> > line#1389~1394 into SetVolumeIndex().
> > IMMO, these codes are more appropriate within that function.
> 
> Yea, it is reasonable change. Since by the patch,
> VolumeStreamState::SetVolumeIndexToAliasDevices() is removed.

In current implementation, it is reasonable. But Bug 1197800 prefers current way of implementation.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Save audio volume of each device to setting db

Apply the comments. Carry "r=alwu".

Comment 20

[Sotaro Ikeda [:sotaro]]

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

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0eed3c26935a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0eed3c26935a