Closed Bug 876334 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mchen, Assigned: mchen)

References

Details

Attachments

(4 files, 17 obsolete files)

7.28 KB, patch
mchen
: review+
Details | Diff | Splinter Review
1.34 KB, patch
mchen
: review+
Details | Diff | Splinter Review
24.99 KB, patch
mchen
: review+
Details | Diff | Splinter Review
5.57 KB, patch
mchen
: review+
Details | Diff | Splinter Review
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.
Attached patch Patch - v1 (obsolete) — Splinter Review
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.
Attachment #757338 - Flags: feedback?(rlin)
Attachment #757338 - Attachment is patch: true
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.
Attachment #757338 - Flags: feedback?(rlin) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #757338 - Attachment is obsolete: true
Attachment #757882 - Flags: review?(mwu)
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.
If you don't mind, can you split the patches? There's a lot going on here.
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.
Attachment #757882 - Attachment is obsolete: true
Attachment #757882 - Flags: review?(mwu)
Attachment #758495 - Flags: review?(mwu)
6. Remove the hack code made for forcing BT_SCO during voice call.
   That is fixed by new BT Web API.
Attachment #758496 - Flags: review?(mwu)
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.
Attachment #758499 - Flags: review?(mwu)
8. Remove code for GB because FxOS doesn't support GB any more.
Attachment #758500 - Flags: review?(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)
Attachment #758495 - Flags: review?(mwu) → review+
Attachment #758496 - Flags: review?(mwu) → review+
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.
Attachment #758500 - Flags: review?(mwu) → review+
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.
(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?
(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.
(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.
1. Change the comment from maximum value to default value.
2. Add reviewer's name into commit comment.
Attachment #758495 - Attachment is obsolete: true
Attachment #759034 - Flags: review+
Add reviewer's name into commit comment.
Attachment #758496 - Attachment is obsolete: true
Attachment #759036 - Flags: review+
1. Address reviewer's comment.
2. Add reviewer's name into commit message.
Attachment #758500 - Attachment is obsolete: true
Attachment #759042 - Flags: review+
(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.
(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.
(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..
(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?
BT_SCO audio path can be used for voice recognition.
And on msm7k platform, there is no bt_sco + speaker audio path.
(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)
(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.
(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 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.
(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.
(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.
(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.
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)
Attachment #758499 - Flags: review?(mwu)
Attachment #758499 - Attachment is obsolete: true
Attachment #761958 - Attachment is patch: true
Comment on attachment 761958 [details] [diff] [review]
Part 3: Control Volume based on AudioChannelType v2

Address the review comment collected in comment 31.
Attachment #761958 - Flags: review?(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.
Attachment #761958 - Flags: review?(amarchesini)
Attachment #761958 - Attachment is obsolete: true
Attachment #761958 - Flags: review?(mwu)
Attachment #761958 - Flags: review?(amarchesini)
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.
Attachment #763312 - Flags: review?(mwu)
Attachment #763312 - Flags: review?(amarchesini)
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 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.
Attachment #763312 - Flags: review?(amarchesini) → review-
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.
Attachment #763312 - Flags: review?(mwu) → review+
(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.
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.
Attachment #763312 - Attachment is obsolete: true
Attachment #764633 - Flags: review?(amarchesini)
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
Attachment #764633 - Flags: review?(amarchesini)
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.
Attachment #764633 - Attachment is obsolete: true
Attachment #764677 - Flags: review?(amarchesini)
baku, will you be able to look at this soon?
Flags: needinfo?(amarchesini)
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.
Attachment #764677 - Flags: review?(amarchesini) → review+
Rebase the patch.
Attachment #759034 - Attachment is obsolete: true
Attachment #777027 - Flags: review+
Rebase the patch.
Attachment #759036 - Attachment is obsolete: true
Attachment #777028 - Flags: review+
1. Rebase the patch.
2. Add reviewer's name.
3. Fix nit of indent.
Attachment #764677 - Attachment is obsolete: true
Attachment #777029 - Flags: review+
1. Rebase the image.
2. Remove unused function for gb routing.
Attachment #759042 - Attachment is obsolete: true
Attachment #777030 - Attachment is patch: true
Attachment #777030 - Flags: review+
Flags: needinfo?(amarchesini)
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
rebase again.
Attachment #777027 - Attachment is obsolete: true
Attachment #777604 - Flags: review+
Attachment #777028 - Attachment is obsolete: true
Attachment #777605 - Flags: review+
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);
Attachment #777029 - Attachment is obsolete: true
Attachment #777606 - Flags: review?
Attachment #777030 - Attachment is obsolete: true
Attachment #777608 - Flags: review+
Attachment #777606 - Flags: review? → review+
Keywords: checkin-needed
Depends on: 895822
It looks like this triggered a regression, confere bug 906296
Blocks: 904531
You need to log in before you can comment on or make changes to this bug.