906296 - [B2G][Settings] Changing volume in Gaia settings has no impact on media_policy

Assignee

Description

•

11 years ago

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:

:gerard-majax

Assignee

Updated

•

11 years ago

blocking-b2g: --- → koi?

:gerard-majax

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

:gerard-majax

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.

:gerard-majax

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.

:gerard-majax

Assignee

Comment 3

•

11 years ago

18/07 cb9a271064cc2dda511972f4720b33f2ff0188b3 ok
19/07 76ce8020e7fa56006c4a48b1916ff6760be0b909 broken

This narrows down to a subset of 268 commits :)

:gerard-majax

Assignee

Updated

•

11 years ago

Assignee: nobody → lissyx+mozillians

:gerard-majax

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)

:gerard-majax

Assignee

Updated

•

11 years ago

Keywords: regression

Michael Wu [:mwu]

Comment 5

•

11 years ago

Please check if the fix in bug 904531 helps out.

Flags: needinfo?(mwu)

Marco Chen [:mchen]

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)

:gerard-majax

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.

:gerard-majax

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 ?

:gerard-majax

Assignee

Comment 9

•

11 years ago

Looks like I got mislead by gdb ; doing printf debug, the bail out effectively happens when testing isInt32().

:gerard-majax

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.

:gerard-majax

Assignee

Comment 11

•

11 years ago

The JSON that we receive is:
{"key":"audio.volume.notification","value":"12.0","message":"fromSettingsChangeNotifier"}

:gerard-majax

Assignee

Comment 12

•

11 years ago

And forcing with parseInt() as suggested previously, I get:
{"key":"audio.volume.notification","value":5,"message":"fromSettingsChangeNotifier"}

:gerard-majax

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.

:gerard-majax

Assignee

Comment 14

•

11 years ago

Attached patch Handle String value and convert to Integer — Details — Splinter Review

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)

Marco Chen [:mchen]

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.

:gerard-majax

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.

:gerard-majax

Assignee

Comment 17

•

11 years ago

And of course one should read "on purpose" and not "on purpore".

Marco Chen [:mchen]

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.

Marco Chen [:mchen]

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.

[:fabrice] Fabrice Desré

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?

Preeti Raghunath(:Preeti)

Comment 21

•

11 years ago

koi+ as it is regression

blocking-b2g: koi? → koi+

:gerard-majax

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.

:gerard-majax

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.

:gerard-majax

Assignee

Updated

•

11 years ago

Blocks: 899487

:gerard-majax

Assignee

Comment 25

•

11 years ago

Attached file Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12912 — Details

Pointer to Github pull-request

:gerard-majax

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)

Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)

Updated

•

11 years ago

Attachment #818355 - Flags: review?(21) → review+

:gerard-majax

Assignee

Comment 27

•

11 years ago

https://github.com/mozilla-b2g/gaia/commit/9bda081b1f089565162268092d535c848621aac5

Status: NEW → RESOLVED

Closed: 11 years ago

Resolution: --- → FIXED

:gerard-majax

Assignee

Updated

•

11 years ago

Flags: needinfo?(kaze)

Ryan VanderMeulen [:RyanVM]

Updated

•

11 years ago

Component: Widget: Gonk → Gaia::Settings

Product: Core → Boot2Gecko

Version: 26 Branch → unspecified

Marco Chen [:mchen]

Updated

•

11 years ago

Attachment #792864 - Flags: review?(mchen)

John Ford [:jhford] CET/CEST Berlin Time

Comment 28

•

11 years ago

Uplifted 9bda081b1f089565162268092d535c848621aac5 to:
v1.2: 6532c040ce892b8cbb03b804696e232cea7007ce

status-b2g-v1.2: --- → fixed

Handle String value and convert to Integer 11 years ago :gerard-majax 2.54 KB, patch		Details \| Diff \| Splinter Review
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/12912 11 years ago :gerard-majax 358 bytes, text/html	vingtetun : review+	Details