852455 - [Audio/FM] To disable FMRadio not just mute when AudioChannelService requests to stop.

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Marco Chen [:mchen]]

Issue:
  Currently FMRadio will be muted when asked by AudioChannelService but the mute of FMRadio still consume the battery capacity.

Idea:
  Implement AudioChannalAgent into DOMFMRadioChild.js and behalf of FM app to enable/disabe FMRadio backend based on communication with AudioChannelService.

UI Synchronization:
  The audio policy now will always allow foreground app to play. So whenever FM app in the foreground (maybe it is stopped by AudioChannelAgent when it is in the background.) FM app will be resumed automatically.

Comment 1

[Marco Chen [:mchen]]

Attachment: Version 1

Hi Justin,

Could you help to review the part of enabling/disabling FMRadio?
And any potential issue with Gaia app.

Hi Andrea,

Could you help to review the part of communicating with AudioChannelService and FM/AudioChannelAgent?

Thanks.

Comment 2

[Marco Chen [:mchen]]

https://bugzilla.mozilla.org/show_bug.cgi?id=850594#c8

Note that leo has concern with we just mute FM but disable it.

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 726534 [details] [diff] [review]
Version 1

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

::: dom/fm/DOMFMRadioChild.js
@@ +325,5 @@
>    set onfrequencychange(callback) {
>      this._onFrequencyChange = callback;
>    },
>  
> +  audio_agent_callback: function(aCanPlay) {

audioAgentCallback

@@ +341,3 @@
>      return this._call("disable", null);
>    },
>  

internalDisable

@@ +341,4 @@
>      return this._call("disable", null);
>    },
>  
> +  internal_enable: function(frequency) {

internalEnable

@@ +363,5 @@
> +    if (!this._audioChannelAgent) {
> +      this._audioChannelAgent =
> +        Cc["@mozilla.org/audiochannelagent;1"].createInstance(
> +        Ci.nsIAudioChannelAgent);
> +      let audioAgent = this._audioChannelAgent;

why do you need this variable?

@@ +365,5 @@
> +        Cc["@mozilla.org/audiochannelagent;1"].createInstance(
> +        Ci.nsIAudioChannelAgent);
> +      let audioAgent = this._audioChannelAgent;
> +      audioAgent.init(Ci.nsIAudioChannelAgent.AUDIO_AGENT_CHANNEL_CONTENT,
> +        this.audio_agent_callback.bind(this));

this.audioAgentCallback.bind(this));

Comment 4

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #3)
> Comment on attachment 726534 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 726534 [details] [diff] [review]:
> -----------------------------------------------------------------

Oh, I should take care the naming issue in the first patch.
Thanks and will follow.

> @@ +363,5 @@
> > +    if (!this._audioChannelAgent) {
> > +      this._audioChannelAgent =
> > +        Cc["@mozilla.org/audiochannelagent;1"].createInstance(
> > +        Ci.nsIAudioChannelAgent);
> > +      let audioAgent = this._audioChannelAgent;
> 
> why do you need this variable?
> 

Just use this variable to reduce the line characters in the following code.
I found some places used this way too.

Comment 5

[Marco Chen [:mchen]]

Attachment: Patch v2

1. Follow the comments from Andrea.
2. Hi Justin, thanks for your review on enabling/disabling FMRadio part in advance.

Comment 6

[Justin Lebar (not reading bugmail)]

It looks like you mean "version 1" and "version 2" instead of "part 1" and "part 2".  You may want to fix the names and obsolete the old patch.

Comment 7

[Justin Lebar (not reading bugmail)]

>+    this._interruptByAudioChannel = false;

Nit: This should be "interruptedByAudioChannel", past tense.  But I think
_audioChannelPaused would be a better name.

>-  disable: function nsIDOMFMRadio_disable() {
>+  audioAgentCallback: function(aCanPlay) {

Nit: The style in this file is not to use the |a| prefix on the argument names.
We use that prefix pretty consistently in C++, but it's hit-and-miss in JS.  I
don't know what the preferred style is, but when in doubt we should match the
style of the surrounding code.

>+  enable: function nsIDOMFMRadio_enable(frequency) {
>+    this._cachedFrequency = frequency;
>+    if (!this._audioChannelAgent) {
>+      this._audioChannelAgent =
>+        Cc["@mozilla.org/audiochannelagent;1"].createInstance(
>+        Ci.nsIAudioChannelAgent);

Nit: We don't usually break a line right after a function call like this.
Instead, you could indent this as

          Cc["@mozilla.org/audiochannelagent;1"]
            .createInstance(Ci.nsIAudioChannelAgent);

>+      let audioAgent = this._audioChannelAgent;
>+      audioAgent.init(Ci.nsIAudioChannelAgent.AUDIO_AGENT_CHANNEL_CONTENT,
>+        this.audioAgentCallback.bind(this));

Nit: Please indent "this" underneath the capital C in "Ci".

>+      audioAgent.setVisibilityState(this._visibility == 'visible');
>+      let canPlay = audioAgent.startPlaying();
>+      if (!canPlay) {
>+        var request = this.createRequest();
>+        Services.DOMRequest.fireSuccess(request, null);
>+        return request;

Should we return success here?  If we return success, the caller may reasonably
expect that FM.enabled will be true.  But it won't be, right?

Is it possible for two audio channels to be playing the FM radio at the same
time?  If so, when one of them is muted, we don't want to disable the radio,
but it looks like that's what this patch does.

This patch looks pretty good, but I'm going to hold r+ until we get these
questions cleared up.

Comment 8

[Marco Chen [:mchen]]

Hi Justin,

Thanks for the review and comments.

> Should we return success here?  If we return success, the caller may reasonably
> expect that FM.enabled will be true.  But it won't be, right?

From the app's or caller's view, this enable is really successful but paused by AudioChannelService's rule. And FM will be automatically enabled when app backs to foreground (foreground apps are always allow to play by AudioChannelService). In this thought, I find I need to set "audioChannelPaused = true". 
So I would keep it.

> Is it possible for two audio channels to be playing the FM radio at the same
> time?  If so, when one of them is muted, we don't want to disable the radio,
> but it looks like that's what this patch does.

As I know that only privileged app can use FMRadio and there is only one (Bug 849335). 
By the way if we want to protect this then I think we should do it on the Parent side (maybe nsDOMFMRadioParent.js or FMRadio.cpp). Because only that role will know how many clients using FMRadio.

So I doesn't consider it on my patch.

Comment 9

[Justin Lebar (not reading bugmail)]

> From the app's or caller's view, this enable is really successful but paused by 
> AudioChannelService's rule.

What happens if the caller reads FM.enabled?  I thought that would return false, and would only return true after the page became visible again.

Returning success and then not setting enabled to true is pretty confusing to me.

I understand that returning failure and then eventually setting enabled to true is also confusing.

I'm not entirely sure what to do here.  Maybe Fabrice or Mounir has an idea based on prior art.

> As I know that only privileged app can use FMRadio and there is only one (Bug 849335).

Anybody can write a privileged app.  Privileged just means you have to be specially reviewed in the app store.

> By the way if we want to protect this then I think we should do it on the Parent side

Yes, we'd have to do it on that side.

Comment 10

[Marco Chen [:mchen]]

Attachment: Version 3

1. To fix issues of the naming and indent.
2. About the case of "AudioChannelService replies to pause when enabling FMRadio", I still return the result as true. And in function of getEnabled(), I also return true when AudioChannelPaused is true. So caller will be not confused.

3. About supporting multiple users to use FMRadio, it should be covered by another bug.

Comment 11

[Marco Chen [:mchen]]

Comment on attachment 726534 [details] [diff] [review]
Version 1

Version 1 is kept for recording the review from Andrea.

Comment 12

[Justin Lebar (not reading bugmail)]

> Version 1 is kept for recording the review from Andrea.

The usual way of doing this is to mark the patch as obsolete.  It's still around, and so someone who wants to see Andrea's r+ can still observe that fact.

Comment 13

[Justin Lebar (not reading bugmail)]

> 3. About supporting multiple users to use FMRadio, it should be covered by another bug.

Although our API already behaves badly when multiple people try to access the FM radio, this patch makes the situation worse.

We have been kicking this can down the road for months now, based on pressure to meet deadlines that turned out not to be important.  I'm not entirely comfortable continuing to do so.  I'd like us to do the right thing here.

Comment 14

[Marco Chen [:mchen]]

Attachment: Version 3

4. When caller calls disable(), we doesn't send disable command to parent if audioChannelPaused is already true.

Comment 15

[Justin Lebar (not reading bugmail)]

Version 3 still doesn't address the multiple users issue, right?

Comment 16

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #13)
> Version 3 still doesn't address the multiple users issue, right?

Yes, it doesn't consider that yet.

And before the implementation, the first thing will be the rule on multiple users using FMRadio. Since FMRadio is a single physicaly resource, I would suggest to let FMRadio be avalible only for first user who tries to eanble it. This will keep thing more easy.
Or any other suggestion?

Thanks.

Comment 17

[Justin Lebar (not reading bugmail)]

> And before the implementation, the first thing will be the rule on multiple users using 
> FMRadio. Since FMRadio is a single physicaly resource, I would suggest to let FMRadio be 
> avalible only for first user who tries to eanble it. This will keep thing more easy.

That sounds great to me.

Comment 18

[Justin Lebar (not reading bugmail)]

But note that the semantics of "processes" are not exposed to the web, so ownership would need to be on a per-window or per-origin basis, not on a per-process basis.

Comment 19

[Marco Chen [:mchen]]

Attachment: Version 4

Hi Justin,

The rough idea for protecting FMRadio resource is
  Add a variable called _servicedClientID in parant side, it is used to record msg.mid (or this._id in child side). That id is uniqe for identify who eanbled FMRadio and limited access from others.

I tested the use case of closing FMRadio app by card view and app will call disable function.

The issue now is that if FM app is crashed then FM will still be on playing. This is a bug without my patch too. May I know any suggestion for this?

Comment 20

[Marco Chen [:mchen]]

oh, I found one nit about space. Will remove it on next version.

Comment 21

[Justin Lebar (not reading bugmail)]

> The issue now is that if FM app is crashed then FM will still be on playing. This is a 
> bug without my patch too. May I know any suggestion for this?

You can listen for the mozbrowsererror type=fatal event on the relevant iframes.  This will notify you of crashes.  If you search for mozbrowsererror in Gaia, you should see examples of how to do this.

Comment 22

[Marco Chen [:mchen]]

Hi Justin,

Thanks for your suggestion. I found that mozbrowsererror is came from observer event - oop-frameloader-crashed in BrowserElementParant.jsm. But I still can't to decide whether this dead frame is the one controlled FM Radio even DOMFMRadioParent.jsm know this event.

What I did now is to use mid from cpmm messages as a uniqe ID for indentifying who controlled the FM Radio now. But the info from oop-frameloader-crashed didn't contain this information.

The second thought is
  In parent side, to monitor "ipc:content-shutdown" by observer then I can know which childID dead.
  In child side, to append childID into every messages for parent to check.

But the problem now is that how do I get ChildID from a JS code DOMFMRadioChild.js?
Very thanks for your suggestion here.

Comment 23

[Marco Chen [:mchen]]

>> In child side, to append childID into every messages for parent to check.

To fix my thought about we should protect resource by window not by process. so

  In parent side, we still use mid to be a uniqe ID to know who controlled FMRadio and listent to "ipc:content-shutdown" too.
  In child side, the child ID will be appended to enable command only.

But the problem is still that how do I get ChildID from a JS code DOMFMRadioChild.js?
Thanks.

Comment 24

[Justin Lebar (not reading bugmail)]

Sending the ChildID from the child side is okay, but it's not a great solution, because a misbehaving and/or malicious child process could send the wrong childID to the parent.

Ideally you'd use the parent process message manager to figure out which child sent you the message.

I /think/ when you receive a message in the parent process, the |target| property on the message will be the relevant mozbrowser iframe.  (See nsIMessageManager.idl :: nsIMessageManager::receiveMessage().)

So you should be able to register a mozbrowsererror event listener on the |target| iframe.  If that doesn't work for some reason, you can get the frameloader off the iframe by doing

  iframe.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader

and then compare this to the |subject| of the oop-frameloader-crashed observer notification.

Does that make sense?

Comment 25

[Marco Chen [:mchen]]

Yes, that makes sense and I learn things here.
I will try this way and thanks for your kindly suggestion in your sleep time.

Comment 26

[Justin Lebar (not reading bugmail)]

mchen told me that I was wrong: |target| is a ChromeMessageManager.  So the question is: Can we go from a ChromeMessageManager to its iframe (or its frameloader)?

I don't see a way to do this in nsIMessageManager, but I'm probably missing something.  smaug, is there a Right Way to do this?

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Which ChromeMessageManager? Is this about process message manager? Then there is no frameloader involved.

Comment 28

[Marco Chen [:mchen]]

Hi smaug,

Please refer to the code position as below.
http://mxr.mozilla.org/mozilla-central/source/dom/fm/DOMFMRadioParent.jsm#391

In order to distinqush which client used FM Radio, we planed to use frameloader as the identifier. And in the case of FM Radio client crashedm, we can
  1. Throught monitoring the oop-frameloader-crashed, we can get which frameloader is crashed in DOMFMRadioParent.jsm.
  2. In order to find out whether this frameloader is assocated with client controlled FM now, we need a way to get client's frameloader. The idea from Justin is to extract it from aMessage.target.

But it seems the answer is false from your reply. So Do you have any suggestion to get client's frameloader?

Very thanks.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

http://mxr.mozilla.org/mozilla-central/source/dom/fm/DOMFMRadioParent.jsm?rev=3dd00821b680&mark=51-53,108-108,#50
Based on that, communication happens between process message managers, which means there is
no frameloader involved anywhere. Child process has only on child process message manager, but
can have several frame message managers.

Parent has one parent process message manager per child process, and each of those parent
process message managers are connected to the top level parent process message manager which is a singleton object in the parent process.
Each <xul:browser>/<iframe mozbrowser> create a separate parent/child message manager pair, which is
not in any way connected to process message managers.

http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageManager.idl has quite a bit
documentation.

So the answer is that there can be several frameloaders, and it is not easy to traverse all those.
We could add something to process message managers so that it would know about each frame message managers.

Comment 30

[Justin Lebar (not reading bugmail)]

If we changed the FM radio child to use the frame mm's instead of the child-process mm, would that improve anything?  Would we have to change the parent to listen to each of the frame mm's?  The diagram in nsIMessageManager suggests not, to me.

Is there a right way to get the frame mm on the child side, given a window?

Comment 31

[Marco Chen [:mchen]]

Hi Justin,

I got news from Gene that we can listen "child-process-shutdown" in ppmm then compare the target in this message with the one we allowed to control FMRadio. So we can know the timing of disabling and releasing FMRadio resource.

Thanks Gene.

Comment 32

[Justin Lebar (not reading bugmail)]

> I got news from Gene that we can listen "child-process-shutdown" in ppmm then compare the target in 
> this message with the one we allowed to control FMRadio.

Ah, that's new.  Looks like a good option.

Comment 33

[Marco Chen [:mchen]]

Attachment: AudioChannel - Version 5

Comment 34

[Marco Chen [:mchen]]

Attachment: ResourceProtection Version 2

Comment 35

[Marco Chen [:mchen]]

Attachment: ResourceProtection Version 2

Comment 36

[Marco Chen [:mchen]]

Comment on attachment 730506 [details] [diff] [review]
AudioChannel - Version 5

This is a part of pausing FM not just muting according to status of AudioChannelAgent. Thanks for the review.

Comment 37

[Marco Chen [:mchen]]

Comment on attachment 730510 [details] [diff] [review]
ResourceProtection Version 2

This is part of protecting FMRadio be controlled by one client only and clear FMRadio resource when serviced client is crashed.

The rough idea in parent side is
  1. New variable called _servicedChildProcess is used to know which process controlled FM now. That is got from msg.target.
  2. New variable called _servicedMessageId is used to know which DOMFMRadioChild controlled FM now. That is gto from msg.mid. There might be multiple DOMFMRadioChild in one process so msg.mid is used to distinqush them.
  3. Block the incoming messages for changing FM state is based on _servicedMessageId and allow incoming messages for reading state only.
  4. Add to listen "child-process-shutdown", so I can clear the _servicedXXX for releasing FM.

Thanks for the review.

Comment 38

[Justin Lebar (not reading bugmail)]

Comment on attachment 730510 [details] [diff] [review]
ResourceProtection Version 2

Suppose apps A and B want to use the FM radio.

Process A is currently using it.  Now process A quits or crashes.  How can process B detect that the FM radio is now available for its use?

I don't see how process B can do so with this patch, except by polling (e.g. calling enable() in a loop until it succeeds).

Comment 39

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #38)
> Comment on attachment 730510 [details] [diff] [review]
> ResourceProtection Version 2
> 
> Suppose apps A and B want to use the FM radio.
> 
> Process A is currently using it.  Now process A quits or crashes.  How can
> process B detect that the FM radio is now available for its use?
> 
> I don't see how process B can do so with this patch, except by polling (e.g.
> calling enable() in a loop until it succeeds).

Hi Justin,

May I know any UX input for why we need this feature? I can't image that it will be used on which scenario.

I think app B should show up the error message when it can't access FMRadio then user can try to enable it again when app A is closed or crashed. That is common behavior for accessing singleton hw resource on mobile platform. Ex: Camera.

Comment 40

[Marco Chen [:mchen]]

By the way, I think process B can listen DOMFMRadio:powerStateChange and query the enabled state. Then it will know when FMRadio is available. 

http://mxr.mozilla.org/mozilla-central/source/dom/fm/DOMFMRadioParent.jsm#200

Comment 41

[Justin Lebar (not reading bugmail)]

DMFMRadio:powerStateChange is not available to apps, right?  I guess you mean the onenabled event?  Maybe that works if the FM radio is always reported as enabled when it's "locked", and is always reported as disabled otherwise.

But what is confusing to me is that the patches here cause the FM radio to lie about whether it's enabled.  If we mute a frame, the FM radio pretends to be enabled within that process.  Then it does

> +    // If _audioChannelPaused is true then parent side will still lock the FM
> +    // Radio for this window to use until this command with _audioChannelPaused
> +    // to false.
> +    return this._call("disable", this._audioChannelPaused);

But I don't see where the parent process reads this audioChannelPaused boolean argument, in either of the two patches here.

Comment 42

[Justin Lebar (not reading bugmail)]

> May I know any UX input for why we need this feature?

Instead of having a "FM didn't work; try again?" button, the app could have a notice which says "FM radio currently in use" or "FM radio no longer in use".  The latter is much better UX.

Comment 43

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #41)
> But I don't see where the parent process reads this audioChannelPaused
> boolean argument, in either of the two patches here.

Hi Justin,

You can search the key word as below in patch of "resource protection"
  
  let audioAgentPaused = msg.data;

> But what is confusing to me is that the patches here cause the FM radio to lie > about whether it's enabled.

Ok, I already do the hack of reporting "enabled" on child side. I can move the hack to parent side according to the status of _servicedMessageId. Then enabled will be consistent to true even paused by audiochannelservice. Is this enough?

-----------------------------------------

Consider to the UI, I would like to add an attribute exposed to app called mozAudioChannelPaused (onMozAudioChannelPaused). The usage is that when paused FM app back to foreground, app can show the "waiting screen" until that attribute to false. Because FM initialization time is slow (about 4s). How about this?

Thanks for your review.

Comment 44

[Justin Lebar (not reading bugmail)]

> You can search the key word as below in patch of "resource protection"
>  
>  let audioAgentPaused = msg.data;

I see now; thanks.

> Then enabled will be consistent to true even paused by audiochannelservice. Is this enough?

I think that would be better, yes.

> Consider to the UI, I would like to add an attribute exposed to app called mozAudioChannelPaused 
> (onMozAudioChannelPaused).

I think this general idea is good.  But maybe this we can do in a separate bug.

Another problem with this approach: What happens if I do seekUp() while the radio is paused?  Since the radio is off, we can't do that operation.  But as far as the page is concerned, the radio is enabled, so seekUp() ought to work.  Even if we expose a "paused" attribute, there's no reason for a page to expect that "paused" means "can't seek".

So maybe this idea of lying about the enabled state is a bad idea.

Comment 45

[Marco Chen [:mchen]]

> I think this general idea is good.  But maybe this we can do in a separate bug

Ok.

> Another problem with this approach: What happens if I do seekUp() while the radio 
> is paused?

1. The seekUp() is performed by user and only app in foreground can be controlled by user.

2. The audio policy defined that foreground app will never be paused so there is no possible that seekUp() performed when audioChannelPaused.

Comment 46

[Justin Lebar (not reading bugmail)]

> 1. The seekUp() is performed by user and only app in foreground can be controlled by user.

Apps in the background can run JavaScript.  There's nothing stopping an app in the background from calling seekUp().

Indeed, an FM radio app might have a "scan" feature.  This feature would call seekUp(), then wait a few seconds, then call seekUp() again, until the user says "stop".  If the user starts an FM scan and then presses the home button, this scan should continue working (and indeed it will under the current implementation).

Comment 47

[Marco Chen [:mchen]]

Yes, you are right on FM doesn't only output the audio but also have other control commands. So there will be two kinds of FM apps which we need to consider here.

  Type 1. App only controls FMRadio in the foreground.
  Type 2. App may control FMRadio in the background.

I would like to define AudioChannel's behavior on FMRadio as below.

  Rule 1. Once FMRadio is asked to pause by AudioChannel then
    a. Mute the FM stream via AudioManager.
    b. Disable the FMRadio.

  Rule 2. Once there are any control commands (not including disable) coming 
    after AudioChannelPaused then we re-enable the FMRadio as well as do that 
    command.

Thus, the current FMRadio app in V1.X will just fall into category of rule 1 then we can get benefit of power saving. If there are any apps in the future fall into category of rule 2 then they still can work. And to mute FMRadio from rule 1 would be also adapted for AudioChannel policy too. The only disadvantage is that the command caused rule 2 will need additional time for re-enabling FMRadio.

How about this?
Thanks for your good comment.

Comment 48

[Marco Chen [:mchen]]

Since we find the way to listen crash process message target, so clear the needinfo.

Comment 49

[Mounir Lamouri (:mounir)]

(In reply to Justin Lebar [:jlebar] from comment #9)
> > From the app's or caller's view, this enable is really successful but paused by 
> > AudioChannelService's rule.
> 
> What happens if the caller reads FM.enabled?  I thought that would return
> false, and would only return true after the page became visible again.
> 
> Returning success and then not setting enabled to true is pretty confusing
> to me.
> 
> I understand that returning failure and then eventually setting enabled to
> true is also confusing.
> 
> I'm not entirely sure what to do here.  Maybe Fabrice or Mounir has an idea
> based on prior art.

Why do content need to read .enabled and call .setEnabled()? Make sure it can use the API and turn on and off the hardware when it needs it. This is something I believe we could have working correctly without allowing the content to worry about. The content could get the FMRadio object asynchronously, which will start the hardware if not already started. When this object is available, the content will be allowed to consider the hardware as enabled unless we consider that it is not allowed to access it. For example, if the page is in the background. We also turn off the hardware on a time out or when the object is garbage collected.

That approach isn't for this bug but a general idea on how I believe this could be fixed. I do not think checking .enabled every time you want to do something is a desirable approach. Even less if .enabled can be false at unexpected moments or if the returned value might not be trustable.

Comment 50

[Justin Lebar (not reading bugmail)]

So content acquires some sort of FM lock object and controls the radio via that object.  And we have the ability to take the lock away from content and content can get notified when the lock may be available...

That sounds much more sane than our current API to me.  What do you think, Marco?

It sounds like we can't fix this bug and also avoid regressing our multi-process story without making that big change.  Given that choice I guess we should just fix this bug (i.e., we should do the simple fix that I'd rejected earlier).

Comment 51

[Marco Chen [:mchen]]

Hi Mounir and Justin,

Thanks for your suggestion here in advance.

So the biggest concern you have is the mechanism of how latter content can know when the FMRadio became available. But not just actively checking .enabled again and again. Is this right? 

And when latter content calls setEnable() but get a fail result, even true of .enabled is not enough to notify content what happened. I think this is why Justin suggest to have a new API right?

I agreed all of you and maybe a new API for acquiring FM control object or return the reason for fail more make sense. 

> (i.e., we should do the simple fix that I'd rejected earlier).

Finally I agreed that we should keep the patch here going for V1.1 and have a mid term plan for further version. maybe V1.2.
If we all agreed this. then I will update patch tomorrow for comment 43 & 47.

Thanks.

Comment 52

[Justin Lebar (not reading bugmail)]

> So the biggest concern you have is the mechanism of how latter content can know when the FMRadio 
> became available. But not just actively checking .enabled again and again. Is this right? 

To be clear, my concern is that we should have a sane way of handling the FM radio across multiple processes.  We were trying to hack around this here, and we've rejected these potential hacks.

Things I'd like in this new API include:

* A "locking" mechanism that prevents other pages / processes from modifying the FM radio.
* A mechanism by which pages can learn when the radio is available for the taking.
* A mechanism by which pages can learn that their lock on the radio has been broken (we could use this e.g. for "FM radio has been muted, and therefore turned off")

Comment 53

[Marco Chen [:mchen]]

Hi Justin,

Would we continue to finish the working here for V1.1?
If I got the answer yes then will update patches tomorrow with comment 43 & 47.
Or I still learn much things here.

Comment 54

[Justin Lebar (not reading bugmail)]

> Would we continue to finish the working here for V1.1?

Let's do the simplest thing we can do in this bug, then file a new bug for the bigger fixes.  This is essentially what you proposed earlier, and I said no to.  Sorry.  :(

Comment 55

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #54)
> Let's do the simplest thing we can do in this bug, then file a new bug for
> the bigger fixes.  This is essentially what you proposed earlier, and I said
> no to.  Sorry.  :(

You are doing a right thing here and that is the responsibility of a reviewer. So I also thanks for your all comments here indeed.

Comment 56

[Marco Chen [:mchen]]

(In reply to Marco Chen [:mchen] from comment #53)
> Hi Justin,
> 
> Would we continue to finish the working here for V1.1?
> If I got the answer yes then will update patches tomorrow with comment 43 &
> 47.
> Or I still learn much things here.

After I implemented comment 47, I found a bigger issue which kills the goal of disabling FMRadio but mute when asking by AudioChannel.

According to rule 2 of comment 47, we can consider the flows as below.

  1. App started to play FMRadio.
  2. FMRadio is asked to disable from Gecko by AudioChannel. (App doesn't know FMRadio is disabled already.)
  3. App is starting to do much commands like seek-up/down.
  4. In order to apply comment 44, I proposed to enable FMRadio from Gecko first then do command from flow 3.

The problem is that enabling FMRadio is an async command and it needs about 4 seconds on unagi device. This means that during I re-enable FMRadio from Gecko, content can still fire a lots of commands. So the time gap for re-enabling FMRadio may break the logic on content side. (ex: content fired a timeout to recover seek with no response.)

------------------------------------

My own suggestion for now is
  1. Gecko remains to mute FMRadio by AudioChannel only. And this can make sure FMRadio followed Audio Competing Policy. (remaining work - Bug 836201)
  2. App used FM can do disable by itself because it know what exactly it needs. And related Web API is setOccupyChannel(...), this function can let app get the channel state (play or pause) from AudioChannelService.
     https://wiki.mozilla.org/WebAPI/AudioChannels#Application_API

  3. Resource protection: We still can do simple work here (second patch) for V1.1 and plan the new Web API for V1.X.

Thanks.

Comment 57

[Justin Lebar (not reading bugmail)]

> 2. App used FM can do disable by itself because it know what exactly it needs.

What API does the FM radio app use to know whether it should disable the radio?  I don't think setOccupyChannel is the right API call?

Or are you proposing we add something to the AudioChannel API?

Comment 58

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #57)
> > 2. App used FM can do disable by itself because it know what exactly it needs.
> 
> What API does the FM radio app use to know whether it should disable the
> radio?  I don't think setOccupyChannel is the right API call?
> 
> Or are you proposing we add something to the AudioChannel API?


Q1: Yes, I would like to modify the return type of setOccupyChannel() from void to some kind of callback interface. So no only app can set a channel to occupy but also be aware the status asked from AudioChannelService. And I will add Justin into Bug 829409 for this discussion. Thanks.

Q2: According to Q1, maybe V1.1 will be kept to mute FM only not pause. If so, could Justin help to review Bug 836201?

Q3: About resource protection, for V1.1 I will use the concept on second patch Is this ok?

Thanks.

Comment 59

[Justin Lebar (not reading bugmail)]

> Q1: Yes, I would like to modify the return type of setOccupyChannel() from void to some kind of 
> callback interface.

Okay.

> Q2: According to Q1, maybe V1.1 will be kept to mute FM only not pause. If so, could Justin help to 
> review Bug 836201?

Yes, I'm sorry I've taken so long to get to reviews this week, but I'll have a look as soon as I can.

> Q3: About resource protection, for V1.1 I will use the concept on second patch Is this ok?

In the past, I've made the mistake of implementing a hack instead of the right thing.  It ended up being a mistake because the hack wasn't much easier than the right thing, and anyway we slipped our schedule dramatically, so we had plenty of time for the right thing.

If you want to do this first, that's fine with me.  But I don't want to rule anything out for v1.1; we should endeavor to land the right thing for the earliest possible version.  I think we probably have plenty of time.

Comment 60

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #59)
> If you want to do this first, that's fine with me.  But I don't want to rule
> anything out for v1.1; we should endeavor to land the right thing for the
> earliest possible version.  I think we probably have plenty of time.

Yes, the right thing is more better then hack only if time is not urgent. So the following is

1. This bug will depend on Bug 829409 then it will be an gaia patch on FM app.

2. To create another bug for considering the new API for resource protection in the view of long term solution.

3. The second patch will be obsoleted then reserve to be a backup for any requesting from tef/shira/leo+.

Let us rock it in the right way.

Comment 61

[Marco Chen [:mchen]]

Release the assigner because 
  1. The new web api is out of scope of the bug I reported here.

  2. The proposed web api AudioChannelManager::SetOccupyChannel() is removed after discussion during work week.

Comment 62

[BMO Automation]

Firefox OS is not being worked on