Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Description • 12 years ago

Whenever the FMRadio API is used to turn on FM Radio audio, we should treat it as if it's part of the "content" audio channel. I.e. any other "content" audio that is playing in a background app should be silenced. And any audio that is playing on the "normal" audio channel should be mixed with the FM Radio audio.

Similarly, if a higher-privileged audio channel is used, the audio from the FM Radio should be muted. We obviously can't "pause" the audio since it's a live radio stream, but we can possibly stop feeding the audio into the audio subsystem. We could even turn off the FM receiver, though that's probably not worth doing immediately since many sounds, like notification sounds, can be quite short.

	Dietrich Ayala (:dietrich)
	Comment 1 • 12 years ago

Is there actual work here once bug 805333 lands? If so, who should own it?

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 2 • 12 years ago

Yes, there's additional work to do here. I don't have suggestions for who should do it, but it requires knowledge of how the audio handling for FM Radio works.

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

This would use the implementation of Bug 815069.

	Dietrich Ayala (:dietrich)
	Comment 4 • 12 years ago

Is there a patch needed for this, or will bug 815069 fix it entirely?

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 5 • 12 years ago

A patch is needed here.

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

This is base on audiochannelAgent patch.
May hit this.Bug 818708

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 7 • 12 years ago

Comment on attachment 689599 [details] [diff] [review]
methodology review

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

Some small comments below. You should probably ask someone that knows this code better to review though. jlebar did the initial review of this code, right?

::: dom/fm/FMRadio.cpp
@@ +119,5 @@
> +    mAudioChannelAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1", &rv);
> +    if (!mAudioChannelAgent) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    mAudioChannelAgent->Init((int32_t)(AUDIO_CHANNEL_CONTENT), (nsIAudioChannelAgentCallback*)(this));

You shouldn't need to cast like this.

And if you do need to cast, use explicit casts like static_cast or similar rather than c-style cast.

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

Don't you need to enable the fm-radio here too?

@@ +245,5 @@
> +    //Restore fm volume, that value is sync as music type
> +    audioManager->GetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_MUSIC, &volIdx);
> +    audioManager->SetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_FM, volIdx);
> +  } else {
> +    audioManager->SetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_FM, 0);

And disable it here.

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

I try to disable/enable FM in FMRadio but found 
1. The UI isn't sync with gonk layer.
2. Enable FM it on gonk layer but found no sound, still debugging.(In reply to Jonas Sicking (:sicking) from comment #7)
> Comment on attachment 689599 [details] [diff] [review]
> methodology review
> 
> Review of attachment 689599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some small comments below. You should probably ask someone that knows this
> code better to review though. jlebar did the initial review of this code,
> right?
> 
> ::: dom/fm/FMRadio.cpp
> @@ +119,5 @@
> > +    mAudioChannelAgent = do_CreateInstance("@mozilla.org/audiochannelagent;1", &rv);
> > +    if (!mAudioChannelAgent) {
> > +      return NS_ERROR_FAILURE;
> > +    }
> > +    mAudioChannelAgent->Init((int32_t)(AUDIO_CHANNEL_CONTENT), (nsIAudioChannelAgentCallback*)(this));
> 
> You shouldn't need to cast like this.
> 
> And if you do need to cast, use explicit casts like static_cast or similar
> rather than c-style cast.
> 
> @@ +243,5 @@
> > +  if (canPlay) {
> > +    int32_t volIdx = 0;
> > +    //Restore fm volume, that value is sync as music type
> > +    audioManager->GetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_MUSIC, &volIdx);
> > +    audioManager->SetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_FM, volIdx);
> 
> Don't you need to enable the fm-radio here too?
> 
> @@ +245,5 @@
> > +    //Restore fm volume, that value is sync as music type
> > +    audioManager->GetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_MUSIC, &volIdx);
> > +    audioManager->SetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_FM, volIdx);
> > +  } else {
> > +    audioManager->SetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_FM, 0);
> 
> And disable it here.

I try to disable/enable FM in FMRadio.cpp but found 
1. The UI isn't sync with gonk layer.
2. Enable FM it on gonk layer but found fm no sound, also under debugging. maybe hook in DOMFMRadioChild.js is better.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 9 • 12 years ago

