Save audio volume for each device to setting db

RESOLVED FIXED

Status

Firefox OS
AudioChannel
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: permafail)

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

3 years ago
+++ 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.
(Assignee)

Updated

3 years ago
No longer blocks: 1194249, 1197800, 1204023, 1204031, 1204033, 1217260
No longer depends on: 1194442
(Assignee)

Updated

3 years ago
Summary: Hold audio volume for each device in setting db → Save audio volume for each device to setting db
Blocks: 1218775
(Assignee)

Updated

3 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Updated

3 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 1

3 years ago
Created attachment 8689975 [details] [diff] [review]
patch - Save audio volume of each device to setting db
(Assignee)

Comment 2

3 years ago
Created attachment 8693423 [details] [diff] [review]
patch - Save audio volume of each device to setting db

Fix build failure on gonk KK.
Attachment #8689975 - Attachment is obsolete: true
(Assignee)

Comment 3

3 years ago
Created attachment 8693425 [details] [diff] [review]
patch - Save audio volume of each device to setting db

Fix build failure on ICS.
Attachment #8693423 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 1229234
(Assignee)

Comment 4

3 years ago
Created attachment 8694030 [details] [diff] [review]
patch - Save audio volume of each device to setting db

Fixed db save problems.
Attachment #8693425 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Created attachment 8694031 [details] [diff] [review]
patch - Save audio volume of each device to setting db

Fix nits.
Attachment #8694030 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

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

Comment 8

3 years ago
(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.
(Assignee)

Comment 9

3 years ago
Created attachment 8694077 [details] [diff] [review]
patch - Save audio volume of each device to setting db

fix ics gonk problem.
Attachment #8694031 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8694077 - Flags: review?(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.
Attachment #8694077 - Flags: review?(alwu)
(Assignee)

Comment 11

3 years ago
(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.
(Assignee)

Comment 12

3 years ago
> > 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.
(Assignee)

Comment 13

3 years ago
(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())".
(Assignee)

Comment 14

3 years ago
Created attachment 8700556 [details] [diff] [review]
patch - Save audio volume of each device to setting db

Apply the comment.
Attachment #8694077 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8700556 - Flags: review?(alwu)
(Assignee)

Comment 15

3 years ago
(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 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.
Attachment #8700556 - Flags: review?(alwu) → review+
(Assignee)

Comment 17

2 years ago
(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.
(Assignee)

Comment 18

2 years ago
(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.
(Assignee)

Comment 19

2 years ago
Created attachment 8701669 [details] [diff] [review]
patch - Save audio volume of each device to setting db

Apply the comments. Carry "r=alwu".
Attachment #8700556 - Attachment is obsolete: true
Attachment #8701669 - Flags: review+

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0eed3c26935a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
See Also: → bug 1234694

Updated

2 years ago
Depends on: 1249437

Updated

2 years ago
Duplicate of this bug: 1179738
You need to log in before you can comment on or make changes to this bug.