Closed
Bug 1179181
Opened 8 years ago
Closed 8 years ago
[B2G] Store separate volume setting into setting database
Categories
(Firefox OS Graveyard :: AudioChannel, defect)
Tracking
(firefox43 fixed)
RESOLVED
FIXED
FxOS-S5 (21Aug)
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: alwu, Assigned: alwu)
References
Details
Attachments
(2 files, 2 obsolete files)
40 bytes,
text/x-review-board-request
|
alwu
:
review+
|
Details |
16.79 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Comment 2•8 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•8 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,
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → alwu
Assignee | ||
Comment 4•8 years ago
|
||
First patch, but still need to discuss with Gaia about the event detail.
Assignee | ||
Comment 5•8 years ago
|
||
WIP, adding the promise mechanism to handle async volume initial event. Still need the modification.
Attachment #8647347 -
Attachment is obsolete: true
Assignee | ||
Comment 6•8 years ago
|
||
Bug 1179181 - store separate volume setting into setting database
Assignee | ||
Comment 7•8 years ago
|
||
Try-server result. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a1a81490dd4
Assignee | ||
Comment 8•8 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•8 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 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=76b3b0c2c007
Assignee | ||
Comment 11•8 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•8 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•8 years ago
|
||
New try, hoping it can fix the test case fails. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0650e8c4d596
Assignee | ||
Comment 15•8 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)
Updated•8 years ago
|
Attachment #8648146 -
Flags: review?(amarchesini) → review+
Comment 16•8 years ago
|
||
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•8 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•8 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•8 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•8 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•8 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•8 years ago
|
Keywords: checkin-needed
Comment 21•8 years ago
|
||
FYI, we can land patches from MozReview, so no need to attach them as a separate attachment.
Comment 22•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bff295ea403
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9bff295ea403
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S5 (21Aug)
Assignee | ||
Comment 24•8 years ago
|
||
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•8 years ago
|
Assignee | ||
Updated•8 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.
Description
•