The easiest way to make things work is probably to let the API act as if the radio was enabled, even when we disable the radio due to channel logic.

Potentially we could even fire the same mozinterruptbegin/mozinterruptend events as we fire on media elements. But that doesn't seem strictly needed.

I don't think that the UI needs to reflect that the audio is muted. That should be pretty obvious to the user.

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

For The canPlay callback part, 
When FM is turning on and disturb by other high channel, The FMRadio got this callback.
After FM turn-off, the agent would be destroied and no callbacks returns any more.
So If CanPlay = true, That means FM is on and he got the playback permission=>Restore the sound volume
False, Keep the FM radio in mute.

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

This patch use AudioChannelAgent to implement audio policy issue.

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

Comment on attachment 689643 [details] [diff] [review]
patch1

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

::: dom/fm/FMRadio.cpp
@@ +125,5 @@
> +  bool canPlay;
> +  mAudioChannelAgent->SetVisibilityState(true);
> +  mAudioChannelAgent->StartPlaying(&canPlay);
> +  if (!canPlay) {
> +    mAudioChannelAgent = nullptr;

Even the return value is false, this agent still registered to service already.
So please call StopPlaying to unregister from service.

@@ +248,5 @@
> +    audioManager->GetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_MUSIC, &volIdx);
> +    audioManager->SetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_FM, volIdx);
> +  } else {
> +    audioManager->SetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_FM, 0);
> +  }

I remembered that AudioManager hook the music volume index to fm.
Once FM is already muted by this code (music player is playing on the foreground now.), FM will be unmuted by user adjusting the volume from volume key.

::: dom/fm/FMRadio.h
@@ +16,1 @@
>  

AudioChannelService.h already included AudioChannelAgent.h.

	Andrea Marchesini [:baku]
	Comment 13 • 12 years ago

Comment on attachment 689643 [details] [diff] [review]
patch1

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

::: dom/fm/FMRadio.cpp
@@ +122,5 @@
> +    }
> +    mAudioChannelAgent->Init(AUDIO_CHANNEL_CONTENT, this);
> +  }
> +  bool canPlay;
> +  mAudioChannelAgent->SetVisibilityState(true);

visibility should be true only when the app is actually visible.

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

I'm happy to look at a r? of a patch with mchen and baku's comments addressed.

	Andrea Marchesini [:baku]
	Comment 15 • 12 years ago

Comment on attachment 689643 [details] [diff] [review]
patch1

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

feedback- just because of these 2 comments from me and Marco.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 16 • 12 years ago

Is someone in PST able to update this patch?

