876334 - [Audio] Gecko should only refer AudioChannel type for controlling volume not StreamType

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

1. In settings.js or nsIAudioManager, there are enum value for Android Stream Types. Android Stream types are platform specific ones and they should be transferred by AudioChannel in Gonk level not appeared directly on Gecko code. We should keep codes on Gecko as platform independent.

2. I proposed to move volume control (based on AudioChannel) from settings.js to AudioChannlService because I would like put AudioChannel things together.

Comment 1

[Marco Chen [:mchen]]

Attachment: Patch - v1

Hi Randy,

Please refer to list as below for what I did in this patch and welcome for any suggestion. Thanks very much.

1. Remove masterVolume & masterMuted from nsIAudioManager:
   a. Currently Gaia/Gecko didn't adjust it directly and just keep it as 1.
   b. Set master to 1 in AudioManager::AudioManager().

2. Remove AudioSystem::setFmVolume() from AudioManager.cpp
   a. This function is not came from AOSP but chip vendor specific function.
   b. Currently AudioManager.cpp use FM stream type to adjust FM volume only.

3. Gecko used AudioChannelType to adjust volume but stream types in Gonk.
   a. Do mapping of AudioChannelType to stream type in AudioManager.
   b. Mapping volume index of SCO as telephony * 3 and reflect by AUDIO_CHANNEL_TELEPHONY.

4. Move audio volume control from settings.js to AudioChannelService.

Discussion Item:
  1. There is no Audio channel for BT_SCO so currently I mapped BT_SCO with Telephony channel.
  2. Or we need an new API for adjusting BT_SCO.

Comment 2

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 757338 [details] [diff] [review]
Patch - v1

Review of attachment 757338 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/AudioManager.cpp
@@ +479,5 @@
> +    if (stream[i] == AUDIO_STREAM_BLUETOOTH_SCO) {
> +      aIndex *= 3;
> +      MOZ_ASSERT(aIndex <= 15);
> +    }
> +

May need BT guys help to confirm this is ok or not.

Comment 3

[Marco Chen [:mchen]]

Attachment: Patch v2

Hi Michael,

What I did in the patch are

1. Remove masterVolume & masterMuted from nsIAudioManager:
   a. Currently Gaia/Gecko didn't adjust it directly and just keep it as 1.
   b. Set master to 1 in AudioManager::AudioManager().

2. Remove AudioSystem::setFmVolume() from AudioManager.cpp
   a. This function is not came from AOSP but chip vendor specific function.
   b. Currently AudioManager.cpp use FM stream type to adjust FM volume only.

3. Gecko used AudioChannelType to adjust volume but stream types in Gonk.
   a. Do mapping of AudioChannelType to stream type in AudioManager.

4. According there is no AudioChannelType for BT_SCO, I directly listen the moz-settings change in AudioManager.cpp then apply the volume index to backend.

5. Move audio volume control from settings.js to AudioChannelService.

6. Remove the hack code made for forcing BT_SCO during voice call.
   That is fixed by new BT Web API.

7. Write wrapper functions for set/getStreamVolumeIndex() according to ICS or JB version.

8. Remove code for GB because FxOS doesn't support GB any more.

9. Add a new API in nsIAudioManager.idl called RecoveryVolumeIndexToBackend().
   This is used for recovering backend configuration when backend is crashed.

Thanks for the review.
If you expected, I can split them into separate patch for you to review easily.

Comment 4

[Marco Chen [:mchen]]

10. For audio volume control on JB version, according to volume settings is under individual output devices and currently FxOS just support volume settings on all devices, I apply a single stream volume to all possible devices in a call.

Comment 5

[Michael Wu [:mwu]]

If you don't mind, can you split the patches? There's a lot going on here.

Comment 6

[Marco Chen [:mchen]]

Attachment: Part 1: Remove SetMaster/FmVolume from AudioManager. v1

1. Remove masterVolume & masterMuted from nsIAudioManager:
   a. Currently Gaia/Gecko didn't adjust it directly and just keep it as 1.
   b. Set master to 1 in AudioManager::AudioManager().

2. Remove AudioSystem::setFmVolume() from AudioManager.cpp
   a. This function is not came from AOSP but chip vendor specific function.
   b. Currently AudioManager.cpp use FM stream type to adjust FM volume only.

Comment 7

[Marco Chen [:mchen]]

Attachment: Part 2: Remove BT Temporary Patch

6. Remove the hack code made for forcing BT_SCO during voice call.
   That is fixed by new BT Web API.

Comment 8

[Marco Chen [:mchen]]

Attachment: Part 3: Control Volume based on AudioChannelType v1

3. Gecko used AudioChannelType to adjust volume but stream types in Gonk.
   a. Do mapping of AudioChannelType to stream type in AudioManager.

4. According there is no AudioChannelType for BT_SCO, I directly listen the moz-settings change in AudioManager.cpp then apply the volume index to backend.

5. Move audio volume control from settings.js to AudioChannelService.

7. Write wrapper functions for set/getStreamVolumeIndex() according to ICS or JB version.

