1179181 - [B2G] Store separate volume setting into setting database

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Alastor Wu [:alwu]]

In Bug1170117, we want to implement the separate volume control settings for different output devices. 

However, we can't get the consistent volume when we change the device after shutdown the phone. That is because we only cache the volume value by types, we don't cache value by devices.

When we start/reboot the phone, we would get the volume value from the cache, and then send the volume event to Gecko in order to set hardware volume. If we don't cache value by devices, the following thing would happen.

STR.
1. Use primary output(speaker/receiver), and set the volume value to 5.
2. Use headphone, and set the volume value to 10. (store volume value 10 to cache)
3. Shutdown the phone 
4. Unplug the headphone
5. Restart the phone 

Expected.
6. Volume value is 5 (because we use the primary output now)

Actual.
6. Volume value is 10 (get value from cache, and get 10)

---

We need to add the device information in volume event and store it into cache, so that we can get the correct volume value from the cache.

The present audio volume event is like that,
"audio.volume.[channel_type_name]"

I think we can add the devices information below the original event, 
"audio.volume.[channel_type_name].[device_name]"

I propose that the devices name should involve "primary", "headset" and "bluetooth".

Example.
audio.volume.alarm.primary
audio.volume.alarm.headphone
audio.volume.alarm.bluetooth

Comment 2

[Alastor Wu [:alwu]]

This bug should include both Gecko and Gaia part.

In Gecko, when we change the devices, we need to send the new event to notify Gaia so that it can update the UI.

In Gaia, sound_manager.js, it need to handle the these events to update UI and sends the new events when user press the hardware button. The Gaia might/should have a variable to save the present device.

Comment 3

[Alastor Wu [:alwu]]

Because this issue will only happen on switching devices after shutdown the phone, this isn't very common scenario. 
I will land the Bug1170117 first, and then land this bug,

Comment 4

[Alastor Wu [:alwu]]

Attachment: Add device information in volume changing event

First patch, but still need to discuss with Gaia about the event detail.

Comment 5

[Alastor Wu [:alwu]]

Attachment: [WIP] Add device information in volume changing event

WIP, adding the promise mechanism to handle async volume initial event.
Still need the modification.

Comment 6

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1179181 - store separate volume setting into setting database. r=baku.

Bug 1179181 - store separate volume setting into setting database

Comment 7

[Alastor Wu [:alwu]]

Try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a1a81490dd4

Comment 8

[Alastor Wu [:alwu]]

Comment on attachment 8648146 [details]
MozReview Request: Bug 1179181 - store separate volume setting into setting database. r=baku.

Bug 1179181 - store separate volume setting into setting database

Comment 9

[Alastor Wu [:alwu]]

New try-server result, it should fix test case fails.

Comment 10

[Alastor Wu [:alwu]]

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

Comment 11

[Alastor Wu [:alwu]]

[Bug situation]
We can't set the correct volume for each profile after we reboot the phone, since we don't store separate volume setting into database. We only have the last profile we used in the database.

Ex,
We use the headset as the last profile, then we shutdown the phone and unplug the headset.
After we reboot the phone, the profile is "primary", but the volume stored in the database is "headset".

---

[Solution]
Therefore, we use the "audio.volume.[channel_type].[profile]" to store the separate volume setting. Everytime we start the phone, we would fetch the volume data from database, and set the correct volume setting for each profile.

In addition, in present design, the sound manager in Gaia would also fetch the volume data from database. That means the sound manager need to know the complete setting name, so that it can add the listener for the specific setting event. However, we don't want that Gaia changes its code whenever we change the total number of the profile. 

For reducing the code dependency, we use the "audio.volume.[channel_type]" to store the volume data of the present profile, and the Gaia only need to fetch this setting event. The "audio.volume.[channel_type].[profile]" is used internally to know the volume setting of different profile by the Gecko.

Simply, the Gaia only knows about the volume data of present profile, and the Gecko know all of that.

Comment 12

[Alastor Wu [:alwu]]

Comment on attachment 8648146 [details]
MozReview Request: Bug 1179181 - store separate volume setting into setting database. r=baku.

Bug 1179181 - store separate volume setting into setting database

Comment 13

[Alastor Wu [:alwu]]

New try, hoping it can fix the test case fails.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0650e8c4d596

Comment 15

[Alastor Wu [:alwu]]

Comment on attachment 8648146 [details]
MozReview Request: Bug 1179181 - store separate volume setting into setting database. r=baku.

Hi, Baku,
Could you help me review this patch?
The analysis is in the comment11.
Very appreciate :)

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 8648146 [details]
MozReview Request: Bug 1179181 - store separate volume setting into setting database. r=baku.

https://reviewboard.mozilla.org/r/16131/#review14497

Ship It!

::: dom/system/gonk/AudioManager.h:126
(Diff revision 3)
> -  void InitProfilesVolume(uint32_t aCatogory, uint32_t aIndex);
> +  void InitProfilesVolume(AudioOutputProfiles aProfile,

InitProfileVolume

::: dom/system/gonk/AudioManager.cpp:229
(Diff revision 3)
> -      if (aName.EqualsASCII(gVolumeData[idx].mChannelName)) {
> +      nsString volumeType;

nsAutoString

::: dom/system/gonk/AudioManager.cpp:230
(Diff revision 3)
> +      volumeType.Assign(NS_ConvertASCIItoUTF16(gVolumeData[idx].mChannelName));

NS_ConvertASCIItoUTF16 volumeType(gVolumeData[idx].mChannelName);

::: dom/system/gonk/AudioManager.cpp:268
(Diff revision 3)
> +      profile.Assign(NS_ConvertASCIItoUTF16(kAudioOutputProfilesTable[idx].tag));

same here:

NS_ConvertASCIItoUTF16 profile(kAudioOutputProfilesTable[idx].tag);

::: dom/system/gonk/AudioManager.cpp:269
(Diff revision 3)
> +      if (StringEndsWith(aName, profile)){

)<space>{

::: dom/system/gonk/AudioManager.cpp:1175
(Diff revision 3)
> -AudioManager::InitProfilesVolume(uint32_t aCategory, uint32_t aIndex)
> +AudioManager::InitProfilesVolume(AudioOutputProfiles aProfile,

InitProfileVolume

Comment 17

[Alastor Wu [:alwu]]

Comment on attachment 8648146 [details]
MozReview Request: Bug 1179181 - store separate volume setting into setting database. r=baku.

Bug 1179181 - store separate volume setting into setting database

Comment 18

[Alastor Wu [:alwu]]

Comment on attachment 8648146 [details]
MozReview Request: Bug 1179181 - store separate volume setting into setting database. r=baku.

Try-server result is in the comment13.

Comment 19

[Alastor Wu [:alwu]]

Comment on attachment 8648146 [details]
MozReview Request: Bug 1179181 - store separate volume setting into setting database. r=baku.

Bug 1179181 - store separate volume setting into setting database. r=baku.

Comment 20

[Alastor Wu [:alwu]]

Comment on attachment 8648146 [details]
MozReview Request: Bug 1179181 - store separate volume setting into setting database. r=baku.

Add reviewer info, r=baku.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

FYI, we can land patches from MozReview, so no need to attach them as a separate attachment.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bff295ea403

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/9bff295ea403

Comment 24

[Alastor Wu [:alwu]]

Attachment: [v2.2r] store separate volume setting into setting database. r=baku.

Please uplift this after the bug1170117.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Solving the problem of the bug1170117.
User impact if declined: The volume setting would be wrong after the phone reboot.
Testing completed: Yes.
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.