1157140 - Manage System app's audio channels in the AudioChannelService.

Status: RESOLVED FIXED

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

Description

[Evan Tseng [:evanxd]]

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.

Comment 1

[Evan Tseng [: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');

Comment 2

[Evan Tseng [:evanxd]]

Figuring out a good way to manage the audio channels in System app...

Comment 3

[Evan Tseng [:evanxd]]

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

Comment 4

[Andrea Marchesini [:baku]]

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

Comment 5

[Evan Tseng [: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

Comment 6

[Dominic Kuo [:dkuo]]

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.

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

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.

Comment 8

[Andrea Marchesini [:baku]]

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?

Comment 9

[Alastor Wu [:alwu]]

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.

Comment 10

[Evan Tseng [:evanxd]]

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

Comment 11

[Evan Tseng [:evanxd]]

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.

Comment 12

[(inactive after 6/18) Alive Kuo [: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?

Comment 13

[Evan Tseng [:evanxd]]

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

Comment 14

[Evan Tseng [:evanxd]]

And add this one.

# mozChromeEvent
* system-audiochannel-state-changed: State of the audio channel is changed.

Comment 15

[GitHub Autolander Bot]

Attachment: [gaia] evanxd:bug-1157140 > mozilla-b2g:master

Comment 16

[Evan Tseng [:evanxd]]

Attachment: The Gaia part

The Gaia part of managing System's audio channels is done. Adding tests now!

For the Gecko part, we need a reviewer.

Comment 17

[Evan Tseng [:evanxd]]

Comment on attachment 8600828 [details]
The Gaia part

https://github.com/mozilla-b2g/gaia/pull/29855/files

Comment 18

[Dominic Kuo [:dkuo]]

Comment 12 and comment 13 looks good.

Comment 19

[Evan Tseng [:evanxd]]

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

Comment 20

[Evan Tseng [:evanxd]]

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

Comment 21

[Evan Tseng [:evanxd]]

Look like the CI result[1] is not good. Fixing it and adding tests.

[1]: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=ee7a52eb0574bcbb7055cf46b571190593b83d62

Comment 22

[Evan Tseng [:evanxd]]

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

Comment 23

[Evan Tseng [:evanxd]]

Sorry, still need info from :baku for Comment 19.

Comment 24

[Andrea Marchesini [:baku]]

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.

Comment 25

[Evan Tseng [:evanxd]]

(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 26

[Evan Tseng [:evanxd]]

Updated patch[1] for Comment 24 and fixing bugs.
Waiting the CI[2].

[1]: https://github.com/evanxd/gaia/commit/1c4ae743581d9a918b398237892941e45a872a3e
[2]: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=1c4ae743581d9a918b398237892941e45a872a3e

Comment 27

[Evan Tseng [:evanxd]]

Comment on attachment 8600828 [details]
The Gaia part

Hi Alive and Dominic,

Could you help to review the patch?

Thanks.

Comment 28

[Evan Tseng [:evanxd]]

Continuing to work on the gecko part after fixing Bug 1161621.

Comment 29

[Evan Tseng [:evanxd]]

Comment on attachment 8600828 [details]
The Gaia part

>https://github.com/mozilla-b2g/gaia/pull/29855

Comment 30

[Evan Tseng [:evanxd]]

Attachment: Gaia part: https://github.com/mozilla-b2g/gaia/pull/29855

Hi Alive and Dominic,

Could you help to review the patch?

Thanks.

Comment 31

[(inactive after 6/18) Alive Kuo [:alive]]

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?

Comment 32

[Evan Tseng [:evanxd]]

Thanks for the review, Alive.

Updated for comments[1].
Now waiting for CI[2].

[1]: https://github.com/evanxd/gaia/commit/84d6d10ab80bf1de3d2edbe7d05b6c68f62ea48f
[2]: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=84d6d10ab80bf1de3d2edbe7d05b6c68f62ea48f

Comment 33

[Evan Tseng [:evanxd]]

If CI is good, we'll start to review the patch again.

Comment 34

[Evan Tseng [:evanxd]]

Some tests were failed in [1].
Fixing it.

[1]: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=84d6d10ab80bf1de3d2edbe7d05b6c68f62ea48f

Comment 35

[Evan Tseng [:evanxd]]

Try to run CI[1] again.

[1]: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=38ced2737f068b3ea2e90fc6a7c35265caf033f5

Comment 36

[Evan Tseng [:evanxd]]

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

Comment 37

[Evan Tseng [:evanxd]]

Attachment: Gecko part

Comment 38

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8603980 [details] [review]
Gaia part: https://github.com/mozilla-b2g/gaia/pull/29855

r=me with nits addressed

Comment 39

[Evan Tseng [:evanxd]]

Comment on attachment 8605762 [details] [diff] [review]
Gecko part

Hi :baku,

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

Thanks.

Comment 40

[Evan Tseng [:evanxd]]

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

Comment 41

[Evan Tseng [:evanxd]]

(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 42

[Evan Tseng [:evanxd]]

Comment on attachment 8605762 [details] [diff] [review]
Gecko part

Let's file a new bug for Gecko part.

Comment 43

[Dominic Kuo [:dkuo]]

Comment on attachment 8603980 [details] [review]
Gaia part: https://github.com/mozilla-b2g/gaia/pull/29855

Looks good with Alive's github comments!

Comment 44

[Evan Tseng [:evanxd]]

Thanks for the review, Dominic.

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

[1]: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=b7d6dca09cd386617a1c520ef91a02aef0f4eccc

Comment 45

[Evan Tseng [:evanxd]]

Run CI one more time: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=cf374d8cfe1f97bb05b5161d47c2d767401fb5df

Comment 46

[Evan Tseng [:evanxd]]

master: https://github.com/mozilla-b2g/gaia/commit/117e5c4e22f51c756ae6198dea0d6123a4396cc8