9. Add a new API in nsIAudioManager.idl called RecoveryVolumeIndexToBackend().
   This is used for recovering backend configuration when backend is crashed.

Comment 9

[Marco Chen [:mchen]]

Attachment: Part 4: Remove supporting of GB. v1

8. Remove code for GB because FxOS doesn't support GB any more.

Comment 10

[Michael Wu [:mwu]]

Comment on attachment 758495 [details] [diff] [review]
Part 1: Remove SetMaster/FmVolume from AudioManager. v1

Review of attachment 758495 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good - just have a nit about a comment.

::: dom/system/gonk/AudioManager.cpp
@@ +264,5 @@
>      AUDIO_DEVICE_OUT_SPEAKER);
>  #endif
>  
> +  // Gecko only control stream volume but master so set to maximum value
> +  // directly.

Not sure if I understand this sentence. Do you mean s/but/not/?

Max volume is a little weird - I think on some mixer controls, setting the volume to max will actually amplify the sound and bring in the risk of clipping. Something like 0dB is more specific about what we actually mean. (assuming 1.0 master volume means 0dB)

Comment 11

[Michael Wu [:mwu]]

Comment on attachment 758500 [details] [diff] [review]
Part 4: Remove supporting of GB. v1

Review of attachment 758500 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/AudioManager.cpp
@@ +366,5 @@
> +    status_t status = AudioSystem::setForceUse(
> +                        (audio_policy_force_use_t)aUsage,
> +                        (audio_policy_forced_cfg_t)aForce);
> +    return status ? NS_ERROR_FAILURE : NS_OK;
> +  } else {

The else can be removed here since we normally return.

@@ +381,4 @@
>      // Dynamically resolved the ICS signature.
>      *aForce = AudioSystem::getForceUse((audio_policy_force_use_t)aUsage);
> +    return NS_OK;
> +  } else {

The else can also be removed here.

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 758499 [details] [diff] [review]
Part 3: Control Volume based on AudioChannelType v1

Review of attachment 758499 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/nsIAudioManager.idl
@@ +52,5 @@
>  
> +  /* The range of volume index is from 0 to N. Ex: 0 ~ 15 */
> +  void setAudioChannelVolume(in long channel, in long index);
> +  long getAudioChannelVolume(in long channel);
> +  long getMaxAudioChannelVolume(in long channel);

If we can make all volumes float types, the max volume will always be 1 and we don't have to determine what the max volume is.

Also, we need to bump the interface uuid for this change.

Comment 13

[Michael Wu [:mwu]]

(In reply to Marco Chen [:mchen] from comment #8)
> Created attachment 758499 [details] [diff] [review]
> Part 3: Control Volume based on AudioChannelType v1
> 
> 3. Gecko used AudioChannelType to adjust volume but stream types in Gonk.
>    a. Do mapping of AudioChannelType to stream type in AudioManager.
> 

> 5. Move audio volume control from settings.js to AudioChannelService.
> 

It makes sense to move the mapping out of the b2g app, but I think putting it in AudioChannelManager.cpp is a poor fit since it's so difficult to use the settings API from C++ code. It should be simpler to write a javascript component which is dedicated to listening for the settings changes and calls AudioManager appropriately.

> 4. According there is no AudioChannelType for BT_SCO, I directly listen the
> moz-settings change in AudioManager.cpp then apply the volume index to
> backend.
> 

Same thing here. We'll need to expose more apis in AudioManager to make this possible which will make things a bit ugly though. It seems strange that BT_SCO is its own stream type though. Isn't it just another output device? Or can an app play through BT_SCO while also playing through speakers?

> 9. Add a new API in nsIAudioManager.idl called
> RecoveryVolumeIndexToBackend().
>    This is used for recovering backend configuration when backend is crashed.

Are you planning to use this elsewhere?

Comment 14

[Marco Chen [:mchen]]

(In reply to Michael Wu [:mwu] from comment #12)
> Comment on attachment 758499 [details] [diff] [review]
> Part 3: Control Volume based on AudioChannelType v1
> 
> Review of attachment 758499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/nsIAudioManager.idl
> @@ +52,5 @@
> >  
> > +  /* The range of volume index is from 0 to N. Ex: 0 ~ 15 */
> > +  void setAudioChannelVolume(in long channel, in long index);
> > +  long getAudioChannelVolume(in long channel);
> > +  long getMaxAudioChannelVolume(in long channel);
> 
> If we can make all volumes float types, the max volume will always be 1 and
> we don't have to determine what the max volume is.

If we put the max volume as 1.0 (float type), the normalization from this float value to platform volume index will become a problem.
  a. We can do directly mapping if platform's volume is float type too. (ex: Master Volume)
  b. If platform' volume is between a range in int type then it's hard to transform float to int. (ex: Some Android stream volume is from 0 ~ 15, then how to map 0.13 or 0.14 to that range.)

In the opposite way, to transform a range as int type to float type is more easy.

>> Also, we need to bump the interface uuid for this change.
I did this in Part 1 already but it seems that I should do it on each patch modified IDL.

Thanks.

> 
> Also, we need to bump the interface uuid for this change.

Comment 15

[Marco Chen [:mchen]]

(In reply to Michael Wu [:mwu] from comment #13)
> (In reply to Marco Chen [:mchen] from comment #8)
> > Created attachment 758499 [details] [diff] [review]
> It makes sense to move the mapping out of the b2g app, but I think putting
> it in AudioChannelManager.cpp is a poor fit since it's so difficult to use
> the settings API from C++ code. It should be simpler to write a javascript
> component which is dedicated to listening for the settings changes and calls
> AudioManager appropriately.
> 

1. I put this modification on AudioChannelService not AudioChannelManager (Web API layer).
2. My initial thought is to put all AudioChannel stuffs into a single place so it is clear to find out what AudioChannel did and ease for maintenance.
3. I think monitoring settings change in C++ is not so "difficult" and there are no much settings I need to filter out.
So I would like to keep this change or of course I am willing to listen your more suggestion about this.

> > 9. Add a new API in nsIAudioManager.idl called
> > RecoveryVolumeIndexToBackend().
> 
> Are you planning to use this elsewhere?

No, currently there is no other place used this function.
So do I need to put this function into AudioManager class not interface then casting nsIAudioManager to AudioManager class for calling this function?

Thanks.

Comment 16

[Marco Chen [:mchen]]

Attachment: Part 1: Remove SetMaster/FmVolume from AudioManager. Check-in Version

1. Change the comment from maximum value to default value.
2. Add reviewer's name into commit comment.

Comment 17

[Marco Chen [:mchen]]

Attachment: Part 2: Remove BT Temporary Patch Check-in Version

Add reviewer's name into commit comment.

Comment 18

[Marco Chen [:mchen]]

Attachment: Part 4: Remove supporting of GB. Check-in Version

1. Address reviewer's comment.
2. Add reviewer's name into commit message.

Comment 19

[Marco Chen [:mchen]]

(In reply to Michael Wu [:mwu] from comment #13)
> Same thing here. We'll need to expose more apis in AudioManager to make this
> possible which will make things a bit ugly though. 

BT_SCO stream type didn't be exposed to Gecko anywhere so there is no additional/new API need to be introduced in AudioManager. Currently the observer of bt_sco from mozsettings is triggered by Gaia system app who will modify audio.volume.bt_sco. I didn't change any design currently just move it from setting.js to AudioManager. So is this still a problem? 

By the way, we really need a way to control BT_SCO volume because
  1. we can't map BT_SCO as same as AUDIO_CHANNEL_TELEPHONY because telephony has 5 levels only but BT Spec ask BT_SCO to has 15 levels.
  2. we can normalize the 5 levels (telephony) to 15 levels (sco) but user can adjust sco volume vi BT headset which will send command back to device. So we can't normalize 15 levels back to 5 levels.
  3. So we may need a Web API for control BT SCO volume.

> It seems strange that
> BT_SCO is its own stream type though. Isn't it just another output device?
> Or can an app play through BT_SCO while also playing through speakers?
> 

I need to emphasize that in AudioFlinger backend, AUDIO_STREAM_BLUETOOTH_SCO is an individual volume control unit. And in gaia/UX view, volume of BT_SCO is separate with all other volume channels so we need a way to adjust volume which only apply on BT_SCO.

Comment 20

[Michael Wu [:mwu]]

(In reply to Marco Chen [:mchen] from comment #14)
> (In reply to Michael Wu [:mwu] from comment #12)
> > Comment on attachment 758499 [details] [diff] [review]
> > Part 3: Control Volume based on AudioChannelType v1
> > 
> > Review of attachment 758499 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/nsIAudioManager.idl
> > @@ +52,5 @@
> > >  
> > > +  /* The range of volume index is from 0 to N. Ex: 0 ~ 15 */
> > > +  void setAudioChannelVolume(in long channel, in long index);
> > > +  long getAudioChannelVolume(in long channel);
> > > +  long getMaxAudioChannelVolume(in long channel);
> > 
> > If we can make all volumes float types, the max volume will always be 1 and
> > we don't have to determine what the max volume is.
> 
> If we put the max volume as 1.0 (float type), the normalization from this
> float value to platform volume index will become a problem.
>   a. We can do directly mapping if platform's volume is float type too. (ex:
> Master Volume)
>   b. If platform' volume is between a range in int type then it's hard to
> transform float to int. (ex: Some Android stream volume is from 0 ~ 15, then
> how to map 0.13 or 0.14 to that range.)
> 
> In the opposite way, to transform a range as int type to float type is more
> easy.
> 

The main problem I have is exposing the int value in mozSettings. There's nothing special about having 16 different volume levels - it's just an implementation detail in AudioFlinger. I would argue it was actually a bad decision there too - it's usually only hardware mixers that require 2^n volume levels. For any audio buffers that come from the host side, float volume levels simply involve multiplying the sample. To work with AudioFlinger, we can just round(volume * max_volume_index). Gaia would have to make sure to use powers of twos to ensure the volume changes mean anything though.

BTW switching to floats is big enough to be spun off to its own bug, so we can worry about that at a later point.

> >> Also, we need to bump the interface uuid for this change.
> I did this in Part 1 already but it seems that I should do it on each patch
> modified IDL.
> 

Ah. Well, if you roll these patches up for committing, it won't matter.

Comment 21

[Michael Wu [:mwu]]

(In reply to Marco Chen [:mchen] from comment #15)
> (In reply to Michael Wu [:mwu] from comment #13)
> > (In reply to Marco Chen [:mchen] from comment #8)
> > > Created attachment 758499 [details] [diff] [review]
> > It makes sense to move the mapping out of the b2g app, but I think putting
> > it in AudioChannelManager.cpp is a poor fit since it's so difficult to use
> > the settings API from C++ code. It should be simpler to write a javascript
> > component which is dedicated to listening for the settings changes and calls
> > AudioManager appropriately.
> > 
> 
> 1. I put this modification on AudioChannelService not AudioChannelManager
> (Web API layer).
> 2. My initial thought is to put all AudioChannel stuffs into a single place
> so it is clear to find out what AudioChannel did and ease for maintenance.
> 3. I think monitoring settings change in C++ is not so "difficult" and there
> are no much settings I need to filter out.
> So I would like to keep this change or of course I am willing to listen your
> more suggestion about this.
> 

Ok. I poked around it and it looks like this is a somewhat common pattern, so.. fine. It's too bad we don't have anything better to work with JSON in C++.

> > > 9. Add a new API in nsIAudioManager.idl called
> > > RecoveryVolumeIndexToBackend().
> > 
> > Are you planning to use this elsewhere?
> 
> No, currently there is no other place used this function.
> So do I need to put this function into AudioManager class not interface then
> casting nsIAudioManager to AudioManager class for calling this function?
> 
> Thanks.

Yeah, though I'm not sure there's a need to break it out into its own function if it only has one caller..

Comment 22

[Michael Wu [:mwu]]

(In reply to Marco Chen [:mchen] from comment #19)
> (In reply to Michael Wu [:mwu] from comment #13)
> > Same thing here. We'll need to expose more apis in AudioManager to make this
> > possible which will make things a bit ugly though. 
> 
> BT_SCO stream type didn't be exposed to Gecko anywhere so there is no
> additional/new API need to be introduced in AudioManager. Currently the
> observer of bt_sco from mozsettings is triggered by Gaia system app who will
> modify audio.volume.bt_sco. I didn't change any design currently just move
> it from setting.js to AudioManager. So is this still a problem? 
> 

Nope, looks like people are already doing lots of manual JSON processing in C++. Go ahead.

> By the way, we really need a way to control BT_SCO volume because
>   1. we can't map BT_SCO as same as AUDIO_CHANNEL_TELEPHONY because
> telephony has 5 levels only but BT Spec ask BT_SCO to has 15 levels.
>   2. we can normalize the 5 levels (telephony) to 15 levels (sco) but user
> can adjust sco volume vi BT headset which will send command back to device.
> So we can't normalize 15 levels back to 5 levels.
>   3. So we may need a Web API for control BT SCO volume.
> 

Yeah. I consider BT SCO a separate output device, and separate output devices should all have different volumes. Our API doesn't really distinguish between source volumes and output volumes though. 

Is BT SCO only for calls, or can it take other audio?

Comment 23

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

BT_SCO audio path can be used for voice recognition.
And on msm7k platform, there is no bt_sco + speaker audio path.

Comment 24

[Marco Chen [:mchen]]

(In reply to Michael Wu [:mwu] from comment #22)
> Is BT SCO only for calls, or can it take other audio?

Currently BT_SCO output device is used on voice_call only and no others. So actually only BT_SCO output device will output audo_stream_bt_sco.
For BT_SCO input device, it will be used as the source of voice_recognition but that is not related to audio.volume.bt_sco (For output only)

Comment 25

[Marco Chen [:mchen]]

(In reply to Michael Wu [:mwu] from comment #21)
> Yeah, though I'm not sure there's a need to break it out into its own
> function if it only has one caller..

1. Ok, since no other one will use it, I should remove it from IDL interface.
2. Currently Set/GetStreamVolumeIndex() is defined as private, so in order to do the same thing in RecoveryVolumeIndexToBackend() and called by RecoveryTask. I need to move them to public function.

May I know this is what in you mind?

-----------------------

By the way there are additional two items should be reviewed.

10. I should use MOZ_WIDGET_GONK to protect the AudioManager in AudioChannelService.

11. For JB part of SetStreamVolumeIndex(), I set volume index into all possible output device based on audio stream type. So this can map to our current design - a volume settings of a AudioChannel is applied into all output device.

Thanks for all your review effort here.

Comment 26

[Michael Wu [:mwu]]

(In reply to Marco Chen [:mchen] from comment #25)
> (In reply to Michael Wu [:mwu] from comment #21)
> > Yeah, though I'm not sure there's a need to break it out into its own
> > function if it only has one caller..
> 
> 1. Ok, since no other one will use it, I should remove it from IDL interface.
> 2. Currently Set/GetStreamVolumeIndex() is defined as private, so in order
> to do the same thing in RecoveryVolumeIndexToBackend() and called by
> RecoveryTask. I need to move them to public function.
> 
> May I know this is what in you mind?
> 

That works. There's other ways of giving that class access to those functions involving protected or friends but it shouldn't matter much.

> -----------------------
> 
> By the way there are additional two items should be reviewed.
> 
> 10. I should use MOZ_WIDGET_GONK to protect the AudioManager in
> AudioChannelService.
> 

Well the entire directory is Gonk only, so I don't think you need to check.

> 11. For JB part of SetStreamVolumeIndex(), I set volume index into all
> possible output device based on audio stream type. So this can map to our
> current design - a volume settings of a AudioChannel is applied into all
> output device.
> 

I'm not actually sure about this part, but we can do a follow up later if we need.

Comment 27

[Michael Wu [:mwu]]

Comment on attachment 758499 [details] [diff] [review]
Part 3: Control Volume based on AudioChannelType v1

Review of attachment 758499 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/AudioManager.cpp
@@ +471,5 @@
> +      stream[0] = AUDIO_STREAM_ALARM;
> +      break;
> +    case AUDIO_CHANNEL_TELEPHONY:
> +      stream[0] = AUDIO_STREAM_VOICE_CALL;
> +      break;

If you put a default case here, you don't need the argument check at the beginning of the function. It'll also be easier to add other audio channel types in the future.

@@ +475,5 @@
> +      break;
> +  }
> +
> +  if (aIndex > sMaxStreamVolumeTbl[stream[0]]) {
> +    return NS_ERROR_INVALID_ARG;

Might make more sense to bundle this error checking in SetStreamVolumeIndex.

@@ +481,5 @@
> +
> +  status_t status = 0;
> +  for (int i = 0; i < 2; i++) {
> +    status += SetStreamVolumeIndex(stream[i], aIndex);
> +    mCurrentStreamVolumeTbl[stream[i]] = aIndex;

I think the setting of mCurrentStreamVolumeTbl belongs in SetStreamVolumeIndex. If you do this, you can just directly call SetStreamVolumeIndex in the switch statement before.

@@ +521,5 @@
> +      *aIndex = mCurrentStreamVolumeTbl[AUDIO_STREAM_ALARM];
> +      break;
> +    case AUDIO_CHANNEL_TELEPHONY:
> +      *aIndex = mCurrentStreamVolumeTbl[AUDIO_STREAM_VOICE_CALL];
> +      break;

Same comment about argument checking applies here.

Comment 28

[Marco Chen [:mchen]]

(In reply to Michael Wu [:mwu] from comment #26)
> > 10. I should use MOZ_WIDGET_GONK to protect the AudioManager in
> > AudioChannelService.
> > 
> 
> Well the entire directory is Gonk only, so I don't think you need to check.
> 

The location of AudioChannelService is under "dom/audiochannel" not under "dom/system/gonk" so I think it should be protected by MOZ_WIDGET_GONK. right?

> > 11. For JB part of SetStreamVolumeIndex(), I set volume index into all
> I'm not actually sure about this part, but we can do a follow up later if we
> need.

Ok, I will remove this from "Part3 patch" and fire another bug for this requirement. Thanks.

Comment 29

[Michael Wu [:mwu]]

(In reply to Marco Chen [:mchen] from comment #28)
> > > 11. For JB part of SetStreamVolumeIndex(), I set volume index into all
> > I'm not actually sure about this part, but we can do a follow up later if we
> > need.
> 
> Ok, I will remove this from "Part3 patch" and fire another bug for this
> requirement. Thanks.

You can keep it - I think it's better than what we have now. It just might not be what we want in the future.

Comment 30

[Michael Wu [:mwu]]

(In reply to Marco Chen [:mchen] from comment #28)
> (In reply to Michael Wu [:mwu] from comment #26)
> > > 10. I should use MOZ_WIDGET_GONK to protect the AudioManager in
> > > AudioChannelService.
> > > 
> > 
> > Well the entire directory is Gonk only, so I don't think you need to check.
> > 
> 
> The location of AudioChannelService is under "dom/audiochannel" not under
> "dom/system/gonk" so I think it should be protected by MOZ_WIDGET_GONK.
> right?
> 

Ahh, I'm looking at the wrong file. That makes sense.

Comment 31

[Marco Chen [:mchen]]

To collect what I should do in next version.

1. Remove AudioManager::RecoveryVolumeIndexToBackend() and replace by calling AudioManager::Set/GetStreamVolumeIndex(). (comment 26)

2. All great suggestions in comment 27 will be addressed into next version.

3. Add protection by MOZ_WIDGET_GONK to AudioChannelService using AudioManager (comment 28)

-------------

Follow up:
  1. expose volume setting as int or float types.
  2. expose Web API for volume conrol on each AudioChannel and other special audio stream (ex: bt_sco, DTMF)

Comment 32

[Marco Chen [:mchen]]

Attachment: Part 3: Control Volume based on AudioChannelType v2

Comment 33

[Marco Chen [:mchen]]

Comment on attachment 761958 [details] [diff] [review]
Part 3: Control Volume based on AudioChannelType v2

Address the review comment collected in comment 31.

Comment 34

[Michael Wu [:mwu]]

Comment on attachment 761958 [details] [diff] [review]
Part 3: Control Volume based on AudioChannelType v2

Review of attachment 761958 [details] [diff] [review]:
-----------------------------------------------------------------

Requesting additional review from baku for the audio channel code since I'm not as familiar with that code.

::: dom/system/gonk/AudioManager.cpp
@@ +65,5 @@
>  {
>  public:
>    RecoverTask() {}
>    NS_IMETHODIMP Run() {
> +    nsCOMPtr<nsIAudioManager> nsIAM = do_GetService(NS_AUDIOMANAGER_CONTRACTID);

nsIAM also sounds like an interface name according to our naming conventions, so I don't think it's a good variable name. Can you directly cast to AudioManager *? You'll probably need a nsRefPtr or RefPtr to hold on to it.

@@ +475,5 @@
> +    case AUDIO_CHANNEL_ALARM:
> +      stream[0] = AUDIO_STREAM_ALARM;
> +      break;
> +    case AUDIO_CHANNEL_TELEPHONY:
> +      stream[0] = AUDIO_STREAM_VOICE_CALL;

I think we can call SetStreamVolumeIndex directly here and avoid the complication of the for loop below.

@@ +499,5 @@
>  }
>  
>  NS_IMETHODIMP
> +AudioManager::GetAudioChannelVolume(int32_t aChannel, int32_t* aIndex) {
> +  if (aIndex == nullptr) {

!aIndex is the standard way to check for null, IIRC. Same comment applies to the next function.

@@ +500,5 @@
>  
>  NS_IMETHODIMP
> +AudioManager::GetAudioChannelVolume(int32_t aChannel, int32_t* aIndex) {
> +  if (aIndex == nullptr) {
> +    return NS_ERROR_NULL_POINTER;

There's a NS_ENSURE_ARG_POINTER for these sort of checks. Some people consider those sort of checks (macros that can return) bad, but we're already using them elsewhere here. I don't mind either way - just pointing this out.

@@ +562,5 @@
> +
> +uint32_t
> +AudioManager::SetStreamVolumeIndex(int32_t aStream, int32_t aIndex) {
> +  if (aIndex < 0 || aIndex > sMaxStreamVolumeTbl[aStream]) {
> +    return NS_ERROR_INVALID_ARG;

This function isn't returning nsresult though. I'm not sure what it's returning, actually. It's returning both Android and Mozilla error codes. Can you pick one?

@@ +607,5 @@
> +  return status;
> +#endif
> +}
> +
> +uint32_t

This function returns nsresult error codes.

Comment 35

[Marco Chen [:mchen]]

Attachment: Part 3: Control Volume based on AudioChannelType v3

Comment 36

[Marco Chen [:mchen]]

Comment on attachment 763312 [details] [diff] [review]
Part 3: Control Volume based on AudioChannelType v3

(In reply to Michael Wu [:mwu] from comment #34)
> nsIAM also sounds like an interface name according to our naming
> conventions, so I don't think it's a good variable name. Can you directly
> cast to AudioManager *? You'll probably need a nsRefPtr or RefPtr to hold on
> to it.
> 

According to there are some magic between do_GetService() and nsCOMPtr::operator= for checking & getting component's instance, I can't directly cast it from do_GetService(). So I still keep the original code but change the variable name. Is this ok?

> I think we can call SetStreamVolumeIndex directly here and avoid the
> complication of the for loop below.
> 

Follow it.

> !aIndex is the standard way to check for null, IIRC. Same comment applies to
> the next function.
> 

Follow it.

> There's a NS_ENSURE_ARG_POINTER for these sort of checks. Some people
> consider those sort of checks (macros that can return) bad, but we're
> already using them elsewhere here. I don't mind either way - just pointing
> this out.
> 
To keep this the same.

> This function isn't returning nsresult though. I'm not sure what it's
> returning, actually. It's returning both Android and Mozilla error codes.
> Can you pick one?

To unify the returning code by Mozilla error codes.

Thanks.

Comment 37

[Andrea Marchesini [:baku]]

Comment on attachment 763312 [details] [diff] [review]
Part 3: Control Volume based on AudioChannelType v3

Review of attachment 763312 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/AudioManager.cpp
@@ +70,5 @@
> +    NS_ENSURE_TRUE(amService, NS_OK);
> +    AudioManager *am = static_cast<AudioManager *>(amService.get());
> +    for (int loop = 0; loop < AUDIO_STREAM_CNT; loop++) {
> +      AudioSystem::initStreamVolume(static_cast<audio_stream_type_t>(loop), 0,
> +                                   sMaxStreamVolumeTbl[loop]);

indentation

Comment 38

[Andrea Marchesini [:baku]]

Comment on attachment 763312 [details] [diff] [review]
Part 3: Control Volume based on AudioChannelType v3

Review of attachment 763312 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know if the new management of JSObject and value is landed on b2g18. But for sure this is needed in m-c.
http://mxr.mozilla.org/mozilla-central/source/js/public/RootingAPI.h has reasonable good documentation.

r- just for this. Can you send a new version? Thanks!

::: dom/audiochannel/AudioChannelService.cpp
@@ +532,5 @@
> +  // change of settings
> +  else if (!strcmp(aTopic, "mozsettings-changed")) {
> +    AutoSafeJSContext cx;
> +    nsDependentString dataStr(aData);
> +    JS::Value val;

I think this cannot done. You should write something like:

JS::Rooted<JS::Value> val(cx);
if (JS_ParserJSON(...., val.address()) {

@@ +540,3 @@
>      }
>  
> +    JSObject& obj(val.toObject());

same here.

JS::Rooted<JSObject>...

@@ +540,4 @@
>      }
>  
> +    JSObject& obj(val.toObject());
> +    JS::Value key;

and here

::: dom/system/gonk/AudioManager.cpp
@@ +219,5 @@
> +  // change of settings
> +  else if (!strcmp(aTopic, "mozsettings-changed")) {
> +    AutoSafeJSContext cx;
> +    nsDependentString dataStr(aData);
> +    JS::Value val;

same problem here.

Comment 39

[Michael Wu [:mwu]]

Comment on attachment 763312 [details] [diff] [review]
Part 3: Control Volume based on AudioChannelType v3

Review of attachment 763312 [details] [diff] [review]:
-----------------------------------------------------------------

Looks close enough to me for my side of the review.

::: dom/system/gonk/AudioManager.cpp
@@ +482,5 @@
>    }
> +
> +  // sync FMRadio's volume with content channel.
> +  if (aChannel == AUDIO_CHANNEL_CONTENT && IsDeviceOn(AUDIO_DEVICE_OUT_FM)) {
> +    status += SetStreamVolumeIndex(AUDIO_STREAM_FM, aIndex);

I think this would look better if you put it in the switch statement.

@@ +520,5 @@
>  }
>  
>  NS_IMETHODIMP
> +AudioManager::GetMaxAudioChannelVolume(int32_t aChannel, int32_t* aMaxIndex) {
> +  if (aMaxIndex == nullptr) {

!aMaxIndex

@@ +550,5 @@
> +  *aMaxIndex = sMaxStreamVolumeTbl[stream];
> +   return NS_OK;
> +}
> +
> +uint32_t

Now that we're using Mozilla error codes, nsresult would be an appropriate return type.

@@ +575,5 @@
> +  if (device != 0) {
> +    return AudioSystem::setStreamVolumeIndex(
> +              static_cast<audio_stream_type_t>(aStream),
> +              aIndex,
> +              device);

This is still returning an Android error code.

Comment 40

[Michael Wu [:mwu]]

(In reply to Marco Chen [:mchen] from comment #36)
> Comment on attachment 763312 [details] [diff] [review]
> Part 3: Control Volume based on AudioChannelType v3
> 
> (In reply to Michael Wu [:mwu] from comment #34)
> > nsIAM also sounds like an interface name according to our naming
> > conventions, so I don't think it's a good variable name. Can you directly
> > cast to AudioManager *? You'll probably need a nsRefPtr or RefPtr to hold on
> > to it.
> > 
> 
> According to there are some magic between do_GetService() and
> nsCOMPtr::operator= for checking & getting component's instance, I can't
> directly cast it from do_GetService(). So I still keep the original code but
> change the variable name. Is this ok?
> 

Sure.

I think the right thing here is to provide a dedicated function to grab an AudioManager singleton which would avoid all the overhead of do_GetService(). However, this code is far from performance sensitive, so we can ignore that.

Comment 41

[Marco Chen [:mchen]]

Attachment: Part 3: Control Volume based on AudioChannelType v4

1. Address the issue from comment 39. :mwu
2. Address the issue from comment 38. :baku

Hi Andrea,

Since the last review comment from you is about JS::Rooted could you help to review that part or please help to transfer to other expert?
Thanks.

Comment 42

[Andrea Marchesini [:baku]]

Comment on attachment 764633 [details] [diff] [review]
Part 3: Control Volume based on AudioChannelType v4

Review of attachment 764633 [details] [diff] [review]:
-----------------------------------------------------------------

it seems correct. Can you see if it works and or leaks?

::: dom/audiochannel/AudioChannelService.cpp
@@ +548,2 @@
>  
> +    JSString *jsKey = JS_ValueToString(cx, key);

JS::Rooted<> ?

::: dom/system/gonk/AudioManager.cpp
@@ +232,5 @@
> +        !key.isString()) {
> +      return NS_OK;
> +    }
> +
> +    JSString *jsKey = JS_ValueToString(cx, key);

JS::Rooted<> or JS::RootedString

Comment 43

[Marco Chen [:mchen]]

Attachment: Part 3: Control Volume based on AudioChannelType v5

1. To use JS::RootedString.

2. I tested on unagi device and at least the function is ok.
   But I don't test the leakage.

Comment 44

[Michael Wu [:mwu]]

baku, will you be able to look at this soon?

Comment 45

[Andrea Marchesini [:baku]]

Comment on attachment 764677 [details] [diff] [review]
Part 3: Control Volume based on AudioChannelType v5

Review of attachment 764677 [details] [diff] [review]:
-----------------------------------------------------------------

change the UUID of nsIAudioManager.idl

::: dom/system/gonk/AudioManager.cpp
@@ +70,5 @@
> +    NS_ENSURE_TRUE(amService, NS_OK);
> +    AudioManager *am = static_cast<AudioManager *>(amService.get());
> +    for (int loop = 0; loop < AUDIO_STREAM_CNT; loop++) {
> +      AudioSystem::initStreamVolume(static_cast<audio_stream_type_t>(loop), 0,
> +                                   sMaxStreamVolumeTbl[loop]);

indent.

@@ +562,5 @@
> +  mCurrentStreamVolumeTbl[aStream] = aIndex;
> +  status_t status;
> +#if ANDROID_VERSION < 17
> +  status = AudioSystem::setStreamVolumeIndex(
> +             static_cast<audio_stream_type_t>(aStream),

nit: Would be nice to avoid all these static_cast and do it just once.

Comment 46

[Marco Chen [:mchen]]

https://tbpl.mozilla.org/?tree=Try&rev=7c4fdb319bac

Comment 47

[Marco Chen [:mchen]]

Attachment: Part 1: Remove SetMaster/FmVolume from AudioManager. Check-in Version v2

Rebase the patch.

Comment 48

[Marco Chen [:mchen]]

Attachment: Part 2: Remove BT Temporary Patch Check-in Version v2

Rebase the patch.

Comment 49

[Marco Chen [:mchen]]

Attachment: Part 3: Control Volume based on AudioChannelType Check-in Version v2

1. Rebase the patch.
2. Add reviewer's name.
3. Fix nit of indent.

Comment 50

[Marco Chen [:mchen]]

Attachment: Part 4: Remove supporting of GB. Check-in Version v2

1. Rebase the image.
2. Remove unused function for gb routing.

Comment 51

[Michael Wu [:mwu]]

Marco, I tried building your patches on gonk-jb. Looks like there's some issues -

../../../../gecko/dom/system/gonk/AudioManager.cpp: In member function 'virtual nsresult mozilla::dom::gonk::AudioManager::SetAudioChannelVolume(int32_t, int32_t)':
../../../../gecko/dom/system/gonk/AudioManager.cpp:460:63: error: cannot convert 'nsresult {aka tag_nsresult}' to 'android::status_t {aka int}' in assignment
../../../../gecko/dom/system/gonk/AudioManager.cpp:461:65: error: no match for 'operator+=' in 'status += mozilla::dom::gonk::AudioManager::SetStreamVolumeIndex(1, aIndex)'
../../../../gecko/dom/system/gonk/AudioManager.cpp:464:63: error: no match for 'operator+=' in 'status += mozilla::dom::gonk::AudioManager::SetStreamVolumeIndex(10, aIndex)'
../../../../gecko/dom/system/gonk/AudioManager.cpp:468:70: error: cannot convert 'nsresult {aka tag_nsresult}' to 'android::status_t {aka int}' in assignment
../../../../gecko/dom/system/gonk/AudioManager.cpp:469:63: error: no match for 'operator+=' in 'status += mozilla::dom::gonk::AudioManager::SetStreamVolumeIndex(2, aIndex)'
../../../../gecko/dom/system/gonk/AudioManager.cpp:472:63: error: cannot convert 'nsresult {aka tag_nsresult}' to 'android::status_t {aka int}' in assignment
../../../../gecko/dom/system/gonk/AudioManager.cpp:475:68: error: cannot convert 'nsresult {aka tag_nsresult}' to 'android::status_t {aka int}' in assignment

Comment 52

[Marco Chen [:mchen]]

Attachment: Part 1: Remove SetMaster/FmVolume from AudioManager. Check-in Version v3

rebase again.

Comment 53

[Marco Chen [:mchen]]

Attachment: Part 2: Remove BT Temporary Patch Check-in Version v3

Comment 54

[Marco Chen [:mchen]]

Attachment: Part 3: Control Volume based on AudioChannelType Check-in Version v3

To fix the build error on Gonk-JB version.
  1. To modify 
     nsresult G/SetStreamVolumeIndex(int32_t aStream, int32_t aIndex);
     to
     status_t G/SetStreamVolumeIndex(int32_t aStream, int32_t aIndex);

Comment 55

[Marco Chen [:mchen]]

Attachment: Part 4: Remove supporting of GB. Check-in Version v3

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/e7cf6294b265
https://hg.mozilla.org/projects/birch/rev/a78b04ca3f1f
https://hg.mozilla.org/projects/birch/rev/c676015224e6
https://hg.mozilla.org/projects/birch/rev/b3a7072e98f4

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e7cf6294b265
https://hg.mozilla.org/mozilla-central/rev/a78b04ca3f1f
https://hg.mozilla.org/mozilla-central/rev/c676015224e6
https://hg.mozilla.org/mozilla-central/rev/b3a7072e98f4

Comment 58

[:gerard-majax]

It looks like this triggered a regression, confere bug 906296