Andrea Marchesini [:baku] Assignee
	Description • 12 years ago

      No description provided.

	Justin Lebar (not reading bugmail)
	Comment 2 • 12 years ago

Comment on attachment 738568 [details] [diff] [review]
patch

I never really understood these manifest files; Kyle, can you vouch that these changes are sane?

	Justin Lebar (not reading bugmail)
	Comment 3 • 12 years ago

Comment on attachment 738568 [details] [diff] [review]
patch

Pin, can you please let us know if this patch looks good to you?

The goal here is to move the AudioChannel business that the FM radio has to do out of the parent process and into the child process.  This is good because it's a more correct usage of the API.  It also is necessary for changes we're planning where, in order to use the AudioChannel API, you need a window (which means that the old way of doing things, where the FM radio interacted with the API from the parent process, wouldn't work).

	Justin Lebar (not reading bugmail)
	Comment 4 • 12 years ago

This looks very good to me, but I'd like to get Pin's sign-off here too, in
case I'm missing something.

You've tested this on a device, right?

>diff --git a/dom/fm/DOMFMRadioChild.js b/dom/fm/DOMFMRadioChild.js
>--- a/dom/fm/DOMFMRadioChild.js
>+++ b/dom/fm/DOMFMRadioChild.js

>   enable: function nsIDOMFMRadio_enable(frequency) {
>-    // FMRadio::Enable() needs the most recent visibility state
>-    // synchronously.
>+    if (!this._audioChannelAgent) {
>+      this._audioChannelAgent = Cc["@mozilla.org/audiochannelagent;1"]
>+                               .createInstance(Ci.nsIAudioChannelAgent);
>+      this._audioChannelAgent.init(Ci.nsIAudioChannelAgent.AUDIO_AGENT_CHANNEL_CONTENT, this);

Please wrap lines at 80 chars.

>+  // Restore fm volume, that value is sync as music type
>+  int32_t volIdx = 0;
>+  audioManager->GetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_MUSIC, &volIdx);
>+  audioManager->SetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_FM, volIdx);

I know it's not your original comment, but I think a better way of saying this
would be "Sync the FM radio's volume with the old music volume".

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 5 • 12 years ago

Comment on attachment 738568 [details] [diff] [review]
patch

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

::: browser/installer/removed-files.in
@@ +1197,5 @@
>    components/dom_geolocation.xpt
>    components/dom_devicestorage.xpt
>    components/dom_html.xpt
>    components/dom_json.xpt
> +  components/dom_audiochannel.xpt

There shouldn't be a need for this.

::: mobile/xul/installer/package-manifest.in
@@ +179,5 @@
>  @BINPATH@/components/dom_indexeddb.xpt
>  @BINPATH@/components/dom_offline.xpt
>  @BINPATH@/components/dom_browserelement.xpt
>  @BINPATH@/components/dom_json.xpt
> +@BINPATH@/components/dom_audiochannel.xpt

mobile/xul is gone.

	Pin Zhang [:pzhang] (inactive)
	Comment 6 • 12 years ago

Comment on attachment 738568 [details] [diff] [review]
patch

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

Actually, I am not farmiliar with the codes related to Audio Manager/Policy, I checked that Randy wrote these, ask Randy for additional review here.

::: dom/fm/DOMFMRadioChild.js
@@ +320,5 @@
>    },
>  
>    disable: function nsIDOMFMRadio_disable() {
> +    if (!this._haveEnabledRadio) {
> +      return;

I don't think we should return here, consider such a scene:
  - Supposed there are two FM Radio apps, say NO.1 and NO.2, in Mike's phone
  - Mike starts NO.1
  - Mike then open the NO.2 fm app
  - Mike hit the `stop` button of NO.2 to stop the FM.
Then `disable` will do nothing, i.e. the FM can't be stopped, right?

In the same scene, if Mike really stopped the NO.2, i.e. the FM HW is stopped, then what happened to the audio channel of NO.1?

@@ +349,1 @@
>      this._haveEnabledRadio = true;

I do think we should rename the var, say `_haveRadioChannelEnabled`, because  we can't assert the radio has been enabled before we send `enable` message to parent, the FM Radio might fail to start (e.g. bug 862672).

	Randy Lin [:rlin] - Be Mozillian on 2015/01/26
	Comment 7 • 12 years ago

Comment on attachment 738568 [details] [diff] [review]
patch

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

::: dom/fm/FMRadio.cpp
@@ +140,5 @@
> +
> +  // Restore fm volume, that value is sync as music type
> +  int32_t volIdx = 0;
> +  audioManager->GetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_MUSIC, &volIdx);
> +  audioManager->SetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_FM, volIdx);

