Closed Bug 815069 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: mchen, Assigned: mchen)

References

Details

Attachments

(6 files, 15 obsolete files)

2.13 MB, application/pdf
Details
4.19 KB, patch
mchen
: review+
Details | Diff | Splinter Review
9.83 KB, patch
mchen
: review+
Details | Diff | Splinter Review
16.58 KB, patch
mchen
: review+
Details | Diff | Splinter Review
16.79 KB, patch
mchen
: review+
Details | Diff | Splinter Review
16.85 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 811649
Bug is being asked to be worked on by one of our partners.  Nom'ing.
blocking-basecamp: --- → ?
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.
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?
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/
(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?"
I'm confused then. Why are you proposing that anyone should read anything from /system or /sdcard? What problem are you trying to solve?
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.
blocking-basecamp: ? → -
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.
(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?
(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!
(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!
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.
So should we change summary of this bug to be about "internal gecko components" joining the policy, rather than "apps" doing it?
Summary: [Audio] A mechanism for Apps without media element to join audio competing policy → [Audio] A mechanism for Gecko components without media element to join audio competing policy
Attached file Proposal (obsolete) —
The proposal about using audiochannelcallback class to instead of nsHTMLMediaElement class holds by audiochannelservicechild.
Attached file 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.
Attachment #686050 - Attachment is obsolete: true
Attachment #686509 - Flags: feedback?(roc)
Attachment #686509 - Flags: feedback?(jonas)
Attachment #686509 - Flags: feedback?(amarchesini)
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.
Attachment #686509 - Flags: feedback?(roc) → feedback+
(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.
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?
Assignee: nobody → mchen
Priority: -- → P1
Target Milestone: --- → B2G C2 (20nov-10dec)
Comment on attachment 686509 [details]
Proposal V2

I'll leave this up to Roc and Andrea
Attachment #686509 - Flags: feedback?(jonas)
Attachment #686509 - Flags: feedback?(amarchesini)
Attached patch Part1: IDL WIPv1 (obsolete) — Splinter Review
1. Follow the comments from Robert.

API flow:
blowFuseOfChannelType() -> canPlay() (optional) -> registerAudioChannelAgentCallback() -> startPlaying() -> callback() -> ... -> stopPlaying() -> unregisterAudioChannelAgentCallback()
Attachment #687709 - Flags: review?(roc)
Attachment #687709 - Flags: feedback?(amarchesini)
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
Attached patch Part1: IDL v2 (obsolete) — Splinter Review
Following the comment from Robert and thanks for the review.
Attachment #687709 - Attachment is obsolete: true
Attachment #687709 - Flags: review?(roc)
Attachment #687709 - Flags: feedback?(amarchesini)
Attachment #688107 - Flags: review?(roc)
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.
(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.
(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.
Attached patch Part1: IDL v3 (obsolete) — Splinter Review
1. Following the comment.
2. Modify the constant of channel type to prefix by agent.
Attachment #688107 - Attachment is obsolete: true
Attachment #688107 - Flags: review?(roc)
Attachment #688219 - Flags: review?(roc)
Attached patch Part 2: Impl WIPv1 (obsolete) — Splinter Review
Thanks for the feedback in advance.
Attachment #688220 - Flags: feedback?(roc)
Attached patch Part1: IDL v3 (obsolete) — Splinter Review
Remove spaces.
Attachment #688219 - Attachment is obsolete: true
Attachment #688219 - Flags: review?(roc)
Attachment #688285 - Flags: review?(roc)
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 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
Attachment #688220 - Flags: feedback?(roc) → feedback+
Attached patch Part 1: IDL v4 (obsolete) — Splinter Review
1. Following the revised comment.
2. Move register of callback function into init() and remove register/unregister callback functions.
Attachment #688285 - Attachment is obsolete: true
Attachment #688285 - Flags: review?(roc)
Attachment #688638 - Flags: review?(roc)
Attached patch Diff Between IDL v3 & v4 (obsolete) — Splinter Review
This is a diff between IDL v3 & v4 for reviewer to easy identify what modified.
Attached patch Part 2: Impl v2 (obsolete) — Splinter Review
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
Attachment #688220 - Attachment is obsolete: true
Attachment #688655 - Flags: review?(roc)
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.
Flags: needinfo?(amarchesini)
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.
(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.
blocking-basecamp: - → ?
Yeah, we need this given that this is how we'll hook up FMRadio and camera. So this needs to block
Blocks: 815452
blocking-basecamp: ? → +
Attached patch Part 2: Impl v3 (obsolete) — Splinter Review
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.
Attachment #688655 - Attachment is obsolete: true
Attachment #688655 - Flags: review?(roc)
Attachment #688714 - Flags: review?(roc)
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.
Attachment #688728 - Flags: feedback?(amarchesini)
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 .
Attachment #688638 - Flags: review?(roc) → review+
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.
Attachment #688728 - Attachment is obsolete: true
Attachment #688728 - Flags: feedback?(amarchesini)
Attachment #689117 - Flags: review?(roc)
Attachment #689117 - Flags: feedback?(amarchesini)
Attachment #688638 - Attachment is obsolete: true
Attachment #689126 - Flags: review+
Attached patch Part 2: Impl Checkin Version (obsolete) — Splinter Review
Attachment #688641 - Attachment is obsolete: true
Attachment #689128 - Flags: review+
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
Attachment #689117 - Flags: review?(roc) → review+
Attached patch Part 2: Impl Checkin Version v2 (obsolete) — Splinter Review
Fixing compile error on OsX and Windows.
  1. Let deconstructor of audio channel agent as Virtual.
  2. Remove gonk log function.
Attachment #689128 - Attachment is obsolete: true
Attachment #689168 - Flags: review+
Attachment #688714 - Attachment is obsolete: true
Attachment #689117 - Attachment is obsolete: true
Attachment #689117 - Flags: feedback?(amarchesini)
Attachment #689170 - Flags: review+
Can you please wait for Bug 818708 or rebase on top of it?
Flags: needinfo?(amarchesini)
(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.
Remove unused header and class forwarding.
Attachment #689168 - Attachment is obsolete: true
Attachment #689193 - Flags: review+
ok! the patch looks good.
Fix indent.
Attachment #689170 - Attachment is obsolete: true
Attachment #689203 - Flags: review+
According to nsHTMLMediaElement.cpp, here also apply the part 3 patch for aurora.
Attachment #689204 - Flags: review+
1. Please landing bug 815322 first. Here depends on that.
2. for patch 3, here also provide the version for aurora.
Keywords: checkin-needed
Change

+      mAudioChannelAgent->Init( mAudioChannelType, this);

to

+      mAudioChannelAgent->Init( static<int32_t>(mAudioChannelType), static<nsIAudioChannelAgentCallback*>(this));
NB: marking RESO FIXED when these bugs hit beta because it's getting hard for b2g folks to track remaining work for upcoming milestones.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 820069
See Also: → 878699
You need to log in before you can comment on or make changes to this bug.