708446 - [Gonk] Audio system control for telephony

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): Implement nsIAudioManager to talk to audio subsystem

This adds an XPCOM shim around gonk's AudioSystem class to allow us to put the audio subsystem in the right kind of state for phone calls, and to control fancy features like muting the mic and setting system volume.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 579954 [details] [diff] [review]
Part 1 (v1): Implement nsIAudioManager to talk to audio subsystem

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

You are lacking the registration part, will need to see that too.

::: dom/system/b2g/Makefile.in
@@ +56,5 @@
>  
> +ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
> +CPPSRCS += \
> +  nsAudioManager.cpp \
> +  $(NULL)

If there's only one you can do single line, e.g. |CPPSRCS += nsAudioManager.cpp|

@@ +60,5 @@
> +  $(NULL)
> +endif
> +
> +XPIDLSRCS = \
> +  nsIAudioManager.idl \

Why build the IDL if not gonk?

::: dom/system/b2g/nsAudioManager.cpp
@@ +48,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsAudioManager::SetMicrophoneMuted(bool aMicrophoneMuted)

Nit, if you're going to prefix this arg with 'a' then you should do the others too. Or not. But be consistent ;)

::: dom/system/b2g/nsAudioManager.h
@@ +38,5 @@
> +#ifndef mozilla_dom_system_b2g_audiomanager_h__
> +#define mozilla_dom_system_b2g_audiomanager_h__
> +
> +#include "nsIAudioManager.h"
> +#include <media/AudioSystem.h>

Move this into the cpp file, it's not used here.

@@ +57,5 @@
> +
> +  nsAudioManager();
> +
> +protected:
> +  ~nsAudioManager();

This won't link, you don't have any implementation (same for your constructor). An empty (and inlined) implementation is probably fine since you don't seem to have any state or need to call any shutdown functions.

::: dom/system/b2g/nsIAudioManager.idl
@@ +36,5 @@
> + * ***** END LICENSE BLOCK ***** */
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(d2124467-7209-4b2e-a91a-cf3f90681e3c)]

Add 'builtinclass' here.

@@ +37,5 @@
> +
> +#include "nsISupports.idl"
> +
> +[scriptable, uuid(d2124467-7209-4b2e-a91a-cf3f90681e3c)]
> +interface nsIAudioManager : nsISupports {

Nit: { on next line.

@@ +52,5 @@
> +
> +  /**
> +   * Master volume muted?
> +   */
> +  attribute boolean masterMuted;

"speakerMuted"?

@@ +64,5 @@
> +  const long PHONE_STATE_RINGTONE         = 1;
> +  const long PHONE_STATE_IN_CALL          = 2;
> +  const long PHONE_STATE_IN_COMMUNICATION = 3;
> +
> +  void setPhoneState(in long state);

Do you want to be able to read this too? Maybe 'attribute long phoneState'?

::: toolkit/library/Makefile.in
@@ +418,5 @@
>  OS_LIBS += -lGLESv2
>  endif
>  
>  ifeq (gonk,$(MOZ_WIDGET_TOOLKIT))
> +OS_LIBS += -lui -lmedia

once you get more than one this needs the form:

  FOO += \
    foo1 \
    foo2 \
    $(NULL)

Comment 3

[Mounir Lamouri (:mounir)]

Comment on attachment 579954 [details] [diff] [review]
Part 1 (v1): Implement nsIAudioManager to talk to audio subsystem

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

::: dom/system/b2g/nsAudioManager.h
@@ +44,5 @@
> +
> +// {b2b51423-502d-4d77-89b3-7786b562b084}
> +#define NS_AUDIOMANAGER_CID {0x94f6fd70, 0x7615, 0x4af9, \
> +      {0x89, 0x10, 0xf9, 0x3c, 0x55, 0xe6, 0x62, 0xec}}
> +#define NS_AUDIOMANAGER_CONTRACTID "@mozilla.org/telephony/audiomanager;1"

You could put this in the idl file. I think it's better but it might be a a matter of taste.

::: dom/system/b2g/nsIAudioManager.idl
@@ +62,5 @@
> +  const long PHONE_STATE_CURRENT          = -1;
> +  const long PHONE_STATE_NORMAL           = 0;
> +  const long PHONE_STATE_RINGTONE         = 1;
> +  const long PHONE_STATE_IN_CALL          = 2;
> +  const long PHONE_STATE_IN_COMMUNICATION = 3;

Weren't we trying to get rid of int constants in favor of strings?
I guess it's depend on how much we want to be consistent wit new Web APIs?

@@ +81,5 @@
> +  const long FORCE_BT_SCO          = 3;
> +  const long FORCE_BT_A2DP         = 4;
> +  const long FORCE_WIRED_ACCESSORY = 5;
> +  const long FORCE_BT_CAR_DOCK     = 6;
> +  const long FORCE_BT_DESK_DOCK    = 7;

dito

@@ +86,5 @@
> +
> +  const long USE_COMMUNICATION     = 0;
> +  const long USE_MEDIA             = 1;
> +  const long USE_RECORD            = 2;
> +  const long USE_DOCK              = 3;

dito

Comment 4

[Philipp von Weitershausen [:philikon]]

Thanks for the quick turn around!


(In reply to ben turner [:bent] from comment #2)
> You are lacking the registration part, will need to see that too.

Oh right, totally forgot to include that.

> > +  $(NULL)
> > +endif
> > +
> > +XPIDLSRCS = \
> > +  nsIAudioManager.idl \
> 
> Why build the IDL if not gonk?

So that we can run the telephony code on desktop Firefox and debug it there. It won't look up the nsIAudioManager service, but we at least it won't error out on the IDL. Meh, I guess it doesn't make much of a difference.

> > +NS_IMETHODIMP
> > +nsAudioManager::SetMicrophoneMuted(bool aMicrophoneMuted)
> 
> Nit, if you're going to prefix this arg with 'a' then you should do the
> others too. Or not. But be consistent ;)

I thought there was different treatment for in and out parameters, but it seems there isn't. Will do.

> > +  nsAudioManager();
> > +
> > +protected:
> > +  ~nsAudioManager();
> 
> This won't link, you don't have any implementation (same for your
> constructor). An empty (and inlined) implementation is probably fine since
> you don't seem to have any state or need to call any shutdown functions.

Uh, right, forgot to do that too :)

> > +  /**
> > +   * Master volume muted?
> > +   */
> > +  attribute boolean masterMuted;
> 
> "speakerMuted"?

Master volume/muted is actually the phone's overall system volume. Setting masterMuted is the equivalent of putting phone into silent mode.

> > +  const long PHONE_STATE_RINGTONE         = 1;
> > +  const long PHONE_STATE_IN_CALL          = 2;
> > +  const long PHONE_STATE_IN_COMMUNICATION = 3;
> > +
> > +  void setPhoneState(in long state);
> 
> Do you want to be able to read this too? Maybe 'attribute long phoneState'?

Yeah, funny that. The audio subsystem only lets us set this property, not read it. It seems like you would prefer keeping that state locally so that we can expose this as an attribute. That's fine with me, once I figure out what a sensible initial state is ;)

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #3)
> > +// {b2b51423-502d-4d77-89b3-7786b562b084}
> > +#define NS_AUDIOMANAGER_CID {0x94f6fd70, 0x7615, 0x4af9, \
> > +      {0x89, 0x10, 0xf9, 0x3c, 0x55, 0xe6, 0x62, 0xec}}
> > +#define NS_AUDIOMANAGER_CONTRACTID "@mozilla.org/telephony/audiomanager;1"
> 
> You could put this in the idl file. I think it's better but it might be a a
> matter of taste.

I couldn't care less. I'll let Ben make the call here.

> @@ +62,5 @@
> > +  const long PHONE_STATE_CURRENT          = -1;
> > +  const long PHONE_STATE_NORMAL           = 0;
> > +  const long PHONE_STATE_RINGTONE         = 1;
> > +  const long PHONE_STATE_IN_CALL          = 2;
> > +  const long PHONE_STATE_IN_COMMUNICATION = 3;
> 
> Weren't we trying to get rid of int constants in favor of strings?
> I guess it's depend on how much we want to be consistent wit new Web APIs?

This isn't a DOM interface. It's an internal interface to talk directly to the metal, so user-friendliness doesn't matter so much. But that aside, these enum values are what the AudioSystem class expects. I didn't make these up. And I don't think we want to introduce strings just to convert them to one of the enum values before making the low level call...

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 579954 [details] [diff] [review]
Part 1 (v1): Implement nsIAudioManager to talk to audio subsystem

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

Also, Gecko coding style is:
if (foo) {
  bar;
}

You always need the brackets.

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #6)
> Also, Gecko coding style is:
> if (foo) {
>   bar;
> }
> 
> You always need the brackets.

Ah, good. I prefer it that way, too. I had seen so much C++ code not do it that I wasn't sure.

Comment 8

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2): Implement nsIAudioManager to talk to audio subsystem

Addressed bent's review comments and some of mounir's offline suggestions. Asking mrbkap for review since bent's on a plane right now...

Comment 9

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): Update audio state when call state changes

This puts the audio system in the (hopefully) correct states when calls are coming in or going out. Will add support for muting, speaker, etc. next.

Comment 10

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v1): Implement mute and speaker

Comment 11

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v3): Implement nsIAudioManager to talk to audio subsystem

Forgot to include the toolkit/library/Makefile.in change to link against gonk's libmedia.

Comment 12

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v2): Update audio state when call state changes

We also need to unmute/mute the radio when a call begins/ends.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 580316 [details] [diff] [review]
Part 1 (v2): Implement nsIAudioManager to talk to audio subsystem

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

::: dom/system/b2g/AudioManager.cpp
@@ +125,5 @@
> +
> +NS_IMETHODIMP
> +AudioManager::GetForceForUse(PRInt32 aUsage, PRInt32* aForce) {
> +  *((AudioSystem::forced_config*)aForce) =
> +    AudioSystem::getForceUse((AudioSystem::force_use)aUsage);

Is the cast on the left hand side here necessary? It's pretty ugly. If it is necessary, please use static_cast<> instead of a c-style cast.

::: dom/system/b2g/AudioManager.h
@@ +45,5 @@
> +#define NS_AUDIOMANAGER_CID {0x94f6fd70, 0x7615, 0x4af9, \
> +      {0x89, 0x10, 0xf9, 0x3c, 0x55, 0xe6, 0x62, 0xec}}
> +#define NS_AUDIOMANAGER_CONTRACTID "@mozilla.org/telephony/audiomanager;1"
> +
> +namespace mozilla { namespace dom { namespace b2g {

Nit: Namespace declarations like this should go on separate lines.

Non-nit: b2g isn't part of the DOM. It looks like this should go in telephony instead.

@@ +55,5 @@
> +  NS_DECL_NSIAUDIOMANAGER
> +
> +  AudioManager()
> +  {
> +    mPhoneState = PHONE_STATE_CURRENT;

Please use an initializer list for this.

@@ +63,5 @@
> +  PRInt32 mPhoneState;
> +};
> +
> +
> +} /* namespace b2g */ } /* namespace dom */ } /* namespace mozilla */

These should go on separate lines.

::: dom/system/b2g/nsIAudioManager.idl
@@ +39,5 @@
> +
> +[scriptable, builtinclass, uuid(d2124467-7209-4b2e-a91a-cf3f90681e3c)]
> +interface nsIAudioManager : nsISupports
> +{
> +

Nit: extra blank line here (and before the }).

@@ +73,5 @@
> +   *
> +   * No, really, this configures a particular device ("force") to be used
> +   * for one of the uses (communication, media playback, etc.)
> +   *
> +   * Don't you hate geeks?

This sort of comment doesn't belong here.

::: layout/build/nsLayoutModule.cpp
@@ +357,1 @@
>  #ifndef MOZ_WIDGET_GONK

This would look better as

#ifdef MOZ_WIDGET_GONK
...
#else
...
#endif

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 580338 [details] [diff] [review]
Part 2 (v2): Update audio state when call state changes

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

::: dom/telephony/nsTelephonyWorker.js
@@ +65,5 @@
> +  microphoneMuted: false,
> +  masterVolume: 1.0,
> +  masterMuted: false,
> +  phoneState: Ci.nsIAudioManager.PHONE_STATE_CURRENT,
> +  _forceForUse: {},

This could be an array, right?

@@ +78,5 @@
> +XPCOMUtils.defineLazyGetter(this, "gAudioManager", function getAudioManager() {
> +  try {
> +    return Cc["@mozilla.org/telephony/audiomanager;1"]
> +             .getService(Ci.nsIAudioManager);
> +  } catch (ex) {

On the phone, it seems like this should raise some sort of error instead of silently not turning on sound. For testing purposes, this seems fine.

@@ +179,5 @@
> +    // Update current state.
> +    if (message.callState == DOM_CALL_READYSTATE_DISCONNECTED) {
> +      delete currentCalls[message.callIndex];
> +    } else {
> +       currentCalls[message.callIndex] = message;

Nit: this line seems overindented.

Comment 15

[Philipp von Weitershausen [:philikon]]

(In reply to Blake Kaplan (:mrbkap) from comment #14)
> > +  microphoneMuted: false,
> > +  masterVolume: 1.0,
> > +  masterMuted: false,
> > +  phoneState: Ci.nsIAudioManager.PHONE_STATE_CURRENT,
> > +  _forceForUse: {},
> 
> This could be an array, right?

Well, it'd be pretty sparse, so I'd feel dirty using an array...

Comment 16

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v4): Implement nsIAudioManager to talk to audio subsystem

Addressed mrbkap's review comments.

Comment 17

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v3): Update audio state when call state changes

Addressed mrbkap's review comments.

This is good to go now. Will land once the tree opens again.

Comment 18

[Philipp von Weitershausen [:philikon]]

Got approval to land on m-c by khuey:

https://hg.mozilla.org/mozilla-central/rev/cf9b7ca68ef5
https://hg.mozilla.org/mozilla-central/rev/5b8987dcd1ff
https://hg.mozilla.org/mozilla-central/rev/34c34de2e964