audioManager->SetFmRadioAudioEnabled(true) has synchronized volume value from music, so please remove it.

@@ +157,5 @@
>    nsCOMPtr<nsIAudioManager> audioManager =
>      do_GetService(NS_AUDIOMANAGER_CONTRACTID);
>    NS_ENSURE_TRUE(audioManager, NS_OK);
>  
> +  audioManager->SetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_FM, 0);

remove it.

	Andrea Marchesini [:baku] Assignee
	Comment 8 • 12 years ago

> I don't think we should return here, consider such a scene:
>   - Supposed there are two FM Radio apps, say NO.1 and NO.2, in Mike's phone
>   - Mike starts NO.1
>   - Mike then open the NO.2 fm app
>   - Mike hit the `stop` button of NO.2 to stop the FM.
> Then `disable` will do nothing, i.e. the FM can't be stopped, right?
> In the same scene, if Mike really stopped the NO.2, i.e. the FM HW is
> stopped, then what happened to the audio channel of NO.1?

Once the NO.2 fm app starts, the NO.1 fm app receives a canPlayChanged(false) call,
then NO.1 fm app calls disable() and the FMRadio is stopped.

Unfortunately the current FM API does not cover this use-case properly.
But this bug should be fixed in a follow up.

	Pin Zhang [:pzhang] (inactive)
	Comment 10 • 12 years ago

