Closed
Bug 906296
Opened 11 years ago
Closed 11 years ago
[B2G][Settings] Changing volume in Gaia settings has no impact on media_policy
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Tracking
(blocking-b2g:koi+, firefox26 affected, b2g18 unaffected, b2g18-v1.0.0 unaffected, b2g18-v1.0.1 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 fixed)
RESOLVED
FIXED
blocking-b2g | koi+ |
Tracking | Status | |
---|---|---|
firefox26 | --- | affected |
b2g18 | --- | unaffected |
b2g18-v1.0.0 | --- | unaffected |
b2g18-v1.0.1 | --- | unaffected |
b2g-v1.1hd | --- | unaffected |
b2g-v1.2 | --- | fixed |
People
(Reporter: gerard-majax, Assigned: gerard-majax)
References
Details
(Keywords: regression)
Attachments
(2 files)
2.54 KB,
patch
|
Details | Diff | Splinter Review | |
358 bytes,
text/html
|
vingtetun
:
review+
|
Details |
While testing bug 899487 on master, I noticed that changing the volumes sliders (Rings & Notifications or Alarms) has no impact on the media_policy applied.
This results in the real volume NOT being touched at all. Following are dumpsys of the media_policy values. The first dump is without touching anything, the second one is after reducing the "Ring & notifications" volume to 1/3. As you can see, no channel volume gets changed in the end.
I'm reproducing this on Inari and Peak, with master.
alex@portable-alex:~/codaz/hycs-hardware_xdandroid-ril$ adb shell dumpsys media.audio_policy
PolicyManager Interface: 0x158df20
Command Thread: 0x158dd40
Tones Thread: 0x158dbf0
AudioCommandThread 0x158dd40 Dump
- Commands:
Command Time Wait pParam
Last Command
02 000003.263 1 0x1590390
AudioCommandThread 0x158dbf0 Dump
- Commands:
Command Time Wait pParam
Last Command
-1 000000.000 0 0x0
AudioPolicyManager Dump: 0x158dfb0
Hardware Output: 1
A2DP Output: 0
Duplicated Output: 0
A2DP device address:
SCO device address:
Output devices: 00000003
Input devices: 00400000
Phone state: 0
Ringer mode: 0
Force use for communications 0
Force use for media 0
Force use for record 0
Force use for dock 0
Outputs dump:
- Output 1 dump:
Sampling rate: 44100
Format: 1
Channels: 00000003
Latency: 54
Flags 00000000
Devices 00000002
Stream volume refCount muteCount
00 1.000 00 00
01 1.000 00 00
02 1.000 00 00
03 1.000 00 00
04 1.000 00 00
05 1.000 00 00
06 -1.000 00 00
07 1.000 00 00
08 1.000 00 00
09 1.000 00 00
10 1.000 00 00
11 1.000 00 00
Inputs dump:
Streams dump:
Stream Index Min Index Max Index Cur Can be muted
00 00 05 01 1
01 00 15 01 1
02 00 15 01 1
03 00 15 01 1
04 00 15 01 1
05 00 15 01 1
06 00 15 00 1
07 00 15 15 0
08 00 15 01 1
09 00 15 01 1
10 00 15 01 1
11 00 01 01 1
Total Effects CPU: 0.000000 MIPS, Total Effects memory: 0 KB
Registered effects:
alex@portable-alex:~/codaz/hycs-hardware_xdandroid-ril$ adb shell dumpsys media.audio_policy
PolicyManager Interface: 0x158df20
Command Thread: 0x158dd40
Tones Thread: 0x158dbf0
AudioCommandThread 0x158dd40 Dump
- Commands:
Command Time Wait pParam
Last Command
02 000003.263 1 0x1590390
AudioCommandThread 0x158dbf0 Dump
- Commands:
Command Time Wait pParam
Last Command
-1 000000.000 0 0x0
AudioPolicyManager Dump: 0x158dfb0
Hardware Output: 1
A2DP Output: 0
Duplicated Output: 0
A2DP device address:
SCO device address:
Output devices: 00000003
Input devices: 00400000
Phone state: 0
Ringer mode: 0
Force use for communications 0
Force use for media 0
Force use for record 0
Force use for dock 0
Outputs dump:
- Output 1 dump:
Sampling rate: 44100
Format: 1
Channels: 00000003
Latency: 54
Flags 00000000
Devices 00000002
Stream volume refCount muteCount
00 1.000 00 00
01 1.000 00 00
02 1.000 00 00
03 1.000 00 00
04 1.000 00 00
05 1.000 00 00
06 -1.000 00 00
07 1.000 00 00
08 1.000 00 00
09 1.000 00 00
10 1.000 00 00
11 1.000 00 00
Inputs dump:
Streams dump:
Stream Index Min Index Max Index Cur Can be muted
00 00 05 01 1
01 00 15 01 1
02 00 15 01 1
03 00 15 01 1
04 00 15 01 1
05 00 15 01 1
06 00 15 00 1
07 00 15 15 0
08 00 15 01 1
09 00 15 01 1
10 00 15 01 1
11 00 01 01 1
Total Effects CPU: 0.000000 MIPS, Total Effects memory: 0 KB
Registered effects:
Assignee | ||
Updated•11 years ago
|
blocking-b2g: --- → koi?
Assignee | ||
Updated•11 years ago
|
status-b2g18:
--- → unaffected
status-b2g18-v1.0.0:
--- → unaffected
status-b2g18-v1.0.1:
--- → unaffected
status-b2g-v1.1hd:
--- → unaffected
status-firefox26:
--- → affected
Assignee | ||
Comment 1•11 years ago
|
||
After using gdb, I'm getting out of the observer code at http://mxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelService.cpp#560
I'd suspect this changeset: https://hg.mozilla.org/mozilla-central/rev/e80edaac3899
I'm currently rebuilding gecko just before this change, to check.
Assignee | ||
Comment 2•11 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #1)
> After using gdb, I'm getting out of the observer code at
> http://mxr.mozilla.org/mozilla-central/source/dom/audiochannel/
> AudioChannelService.cpp#560
>
> I'd suspect this changeset:
> https://hg.mozilla.org/mozilla-central/rev/e80edaac3899
>
> I'm currently rebuilding gecko just before this change, to check.
Turns out it was broken before. Using pvtbuilds for now, I can state that with gecko at 6d766118c832fb2879d7db3e46ae535ccdac7b4a it's okay, and with 8dde80d3f4b229a382c83efa554d174d197b3d72 it's broken. There's a timeframe of a week between both. I'm gonna continue to bisect this.
Assignee | ||
Comment 3•11 years ago
|
||
18/07 cb9a271064cc2dda511972f4720b33f2ff0188b3 ok
19/07 76ce8020e7fa56006c4a48b1916ff6760be0b909 broken
This narrows down to a subset of 268 commits :)
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → lissyx+mozillians
Assignee | ||
Comment 4•11 years ago
|
||
Okay, I just finished bisecting and identified a nice commit. I'll continue working on this tomorrow, but I'm putting you in needinfo? and CC in case you have any idea
5fb7ee16a27ebfa0374ce065736284b033139382 is the first bad commit
commit 5fb7ee16a27ebfa0374ce065736284b033139382
Author: Marco Chen <mchen@mozilla.com>
Date: Thu Jul 18 10:21:24 2013 +0800
Bug 876334 - Remove master/FM volume control from nsIAudioManager. r=mwu
:040000 040000 9d0ed685765a8fd21d8b7cdcc3cb6212a7a5107f 88b371573cb36e2c8bd034f6c409271d050e0544 M b2g
:040000 040000 e043f88d30694bd5e3ca498d33e72b1962d21ba3 5ed456cf068d0b4f97b78b04bd2a0645b2a829b8 M dom
Flags: needinfo?(mwu)
Flags: needinfo?(mchen)
Assignee | ||
Updated•11 years ago
|
Keywords: regression
Comment 6•11 years ago
|
||
I found the root cause is that Gaia [1] set audio volume as float into settings but gecko expected it is integer [2].
[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/settings.js#L569
[2] http://mxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelService.cpp#559
Flags: needinfo?(mchen)
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Michael Wu [:mwu] from comment #5)
> Please check if the fix in bug 904531 helps out.
It does not.
Assignee | ||
Comment 8•11 years ago
|
||
(In reply to Marco Chen [:mchen] from comment #6)
> I found the root cause is that Gaia [1] set audio volume as float into
> settings but gecko expected it is integer [2].
>
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/settings.
> js#L569
> [2]
> http://mxr.mozilla.org/mozilla-central/source/dom/audiochannel/
> AudioChannelService.cpp#559
It's strange, because what I see is this:
(gdb) break AudioChannelService.cpp:558
Breakpoint 1 at 0x41026928: file /media/alex/FirefoxOS/Inari/B2G/gecko/dom/audiochannel/AudioChannelService.cpp, line 558.
(gdb) continue
Continuing.
Breakpoint 1, mozilla::dom::AudioChannelService::Observe (this=<value optimized out>, aSubject=<value optimized out>, aTopic=<value optimized out>, aData=<value optimized out>)
at /media/alex/FirefoxOS/Inari/B2G/gecko/dom/audiochannel/AudioChannelService.cpp:558
558 JS::Rooted<JS::Value> value(cx);
(gdb) n
559 if (!JS_GetProperty(cx, obj, "value", &value)) {
(gdb)
558 JS::Rooted<JS::Value> value(cx);
(gdb)
559 if (!JS_GetProperty(cx, obj, "value", &value)) {
(gdb)
411 return l.s.tag == JSVAL_TAG_INT32;
(gdb)
581 }
(gdb)
586 }
(gdb)
nsObserverList::NotifyObservers (this=<value optimized out>, aSubject=0x4624a7a0, aTopic=0x46b8a620 "mozsettings-changed", someData=0x4498ee20)
at /media/alex/FirefoxOS/Inari/B2G/gecko/xpcom/ds/nsObserverList.cpp:95
95 for (int32_t i = 0; i < observers.Count(); ++i) {
Please note that for this backtrace, I've seperated both tests you pointed at that are on line 559, so the code should be read as:
JS::Rooted<JS::Value> value(cx);
. if (!JS_GetProperty(cx, obj, "value", &value)) {
+ return NS_OK;
+ }
+
+ if (!value.isInt32()) {
return NS_OK;
}
nsCOMPtr<nsIAudioManager> audioManager = do_GetService(NS_AUDIOMANAGER_CONTRACTID);
NS_ENSURE_TRUE(audioManager, NS_OK);
int32_t index = value.toInt32();
if (keyStr.EqualsLiteral("audio.volume.content")) {
audioManager->SetAudioChannelVolume(AUDIO_CHANNEL_CONTENT, index);
} else if (keyStr.EqualsLiteral("audio.volume.notification")) {
audioManager->SetAudioChannelVolume(AUDIO_CHANNEL_NOTIFICATION, index);
} else if (keyStr.EqualsLiteral("audio.volume.alarm")) {
audioManager->SetAudioChannelVolume(AUDIO_CHANNEL_ALARM, index);
} else if (keyStr.EqualsLiteral("audio.volume.telephony")) {
audioManager->SetAudioChannelVolume(AUDIO_CHANNEL_TELEPHONY, index);
} else {
MOZ_ASSERT("unexpected audio channel for volume control");
}
As you can see, the "value" test fails. Would it fail because of the float/int difference ?
Assignee | ||
Comment 9•11 years ago
|
||
Looks like I got mislead by gdb ; doing printf debug, the bail out effectively happens when testing isInt32().
Assignee | ||
Comment 10•11 years ago
|
||
value.isInt32() and value.isDouble() fails, but value.isString() works. Dumping the string content it seems we have a string with the correct integer value.
Assignee | ||
Comment 11•11 years ago
|
||
The JSON that we receive is:
{"key":"audio.volume.notification","value":"12.0","message":"fromSettingsChangeNotifier"}
Assignee | ||
Comment 12•11 years ago
|
||
And forcing with parseInt() as suggested previously, I get:
{"key":"audio.volume.notification","value":5,"message":"fromSettingsChangeNotifier"}
Assignee | ||
Comment 13•11 years ago
|
||
And then it comes to light ...
In this code, |value = parseFloat(input.value).toFixed(1)| toFixed will convert to a String.
Assignee | ||
Comment 14•11 years ago
|
||
Please find attached a patch that fixes the issue on Gecko side, by handling the String case. As far as I could talk with :kazé, the current Gaia behavior in the settings app, i.e. |value = parseFloat(input.value).toFixed(1);| is done on purpose. So I decided to handle this as a legitimate case on Gecko side, and process the string through the Decimal class. If it turns out the string once parsed gives NaN, we will stop, but in case it worked and we read a valid Double, then let's use it!
As far as I could test, it made media.audio_policy working again on my Inari.
Attachment #792864 -
Flags: review?(mchen)
Flags: needinfo?(kaze)
Comment 15•11 years ago
|
||
(In reply to Alexandre LISSY :gerard-majax from comment #14)
> Created attachment 792864 [details] [diff] [review]
> Handle String value and convert to Integer
>
Hi,
Thanks for your effort on this patch.
But I want to know that why do we need to add these codes in order to get a simple integer values? We can simply ask Gaia's help to store integer into settings DB just like what HW volume keys did in a integer only.
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to Marco Chen [:mchen] from comment #15)
> (In reply to Alexandre LISSY :gerard-majax from comment #14)
> > Created attachment 792864 [details] [diff] [review]
> > Handle String value and convert to Integer
> >
> Hi,
>
> Thanks for your effort on this patch.
>
> But I want to know that why do we need to add these codes in order to get a
> simple integer values? We can simply ask Gaia's help to store integer into
> settings DB just like what HW volume keys did in a integer only.
As I already stated, the Gaia code that is triggered for setting the value from the <input> to the Settings is generic, so:
- we cannot just force it to store int without risking breaking other things
- the toFixed(1) was done *on purpore* according to kaze, but I don't know much more
As far as I can tell, hardware volume keys are not handled by the same code path, and the setting in this case is handled by the changeVolume() function that lives within apps/system/js/sound_manager.js. And in this case, we can use an int as we want.
Fabien, if you could find the reason for the toFixed(1) since it's the root of this and I could not find anything about this behavior. Maybe we don't really need this and in this case we can rely on a JS Number type.
Assignee | ||
Comment 17•11 years ago
|
||
And of course one should read "on purpose" and not "on purpore".
Comment 18•11 years ago
|
||
Hi,
I will postpone this review because it is strange that Gecko needs to check the type of settings value. It should be dealt with Gaia & Gecko to set a consistent type of value. And I would prefer to be a integer only like what HW volume keys did.
Comment 19•11 years ago
|
||
Hi all,
May I know who can make the decision from Gaia side that whether we can store the audio volume level into integer and that may need to modify the code for input field? Thanks.
Comment 20•11 years ago
|
||
I don't see any good reason either to have a string -> int conversion on the gecko side. The correct fix seems clearly in gaia.
Kaze, can you elaborate about comment 16?
Assignee | ||
Comment 23•11 years ago
|
||
(In reply to Preeti Raghunath(:Preeti) from comment #21)
> koi+ as it is regression
The fix is ready for a while now, whether we should be applying it on Gaia or Gecko.
I'll recheck on Gaia side if there are other users of this feature or not, and if I don't find any, then I'll go for the Gaia patch since it's the less invasive and the one that makes more sense.
Meanwhile if someone sees any good reason to handle it on Gecko side, please, raise your voice.
Assignee | ||
Comment 24•11 years ago
|
||
The only user that might be impacted by this change seems to be the screen brightness:
apps/settings/elements/display.html: <input type="range" name="screen.brightness" step="0.01" min="0.1" value="0.5" max="1" />
However, that is already quite strange, the step here being 0.01 and the toFixed(1) buggy call will round at 0.1.
Assignee | ||
Comment 25•11 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Comment 26•11 years ago
|
||
Comment on attachment 818355 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12912
Please find attached a link to the github pull request https://github.com/mozilla-b2g/gaia/pull/12912. I chose to keep the toFixed(1) call and thus add another parseFloat() after this, to make sure that we have a:
- rounded to one digit
- Number type
Attachment #818355 -
Flags: review?(21)
Attachment #818355 -
Flags: review?(21) → review+
Assignee | ||
Comment 27•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(kaze)
Updated•11 years ago
|
Component: Widget: Gonk → Gaia::Settings
Product: Core → Boot2Gecko
Version: 26 Branch → unspecified
Updated•11 years ago
|
Attachment #792864 -
Flags: review?(mchen)
Comment 28•11 years ago
|
||
Uplifted 9bda081b1f089565162268092d535c848621aac5 to:
v1.2: 6532c040ce892b8cbb03b804696e232cea7007ce
status-b2g-v1.2:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•