Hook up FM Radio to audio channels backend

VERIFIED FIXED in Firefox 18

Status

Firefox OS
General
P1
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: sicking, Assigned: rlin)

Tracking

(Blocks: 1 bug)

unspecified
B2G C2 (20nov-10dec)
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [LOE:S])

Attachments

(2 attachments, 9 obsolete attachments)

12.77 KB, patch
sicking
: review+
Details | Diff | Splinter Review
3.10 KB, patch
sicking
: review+
Details | Diff | Splinter Review
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.
Summary: Hook up telephony FM Radio to audio channels backend → Hook up FM Radio to audio channels backend
Is there actual work here once bug 805333 lands? If so, who should own it?
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.
Assignee: nobody → rlin
blocking-basecamp: --- → +
Priority: -- → P1
Target Milestone: --- → B2G C2 (20nov-10dec)

Updated

6 years ago
Blocks: 815046

Updated

6 years ago
QA Contact: jshih

Updated

6 years ago
Whiteboard: [LOE:S]
This would use the implementation of Bug 815069.
Blocks: 812691
Is there a patch needed for this, or will bug 815069 fix it entirely?
A patch is needed here.
Created attachment 689599 [details] [diff] [review]
methodology review

This is base on audiochannelAgent patch.
May hit this.Bug 818708
Attachment #689599 - Flags: feedback?(jonas)
Attachment #689599 - Flags: feedback?(amarchesini)
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.
Attachment #689599 - Flags: feedback?(jonas)
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.
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.
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.
Created attachment 689643 [details] [diff] [review]
patch1

This patch use AudioChannelAgent to implement audio policy issue.
Attachment #689599 - Attachment is obsolete: true
Attachment #689599 - Flags: feedback?(amarchesini)
Attachment #689643 - Flags: feedback?(mchen)
Attachment #689643 - Flags: feedback?(justin.lebar+bug)
Attachment #689643 - Flags: feedback?(amarchesini)

Comment 12

6 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.
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.
I'm happy to look at a r? of a patch with mchen and baku's comments addressed.
Attachment #689643 - Flags: feedback?(justin.lebar+bug)
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.
Attachment #689643 - Flags: feedback?(amarchesini) → feedback-

Updated

6 years ago
Flags: needinfo?(jonas)
Is someone in PST able to update this patch?

I think that I could, except for bug 819521 :(.
Blocks: 810780
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.
Created attachment 690336 [details] [diff] [review]
WIP

Fixes the bug assigned here, but I don't think addresses all the previous review comments.  Looking at that next.
Created attachment 690338 [details] [diff] [review]
WIP 2

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?
Attachment #690336 - Attachment is obsolete: true
Flags: needinfo?(rlin)
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?
Flags: needinfo?(rlin)
Attachment #690338 - Flags: feedback+
Maybe hook the agent in DOMFMRadioChild.js is another way to avoid the visibility change problem.
Yes, it looks like the FM radio impl is a bit odd.
Created attachment 690536 [details] [diff] [review]
Hook up FM radio to the audio manager

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.
Attachment #690338 - Attachment is obsolete: true
Attachment #690536 - Flags: review?(justin.lebar+bug)
Attachment #690536 - Flags: review?(jonas)
Attachment #690536 - Flags: feedback?(rlin)
Created attachment 690542 [details] [diff] [review]
avoid fm volume can be unmute by music app
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.
Created attachment 690548 [details] [diff] [review]
Hook up FM radio to the audio manager, v1.1

Removes a missed logging statement and the broken |mHidden| check from CanPlay().
Attachment #690536 - Attachment is obsolete: true
Attachment #690536 - Flags: review?(justin.lebar+bug)
Attachment #690536 - Flags: review?(jonas)
Attachment #690536 - Flags: feedback?(rlin)
Attachment #690548 - Flags: review?
Attachment #690548 - Flags: feedback?
Created attachment 690570 [details] [diff] [review]
avoid fm volume can be unmute by adjust volume key
Attachment #690542 - Attachment is obsolete: true
Attachment #690570 - Flags: review?(amarchesini)
Attachment #690570 - Flags: review?(amarchesini) → review?(jones.chris.g)
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.
Attachment #690570 - Flags: review?(jones.chris.g) → review?(jonas)
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.
Flags: needinfo?(jonas)
(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.
Comment on attachment 690548 [details] [diff] [review]
Hook up FM radio to the audio manager, v1.1

Oh jesus, I could kick myself :(.
Attachment #690548 - Flags: review?(justin.lebar+bug)
Attachment #690548 - Flags: review?(jonas)
Attachment #690548 - Flags: review?
Attachment #690548 - Flags: feedback?(rlin)
Attachment #690548 - Flags: feedback?
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.
Attachment #690548 - Flags: feedback?(rlin) → feedback+
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.
Attachment #690548 - Flags: review?(justin.lebar+bug) → review-
(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().
Created attachment 690696 [details] [diff] [review]
Hook up FM radio to the audio manager, v2
Attachment #690548 - Attachment is obsolete: true
Attachment #690548 - Flags: review?(jonas)
Attachment #690696 - Flags: review?(justin.lebar+bug)
Attachment #690696 - Flags: review?(jonas)
Attachment #690696 - Attachment description: Hook up FM radio to the audio manager, v1.1 → Hook up FM radio to the audio manager, v2
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.
Attachment #690696 - Flags: review?(jonas) → review-
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.
(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?
(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.
(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.
Created attachment 690769 [details] [diff] [review]
Hook up FM radio to the audio manager, v3

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.
Attachment #690696 - Attachment is obsolete: true
Attachment #690696 - Flags: review?(justin.lebar+bug)
Attachment #690769 - Flags: review?(jonas)
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!
Attachment #690769 - Flags: review?(jonas) → review+

Comment 44

6 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.
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.

Updated

6 years ago
status-firefox18: --- → fixed
status-firefox19: --- → fixed
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
Attachment #690570 - Flags: review?(jonas) → review+
Created attachment 690790 [details] [diff] [review]
fix fm can be mute patch 2

fix format
Attachment #690570 - Attachment is obsolete: true
Attachment #689643 - Attachment is obsolete: true
Attachment #689643 - Flags: feedback?(mchen)
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.
Blocks: 820313
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.

Comment 52

6 years ago
verified on 2012-12-12 unagi daily build
build info:
releases/gaia : db435724709181ffa3348c06cfcc80b71375e73d
releases/gecko : 8e66dde61aec02ba5a4a4e44583679aedc609a73
Status: RESOLVED → VERIFIED
status-firefox20: --- → fixed
Whiteboard: [LOE:S] → [LOE:S][status-b2g18:fixed]
status-b2g18: --- → fixed
Whiteboard: [LOE:S][status-b2g18:fixed] → [LOE:S]
You need to log in before you can comment on or make changes to this bug.