Closed Bug 1157140 Opened 6 years ago Closed 5 years ago

Manage System app's audio channels in the AudioChannelService.

Categories

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: evanxd, Assigned: evanxd)

References

Details

Attachments

(3 files, 1 obsolete file)

We need the capability to manage the notification audio channel of System app in the AudioChannelManager(Bug 1100822).

Current proposals:
1. Add a notification app to play the notification sound.
2. Send the allowed audio channels(sysApp.allowedAudioChannels) to System app with `mozChromeEvent`.
3. The all audio channels of System app could be played by default.
Summary: Control the notification audio channel of System app in the AudioChannelManager. → Manage System app's audio channels in the AudioChannelManager.
Assignee: nobody → evanxd
We need to handle the below audio in System app.
1. gaia/apps/system/js/accessibility.js:
  266:         this.sounds[aSoundKey] = new Audio(this.soundURLs[aSoundKey]);

2. gaia/apps/system/js/carrier_info_notifier.js:
   66:     var ringtonePlayer = new Audio();

3. gaia/apps/system/js/dialer_agent.js:
   47:     var player = new Audio();

4. gaia/apps/system/js/icc_worker.js:
  193:     var tonePlayer = new Audio();

/5. gaia/apps/system/js/notifications.js:
  549:         var ringtonePlayer = new Audio();

6. gaia/apps/system/lockscreen/js/lockscreen.js:
  663:       var unlockAudio = new Audio('/resources/sounds/unlock.opus');
Figuring out a good way to manage the audio channels in System app...
Blocks: 1113086
Hi Alastor, Alive, Baku, and Dominic.

AudioChannelManager[1](the audio channel management module in System app) cannot manage System app's `allowedAudioChannels`. Because we just cannot access the `allowedAudioChannels` directly in System app.

In my opinion, we should find a way to access the `allowedAudioChannels` in System app. The below could be a solution for it.

Solution:
We could add a new mozChromeEvent called `system-audio-channels-initialized` to pass the `allowedAudioChannels` to System app. (or any other way to pass the `allowedAudioChannels` to System app)

Then we could just add the below code to manage the `allowedAudioChannels` in System app. Simple and easy.
```
function AudioChannelManager() {
  this.systemAudioChannels = new Map();
}

AudioChannelManager.prototype = {
  systemAudioChannels: null,

  handleMozChromeEvent: function(evt) {
    switch(evt.detail.type) {
      case 'system-audio-channels-initialized':
        var audioChannels = evt.detail.allowedAudioChannels;
        this.systemAudioChannels.forEach((audioChannel) => {
          this.audioChannels.set(
            audioChannel.name,
            // AudioChannelController[2] is a part of our audio channel management module in System app.
            new AudioChannelController({ instanceID: 'systemAppID' }, audioChannel)
          );
        });
        break;
    }
  }
};
```

How do you guys think about the above idea?

[1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/audio_channel_manager.js
[2]: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/audio_channel_controller.js
Flags: needinfo?(dkuo)
Flags: needinfo?(amarchesini)
Flags: needinfo?(alwu)
Flags: needinfo?(alive)
:evanxd, can you tell me why the system app cannot access the allowedAudioChannels?
In theory if you have access to the mozbrowser iframe, from there you can play with allowedAudioChannels.
Flags: needinfo?(amarchesini) → needinfo?(evanxd)
Because the mozbrowser iframe of System app is created by shell.js[1]. So we can access the System app's allowedAudioChannels in shell.js, not in the System app.

:baku, or do you know any way to access the (System app's) allowedAudioChannels in System app?

[1]: https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#307-313
Flags: needinfo?(evanxd) → needinfo?(amarchesini)
Our original intention of the new audio channel management is to use the system app to manage all the audio channels under gaia, in theory, system app should manage its own audio channels and those logic should also in system app. So if it's possible for system app to get its allowedAudioChannels then manage them, we can deal with those audio elements/context that directly in system app, without moving them outside of system app and probably wrapped in iframes.

But looks like if we want to use the system app's allowedAudioChannels, it can only be in shell.js? if so, I am assuming there will be some extra/duplicate logic in shell.js, which does exactly the same logic that system app's AudioChannelManager does. This should be Evan's question.
Flags: needinfo?(dkuo)
I would like to suggest to bypass the mozbrowser events in shell.js instead of exposing the audioChannel object to the app. But if gecko dev has different opinions let's listen.
Flags: needinfo?(alive)
I'm lost. Why do we want to use the System app's allowedAudioChannels?
Does the system app play some audio?

Any app, any iframe mozbroser has its own allowedAudioChannels. I guess that the system app has access to each iframe of each app running. If you want to mute the app A, you should mute any iframeAppA.allowedAudioChannels.
If you mute the allowedAudioChannels of the system app you are muting just the system app.
Is this what you mean? Plus, the system app cannot have access to its own allowedAudioChannels. We have to go up to the shell.js. But really I don't understand why you need this.
Can you tell me more?
Flags: needinfo?(amarchesini)
Hi, Baku,
Because of some reasons, Gaia don't want the notification belong to the specific app.
Therefore, the notification would be playback by the system app.
That is why we need to pass the events of the shell.js to the system app.
Flags: needinfo?(alwu)
(In reply to Andrea Marchesini (:baku) from comment #8)
> I'm lost. Why do we want to use the System app's allowedAudioChannels?
> Does the system app play some audio?
Yes, system app play some audio.

> Any app, any iframe mozbroser has its own allowedAudioChannels. I guess that
> the system app has access to each iframe of each app running. If you want to
> mute the app A, you should mute any iframeAppA.allowedAudioChannels.
> If you mute the allowedAudioChannels of the system app you are muting just
> the system app.
We're trying to find a good way to manage the audio channels in System app.

> Is this what you mean? Plus, the system app cannot have access to its own
> allowedAudioChannels. We have to go up to the shell.js. But really I don't
> understand why you need this.
> Can you tell me more?
We just need to manage the audio channels in System app.
We would like to try to use mozChromeEvent and mozContentEvent to control the allowed audio channels in System app, if we can not access its own audio channel in System app yet.

How about defining the below mozContentEvent and mozChromeEvent to do that?

# mozContentEvent
* system-audiochannel-list: Get the list of allowed audio channel name.
* system-audiochannel-play: Play the audio channel.
* system-audiochannel-pause: Pause the audio channel.
* system-audiochannel-volume: Set the volume of the audio channel.

# mozChromeEvent
* system-audiochannel-list: Get the list of allowed audio channel name.
* system-audiochannel-onsuccess: If it is successful, play, pause, set volume for an audio channel.
* system-audiochannel-onerror: If it is failed, play, pause, set volume for an audio channel.
Flags: needinfo?(dkuo)
Flags: needinfo?(alive)
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #11)
> We would like to try to use mozChromeEvent and mozContentEvent to control
> the allowed audio channels in System app, if we can not access its own audio
> channel in System app yet.
> 
> How about defining the below mozContentEvent and mozChromeEvent to do that?
> 
> # mozContentEvent
> * system-audiochannel-list: Get the list of allowed audio channel name.
> * system-audiochannel-play: Play the audio channel.
> * system-audiochannel-pause: Pause the audio channel.
> * system-audiochannel-volume: Set the volume of the audio channel.
> 
> # mozChromeEvent
> * system-audiochannel-list: Get the list of allowed audio channel name.

All above is fine

> * system-audiochannel-onsuccess: If it is successful, play, pause, set
> volume for an audio channel.
> * system-audiochannel-onerror: If it is failed, play, pause, set volume for
> an audio channel.

This is confusing.... how do you know what's the success/failed operation?
Flags: needinfo?(alive)
> > * system-audiochannel-onsuccess: If it is successful, play, pause, set
> > volume for an audio channel.
> > * system-audiochannel-onerror: If it is failed, play, pause, set volume for
> > an audio channel.
> 
> This is confusing.... how do you know what's the success/failed operation?

Ah, a mistake.
We should do the below things.
# mozChromeEvent
* system-audiochannel-play-onsuccess: Successful to play.
* system-audiochannel-play-onerror: Failed to play.
* system-audiochannel-pause-onsuccess: Successful to pause.
* system-audiochannel-pause-onerror: Failed to pause.
* system-audiochannel-volume-onsuccess: Successful to set volume.
* system-audiochannel-volume-onerror: Failed to set volume.
And add this one.

# mozChromeEvent
* system-audiochannel-state-changed: State of the audio channel is changed.
Summary: Manage System app's audio channels in the AudioChannelManager. → Manage System app's audio channels in the AudioChannelService.
Attached file The Gaia part (obsolete) —
The Gaia part of managing System's audio channels is done. Adding tests now!

For the Gecko part, we need a reviewer.
Comment on attachment 8600828 [details]
The Gaia part

https://github.com/mozilla-b2g/gaia/pull/29855/files
Attachment #8600828 - Attachment mime type: text/plain → text/x-github-pull-request
Comment 12 and comment 13 looks good.
Flags: needinfo?(dkuo)
Hi :baku,

After discussed in the above, we want to create new MozContentEvent and MozChromeEvent to control the System app's audio channel in the System app.

We will have the below events to do that.
# mozContentEvent
* system-audiochannel-list: Get the list of allowed audio channel name.
* system-audiochannel-play: Play the audio channel.
* system-audiochannel-pause: Pause the audio channel.
* system-audiochannel-volume: Set the volume of the audio channel.

# mozChromeEvent
* system-audiochannel-list: Get the list of allowed audio channel name.
* system-audiochannel-state-changed: State of the audio channel is changed.
* system-audiochannel-play-onsuccess: Successful to play.
* system-audiochannel-play-onerror: Failed to play.
* system-audiochannel-pause-onsuccess: Successful to pause.
* system-audiochannel-pause-onerror: Failed to pause.
* system-audiochannel-volume-onsuccess: Successful to set volume.
* system-audiochannel-volume-onerror: Failed to set volume.

How do you think of that?
And could you be the reviewer for the Gecko part(shell.js) in the future.
(I'm learning to contribute code to Gecko)
Thanks.

At same time, we're discussing "Exposing the browser api reference to the internal/certified apps(eg. System app) themselves?"[1] in the webapi mailing list.

For sure, if we expose the browser APIs, we don't need the above MozContentEvent and MozChromeEvent. 

[1]: https://groups.google.com/forum/#!topic/mozilla.dev.webapi/Ja6_ZuvPVqw
Flags: needinfo?(amarchesini)
Added tests[1] for System app's audio channel controller.

Continue to add tests for `SystemWindow`, then we could send review request.

[1]: https://github.com/evanxd/gaia/commit/ebdbc777d6f31237168c789511de4e6ed2ab7983
Look like the CI result[1] is not good. Fixing it and adding tests.

[1]: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=ee7a52eb0574bcbb7055cf46b571190593b83d62
Added tests for SystemWindow[1] and fixed the failed test.

The patch is done.
Waiting for CI[2] now.

Once CI is good, then we could start to review the patch.

[1]: https://github.com/evanxd/gaia/commit/98f0a3d91887aa1c782d88c15bb61af374708ac5
[2]: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=98f0a3d91887aa1c782d88c15bb61af374708ac5
Flags: needinfo?(amarchesini)
Sorry, still need info from :baku for Comment 19.
Flags: needinfo?(amarchesini)
Sorry for the delay,

> After discussed in the above, we want to create new MozContentEvent and
> MozChromeEvent to control the System app's audio channel in the System app.

well, this seems to be the fastest way to implement this feature.
I don't know if this is good for a performance point of view, but we can debug it later.

> * system-audiochannel-play: Play the audio channel.
> * system-audiochannel-pause: Pause the audio channel.

Why these 2? Can we have just 'mute' with a boolean?

> And could you be the reviewer for the Gecko part(shell.js) in the future.
> (I'm learning to contribute code to Gecko)

I can review this shell.js code.
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #24)
> Sorry for the delay,
> 
> > After discussed in the above, we want to create new MozContentEvent and
> > MozChromeEvent to control the System app's audio channel in the System app.
> 
> well, this seems to be the fastest way to implement this feature.
> I don't know if this is good for a performance point of view, but we can
> debug it later.
> 
Let's try to do it.

> > * system-audiochannel-play: Play the audio channel.
> > * system-audiochannel-pause: Pause the audio channel.
> 
> Why these 2? Can we have just 'mute' with a boolean?
Sure, we could do that(system-audiochannel-mute).

> 
> > And could you be the reviewer for the Gecko part(shell.js) in the future.
> > (I'm learning to contribute code to Gecko)
> 
> I can review this shell.js code.
Thanks. :)
Comment on attachment 8600828 [details]
The Gaia part

Hi Alive and Dominic,

Could you help to review the patch?

Thanks.
Attachment #8600828 - Flags: review?(alive)
Attachment #8600828 - Flags: review?(dkuo)
Continuing to work on the gecko part after fixing Bug 1161621.
Attachment #8600828 - Attachment is obsolete: true
Attachment #8600828 - Flags: review?(dkuo)
Attachment #8600828 - Flags: review?(alive)
Hi Alive and Dominic,

Could you help to review the patch?

Thanks.
Attachment #8603980 - Flags: review?(alive)
Attachment #8603980 - Flags: review?(dkuo)
Attachment #8603980 - Attachment description: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/29855 → Gaia part: https://github.com/mozilla-b2g/gaia/pull/29855
Comment on attachment 8603980 [details] [review]
Gaia part: https://github.com/mozilla-b2g/gaia/pull/29855

The most concern is the way to instantiate ACC.
Please do |new AudioChannelController(this, name);| in SystemWindow then you don't need to have the strange argument assignment in ACC's constructor.

One thing else: why need to instantiate SystemWindow in bootstrap instead of Core? Why are we waiting for applicationready here?
Attachment #8603980 - Flags: review?(alive)
If CI is good, we'll start to review the patch again.
Comment on attachment 8603980 [details] [review]
Gaia part: https://github.com/mozilla-b2g/gaia/pull/29855

CI[1] is good now.

Alive, could you help to review it again?
Thanks.

[1]: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=8661d86d6d45312e8811a3d3c860f2660e2b3e35
Attachment #8603980 - Flags: review?(alive)
Attached patch Gecko partSplinter Review
Comment on attachment 8603980 [details] [review]
Gaia part: https://github.com/mozilla-b2g/gaia/pull/29855

r=me with nits addressed
Attachment #8603980 - Flags: review?(alive) → review+
Comment on attachment 8605762 [details] [diff] [review]
Gecko part

Hi :baku,

Could you help to review the patch for shell.js?

Thanks.
Attachment #8605762 - Flags: review?(amarchesini)

:baku, one more question about `shell.sendChromeEvent` method in shell.js.
We only can do `shell.sendChromeEvent` after the `MozAfterPaint` event is triggered, right?

In my experiment, if I do `shell.sendChromeEvent` too early(before `MozAfterPaint` event is triggered), System app just cannot receive the MozChromeEvent.

Is above correct? Or I misunderstood something?

Thank.
Flags: needinfo?(amarchesini)
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #38)
> Comment on attachment 8603980 [details] [review]
> Gaia part: https://github.com/mozilla-b2g/gaia/pull/29855
> 
> r=me with nits addressed

Alive, thanks for the review.
I'll update the patch.
Comment on attachment 8605762 [details] [diff] [review]
Gecko part

Let's file a new bug for Gecko part.
Attachment #8605762 - Flags: review?(amarchesini)
Flags: needinfo?(amarchesini)
Blocks: 1165134
Comment on attachment 8603980 [details] [review]
Gaia part: https://github.com/mozilla-b2g/gaia/pull/29855

Looks good with Alive's github comments!
Attachment #8603980 - Flags: review?(dkuo) → review+
Thanks for the review, Dominic.

We will land it once CI[1] is good.

[1]: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=b7d6dca09cd386617a1c520ef91a02aef0f4eccc
master: https://github.com/mozilla-b2g/gaia/commit/117e5c4e22f51c756ae6198dea0d6123a4396cc8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.