1100822 - Implement AudioChannelManager module

Status: RESOLVED FIXED

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

Description

[Evan Tseng [:evanxd]]

Base on the architecture of audio channel manager[1], implement AudioChannelManager with System app's BaseModule[2].

[1] https://gist.github.com/evanxd/c5ee3c5b8a389e01227a
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/base_module.js

Comment 1

[Evan Tseng [:evanxd]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

WIP patch.

Comment 2

[Evan Tseng [:evanxd]]

We might use configuration policy to manage audio channel, and the logic(code) will be in the gecko module.

JSON data of audio channel competing table:
https://gist.github.com/evanxd/3faa81ba13cfd03bb601

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Please put written update on the bug, or a link if it's written somewhere else. Thanks.

Comment 4

[Evan Tseng [:evanxd]]

Hi Tim,

We're waiting for the implementation of audio channel management APIs(Bug 1113086) and some experiments for the performance of the api call.

After the API is ready, we will continue to work on the gaia module.

Comment 5

[Evan Tseng [:evanxd]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Hi Alive,

Could you give feedbacks for the architecture of audio channel manager?

Thanks.

Comment 6

[Evan Tseng [:evanxd]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Hi Dominic,

Could you give feedbacks for the architecture of audio channel manager?

Thanks.

Comment 7

[Evan Tseng [:evanxd]]

The below is the API interface of BrowserElementAudioChannel[1].

```
+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+[Pref="dom.mozBrowserFramesEnabled",
+ CheckPermissions="browser"]
+interface BrowserElementAudioChannel : EventTarget {
+  readonly attribute AudioChannel name;
+
+  // This event is dispatched when this audiochannel is actually in used by the
+  // app or one of the sub-iframes.
+  attribute EventHandler onactivestatechanged;
+
+  [Throws]
+  DOMRequest getVolume();
+
+  [Throws]
+  DOMRequest setVolume(float aVolume);
+
+  [Throws]
+  DOMRequest getMuted();
+
+  [Throws]
+  DOMRequest setMuted(boolean aMuted);
+
+  [Throws]
+  DOMRequest isActive();
+};
```

And you could see the UX spec in [2].

[1]: https://bug1113086.bugzilla.mozilla.org/attachment.cgi?id=8553107
[2]: https://bug961967.bugzilla.mozilla.org/attachment.cgi?id=8541542

Comment 8

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

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

I am not giving f+ because I don't see how compete() works; it's the most important part of this module but it's empty in the patch. All others are fine to me.

Comment 9

[Evan Tseng [:evanxd]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Hi Alvie,

How about the implementation[1] of `_compete` method?

Thanks for the feedbacks.

[1]: https://github.com/mozilla-b2g/gaia/pull/26512/files#diff-b7265a16f6393c7563f4f3ece331d565R108

Comment 10

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

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Several comments but basically fine for round 1.

Comment 11

[Dominic Kuo [:dkuo]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Overall it looks good, just some naming issues and thanks for working on this! there will be some changes about the telephony and voip channels, which I am not sure either but I will let you know after the gecko changes are confirmed.

Comment 12

[Evan Tseng [:evanxd]]

cc Steve Lee.

Comment 13

[GitHub Autolander Bot]

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

Comment 14

[Evan Tseng [:evanxd]]

Debugging the WIP patch to handle foreground app with content type and background app with content type.

And updated the UX spec for handeling two content type apps(discussed with Jenny):
PAUSE the current channel, and RESUME the current channel after the new channel ends.

Comment 15

[Evan Tseng [:evanxd]]

Fixed bugs described on Comment 14.

Currently we could handle the cases for FM and Music apps, but could not resume the paused audio channel. Continue to work on that.

Comment 16

[Evan Tseng [:evanxd]]

Now we can resume the paused audio channel to handle the cases of content and alarm channels.

But there might be bugs in gecko module. The notification channel will play without doing `channel.setMuted(false)` when content channel is playing.

Comment 17

[Evan Tseng [:evanxd]]

Discussed with Dominic in person. We all think we should update the spec[1].

In current implementation of gecko module, we always resume the paused channel in different situations. And looks like that is also what we want.

So let's change
`1. PAUSE the current channel, no RESUME the current channel after the new channel ends`
to
`1. PAUSE the current channel, RESUME the current channel after it goes back to foreground`

[1]: https://bugzilla.mozilla.org/attachment.cgi?id=8541542

Comment 18

[Evan Tseng [:evanxd]]

Added a WIP commit for handling `3. VOLUME down the current channel` and `4. VOLUME down the new channel` cases in the spec.

The gecko module might cause problems of home button, like nothing happen after clicking home button. And currently we don't know the reasons. Alastor is figuring out that.

Comment 19

[Evan Tseng [:evanxd]]

The WIP commit: https://github.com/evanxd/gaia/commit/c5e4ddcd44913e89da8c7cc52d863eb418daf713

Comment 20

[Evan Tseng [:evanxd]]

Currently, we can handle fade out cases. :)

Comment 21

[Evan Tseng [:evanxd]]

Now let's start to code for handling the `1. PAUSE the current channel, RESUME the current channel after it goes back to foreground` case in Comment 17.

Comment 22

[Evan Tseng [:evanxd]]

A WIP patch to handle the case 1 in the spec: https://github.com/evanxd/gaia/commit/f1df980502f20fe198a7fa0135332bc2a379e2d8

Comment 23

[Evan Tseng [:evanxd]]

Currently, we supported the case 1(`1. PAUSE the current channel, RESUME the current channel after it goes back to foreground`).

Comment 24

[Evan Tseng [:evanxd]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Hi Alive,

Could you help to review the patch?

Thanks.

Comment 25

[Evan Tseng [:evanxd]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Hi Dominic,

Could you help to review the patch?

Thanks.

Comment 26

[Evan Tseng [:evanxd]]

You guys could apply the patches[1] to build the gecko module for running the audio channel management module.

Or later, Alastor could provide the image included gecko module here.

[1] http://bugzil.la/1113086

Comment 27

[Evan Tseng [:evanxd]]

Now I'm going to add tests. Unit tests first, then UI tests.

Comment 28

[Evan Tseng [:evanxd]]

How to enable the AudioChannelManager.
1. To install the gecko module to enable the APIs:
* Unzip the b2g.tar.gz attachment[1].
* Run `adb shell mount -o remount,rw /system` to have capability to write file system if you don't have it.
* Run `adb push path/to/the/attachment/directory/b2g /system/b2g/`
2. Install AudioChannelManager.
* Git rebase to `f6b79487eb188e7a818d578a23c54b10b684b6d3`. (because of gecko issues)
* Apply our patches to install the gaia module in [2].
* Run `make reset-gaia`.
3. Done.

[1]: https://drive.google.com/open?id=0B3MxK4_nj3bpcnZwUFdNWlM3Nkk&authuser=1
[2]: https://github.com/mozilla-b2g/gaia/pull/26512/commits

Comment 29

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

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Offline discussion: the next goal of this patch is to split AudioChannelManager into Manager and AudioChannelController.

Comment 30

[Evan Tseng [:evanxd]]

Update UX spec:
v1.5.1: https://bug1068219.bugzilla.mozilla.org/attachment.cgi?id=8579177

Comment 31

[Evan Tseng [:evanxd]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Hi Alive,

Could you help to review the patch?

Thanks.

Comment 32

[Evan Tseng [:evanxd]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Hi Dominic,

Could you help to review the patch?

Thanks.

Comment 33

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

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Cancel review per offline discussion. It's already in a good shape though.

Comment 34

[Dominic Kuo [:dkuo]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

This review is mostly done by offline discussion, nice work and Evan will address the issues then request another review.

Comment 35

[Evan Tseng [:evanxd]]

We're fixing race condition issues and updating patch for review comments.

Comment 36

[Evan Tseng [:evanxd]]

Sent 4 comments to update patch for review comments:
* Remove all promises: https://github.com/evanxd/gaia/commit/3cfaaf279d5937bb7c71120d3bbafc3cbeb8d7e2
* Remove the _interruptedAudioChannels map: https://github.com/evanxd/gaia/commit/6489b1cf52c77ab5817ed06f95d8e70518f362a8
* Add audiochannelterminated event: https://github.com/evanxd/gaia/commit/ac6f922298731a3d1b1c9d7c7992871a5f140218
* WIP: Fix bugs of _handle_hierarchytopmostwindowchanged method: https://github.com/evanxd/gaia/commit/a902c8cc7eb89d3fb37ae235f6f9aa09ff7ea044

Comment 37

[Evan Tseng [:evanxd]]

Send comments[1] to update patch for review comments.
* Refactor the resume things
* Refactor _resumeAudioChannels method
* Add policy attribute in ACC
* Remove useless variable
* Add proceedPolicy method to handle audio channel in ACC
* Fix test
* Register _created event for all types of app window`

The work of updating patch for review comments is almost done, I think we could send review request again today.

[1] https://github.com/mozilla-b2g/gaia/pull/26512/commits

Comment 38

[Evan Tseng [:evanxd]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Hi Alive,

Could you help to review the patch?

Thanks.

Comment 39

[Evan Tseng [:evanxd]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Hi Dominic,

Could you help to review the patch?

Thanks.

Comment 40

[Evan Tseng [:evanxd]]

Then let's write tests. :D

Comment 41

[Evan Tseng [:evanxd]]

Added unit tests for audio_channel_manager.js. https://github.com/evanxd/gaia/commit/2385e505b50efc79f35807d8f0bfcd931b632b73

Comment 42

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

(I will read this but maybe tomorrow or by end of Thursday.)

Comment 43

[Evan Tseng [:evanxd]]

Added unit tests:
* Add tests for AudioChannelController: https://github.com/evanxd/gaia/commit/3af800275ad7e10850fd0d777bd9e563f8c3ebc9
* WIP: add tests for AudioChannelPolicy: https://github.com/evanxd/gaia/commit/35f1dad3353472819736efb7163faa7bf45eaac4

Comment 44

[Evan Tseng [:evanxd]]

Almost done for tests. GO GO!

Comment 45

[Evan Tseng [:evanxd]]

Added tests for AudioChannelPolicy:
* Fix bugs and add tests for policy 1: https://github.com/evanxd/gaia/commit/d423a3f1fe1ac9d30e1b2120efac9c5a4553708c
* Refactor tests: https://github.com/evanxd/gaia/commit/91baa5cd988b33417158942073872cc418a657c5
* Add tests for AudioChannelPolicy: https://github.com/evanxd/gaia/commit/8d51e7ec05c6741d3c7f04553c75b17d33ea10d1

Comment 46

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

Quickly read the code and don't see big issue. Well done!
I'd like to read whole tests once you finished so I am going to cancel review for this time. Thanks!

Comment 47

[Evan Tseng [:evanxd]]

Thanks, Alive and Dominic.
I'll let you know when I finish the tests.

Comment 48

[Evan Tseng [:evanxd]]

Add tests for AudioChannelManager:
* Add test for audio channels in same app: https://github.com/evanxd/gaia/commit/01ff922421fc8de29760632a5a391f790cb0bb72
* Add tests for AudioChannelManager: https://github.com/evanxd/gaia/commit/7e6520a3ba9112d20e53f31762da73adc11a4757

Comment 49

[Evan Tseng [:evanxd]]

Add tests for AudioChannelManager:
* Add tests for _isAudioChannelInBackground: https://github.com/evanxd/gaia/commit/127518f04300751e0ff9349ca322ae539476d5eb
* Add tests for _deleteAudioChannelFromInterruptedAudioChannels: https://github.com/evanxd/gaia/commit/94a0dc1858eab1e0831741de08ea1cd9e1019744

Comment 50

[Evan Tseng [:evanxd]]

Finished the task of adding tests, and did rebase.
Waiting for the CI result[1].

If everything is good, then Alive and Dominic will check the tests.
And I think we could land the patch if the tests are also good.
GOGO!

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

Comment 51

[Evan Tseng [:evanxd]]

Fixing the failed marionette tests.

Comment 52

[Evan Tseng [:evanxd]]

Found the bug. The script tag was just not typed completely.

`<script defer src="js/audio_channel_controller.js"></`

=>

`<script defer src="js/audio_channel_controller.js"></script>`[1]

Waiting the CI result.

[1]: https://github.com/mozilla-b2g/gaia/pull/26512/files#diff-61c16a0a0190e95d51031835f62b30d7R373

Comment 53

[Evan Tseng [:evanxd]]

The CI looks good.
https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=c5390d6594a3b4b7e212c5cc71ceadb5b02f836f

Comment 54

[Evan Tseng [:evanxd]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Hi Alive,

Test added.
Could you help to review the patch again?

CI is good[1].

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

Comment 55

[Evan Tseng [:evanxd]]

Another try[1], still good, nice! :)

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

Comment 56

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

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Some nits but generally this is already good enough. r=me, thanks for your patience for such a long time. Hope you'd learned something during the review progress.

Comment 57

[Dominic Kuo [:dkuo]]

Comment on attachment 8529528 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512

Thanks Evan, the updated patch looks nice, I have found some indent issues and just address them before landing it. Let's move on to the next bug for the new audio channel service!

Comment 58

[Evan Tseng [:evanxd]]

Thanks Alvie and Dominic.
Really learned a lot from the review.

Updated the test and nits.
Waiting for CI[1].

Once CI is good, we will land the code. \o/

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

Comment 59

[Evan Tseng [:evanxd]]

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