[B2G] Store separate volume setting into setting database

RESOLVED FIXED in Firefox 43

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: alwu, Assigned: alwu)

Tracking

unspecified
FxOS-S5 (21Aug)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Updated

3 years ago
Duplicate of this bug: 1179173
(Assignee)

Updated

3 years ago
Blocks: 1170117
(Assignee)

Comment 2

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

Comment 3

3 years ago
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,
No longer blocks: 1170117
Depends on: 1170117

Updated

3 years ago
Blocks: 1184876
(Assignee)

Updated

3 years ago
Assignee: nobody → alwu
(Assignee)

Comment 4

3 years ago
Created attachment 8647347 [details] [diff] [review]
Add device information in volume changing event

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

Comment 5

3 years ago
Created attachment 8647983 [details] [diff] [review]
[WIP] Add device information in volume changing event

WIP, adding the promise mechanism to handle async volume initial event.
Still need the modification.
Attachment #8647347 - Attachment is obsolete: true
(Assignee)

Comment 6

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

Comment 8

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

Comment 9

3 years ago
New try-server result, it should fix test case fails.
Summary: [B2G] Add devices information in audio volume event → [B2G] Store separate volume setting into setting database
(Assignee)

Comment 11

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

Comment 12

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

Comment 13

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

Updated

3 years ago
Duplicate of this bug: 1184876

Updated

3 years ago
No longer blocks: 1184876
(Assignee)

Comment 15

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

Comment 17

3 years ago
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
Attachment #8648146 - Flags: review+
(Assignee)

Comment 18

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

Updated

3 years ago
Attachment #8648146 - Attachment description: MozReview Request: Bug 1179181 - store separate volume setting into setting database → MozReview Request: Bug 1179181 - store separate volume setting into setting database. r=baku.
Attachment #8648146 - Flags: review+ → review?(amarchesini)
(Assignee)

Comment 19

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

Comment 20

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

Add reviewer info, r=baku.
Attachment #8648146 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
FYI, we can land patches from MozReview, so no need to attach them as a separate attachment.
https://hg.mozilla.org/mozilla-central/rev/9bff295ea403
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S5 (21Aug)
Depends on: 1196358
(Assignee)

Comment 24

3 years ago
Created attachment 8650258 [details] [diff] [review]
[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.
Attachment #8647983 - Attachment is obsolete: true
Attachment #8650258 - Flags: approval‑mozilla‑b2g37_v2_2r?
(Assignee)

Updated

3 years ago
Blocks: 1196358
No longer depends on: 1196358

Updated

3 years ago
Depends on: 1199707
(Assignee)

Updated

3 years ago
Attachment #8650258 - Flags: approval‑mozilla‑b2g37_v2_2r?
You need to log in before you can comment on or make changes to this bug.