Closed Bug 1100822 Opened 10 years ago Closed 9 years ago

Implement AudioChannelManager module

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::System::Window Mgmt
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: evanxd, Assigned: evanxd)

References

Details

Attachments

(2 files)

Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26512
46 bytes, text/x-github-pull-request
dkuo
: review+
alive
: review+
dkuo
: feedback+
alive
: feedback+
Details | Review
[gaia] evanxd:bug-1100822-patch > mozilla-b2g:master
46 bytes, text/x-github-pull-request
Details | Review 
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
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
Please put written update on the bug, or a link if it's written somewhere else. Thanks.
Flags: needinfo?(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.
Flags: needinfo?(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.
Attachment #8529528 - Flags: feedback?(alive)
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.
Attachment #8529528 - Flags: feedback?(dkuo)
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 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.
Attachment #8529528 - Flags: feedback?(alive)
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
Attachment #8529528 - Flags: feedback?(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.
Attachment #8529528 - Flags: feedback?(alive) → feedback+
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.
Attachment #8529528 - Flags: feedback?(dkuo) → feedback+
cc Steve Lee.
Blocks: 1113086
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.
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.
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.
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
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.
Currently, we can handle fade out cases. :)
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.
A WIP patch to handle the case 1 in the spec: https://github.com/evanxd/gaia/commit/f1df980502f20fe198a7fa0135332bc2a379e2d8
Currently, we supported the case 1(`1. PAUSE the current channel, RESUME the current channel after it goes back to foreground`).
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.
Attachment #8529528 - Flags: review?(alive)
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.
Attachment #8529528 - Flags: review?(dkuo)
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
Now I'm going to add tests. Unit tests first, then UI tests.
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 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.
Attachment #8529528 - Flags: review?(dkuo)
Attachment #8529528 - Flags: review?(alive)
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.
Attachment #8529528 - Flags: review?(alive)
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.
Attachment #8529528 - Flags: review?(dkuo)
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.
Attachment #8529528 - Flags: review?(alive)
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.
Attachment #8529528 - Flags: review?(dkuo)
We're fixing race condition issues and updating patch for review comments.
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
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 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.
Attachment #8529528 - Flags: review?(alive)
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.
Attachment #8529528 - Flags: review?(dkuo)
Then let's write tests. :D
Added unit tests for audio_channel_manager.js. https://github.com/evanxd/gaia/commit/2385e505b50efc79f35807d8f0bfcd931b632b73
(I will read this but maybe tomorrow or by end of Thursday.)
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
Almost done for tests. GO GO!
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!
Thanks, Alive and Dominic.
I'll let you know when I finish the tests.
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
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
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
Fixing the failed marionette tests.
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 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
Attachment #8529528 - Flags: review?(alive)
Blocks: 1157140
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.
Attachment #8529528 - Flags: review?(alive) → review+
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!
Attachment #8529528 - Flags: review?(dkuo) → review+
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
master: https://github.com/mozilla-b2g/gaia/commit/a69de6f18a940064f831a1c282c697965a7ac8fe
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: