815069 - [Audio] A mechanism for Gecko components without media element to join audio competing policy

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Marco Chen [:mchen]]

The Apps like video/audio recorder, FM, VoIP communicator and speech reorganizing who didn't have media elements need to join the audio competing policy too.
But refer to the bug 805333, it adjudged the policy by monitoring media elements. So we need a mechanism to let Apps above join policy.

Comment 1

[Chris Lee [:clee]]

Bug is being asked to be worked on by one of our partners.  Nom'ing.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Just to clarify, this bug is about applications that aren't playing audio themselves, but still need to silence audio coming from other apps.

In particular the camera app needs to silence a background music app as to avoid getting the background music recorded when recording a video.

Similarly software that is voice controlled wants to silence background audio playing in order to not get interference from this audio.

So far all the use cases *seem* to be related to enabling the microphone and wanting to silence other audio, but I'm not sure that is always the case.


A potential workaround for this bug is to use an <audio mozaudiochannel="..."> element which just plays a silent audio clip. It's unclear what disadvantages that has though, but it might be good enough for v1.

Comment 3

[Marco Chen [:mchen]]

I remembered that DeviceStorageAPI just allow Web App to access /sdcard folder.
If we put this special silent file into /sdcard/XXX then there is a risk of deleting by user.

In order to prevent this, we can only choose to put file into system images.
And any method can let web app play the file in /system folder?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How big is a silent .wav file? I would imagine just a handful [1] of kB big. So doesn't seem like a problem to put a copy if that file in each app that needs to do this silencing if we decide to go with that workaround.

[1] http://xkcd.com/1070/

Comment 5

[Marco Chen [:mchen]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> How big is a silent .wav file? I would imagine just a handful [1] of kB big.
> So doesn't seem like a problem to put a copy if that file in each app that
> needs to do this silencing if we decide to go with that workaround.
> 
> [1] http://xkcd.com/1070/

What I concern here is not about the file size?
The problem is that DeviceStorageAPI didn't allow Web App to access folder of /system but just /sdcard. "So the concern here is is there any Web API for accessing folder of /system?"

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm confused then. Why are you proposing that anyone should read anything from /system or /sdcard? What problem are you trying to solve?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Minusing this for two reasons:

1. I'm not convinced that it's a blocker that background audio automatically silences when you start recording. The user can always switch to the music app and pause the music.

2. There is a workaround possible by playing a silent audio file.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems to me that whatever Gecko APIs we use to play audio output, or capture audio input, should have the ability to specify a channel type for the output (or for input, a channel type indicating that lower-priority channels should be silenced). So, our FM radio API, getUserMedia, etc, should have this ability.

Comment 9

[Marco Chen [:mchen]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> It seems to me that whatever Gecko APIs we use to play audio output, or
> capture audio input, should have the ability to specify a channel type for
> the output (or for input, a channel type indicating that lower-priority
> channels should be silenced). So, our FM radio API, getUserMedia, etc,
> should have this ability.

So maybe we doesn't need to expose a new Web API for some Apps to ask for joining audio competing policy. Instead of that, we can expose an API from AudioChannelService for some Gecko components to join. Ex: In video recording, camera manager should be responsible to join and leaving audio competing policy.

Then we doesn't need to hook services (Ex: Telephony, FM, Video Recording) into AudioChannelService. Is this possible?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Marco Chen [:mchen] from comment #9)
> So maybe we doesn't need to expose a new Web API for some Apps to ask for
> joining audio competing policy. Instead of that, we can expose an API from
> AudioChannelService for some Gecko components to join. Ex: In video
> recording, camera manager should be responsible to join and leaving audio
> competing policy.

That sounds good to me!

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> So, our FM radio API, getUserMedia, etc, should have this ability.

Hooking up FM radio to the channels backend is bug 815452. I don't believe we need a way to specify what channel the FM radio should use. I don't think there are any use cases for using any channel other than "content". At least not yet.

Likewise, telephony is covered by bug 815445.

(In reply to Marco Chen [:mchen] from comment #9)
> So maybe we doesn't need to expose a new Web API for some Apps to ask for
> joining audio competing policy. Instead of that, we can expose an API from
> AudioChannelService for some Gecko components to join. Ex: In video
> recording, camera manager should be responsible to join and leaving audio
> competing policy.

Ah, yes, we can simply hook up the video recording backend to the audio channel internals such that it always silences "notification" and lower? That would mean that no changes are needed on the application side.

> Then we doesn't need to hook services (Ex: Telephony, FM, Video Recording)
> into AudioChannelService. Is this possible?

We'd still have to make gecko changes to hook in FM, Telephony and video recording to the audio channels backend. But yes, it seems like we wouldn't need to do any changes to gaia if we took this approach. Sounds like a good solution to me!

Comment 12

[Marco Chen [:mchen]]

After bug 805333 is landed we need to extend the APIs from AudioChannelService so other gecko components can register/unregister (component -> AudioChannelService), notifyStart/stop (component -> AudioChannelService) and beNotified (AudioChannelService -> component).

Then each components who need to join can be done by themselves. And leave the flexibility to new components in the future but keep AudioChannelService fixed.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So should we change summary of this bug to be about "internal gecko components" joining the policy, rather than "apps" doing it?

Comment 14

[Marco Chen [:mchen]]

Attachment: Proposal

The proposal about using audiochannelcallback class to instead of nsHTMLMediaElement class holds by audiochannelservicechild.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That looks reasonable.

Comment 16

[Marco Chen [:mchen]]

Attachment: Proposal V2

Dear Robert,

Thanks for your kindly feedback on the proposal.

Dear all,

After discussing with Telephony (Hsin-Yi), FM (Steven) and Audio (Randy) collages, we propose a new XPCOM component called nsIAudioChannelAgent. Gecko components which want to join audio competing policy will create it as the agent to talk with AudioChannelService.

The main reason for this change is in order to let components implemented by JS or C++ can use it to join policy. The old mechanism just can be used by C++ components.

If all you agree it, then the next step is the review of idl file.

Thanks.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 686509 [details]
Proposal V2

Basic idea looks good.

> bool startToUseChannelType (AudioChannelType type);
> void stopToUseChannelType (AudioChannelType type);

Maybe the channel type should be an attribute of the nsIAudioChannelAgent that you can only set once? Since it wouldn't make sense to start with one type and stop with another.

Then call the methods "startPlaying" and "stopPlaying".

> bool getUsable (AudioChannelType type, bool hidden);

Call this "canPlay". I'm not sure what the "hidden" parameter means.

Comment 18

[Marco Chen [:mchen]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> Comment on attachment 686509 [details]
> Proposal V2
> Maybe the channel type should be an attribute of the nsIAudioChannelAgent
> that you can only set once? Since it wouldn't make sense to start with one
> type and stop with another.
> 

Good point, will take this consideration into nsIAudioChannelAgent.

> 
> > bool getUsable (AudioChannelType type, bool hidden);
> 
> Call this "canPlay". I'm not sure what the "hidden" parameter means.

For audio competing policy, the channels other then normal can be played in the background but normal not. So the hidden here will be combined with channel type to judge by audio channel service for whether it should keep to play or not.

Comment 19

[Alex Keybl [:akeybl]]

This blocks C2/P1 bug 811649, so moving this into C2 as well. Do we have an estimate for when we think this will be ready to land?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 686509 [details]
Proposal V2

I'll leave this up to Roc and Andrea

Comment 21

[Marco Chen [:mchen]]

Attachment: Part1: IDL WIPv1

1. Follow the comments from Robert.

API flow:
blowFuseOfChannelType() -> canPlay() (optional) -> registerAudioChannelAgentCallback() -> startPlaying() -> callback() -> ... -> stopPlaying() -> unregisterAudioChannelAgentCallback()

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 687709 [details] [diff] [review]
Part1: IDL WIPv1

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

::: dom/audiochannel/nsIAudioChannelAgent.idl
@@ +5,5 @@
> +[scriptable, uuid(35dddb4c-3d35-11e2-978c-10bf48d64bd4)]
> +interface nsIAudioChannelAgentCallback : nsISupports
> +{
> +  /**
> +   * Notified when a playable status of channel is changed .

when the playable status of a channel is changed

@@ +10,5 @@
> +   *
> +   * @param status
> +   *        Callback from agent for notifying component about playable status 
> +   *        of the channel. If status is false, component SHOULD stop to play
> +   *        media associated with this channel as soon as possible.

Call the parameter "aCanPlay"?

Why does agent->CanPlay require the "hidden" parameter but the callback function can be told whether it's playing or not without any "hidden" parameter being provided?

@@ +17,5 @@
> +};
> +
> +/**
> + * This interface is an agent for gecko components to join 
> + * audio channel service. Gecko compoents are responsible to tell agent about

components

@@ +19,5 @@
> +/**
> + * This interface is an agent for gecko components to join 
> + * audio channel service. Gecko compoents are responsible to tell agent about
> + *   1. What channel type is used and can blow it's fuse with one time only.
> + *   2. Before playing, activly to ask playable status of the channel.

actively

@@ +38,5 @@
> +  const long AUDIO_CHANNEL_TYPE_TELEPHONY   = 4;
> +  const long AUDIO_CHANNEL_TYPE_RINGER   = 5;
> +  const long AUDIO_CHANNEL_TYPE_PUBLICNOTIFICATION   = 6;
> +
> +  const long AUDIO_CHANNEL_TYPE_ERROR   = 1000;

If you're not going to align these declarations, just put one space before the =

@@ +48,5 @@
> +   *
> +   * @param channelType
> +   *    Audio Channel Type in audiochanneltype.h 
> +   */
> +  boolean blowFuseOfChannelType(in long channelType);

I don't think we should use the term "blow fuse" here. It's unclear. I don't actually know what you mean, so I can't suggest a better name :-)

@@ +52,5 @@
> +  boolean blowFuseOfChannelType(in long channelType);
> +
> +   /**
> +   * Notify the agent we start to play. If return false, component
> +   & SHOULD stop to play as soon as possible.

& should be *

@@ +61,5 @@
> +   */
> +  boolean startPlaying();
> +
> +   /**
> +   * Notify the agent we stop to play.

Fix alignment

@@ +63,5 @@
> +
> +   /**
> +   * Notify the agent we stop to play.
> +   *
> +   * Note that even the result of startPlaying() is false, agent still regsiter into

register

"even if the result of startPlaying is false"

@@ +64,5 @@
> +   /**
> +   * Notify the agent we stop to play.
> +   *
> +   * Note that even the result of startPlaying() is false, agent still regsiter into
> +   * audio channel service then callback while status change. So we also need to 

"then issue callbacks while the status changes"

@@ +70,5 @@
> +   */
> +  void stopPlaying();
> +
> +   /**
> +   * Before startPlaying(), actively call canPlay() to check whether this channel will 

Fix alignment

Comment 23

[Marco Chen [:mchen]]

Attachment: Part1: IDL v2

Following the comment from Robert and thanks for the review.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 688107 [details] [diff] [review]
Part1: IDL v2

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

::: dom/audiochannel/nsIAudioChannelAgent.idl
@@ +10,5 @@
> +   *
> +   * @param canPlay
> +   *        Callback from agent for notifying component about playable status 
> +   *        of the channel. If canPlay is false, component SHOULD stop to play
> +   *        media associated with this channel as soon as possible.

"Callback from agent to notify component of the playable status
of the channel. If canPlay is false, the component SHOULD stop playing
media associated with this channel as soon as possible."

@@ +21,5 @@
> + * audio channel service. Gecko components are responsible to tell agent about
> + *   1. What channel type is used via init() member function.
> + *   2. Before playing, actively to ask playable status of the channel.
> + *   3. Notify the timing of starting/stopping to use this channel.
> + *   4. Notify the status of visibility on element to use this channel.

This interface provides an agent for gecko components to
participate in the audio channel service. Gecko components are responsible
for
1. Indicating what channel type they are using (via the init() member function)
2. Before playing, checking the playable status of the channel
3. Notifying the agent when they start/stop using this channel
4. Notifying the agent of changes to the visibility of the component using this channel

@@ +24,5 @@
> + *   3. Notify the timing of starting/stopping to use this channel.
> + *   4. Notify the status of visibility on element to use this channel.
> + *
> + * Agent will do callback to notify Gecko components about
> + *   1. Playable status of this channel.

The agent will invoke a callback to notify Gecko components of
1. Changes to the playable status of this channel

@@ +38,5 @@
> +  const long AUDIO_CHANNEL_TYPE_TELEPHONY          = 4;
> +  const long AUDIO_CHANNEL_TYPE_RINGER             = 5;
> +  const long AUDIO_CHANNEL_TYPE_PUBLICNOTIFICATION = 6;
> +
> +  const long AUDIO_CHANNEL_TYPE_ERROR              = 1000;

What's this for?

@@ +40,5 @@
> +  const long AUDIO_CHANNEL_TYPE_PUBLICNOTIFICATION = 6;
> +
> +  const long AUDIO_CHANNEL_TYPE_ERROR              = 1000;
> +
> +  readonly attribute long audioChannelType;

This should probably return an error if it's read before init() is called. Document that.

@@ +49,5 @@
> +   *
> +   * @param channelType
> +   *    Audio Channel Type listed as above 
> +   */
> +  boolean init(in long channelType);

I don't think you need to return anything here. Just return void. If called more than once, throw an exception.

@@ +53,5 @@
> +  boolean init(in long channelType);
> +
> +  /**
> +   * Notify the agent we start to play. If return false, component
> +   * SHOULD stop to play as soon as possible.

If it returns false, then the component shouldn't even start playing. Right? Playing for a very short time and then stopping would be bad.

@@ +75,5 @@
> +   * Before startPlaying(), actively call canPlay() to check whether this channel will 
> +   * be stopped by another channels with higher priority in this moment. So maybe
> +   * component can choose to notify user by another way.
> +   */
> +  boolean canPlay();

Do we really need this or does checking the return value of startPlaying() cover all our needs?

I would worry a bit about race conditions. E.g. if the application calls canPlay(), which returns true, then calls startPlaying(), which returns false because something else started playing in between, the author might be confused and not handle that case properly. So if we can just make sure authors check the result of startPlaying(), I think that would be good.

@@ +85,5 @@
> +   */
> +  void setVisibilityState(in boolean hidden);
> +
> +  void registerAudioChannelAgentCallback(in nsIAudioChannelAgentCallback callback);
> +  void unregisterAudioChannelAgentCallback(in nsIAudioChannelAgentCallback callback);

Are multiple callbacks per agent supported? If not, please say so.

Comment 25

[Marco Chen [:mchen]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> Comment on attachment 688107 [details] [diff] [review]
> Part1: IDL v2
> 
> Review of attachment 688107 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/audiochannel/nsIAudioChannelAgent.idl
> @@ +38,5 @@
> > +  const long AUDIO_CHANNEL_TYPE_TELEPHONY          = 4;
> > +  const long AUDIO_CHANNEL_TYPE_RINGER             = 5;
> > +  const long AUDIO_CHANNEL_TYPE_PUBLICNOTIFICATION = 6;
> > +
> > +  const long AUDIO_CHANNEL_TYPE_ERROR              = 1000;
> 
> What's this for?

This is used to be initial value of mAudioChanelType inside AudioChannelAgent class, so we  allow to set type from this value to others only.

> @@ +53,5 @@
> > +  boolean init(in long channelType);
> > +
> > +  /**
> > +   * Notify the agent we start to play. If return false, component
> > +   * SHOULD stop to play as soon as possible.
> 
> If it returns false, then the component shouldn't even start playing. Right?
> Playing for a very short time and then stopping would be bad.
> 

Yes, will document that this should be called before really starting to play a audio stream.

> @@ +75,5 @@
> > +   * Before startPlaying(), actively call canPlay() to check whether this channel will 
> > +   * be stopped by another channels with higher priority in this moment. So maybe
> > +   * component can choose to notify user by another way.
> > +   */
> > +  boolean canPlay();
> 
> Do we really need this or does checking the return value of startPlaying()
> cover all our needs?
> 

The difference between canPlay() and startPlaying() is on whether agent registers into audio channel service. I leave it for someone will want to ask status but didn't want to join audio channel service. Now I can't figure out who is potentially to use it. So will remove it first.

> @@ +85,5 @@
> > +   */
> > +  void setVisibilityState(in boolean hidden);
> > +
> > +  void registerAudioChannelAgentCallback(in nsIAudioChannelAgentCallback callback);
> > +  void unregisterAudioChannelAgentCallback(in nsIAudioChannelAgentCallback callback);
> 
> Are multiple callbacks per agent supported? If not, please say so.

No, we didn't allow it and will document it. 

Thanks.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Marco Chen [:mchen] from comment #25)
> The difference between canPlay() and startPlaying() is on whether agent
> registers into audio channel service.

I see.

> I leave it for someone will want to
> ask status but didn't want to join audio channel service. Now I can't figure
> out who is potentially to use it. So will remove it first.

Yes, let's leave it out until we find a need for it.

Comment 27

[Marco Chen [:mchen]]

Attachment: Part1: IDL v3

1. Following the comment.
2. Modify the constant of channel type to prefix by agent.

Comment 28

[Marco Chen [:mchen]]

Attachment: Part 2: Impl WIPv1

Thanks for the feedback in advance.

Comment 29

[Marco Chen [:mchen]]

Attachment: Part1: IDL v3

Remove spaces.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 688285 [details] [diff] [review]
Part1: IDL v3

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

::: dom/audiochannel/nsIAudioChannelAgent.idl
@@ +44,5 @@
> +
> +  const long AUDIO_AGENT_CHANNEL_ERROR              = 1000;
> +
> +  /**
> +   * If reading this attribute before init() is called, then an error will be returned.

"Before init() is called, this returns AUDIO_AGENT_CHANNEL_ERROR."

@@ +49,5 @@
> +   */
> +  readonly attribute long audioChannelType;
> +
> +  /**
> +   * To initialize the agent with a channel type.

"Initialize the agent with a channel type."

@@ +50,5 @@
> +  readonly attribute long audioChannelType;
> +
> +  /**
> +   * To initialize the agent with a channel type.
> +   * Note: This function can just only be called by one time or will return error.

"Note: This function should only be called once."

@@ +58,5 @@
> +   */
> +  void init(in long channelType);
> +
> +  /**
> +   * Notify the agent we start to play.

"Notify the agent that we want to start playing."

@@ +63,5 @@
> +   * Note: Gecko component SHOULD call this function first then start to
> +   *          play audio stream only when return value is true.
> +   *
> +   * @return
> +   *    true: register into audio channel service and allow to play.

"true: the agent has registered with audio channel service and the component should start playback."

@@ +64,5 @@
> +   *          play audio stream only when return value is true.
> +   *
> +   * @return
> +   *    true: register into audio channel service and allow to play.
> +   *    false: register into audio channel service but didn't allow to play.

"false: the agent has registered with audio channel service but the component should not start playback."

@@ +75,5 @@
> +   * Note that even if the result of startPlaying() is false, agent still register into
> +   * audio channel service then issue callback while the status changes. So we also need to 
> +   * call stopPlaying() for agent to unregister from audio channel service.
> +   */
> +  void stopPlaying();

"Notify the agent we no longer want to play.

Note that even if startPlaying() returned false, the agent would still be
registered with the audio channel service and receive callbacks for status changes.
So stopPlaying must still eventually be called to unregister the agent with the channel service."

@@ +78,5 @@
> +   */
> +  void stopPlaying();
> +
> +  /**
> +   * Notify the agent about the visibility state of the window using this agent.

Notify the agent of the visibility state of the window using this agent.

@@ +82,5 @@
> +   * Notify the agent about the visibility state of the window using this agent.
> +   * @param hidden
> +   *    The window assocated with the agent is visible or not.
> +   */
> +  void setVisibilityState(in boolean hidden);

It would be better for this to be "visible" instead of "hidden", so that "setVisibilityState(true)" means "visible", which makes more sense to the reader. Then the comment would be

@param visible
  True if the window associated with the agent is visible.

@@ +90,5 @@
> +   * playable status changes.
> +   * Note: Here just allow one callback function to be registered.
> +   */
> +  void registerAudioChannelAgentCallback(in nsIAudioChannelAgentCallback callback);
> +  void unregisterAudioChannelAgentCallback(in nsIAudioChannelAgentCallback callback);

Could we just pass in the callback as an extra parameter to init() and not allow it to be unregistered? Is there a need to unregister the callback and/or register a new one?

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 688220 [details] [diff] [review]
Part 2: Impl WIPv1

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

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +30,5 @@
> +      return AUDIO_CHANNEL_TELEPHONY;
> +    case nsIAudioChannelAgent::AUDIO_AGENT_CHANNEL_RINGER:
> +      return AUDIO_CHANNEL_RINGER;
> +    case nsIAudioChannelAgent::AUDIO_AGENT_CHANNEL_PUBLICNOTIFICATION	:
> +      return AUDIO_CHANNEL_PUBLICNOTIFICATION;

How about making the values just match, and adding a bunch of MOZ_STATIC_ASSERTS that the values all match?

@@ +57,5 @@
> +/* readonly attribute long audioChannelType; */
> +NS_IMETHODIMP AudioChannelAgent::GetAudioChannelType(int32_t *aAudioChannelType)
> +{
> +  if (mAudioChannelType == AUDIO_AGENT_CHANNEL_ERROR) {
> +    return NS_ERROR_FAILURE;

We probably don't need this. Just return AUDIO_AGENT_CHANNEL_ERROR

@@ +69,5 @@
> +NS_IMETHODIMP AudioChannelAgent::Init(int32_t channelType)
> +{
> +  if (mAudioChannelType != AUDIO_AGENT_CHANNEL_ERROR ||
> +      channelType > AUDIO_AGENT_CHANNEL_PUBLICNOTIFICATION) {
> +    return NS_ERROR_FAILURE;

If you're going to check this, then check for negative values as well

::: dom/audiochannel/AudioChannelAgent.h
@@ +30,5 @@
> +  virtual void NotifyAudioChannelStateChanged();
> +
> +private:
> +  ~AudioChannelAgent();
> +  long mAudioChannelType;

int32_t, not long, for XPIDL longs

@@ +31,5 @@
> +
> +private:
> +  ~AudioChannelAgent();
> +  long mAudioChannelType;
> +  bool mVisibility;

Call this mVisible. (Right now it means "hidden", which is super confusing, but hopefully you'll take my suggestion to switch the parameter to visible.)

@@ +34,5 @@
> +  long mAudioChannelType;
> +  bool mVisibility;
> +  nsCOMPtr<nsIAudioChannelAgentCallback> mCallback;
> +  nsRefPtr<AudioChannelService> mAudioChannelService;
> +  bool mIsRegToService;

Order the fields from largest to smallest. Pointers first, then mAudioChannelType, then the bools

Comment 32

[Marco Chen [:mchen]]

Attachment: Part 1: IDL v4

1. Following the revised comment.
2. Move register of callback function into init() and remove register/unregister callback functions.

Comment 33

[Marco Chen [:mchen]]

Attachment: Diff Between IDL v3 & v4

This is a diff between IDL v3 & v4 for reviewer to easy identify what modified.

Comment 34

[Marco Chen [:mchen]]

Attachment: Part 2: Impl v2

1. Following the comments.
2. Remove register/unregister callback functions and move callback register into init().
3. In init(), I allow callback is null. Ex: telephony doesn't need to listen callback function for stopping itself.
3. Add stuffs into nsLayoutModule.cpp

Comment 35

[Marco Chen [:mchen]]

Hi all & Baku,

During implementation, I found one strange thing on AudioChannelService::GetAudioChannelService().

1. AudioChannelService::GetAudioChannelService() returns a pure pointer of AudioChannelService instance.
2. AudioChannelService holds a global variable "gAudioChannelService" to store a single instance then return to other from here. (seems to be a singleton pattern).
3. MediaElement uses nsRefPtr<> to retrieve a pure pointer of AudioChannelService.
   (ref count will be 1) .
4. After this media element is gone, ref count is reduced to 0 then this instance of AudioChannelService will be deleted.

Maybe my concern is wrong but want to check with someone else here.

Thanks.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marco: gAudioChannelService uses StaticRefPtr which means that as soon as the gAudioChannelService global is set, we will call addref on it, which will increase the refcount to 1. So when a HTMLMediaElement grabs a pointer to the AudioChannelService the refcount will go up to 2, and then back to 1 again once the HTMLMediaElement releases the service.

Comment 37

[Marco Chen [:mchen]]

(In reply to Robert O'Callahan (:roc) 
(Mozilla Corporation) from comment #8)
> It seems to me that whatever Gecko APIs we use to play audio output, or
> capture audio input, should have the ability to specify a channel type for
> the output (or for input, a channel type indicating that lower-priority
> channels should be silenced). So, our FM radio API, getUserMedia, etc,
> should have this ability.

Re-ask the blocking-basecamp and please refer to commen 8 as above.
The reason is that the component introduced here can be used by another one (ex: telephony, FM, Video recording) to join audio channel service.

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, we need this given that this is how we'll hook up FMRadio and camera. So this needs to block

Comment 39

[Marco Chen [:mchen]]

Attachment: Part 2: Impl v3

1. Inherited from v2 version.
2. Remove to use AudioChannelService in AudioChannelAgent.h. (so we don't need to include AudioChannelService.h) Or there is a circular include between AudioChannelAgent.h and AudioChannelService.h when I modify AudioChannelService codes.

Comment 40

[Marco Chen [:mchen]]

Attachment: Part 3: AudioChannelService & MediaElement

Hi Andrea,

Could you give some suggestions for this patch on working in progress especially the modification in nsHTMLMediaElement?

The most important thing is that nsIAudioChannelAgent::StartPlaying(bool canPlay) will return a value and let you know whether the stream should be truly played or not. So we need to find a proper point which is before the playing but after asking to play.

This tries to avoid a short sound from media element if there are another higher priority channels playing already.

Finally this audio channel agent will try to apply on FMRadio and Video Recording too.

thanks.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 688638 [details] [diff] [review]
Part 1: IDL v4

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

::: dom/audiochannel/nsIAudioChannelAgent.idl
@@ +7,5 @@
> +[scriptable, uuid(35dddb4c-3d35-11e2-978c-10bf48d64bd4)]
> +interface nsIAudioChannelAgentCallback : nsISupports
> +{
> +  /**
> +   * Notified when the playable status of channel is changed .

Remove space before .

Comment 42

[Marco Chen [:mchen]]

Attachment: Part 3: AudioChannelService & MediaElement

Dear Robert & Matthew

This part 3 is patch for adapting audio channel service & media element to audio channel agent we done here.
According to the reviewer of audio channel agent is you, I set this part of reviewer to you too. But maybe it is suitable to review by Matthew (reviewer of audio channel service and media element). Thanks.

Dear Andrea,

If you can have a feedback to me then it will be better too.
Because you are the author of them. Thanks.

Comment 43

[Marco Chen [:mchen]]

Attachment: Part 1: IDL Checkin Version

Comment 44

[Marco Chen [:mchen]]

Attachment: Part 2: Impl Checkin Version

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 689117 [details] [diff] [review]
Part 3: AudioChannelService & MediaElement

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

nice

::: dom/audiochannel/AudioChannelService.h
@@ +42,1 @@
>                                      AudioChannelType aType);

fix indent

::: dom/audiochannel/AudioChannelServiceChild.h
@@ +33,1 @@
>                                      AudioChannelType aType);

Fix indent

Comment 46

[Marco Chen [:mchen]]

Attachment: Part 2: Impl Checkin Version v2

Fixing compile error on OsX and Windows.
  1. Let deconstructor of audio channel agent as Virtual.
  2. Remove gonk log function.

Comment 47

[Marco Chen [:mchen]]

Attachment: Part 3: AudioChannelService & MediaElement Checkin Version

Comment 48

[Andrea Marchesini [:baku]]

Can you please wait for Bug 818708 or rebase on top of it?

Comment 49

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #48)
> Can you please wait for Bug 818708 or rebase on top of it?

Hi Andrea, 

It seems there is only a feedback? on 818708 but here is all done already.
And also it blocks the FM and video recording to join audio channel service.
So I would like to land them first. Sorry to make you more troubles.

Comment 50

[Marco Chen [:mchen]]

Attachment: Part 2: Impl Checkin Version v3

Remove unused header and class forwarding.

Comment 51

[Andrea Marchesini [:baku]]

ok! the patch looks good.

Comment 52

[Marco Chen [:mchen]]

Attachment: Part 3: AudioChannelService & MediaElement Checkin Version v2

Fix indent.

Comment 53

[Marco Chen [:mchen]]

Attachment: Part 3: AudioChannelService & MediaElement Checkin Version Aurora

According to nsHTMLMediaElement.cpp, here also apply the part 3 patch for aurora.

Comment 54

[Marco Chen [:mchen]]

1. Please landing bug 815322 first. Here depends on that.
2. for patch 3, here also provide the version for aurora.

Comment 55

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d02a596b470
https://hg.mozilla.org/integration/mozilla-inbound/rev/d88785a37287
https://hg.mozilla.org/integration/mozilla-inbound/rev/c82c7a4a7ca6

Comment 56

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://hg.mozilla.org/releases/mozilla-aurora/rev/18fd500e7ae6
https://hg.mozilla.org/releases/mozilla-aurora/rev/cc11426b1fda
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe20163490ec

https://hg.mozilla.org/releases/mozilla-beta/rev/28c55db82bbf
https://hg.mozilla.org/releases/mozilla-beta/rev/970644875986
https://hg.mozilla.org/releases/mozilla-beta/rev/f4306798bbba

Comment 57

[Marco Chen [:mchen]]

Attachment: Part 3: AudioChannelService & MediaElement Checkin Version Beta

Change

+      mAudioChannelAgent->Init( mAudioChannelType, this);

to

+      mAudioChannelAgent->Init( static<int32_t>(mAudioChannelType), static<nsIAudioChannelAgentCallback*>(this));

Comment 58

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

And bustage fix:
https://hg.mozilla.org/releases/mozilla-beta/rev/26f2167449c3

Comment 59

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

NB: marking RESO FIXED when these bugs hit beta because it's getting hard for b2g folks to track remaining work for upcoming milestones.

Comment 60

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2d02a596b470
https://hg.mozilla.org/mozilla-central/rev/d88785a37287
https://hg.mozilla.org/mozilla-central/rev/c82c7a4a7ca6