(In reply to Andrea Marchesini (:baku) from comment #8)
> > I don't think we should return here, consider such a scene:
> >   - Supposed there are two FM Radio apps, say NO.1 and NO.2, in Mike's phone
> >   - Mike starts NO.1
> >   - Mike then open the NO.2 fm app
> >   - Mike hit the `stop` button of NO.2 to stop the FM.
> > Then `disable` will do nothing, i.e. the FM can't be stopped, right?
> > In the same scene, if Mike really stopped the NO.2, i.e. the FM HW is
> > stopped, then what happened to the audio channel of NO.1?
> 
> Once the NO.2 fm app starts, the NO.1 fm app receives a
> canPlayChanged(false) call,
> then NO.1 fm app calls disable() and the FMRadio is stopped.
> 
So `mozFMRadio.enable()` called by the NO.2 app will fail, and the end user will hit the play button again to make it work, right? 
And does it mean there will always be only one FM app could play at a time?

	Andrea Marchesini [:baku] Assignee
	Comment 11 • 12 years ago

> So `mozFMRadio.enable()` called by the NO.2 app will fail, and the end user
> will hit the play button again to make it work, right? 

Unfortunately yes. The second click will make the radio work.

> And does it mean there will always be only one FM app could play at a time?

There is only 1 FMRadio backend and it's not possible to play 2 "frequency" at the same time.

	Justin Lebar (not reading bugmail)
	Comment 12 • 12 years ago

I need to work on other blockers urgently this week, so I will probably not be responsive here until those are fixed.  Sorry!

	Pin Zhang [:pzhang] (inactive)
	Comment 13 • 12 years ago

(In reply to Andrea Marchesini (:baku) from comment #11)
> > So `mozFMRadio.enable()` called by the NO.2 app will fail, and the end user
> > will hit the play button again to make it work, right? 
> 
> Unfortunately yes. The second click will make the radio work.
> 
It's kind of weird to me ...

> > And does it mean there will always be only one FM app could play at a time?
> 
> There is only 1 FMRadio backend and it's not possible to play 2 "frequency"
> at the same time.
Yes, but if we change the frequency in one FM app, and the other will receive the `onfrequencychange` event and get a chance to update the displayed frequency number, and that is exactly what the FM app did in the Gaia.

	Andrea Marchesini [:baku] Assignee
	Comment 14 • 12 years ago

(In reply to Pin Zhang [:pzhang] from comment #13)
> (In reply to Andrea Marchesini (:baku) from comment #11)
> > > So `mozFMRadio.enable()` called by the NO.2 app will fail, and the end user
> > > will hit the play button again to make it work, right? 
> > 
> > Unfortunately yes. The second click will make the radio work.
> > 
> It's kind of weird to me ...

This is actually hard to test. I have to implement a second app just for this test.
If you don't mind I prefer to do this in a separated bug, since this behaviour is untouched by this patch.

> 
> > > And does it mean there will always be only one FM app could play at a time?
> > 
> > There is only 1 FMRadio backend and it's not possible to play 2 "frequency"
> > at the same time.
> Yes, but if we change the frequency in one FM app, and the other will
> receive the `onfrequencychange` event and get a chance to update the
> displayed frequency number, and that is exactly what the FM app did in the
> Gaia.

This still works.

	Pin Zhang [:pzhang] (inactive)
	Comment 15 • 12 years ago

(In reply to Andrea Marchesini (:baku) from comment #14)
> (In reply to Pin Zhang [:pzhang] from comment #13)
> > (In reply to Andrea Marchesini (:baku) from comment #11)
> > > > So `mozFMRadio.enable()` called by the NO.2 app will fail, and the end user
> > > > will hit the play button again to make it work, right? 
> > > 
> > > Unfortunately yes. The second click will make the radio work.
> > > 
> > It's kind of weird to me ...
> 
> This is actually hard to test. I have to implement a second app just for
> this test.
You can just simply copy $GAIA/apps/fm folder, say $GAIA/test_apps/fm2, to generate another FM app.

> If you don't mind I prefer to do this in a separated bug, since this
> behaviour is untouched by this patch.
> 
I don't know how urgent is this bug and how hard to fix the two-fm-app issue, if it's not so urgent, I prefer to fix it.

@Jonas, what do you think? Justin is too busy right now, and I think we need someone who know the schedule better to make a judge.

	Marco Chen [:mchen]
	Comment 16 • 12 years ago

(In reply to Pin Zhang [:pzhang] from comment #15)
> I don't know how urgent is this bug and how hard to fix the two-fm-app
> issue, if it's not so urgent, I prefer to fix it.
> 
> @Jonas, what do you think? Justin is too busy right now, and I think we need
> someone who know the schedule better to make a judge.

According bug 852455 did some works related to this bug, so I moved the conclusion to here. And if all you agree to do these actions in this bug, I will set duplicate status to 852455. Because it is related to introduce new FM Web APIs, it will be suitable by DOM developer.

Issues considered from bug 852455,

  1. How to prevent more then one app to access FM HW in the same time? (the same with comment 15 here.)
  2. How to recover FM HW's status, if App granted to use FM is crashed?
  3. How does App not granted to use FM HW have a way to know FM HW is available now?
  4. How does App grated to use FM HW know FM audio is muted by AudioChannel? (then it can consider to disable FM for power saving)

Following the Pin's comment, do we want to fix them all since there is no schedule on this?

	Marco Chen [:mchen]
	Comment 17 • 12 years ago

By the way, 
  1. I didn't see any code flow in this patch for "muting" the FM when AudioChannelAgent returns the result of not to play. That is what we did in original code of parent side.

> // nsIAudioChannelAgentCallback
> canPlayChanged: function(canPlay) {
>   if (!this._haveRadioChannelEnabled || canPlay) {
>     return;
>   }
>		 
>  this.disable();
>  }		
   But just a disable() here, do we want to replace the mute to disable in this patch? Even though where is the resume code flow when AudioChannelAgent returns FM can be played?

> if (!this._audioChannelAgent.startPlaying()) {
>  var req = this.createRequest();
>  Services.DOMRequest.fireError(request, "Audio not allowed");
>  return req;
> }

  2. If we directly return error to app here, do we also need to call AudioChannelAgent.StopPlaying() too? Or app will see fail of enabling FM but that AudioChannelAgent still occupy the channel.

	Andrea Marchesini [:baku] Assignee
	Comment 18 • 12 years ago

> According bug 852455 did some works related to this bug, so I moved the
> conclusion to here. And if all you agree to do these actions in this bug, I
> will set duplicate status to 852455. Because it is related to introduce new
> FM Web APIs, it will be suitable by DOM developer.

I think bug 852455 should be based on this bug. This is needed by the new AudioChannel API.

>   1. How to prevent more then one app to access FM HW in the same time? (the
> same with comment 15 here.)
>   2. How to recover FM HW's status, if App granted to use FM is crashed?
>   3. How does App not granted to use FM HW have a way to know FM HW is
> available now?
>   4. How does App grated to use FM HW know FM audio is muted by
> AudioChannel? (then it can consider to disable FM for power saving)

I agree, there are still open issues with the FM Radio, but the reason of this bug and this patch was just to have the window object in the AudioChannelAgent.

	Andrea Marchesini [:baku] Assignee
	Comment 19 • 12 years ago

>   1. I didn't see any code flow in this patch for "muting" the FM when
> AudioChannelAgent returns the result of not to play. That is what we did in
> original code of parent side.

Right. Muting was broken in the previous code. 2 apps mute each other in the original code.
Because the visibility was propagated to the parent object without any lock system, or stack of apps, etc.

Once we'll have a lock system for this API, or a better solution, we can implement the mute.

>   2. If we directly return error to app here, do we also need to call
> AudioChannelAgent.StopPlaying() too? Or app will see fail of enabling FM but
> that AudioChannelAgent still occupy the channel.

Yep. new patch is coming.

	Randy Lin [:rlin] - Be Mozillian on 2015/01/26
	Comment 20 • 12 years ago

Comment on attachment 740232 [details] [diff] [review]
patch

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

Hi :baku, 
It seems fmradio would change to write in c++,
BTW, the audioManager related code seems fine to me.

	Pin Zhang [:pzhang] (inactive)
	Comment 21 • 12 years ago

(In reply to Pin Zhang [:pzhang] from comment #15)
> (In reply to Andrea Marchesini (:baku) from comment #14)
> > (In reply to Pin Zhang [:pzhang] from comment #13)
> > > (In reply to Andrea Marchesini (:baku) from comment #11)
> > > > > So `mozFMRadio.enable()` called by the NO.2 app will fail, and the end user
> > > > > will hit the play button again to make it work, right? 
> > > > 
> > > > Unfortunately yes. The second click will make the radio work.
> > > > 
> > > It's kind of weird to me ...
> > 
> > This is actually hard to test. I have to implement a second app just for
> > this test.
> You can just simply copy $GAIA/apps/fm folder, say $GAIA/test_apps/fm2, to
> generate another FM app.
> 
> > If you don't mind I prefer to do this in a separated bug, since this
> > behaviour is untouched by this patch.
> > 
> I don't know how urgent is this bug and how hard to fix the two-fm-app
> issue, if it's not so urgent, I prefer to fix it.

@Justin, what do you think?

	Justin Lebar (not reading bugmail)
	Comment 22 • 12 years ago

> @Justin, what do you think?

I don't care either way.  :)

In case of ties, we usually defer to the reviewer.

	Pin Zhang [:pzhang] (inactive)
	Comment 23 • 12 years ago

(In reply to Justin Lebar [:jlebar] from comment #22)
> > @Justin, what do you think?
> 
> I don't care either way.  :)
> 
> In case of ties, we usually defer to the reviewer.

:baku, there is no tracking flags which means it's not prioritized, and the two-fm-app issue is introduced by the patch, I think we need to fix it if it's not too hard, 

Can you estimate how much work it would cost? then we can make a decision.

	Andrea Marchesini [:baku] Assignee
	Comment 24 • 11 years ago

> :baku, there is no tracking flags which means it's not prioritized, and the
> two-fm-app issue is introduced by the patch, I think we need to fix it if
> it's not too hard, 
> 
> Can you estimate how much work it would cost? then we can make a decision.

sorry for this late answer. I don't think this issue is introduced by this patch. I would like to fix this problem in a separated bug and then base my patch on that new one. Makes sense?

	Marco Chen [:mchen]
	Comment 26 • 11 years ago

Since this blocks Bug 876631 (and some bugs also depend on it), may I know any plan to keep this patch going? Thanks.

	Andrea Marchesini [:baku] Assignee
	Comment 27 • 11 years ago

This patch can be just ignored. I want to see the FMRadio implemented in C++.
Then I'll write this patch on this new code.

	Randy Lin [:rlin] - Be Mozillian on 2015/01/26
	Comment 28 • 11 years ago

Hi Baku, 
Do you have plan to implement this one?

	Andrea Marchesini [:baku] Assignee
	Comment 29 • 11 years ago

(In reply to Randy Lin [:rlin] from comment #28)
> Hi Baku, 
> Do you have plan to implement this one?

Yes, I'll take it. Next week I'll work on this.

	Marco Chen [:mchen]
	Comment 31 • 11 years ago

Ask for ko+.

If this is landed into V1.2 then FM Radio didn't join audio competing policy. So it will be playing with music/video/phone/camera recording in the same time.

	Preeti Raghunath(:Preeti)
	Comment 32 • 11 years ago

Marco

Is this a new feature that is broken? OR is it a regression issue.

Please provide justification for blocker, moving to koi? till then.

	Marco Chen [:mchen]
	Comment 33 • 11 years ago

(In reply to Preeti Raghunath(:Preeti) from comment #32)
This is a regression issue not new feature.

The work of rewriting FMRadio from JS to C++ didn't add AudioChannel into consideration.
And that is originally added into old FMRadio in JS version.

	Preeti Raghunath(:Preeti)
	Comment 34 • 11 years ago

Thanks Marco.

We've missed adding AudioChannel in C++. koi+

	Preeti Raghunath(:Preeti)
	Comment 35 • 11 years ago

Baku

ANy progress on this bug fix?

	Andrea Marchesini [:baku] Assignee
	Comment 36 • 11 years ago

I think I'll have something ready in the next 2 days.

	Andrea Marchesini [:baku] Assignee
	Comment 37 • 11 years ago

FMRadio doesn't have any concept about process, childID or Agent.
This patch brings the FMRadio C++ implementation at the same level of the JS one.

For 1.3 we can change the IPDL protocol so that the FMRadioService knows which app is visible. I'll file a follow up.

https://tbpl.mozilla.org/?tree=Try&rev=4d2fdc68450d

	Marco Chen [:mchen]
	Comment 39 • 11 years ago

Comment on attachment 824049 [details] [diff] [review]
fmradio.patch

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

Hi Andrea,

One question for this patch.

1. AudioChannelAgent::StartPlaying() is called in the FMRadio::Enable() and the status of enabling will be reported by another runnable event dispatched to main thread.
2. Once the enabling is failed, the web content may not call FMRadio::Disable() later.

This will cause AudioChannelAgent always on the playing state and not be unlocked.

	Andrea Marchesini [:baku] Assignee
	Comment 40 • 11 years ago

Good point! I moved Start/StopPlaying() in the Notify().

	Marco Chen [:mchen]
	Comment 41 • 11 years ago

Comment on attachment 824594 [details] [diff] [review]
fmradio.patch

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

According to there are some discussion out of AudioChannel issue, ask for Kyle's suggestion. Thanks.

::: dom/fmradio/FMRadio.cpp
@@ +345,5 @@
> +
> +void
> +FMRadio::SetCanPlay(bool aCanPlay)
> +{
> +  nsCOMPtr<nsIAudioManager> audioManager =

The AudioManager now is only created on parent process as a singleton service so it can't be created here.
And content process should not have permission to change the audio routing patch.

Maybe we need an additional IPC call here for asking mute/unmute FMRadio.

@@ +363,3 @@
>  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(FMRadio)
>    NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
> +  NS_INTERFACE_MAP_ENTRY(nsIAudioChannelAgentCallback)

Do we need to add this into CC list?
Because we already use weak reference between them so there is no circular reference.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 43 • 11 years ago

Comment on attachment 826129 [details] [diff] [review]
fmradio.patch

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

::: dom/fmradio/FMRadio.cpp
@@ +120,5 @@
> +  target->AddSystemEventListener(NS_LITERAL_STRING("visibilitychange"), this,
> +                                 /* useCapture = */ true,
> +                                 /* wantsUntrusted = */ false);
> +
> +  mAudioChannelAgent = new AudioChannelAgent();

Other code (HTMLAudio/MediaElement) use do_CreateInstance with the contract id.  Why not here?

	Andrea Marchesini [:baku] Assignee
	Comment 45 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=719758abca4e

I changed that to do_CreateInterface.

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/319c8d1250f8

	Marco Chen [:mchen]
	Comment 48 • 11 years ago

(In reply to Marco Chen [:mchen] from comment #41)
> @@ +363,3 @@
> >  NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(FMRadio)
> >    NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference)
> > +  NS_INTERFACE_MAP_ENTRY(nsIAudioChannelAgentCallback)
> 
> Do we need to add this into CC list?
> Because we already use weak reference between them so there is no circular
> reference.

I am totally misunderstanding the function of CC and comment above is a wrong question.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 49 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/319c8d1250f8

	Jason Smith [:jsmith]
	Comment 50 • 11 years ago

Note for verifying this bug - see the dupes for test cases you can use to verify this bug.

	Ryan VanderMeulen [:RyanVM]
	Comment 51 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/93a04a7cd547

	Montoya Clemmons (MClemmons)
	Comment 52 • 11 years ago

Created https://bugzilla.mozilla.org/show_bug.cgi?id=937302 as issue from resolved duplicate bug https://bugzilla.mozilla.org/show_bug.cgi?id=934540 is reproducing intermittently.

	Montoya Clemmons (MClemmons)
	Comment 53 • 11 years ago

Resolved Duplicate bugs attached to this case:

https://bugzilla.mozilla.org/show_bug.cgi?id=914387 verified fixed confirmed after 5 successful test attempts

https://bugzilla.mozilla.org/show_bug.cgi?id=932528 verified fixed confirmed after 5 successful test attempts