Right now nsIAudioManager expose a mastervolume property that let the front-end control the volume of both master and voice. Sadly this is not so scalable and prevent the front-end to shut down all sounds coming from webpages when there is a call. Exposing voicevolume would help here.

Vivien: Are we exposing nsIAudioManager to any apps, such as the system app? Or do we additionally need to add an API which does that?

(In reply to Jonas Sicking (:sicking) from comment #1) > Vivien: Are we exposing nsIAudioManager to any apps, such as the system app? > Or do we additionally need to add an API which does that? Nothing is exposed. b2g/chrome/content/settings.js listen for a specific setting change and reflect that by calling nsIAudioManager.masterVolume = value. So I don't think we need an API just something added to that .idl that let us add a new setting to control it.

philikon, would you mind taking a look (since you committed the existing .idl)?

As my investigation, Android ICS didn't call setVoiceVolume() from AudioSystem.cpp anymore. And from the aspect of audio hal view, the setVoice/MasterVolume all tune it on SND_DEVICE directly (msm7627a AudioHardware::setVoiceVolume). The only difference is the level compensation of voice is higher then master. So if you want to use setVoiceVolume() from AudioSystem.cpp for adjusting in-call volume then you need to set master volume back once call is ended. And if you think this can be used to mute the master volume for music playing then it can't be achieved. The correct method is to adjust the stream volume for each kind of stream type (ex: voice, music, system, DTMF). But as you knew that the B2G now hardcoded the system stream type for creating AudioTrack. (http://mxr.mozilla.org/mozilla-central/source/media/libsydneyaudio/src/sydney_audio_gonk.cpp#126) So we may need new API for configuring the stream type not hardcoded to system of stream type.

This separates voice volume out of the master volume. The only problem is that we can't *get* the voice volume for some reason. So initially the value will be -1.0 until the UI sets it. I recommend we store the voice volume in a setting and then set the volume to that setting's value after booting. What do you think, Vivien?

Oh I entirely missed Marco's comment. Marco, you clearly know more about this. I'll let you and Vivien hash this out. Thanks!

Should we have a general solution rather than adjusting the volume for this? How about this: When there comes a phone call, let's have a notification or some kinds of change event sent to running apps(or apps can determine if they want to listen to this events). Then, each app can determine what should do next. For example, FM radio could just turn off the FM radio; Music can pause; ... In my opinion, other apps don't only want to set volume, but also for other actions. For example, games app might want to pause the whole game, rather than just set the volume to zero. For different types of voice volume is what we can discuss and work on. But, for cases like: playing music and get a phone call. IMO, with a general solution for this, it might be better. How do you feel?

May I make a conclusion first? 1. If you want to mute all sounds (music playing, system tone) but just keep the voice volume then it can't be achieved by setMasterVolume() and setVoiceVolume(). 2. If you really think this should be landed on v1, then two directions we can do a. Follow :khu 's comment, asking related apps to listen phone call status then pause their audio stream. (I saw this on Android too.) b. Since all audio tracks are hardcoded by system of stream type but only voice are provided by modem processor to DSP/Codec with voice_call of stream type. So we can replace the setMasterVolume() by setStreamVolumeIndex(SYSTEM_STREAM) and setVoiceVolume() by setStreamVolumeIndex(VOICE_CALL_STREAM). 3. The direction b also give us a feature for volume changing on system & voice individually.

1. Let us skip the requirement of muting other source but voice during in-call state. And focus on how to set voice volume on a separated path then system volume. 2. This version provides API for set/getStreamVolume by different stream type. According to the status now, we only have two stream type on system and voice-call. 3. What can do on this patch from user of view? a. User can adjust the system volume via volume keys or settings. b. User can adjust the voice volume via volume keys during in-call state.

Just want to make sure, does it mean we will be able to specify the level for: voice, ring, alarm, music, notification and others (less useful for now) ?

Yes, it should be able to control level on different stream types. But for now, only system and voice can be useful.

Comment on attachment 662867 [details] [diff] [review] WIPv1 Review of attachment 662867 [details] [diff] [review]: ----------------------------------------------------------------- Nice work! Please fix the nits listed below and I'll give you r+ :) ::: dom/system/gonk/AudioManager.cpp @@ +42,5 @@ > + 7, // notification > + 15, // BT SCO > + 7, // enforced audible > + 15, // DTMF > + 15, // TTS Nit: align those comments? @@ +105,5 @@ > InternalSetAudioRoutes(GetCurrentSwitchState(SWITCH_HEADPHONES)); > + > + for (int loop = 0; loop < AUDIO_STREAM_CNT; loop++) { > + AudioSystem::initStreamVolume(static_cast<audio_stream_type_t>(loop), 0, > + sMaxStreamVolumeTbl[loop]); Nit: align last parameter with the others: AudioSystem::initStreamVolume(static_cast<audio_stream_type_t>(loop), 0, sMaxStreamVolumeTbl[loop]); @@ +138,4 @@ > if (AudioSystem::getMasterVolume(aMasterVolume)) { > return NS_ERROR_FAILURE; > } > + LOG("get %f\n", *aMasterVolume); If we want to keep this log message (I'm not sure we do), we should make it more meaningful. At the very least s/get/get master volume/ @@ +144,5 @@ > > NS_IMETHODIMP > AudioManager::SetMasterVolume(float aMasterVolume) > { > + LOG("set %f\n", aMasterVolume); Ditto. ::: dom/system/gonk/nsIAudioManager.idl @@ +55,5 @@ > void setForceForUse(in long usage, in long force); > long getForceForUse(in long usage); > + > + /** > + * Configurre volume with each audio stream types Nit: typo in configurre. Also: grammar. Better: Control the volume of various audio streams. @@ +70,5 @@ > + const long STREAM_TYPE_TTS = 9; > + > + void setStreamVolumeIndex(in long stream, in long index); > + long getStreamVolumeIndex(in long stream); > + long getMaxStreamVolumeIndex(in long stream); You also need to update the interface's UUID.

Follow philikon's comment.

1. Add reviewer 2. Merge with newest code from FM feature.

1. Add reviewer 2. Merge with newest code from FM feature. Review Item: 3. Before gaia migrate to use set/getStreamVolumeIndex(), set each stream to the middle volume initially. Hi Philikon, Sorry to miss item 3 or the stream volume will be set lowest in default if no one set it explicitly. Thanks.

I should check more cautiously. sorry about update again.

Comment on attachment 663972 [details] [diff] [review] WIPv3 Review of attachment 663972 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/AudioManager.cpp @@ +123,5 @@ > + sMaxStreamVolumeTbl[loop]); > + // Before gaia migrate to use set/getStreamVolumeIndex APIs, set volume to the > + // middle initially. > + AudioSystem::setStreamVolumeIndex(static_cast<audio_stream_type_t>(loop), > + sMaxStreamVolumeTbl[loop] / 2); This is the wrong place to do this. This should be in b2g's shell.js or settings.js. That's where the final code for this will live anyway, so there's no reason to do this hack job here. In b2g'js shell.js and/or settings.js, I think we want to read the volume settings for the various audio streams and then call Services.audioManager.setStreamVolumeIndex() accordingly. I would suggest doing this in a separate patch. Perhaps also a separate bug if your patch can land without this (it seems not if all audio streams are going to be silent.)

Thanks for your review first. I can't land this patch without stream volume initilization or the sound will be very small even master volume is maximun. As your suggestion, so we should hack this initilization on shell/setting.js. I remembered that one of them hack the master volume to 0.5 too. Is this what you suggest?

(In reply to Marco Chen [:mchen] from comment #20) > Thanks for your review first. > > I can't land this patch without stream volume initilization or the sound > will be very small even master volume is maximun. > As your suggestion, so we should hack this initilization on > shell/setting.js. I remembered that one of them hack the master volume to > 0.5 too. Is this what you suggest? I guess you're referring to https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#158. If I read that comment right, that's just a hack and it can now be removed because we initialize the audio volume from settings: https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#50 So I think we should remove the hack in shell.js and initialize all (relevant) audio streams from settings. All we need to do is commit default settings values to Gaia before landing the patches.

https://github.com/mozilla-b2g/gaia/pull/5197 This gaia pull request gives the initial value of channels we now need: system, voice_call, fm

Follow the comment for WIPv3 to 1. Add setting observer in settings.js. 2. The initialization of new stream volume types already landed on Gaia.

Will remove the space in AudioSystem.h. Sorry about that.

Comment on attachment 665337 [details] [diff] [review] WIPv4 Review of attachment 665337 [details] [diff] [review]: ----------------------------------------------------------------- You did not address this from comment 21: So I think we should remove the hack in shell.js Unless you think it's needed still? ::: b2g/chrome/content/settings.js @@ +60,5 @@ > + let audioManager = Services.audioManager; > + if (!audioManager) > + return; > + > + audioManager.setStreamVolumeIndex(audioManager.STREAM_TYPE_VOICE_CALL, value); OMG repitition. Here's a suggestion: let audioSettings = [ // settings name, default value, stream type ['audio.volume.system', 7, 'STREAM_TYPE_SYSTEM'], ['audio.volume.voice_call', 5, 'STREAM_TYPE_VOICE_CALL'], ... etc. ]; for (let [setting, defaultValue, streamType] of audioSettings) { SettingsListener.observe(setting, defaultValue, function(value) { let audioManager = Services.audioManager; if (!audioManager) return; audioManager.setStreamVolumeIndex(audioManager[streamType], value); }); } r=me with that.

I think that the hack code for master volume in shell.js is not related to this bug. So if we want to remove it then a new bug and patch are suitable for individual purpose.

(In reply to Marco Chen [:mchen] from comment #26) > I think that the hack code for master volume in shell.js is not related to > this bug. > So if we want to remove it then a new bug and patch are suitable for > individual purpose. Fine by me.

Reply comment25 about the code for audioSettings. I modified the codes as below. The most important part is on the issue related to closure on java script. The callback functions in SettingsListener.observe() will be called after the for loop, so the variables used for each setStreamVolumeIndex() will be the final value from for loop. This version uses anoymous function to create closure for each callback functions. So the parameters for setStreamVolumeIndex() will just be the correct values. ---------------------------------------------------------------------------- let audioSettings = [ // settings name, default value, stream type ['audio.volume.voice_call', 5, Components.interfaces.nsIAudioManager.STREAM_TYPE_VOICE_CALL], ['audio.volume.system', 7, Components.interfaces.nsIAudioManager.STREAM_TYPE_SYSTEM], ['audio.volume.ring', 7, Components.interfaces.nsIAudioManager.STREAM_TYPE_RING], ['audio.volume.music', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_MUSIC], ['audio.volume.alarm', 7, Components.interfaces.nsIAudioManager.STREAM_TYPE_ALARM], ['audio.volume.notification', 7, Components.interfaces.nsIAudioManager.STREAM_TYPE_NOTIFICATION], ['audio.volume.bt_sco', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_BLUETOOTH_SCO], ['audio.volume.enforced_audible', 7, Components.interfaces.nsIAudioManager.STREAM_TYPE_ENFORCED_AUDIBLE], ['audio.volume.dtmf', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_DTMF], ['audio.volume.tts', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_TTS], ['audio.volume.fm', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_FM], ]; for each (let [setting, defaultValue, streamType] in audioSettings) { (function AudioStreamSettings(s, d, t) { SettingsListener.observe(s, d, function(value) { let audioManager = Services.audioManager; if (!audioManager) return; audioManager.setStreamVolumeIndex(t, d); }); })(setting, defaultValue, streamType); }

The suggested from Comment 25 has the issue related to JS closure. So use anoymous function to create individual closure for callback function.

Comment on attachment 666445 [details] [diff] [review] WIPv5 Philikon seems to be on vocation so change to another reviewer.

Comment on attachment 666445 [details] [diff] [review] WIPv5 Review of attachment 666445 [details] [diff] [review]: ----------------------------------------------------------------- There is no test cases in this patch, would you create some or file a follow-up bug for this issue? ::: b2g/chrome/content/settings.js @@ +67,5 @@ > + ['audio.volume.bt_sco', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_BLUETOOTH_SCO], > + ['audio.volume.enforced_audible', 7, Components.interfaces.nsIAudioManager.STREAM_TYPE_ENFORCED_AUDIBLE], > + ['audio.volume.dtmf', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_DTMF], > + ['audio.volume.tts', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_TTS], > + ['audio.volume.fm', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_FM], I think the indentation here should be 2 spaces, @@ +68,5 @@ > + ['audio.volume.enforced_audible', 7, Components.interfaces.nsIAudioManager.STREAM_TYPE_ENFORCED_AUDIBLE], > + ['audio.volume.dtmf', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_DTMF], > + ['audio.volume.tts', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_TTS], > + ['audio.volume.fm', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_FM], > + ]; and zero here.

Thanks for the review first. 1. Does the test cases mean auto test case or the test case used by verifier? 2. If it means for verifier then maybe I need to create a new case to wait Gaia UI for verification or attach a test Web App.

(In reply to Marco Chen [:mchen] from comment #32) > Thanks for the review first. > > 1. Does the test cases mean auto test case or the test case used by verifier? > > 2. If it means for verifier then maybe I need to create a new case to wait > Gaia UI for verification or attach a test Web App. Marionette tests.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #33) > (In reply to Marco Chen [:mchen] from comment #32) > > 1. Does the test cases mean auto test case or the test case used by verifier? > > Marionette tests. Oh, I don't know it's submitted. I was having a more deep inside investigation on those duplicated functions in AudioSystem. I guess you can't do anything to them for now. But note, there are two stream types defined! So if you're going to add FM in one, you'd better add it in another, too. Marionette tests, yes. There must be some way to verify your code, isn't it? And, in writing the test script, you'll find out there is no call path to `getStreamVolumeIndex` at all. What's the function meant for? You don't have to wait for Gaia UI to write a test. Marionette itself is the UI.

> Oh, I don't know it's submitted. I was having a more deep inside > investigation on those duplicated functions in AudioSystem. I guess you > can't do anything to them for now. But note, there are two stream types > defined! So if you're going to add FM in one, you'd better add it in > another, too. Yes, thanks for your suggestion on duplicated FM stream type in AudioSystem.h. I added it. > Marionette tests, yes. There must be some way to verify your code, isn't it? Due to this change effects the physical volume level, it need some real person to hear/verify the corresponding volume level on adjusting each stream type. I think it can't be verified by auto testing. > And, in writing the test script, you'll find out there is no call path to > `getStreamVolumeIndex` at all. What's the function meant for? You don't have Currently there is no WebAPI for accessing audio stream type, please refer to Bug 795237. In Gaia side, currently we just set each stream volume to the middle value and still use master volume to do control. > to wait for Gaia UI to write a test. Marionette itself is the UI. So I just can provide a manual test application for adjusting the FM, Voice Call and system stream volume. Is this enough? Thanks.

Bug 796874 is created for tracking Marionette tests due to no Web API for verification audio volume values.

Comment on attachment 666445 [details] [diff] [review] WIPv5 Review of attachment 666445 [details] [diff] [review]: ----------------------------------------------------------------- Ask mwu's review for this patch touches native audio system code.

Comment on attachment 666445 [details] [diff] [review] WIPv5 Review of attachment 666445 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/settings.js @@ +55,5 @@ > > audioManager.masterVolume = Math.max(0.0, Math.min(value, 1.0)); > }); > > +let audioSettings = [ Can you minimize the number of new volume settings to just the ones we need? We would like to move away from the Android audio system in the future and mirroring all of the Android audio api to the settings api like this may make things harder. @@ +79,5 @@ > + return; > + > + audioManager.setStreamVolumeIndex(t, d); > + }); > + })(setting, defaultValue, streamType); Hm, why not do it the way philikon suggested? It should still do the right thing with less complexity.

(In reply to Michael Wu [:mwu] from comment #38) > > +let audioSettings = [ > > Can you minimize the number of new volume settings to just the ones we need? > We would like to move away from the Android audio system in the future and > mirroring all of the Android audio api to the settings api like this may > make things harder. > What audio steam types we need are discussed on bug 795237? It seems to be not only voice, fm but also others. > @@ +79,5 @@ > > + return; > > + > > + audioManager.setStreamVolumeIndex(t, d); > > + }); > > + })(setting, defaultValue, streamType); > > Hm, why not do it the way philikon suggested? It should still do the right > thing with less complexity. Please refer to the comment 28. the code from philikon has issue there.

Comment on attachment 666445 [details] [diff] [review] WIPv5 Philikon, do you know why your suggestion doesn't work? AFAICT, it creates a closure too and should work properly.

Comment on attachment 666445 [details] [diff] [review] WIPv5 Review of attachment 666445 [details] [diff] [review]: ----------------------------------------------------------------- Oh yeah, good catch in comment 28 about the closure. Rookie mistake on my part. Looks good! ::: b2g/chrome/content/settings.js @@ +67,5 @@ > + ['audio.volume.bt_sco', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_BLUETOOTH_SCO], > + ['audio.volume.enforced_audible', 7, Components.interfaces.nsIAudioManager.STREAM_TYPE_ENFORCED_AUDIBLE], > + ['audio.volume.dtmf', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_DTMF], > + ['audio.volume.tts', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_TTS], > + ['audio.volume.fm', 15, Components.interfaces.nsIAudioManager.STREAM_TYPE_FM], At the very least you should use `Ci` instead of `Components.interfaces`, but since you're accessing it that often, you probably want to const nsIAudioManager = Ci.nsIAudioManager; above and then just refer to nsIAudioManager.

1. Following the suggestion about "const nsIAudioManager = Ci.nsIAudioManager;" 2. Modify the max stream volume level of system, voice and FM to 10. This is in order to map the UI design now. 3. Will change the settings.json from Gaia to map the default value of FM, voice and system to 10 too. (before landing this patch) 4. After landing the patch, Gaia can start to transfer volume control from master volume to stream volume. But this patch will not effect the volume behavior of current Gaia. Finally, give some explanation of three stream types which we can use now. a. voice_call: This is a stream from Modem chip or processor and will be treated as voice_call only. b. fm: This is a stream from FM chip and will be treated as FM only. c. system: All of streams generated by Firefox OS now is hardcoded as system now. After Bug 795237 is landing, we can just have chance to use others.

Try run for a66986d595c4 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=a66986d595c4 Results (out of 250 total builds): success: 222 warnings: 27 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mchen@mozilla.com-a66986d595c4

Modify the voice,FM and system's default value to 10 in order to meet the UI design now.

Add the review & aurora into commit message.

Marco, I have a question here. If from now on we don't use audio.volume.master anymore(Which controls the whole-over system volume). Does 'audio.volume.system' contains other types of volume in your patch? I am wondering if we couldn't set music/ringtone/alarm....'s volume if we abandon the master volume now.

I am told by bluetooth peers(Gina, Eric) that bluetooth earphone settings is supenior to the phone settings. We should have individual 'audio.volume.bluetooth' settings.

(In reply to Alive Kuo [:alive] from comment #46) > Marco, I have a question here. > If from now on we don't use audio.volume.master anymore(Which controls the > whole-over system volume). Does 'audio.volume.system' contains other types > of volume in your patch? I am wondering if we couldn't set > music/ringtone/alarm....'s volume if we abandon the master volume now. Before bug 795237 is landed, the whole audio streams from B2G will be the type of 'audio.volume.system'. So just use it to control all the volume of streams generated by B2G process.

(In reply to Alive Kuo [:alive] from comment #47) > I am told by bluetooth peers(Gina, Eric) that bluetooth earphone settings is > supenior to the phone settings. We should have individual > 'audio.volume.bluetooth' settings. The stream type contained in this patch (refer to Android) also include one called AUDIO_STREAM_BLUETOOTH_SCO. This is used for BT_SCO volume. Please refer to link as below for reference. https://www.codeaurora.org/gitweb/quic/la/?p=platform/packages/apps/Phone.git;a=blob;f=src/com/android/phone/BluetoothHandsfree.java;h=bc37881ad0e830e4ea4c771f4fc9fa64cba60aa9;hb=HEAD#l1550

https://hg.mozilla.org/integration/mozilla-inbound/rev/098958fa76f3 Should this have tests?

About testsuite please refer to Bug 796874.

I think this breaks non-RIL builds.

(In reply to Eitan Isaacson [:eeejay] from comment #53) > I think this breaks non-RIL builds. It does. :/ Filed bug 809972,