I think that I could, except for bug 819521 :(.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 17 • 12 years ago

The patch here doesn't fix bug 812691.  STR
 (1) Load FM radio app, tune station
 (2) Load music app, play audio file

Both tracks are output to headphones.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 18 • 12 years ago

Fixes the bug assigned here, but I don't think addresses all the previous review comments.  Looking at that next.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 19 • 12 years ago

Addresses the review comments except this from comment 12

> I remembered that AudioManager hook the music volume index to fm.
> Once FM is already muted by this code (music player is playing on the
> foreground now.), FM will be unmuted by user adjusting the volume from 
> volume key.

Randy, do you know how to fix this?

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

Comment on attachment 689643 [details] [diff] [review]
patch1

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

I try to test the FM app and found visibilitychange isn't fired..
Play fm first time, also found
FMRadio::GetHidden()
{
  nsCOMPtr<nsPIDOMWindow> window = GetOwner();
  bool hidden = false;
  LOG("enter");
  if (window) {
    LOG("window");  <--can't enter this block......when try to switch to music apps and back.

Is this cause by FM apps and fmradio locate in different process?

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

Maybe hook the agent in DOMFMRadioChild.js is another way to avoid the visibility change problem.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 22 • 12 years ago

Yes, it looks like the FM radio impl is a bit odd.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 23 • 12 years ago

Fixes the known issues, maybe except for comment 19.

Randy, do you if this patch addresses comment 19, and if not, how to address it?

I should note that with this patch FM radio just stops when the app goes to the background.  The same behavior happens with the music app.  So I suspect we're missing some gaia changes.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 25 • 12 years ago

With latest gaia+gecko+ this patch, here's what I see

Case 1
 1. Open music app, play a track
 2. Send music app to background: track continues playing
 3. Open FM radio app, tune channel: music app pauses, FM plays

Yay!

Case 2
 1. Open FM radio app, tune channel
 2. Send FM radio app to background: FM radio pauses [BROKEN]
 3. Load music app, play a track
 4. Send music app to background: track pauses [BROKEN]

Nooo!

I suspect this is related to comment 19.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 26 • 12 years ago

Removes a missed logging statement and the broken |mHidden| check from CanPlay().

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 28 • 12 years ago

Comment on attachment 690570 [details] [diff] [review]
avoid fm volume can be unmute by adjust volume key

Sorry, I don't understand this code very well.  Let's see if Jonas can grab this too.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 29 • 12 years ago

I don't understand how this patch is intended to work. I'll try to find you on irc tonight around 9-10pm Pacific time.

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

(In reply to Jonas Sicking (:sicking) from comment #29)
> I don't understand how this patch is intended to work. I'll try to find you
> on irc tonight around 9-10pm Pacific time.

For this patch, we want to allow FM volume only sync with music stream when the fm volume isn't mute by audiochannelServices (hacking, Only AudioChannelServices set fm stream type volume). Otherwise, the FM would be unmuted even the audiopolicy choose FM to be mute, but user would find fm is unmuted when he adjusts the context volume in settings.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 31 • 12 years ago

Comment on attachment 690548 [details] [diff] [review]
Hook up FM radio to the audio manager, v1.1

Oh jesus, I could kick myself :(.

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

Comment on attachment 690548 [details] [diff] [review]
Hook up FM radio to the audio manager, v1.1

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

This patch looks good to me.

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

Comment on attachment 690548 [details] [diff] [review]
Hook up FM radio to the audio manager, v1.1

I know it's hard to remember to pass -U8 to git.  If you use |git bz attach|,
it will do that for you.  Otherwise, perhaps you can set up an alias.  For
example, I have

  [alias]
    dif = diff -U8 --patience -M -C

I don't believe in blaming users, so I filed bug 820234 to see if we can get
this improved on the bugzilla side.

>diff --git a/dom/fm/nsIFMRadio.idl b/dom/fm/nsIFMRadio.idl
>index afa2145..df879e4 100644
>--- a/dom/fm/nsIFMRadio.idl
>+++ b/dom/fm/nsIFMRadio.idl

>@@ -80,6 +80,11 @@ interface nsIFMRadio : nsIDOMEventTarget {
>     void setFrequency(in long frequency);
> 
>     /**
>+     * Update the visibility state of our client.
>+     */
>+    void updateVisible(in boolean visible);

The client of the FM radio is not well-defined.  I think that's the essential
flaw of this patch.

>diff --git a/dom/fm/DOMFMRadioChild.js b/dom/fm/DOMFMRadioChild.js
>index df07e07..0247e92 100644
>--- a/dom/fm/DOMFMRadioChild.js
>+++ b/dom/fm/DOMFMRadioChild.js

>@@ -73,6 +73,14 @@ DOMFMRadioChild.prototype = {
>                       "DOMFMRadio:powerStateChange",
>                       "DOMFMRadio:antennaChange"];
>     this.initHelper(aWindow, messages);
>+
>+    let els = Cc["@mozilla.org/eventlistenerservice;1"]
>+                .getService(Ci.nsIEventListenerService);
>+
>+    els.addSystemEventListener(aWindow, "visibilitychange",
>+                               this._updateVisibility.bind(this),
>+                               /* useCapture = */ true);
>+    this._updateVisibility({ target: aWindow.document });
>   },

This does not seem correct.

init() is called when someone touches the FM radio property, right?  ("nsIDOMGlobalPropertyInitializer implementation"?)

So as I understand this patch, if a window touches the FM radio property (say to see if the antenna is available, or maybe just to see if FM radio is supported at all), it is forever a "client" of the FM radio.  Then when that window becomes hidden, we fire _updateVisibility, even if the window is not currently interacting with the radio, and we will eventually set the FM radio state to hidden, even if a visible window is interacting with the radio.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 34 • 12 years ago

(In reply to Justin Lebar [:jlebar] from comment #33)
> Comment on attachment 690548 [details] [diff] [review]
> Hook up FM radio to the audio manager, v1.1
> 
> I know it's hard to remember to pass -U8 to git.  If you use |git bz attach|,
> it will do that for you.  Otherwise, perhaps you can set up an alias.  For
> example, I have
> 
>   [alias]
>     dif = diff -U8 --patience -M -C
> 

D'oh, my fault.  I never even thought to set this up for git; I just develop in there for one-off patches.  Thanks.

> >diff --git a/dom/fm/nsIFMRadio.idl b/dom/fm/nsIFMRadio.idl
> >index afa2145..df879e4 100644
> >--- a/dom/fm/nsIFMRadio.idl
> >+++ b/dom/fm/nsIFMRadio.idl
> 
> >@@ -80,6 +80,11 @@ interface nsIFMRadio : nsIDOMEventTarget {
> >     void setFrequency(in long frequency);
> > 
> >     /**
> >+     * Update the visibility state of our client.
> >+     */
> >+    void updateVisible(in boolean visible);
> 
> The client of the FM radio is not well-defined.  I think that's the essential
> flaw of this patch.

Per IRC chat, we're going to update this patch to leave things no worse than they already are by having clients only fire visibility notifications when they've called enable() more recently than disable().

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 36 • 12 years ago

Comment on attachment 690696 [details] [diff] [review]
Hook up FM radio to the audio manager, v2

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

::: dom/fm/DOMFMRadioParent.jsm
@@ +459,5 @@
>            self._sendMessage("DOMFMRadio:cancelSeek:Return", true, null, msg);
>          }
>          break;
> +      case "DOMFMRadio:updateVisibility":
> +        FMRadio.updateVisible(msg != 'hidden');

Nit: I would have made this |msg == "visible"| given that on the receiving side the argument is named aVisible. What you have now is sort of a double-negation.

Up to you.

::: dom/fm/FMRadio.cpp
@@ +135,5 @@
> +  bool canPlay;
> +  mAudioChannelAgent->SetVisibilityState(!mHidden);
> +  mAudioChannelAgent->StartPlaying(&canPlay);
> +  if (!canPlay) {
> +    mAudioChannelAgent->StopPlaying();

This doesn't seem right. I don't think you should call StopPlaying when this happens, you should simply not enable the radio hardware.

@@ +172,5 @@
>    NS_ENSURE_TRUE(audioManager, NS_OK);
>  
>    audioManager->SetFmRadioAudioEnabled(false);
> +
> +  if (!mAudioChannelAgent) {

Surely this should be |if (mAudioChannelAgent)|?

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

This isn't enough if we never enabled the radio when we first called FMRadio::Enable. Consider the following:

1. The user enters the FMRadio app.
2. Before the user manages to turn the radio on, he receives an SMS
   message.
3. The notification API will play a short audio clip using the
   "notification" channel.
4. While the audio clip is still playing, the user hits the button
   to enable the radio.
5. We get into FMRadio::Enable.
6. FMRadio::Enable creates a audioChannelAgent and call
   startPlaying.
7. startPlaying sets canPlay to false since we are still using the
   "notification" channel. So the radio hardware is never switched
   on.
8. A fraction of a second later the notification audio stops playing.

At this point I'd expect the radio audio to turn on. However since we never turned on the radio, this won't work.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 37 • 12 years ago

The easiest way to fix this is likely to let FMRadio::Enable turn on the radio even if canPlay is false. Though it should first set the volume to 0.

That way when we get into FMRadio::CanPlayChanged, we can simply turn up the volume.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 38 • 12 years ago

(In reply to Jonas Sicking (:sicking) from comment #36)
> Comment on attachment 690696 [details] [diff] [review]
> Hook up FM radio to the audio manager, v2
> 
> Review of attachment 690696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/fm/DOMFMRadioParent.jsm
> @@ +459,5 @@
> >            self._sendMessage("DOMFMRadio:cancelSeek:Return", true, null, msg);
> >          }
> >          break;
> > +      case "DOMFMRadio:updateVisibility":
> > +        FMRadio.updateVisible(msg != 'hidden');
> 
> Nit: I would have made this |msg == "visible"| given that on the receiving
> side the argument is named aVisible. What you have now is sort of a
> double-negation.
> 
> Up to you.
> 

My thought was to be generous about what visibility states we allow playback to continue through, but on second though it's better to be conservative here.  Will change.

> ::: dom/fm/FMRadio.cpp
> @@ +135,5 @@
> > +  bool canPlay;
> > +  mAudioChannelAgent->SetVisibilityState(!mHidden);
> > +  mAudioChannelAgent->StartPlaying(&canPlay);
> > +  if (!canPlay) {
> > +    mAudioChannelAgent->StopPlaying();
> 
> This doesn't seem right. I don't think you should call StopPlaying when this
> happens, you should simply not enable the radio hardware.
> 

I'm guessing the idea here was to reduce the amount of state that FMRadio::CanPlayChanged() has to check.  If we don't enable the radio here, then we have to check whether the hardware was enabled in CanPlayChanged(), and additionally suppress any notifications triggered by turning the hardware on there.

This isn't ideal because it's not great for power usage while FM is muted (unless the hardware is smarter than I bet it is), but I think this is material for a followup.

> @@ +172,5 @@
> >    NS_ENSURE_TRUE(audioManager, NS_OK);
> >  
> >    audioManager->SetFmRadioAudioEnabled(false);
> > +
> > +  if (!mAudioChannelAgent) {
> 
> Surely this should be |if (mAudioChannelAgent)|?
> 

Sure looks like it.

> @@ +267,5 @@
> > +  if (canPlay) {
> > +    int32_t volIdx = 0;
> > +    // Restore fm volume, that value is sync as music type
> > +    audioManager->GetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_MUSIC, &volIdx);
> > +    audioManager->SetStreamVolumeIndex(nsIAudioManager::STREAM_TYPE_FM, volIdx);
> 
> This isn't enough if we never enabled the radio when we first called
> FMRadio::Enable. Consider the following:
> 

If we do enable the radio, is it enough?

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 39 • 12 years ago

(In reply to Chris Jones [:cjones] [:warhammer] from comment #38)
> If we do enable the radio, is it enough?

Yeah, I think that if we make FMRadio::Enable always enable the radio, and optionally set the volume to 0 if canPlay is false, then I think we're golden.

And file a followup to actually turn off the radio hardware as needed rather than just muting it.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 40 • 12 years ago

(In reply to Chris Jones [:cjones] [:warhammer] from comment #38)
> (In reply to Jonas Sicking (:sicking) from comment #36)
> > Comment on attachment 690696 [details] [diff] [review]
> > Hook up FM radio to the audio manager, v2
> > 
> > Review of attachment 690696 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/fm/DOMFMRadioParent.jsm
> > @@ +459,5 @@
> > >            self._sendMessage("DOMFMRadio:cancelSeek:Return", true, null, msg);
> > >          }
> > >          break;
> > > +      case "DOMFMRadio:updateVisibility":
> > > +        FMRadio.updateVisible(msg != 'hidden');
> > 
> > Nit: I would have made this |msg == "visible"| given that on the receiving
> > side the argument is named aVisible. What you have now is sort of a
> > double-negation.
> > 
> > Up to you.
> > 
> 
> My thought was to be generous about what visibility states we allow playback
> to continue through, but on second though it's better to be conservative
> here.  Will change.

In fact for "prerender" this code is just wrong.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 41 • 12 years ago

jlebar, sicking said he understand your comments well enough to speak for them, and agrees with auditing the impl for multiple clients in a followup.

This should address the review comments.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 42 • 12 years ago

Comment on attachment 690769 [details] [diff] [review]
Hook up FM radio to the audio manager, v3

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

ship it!

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 43 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd00821b680
https://hg.mozilla.org/releases/mozilla-aurora/rev/632b753f0878
https://hg.mozilla.org/releases/mozilla-beta/rev/f37d0ac604ad

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

The patch with review+ seems not to resolve the comment 19 yet. And it could be solved on the patch provided by Randy even the patch is a hack way.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 45 • 12 years ago

Yes, you're right.  I would happy landing that in a followup, but if you'd prefer to reopen this bug, that's fine too.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 46 • 12 years ago

Comment on attachment 690570 [details] [diff] [review]
avoid fm volume can be unmute by adjust volume key

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

r=me with the below things fixed.

::: dom/system/gonk/AudioManager.cpp
@@ +392,5 @@
>  NS_IMETHODIMP
>  AudioManager::SetStreamVolumeIndex(int32_t aStream, int32_t aIndex) {
>    status_t status =
>      AudioSystem::setStreamVolumeIndex(static_cast<audio_stream_type_t>(aStream), aIndex);
> +  // sync the fm stream volume with music volume, except set fm volume by audioChannelServices

This whole block is pretty hard to read. At least use {} consistently. In most places, including in this file, even single-line if-statements get {}. This helps since it makes future changes easier to read in a diff.

And please add an empty line after the initial setStreamVolumeIndex call.

@@ +396,5 @@
> +  // sync the fm stream volume with music volume, except set fm volume by audioChannelServices
> +  if (aStream == AUDIO_STREAM_FM && IsDeviceOn(AUDIO_DEVICE_OUT_FM) && aIndex == 0)
> +    mIsMuteByAudioChannelService = true;
> +  else if (aStream == AUDIO_STREAM_FM && IsDeviceOn(AUDIO_DEVICE_OUT_FM) && aIndex > 0)
> +    mIsMuteByAudioChannelService = false;

This can be more clearly written as:

if (aStream == AUDIO_STREAM_FM && IsDeviceOn(AUDIO_DEVICE_OUT_FM)) {
  mIsMuteByAudioChannelService = aIndex == 0;
}

::: dom/system/gonk/AudioManager.h
@@ +51,5 @@
>    int32_t mPhoneState;
>  
>  private:
>    nsAutoPtr<mozilla::hal::SwitchObserver> mObserver;
> +  bool mIsMuteByAudioChannelService;

Since this is FM-channel specific, call it something like:

mFMChannelIsMutedByAudioChannelService

or simply

mFMChannelIsMuted

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

fix format

	Jonas Sicking (:sicking) No longer reading bugmail consistently Reporter
	Comment 48 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd00821b680
https://hg.mozilla.org/integration/mozilla-inbound/rev/8718e5b8bab9

https://hg.mozilla.org/releases/mozilla-aurora/rev/632b753f0878
https://hg.mozilla.org/releases/mozilla-aurora/rev/e0110b95aac9

https://hg.mozilla.org/releases/mozilla-beta/rev/f37d0ac604ad
https://hg.mozilla.org/releases/mozilla-beta/rev/0efb859fe428

	Andrea Marchesini [:baku]
	Comment 49 • 12 years ago

Good work! I was proposing something similar. I have a question:

what about if 2 apps are using FMRadio at the same time? The visibility should not be a boolean in FMRadio.cpp but a counter. If the counter goes to 0, no apps are visible, if counter greater than 0, at least we have 1 visible able.

This is not a priority but probably it's good to create a follow up bug.

	Ed Morley [:emorley]
	Comment 50 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/3dd00821b680
https://hg.mozilla.org/mozilla-central/rev/8718e5b8bab9

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 51 • 12 years ago

Yeah, the backend doesn't really handle multiple clients at all right now.  I filed bug 820241 for that; I'll dup your bug over to that.

	John Shih
	Comment 52 • 12 years ago

verified on 2012-12-12 unagi daily build
build info:
releases/gaia : db435724709181ffa3348c06cfcc80b71375e73d
releases/gecko : 8e66dde61aec02ba5a4a4e44583679aedc609a73