1206621 - Merge AudioChannelManager to TV System App

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::TV::System, defect, P1)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

[Blocking Requested - why for this release]:

Audio policy management is changed after v2.1. We need the corresponding modification to be merged into TV System App.

Comment 1

[Evelyn Hung]

WIP: https://github.com/evelynhung/gaia/commit/433a39567393db2910ea2c97b2fa19ca7d4f7b9c

we also need bug 1206641 fixed for testing this patch.

Comment 2

[Evan Tseng [:evanxd]]

Hi Evelyn,

The below are all regression bugs caused by Bug 1100822.

Unresolved
* Bug 1199605 - The music play icon does not disappear after we kill a playing music app
* Bug 1180618 - Ringer is not managed by Audio Channel Service

Resolved
* Bug 1185051 - [B2G] Manage the sound of any website which is opened from the browser app
* Bug 1183870 - [B2G] Set current audio channel to "none" when there is no any audio playing
* Bug 1181982 - [B2G] Manage the sound of the attention window
* Bug 1180614 - Keyboard doesn't have click sound.
* Bug 1175050 - Listen mozSystemWindowChromeEvent event in SystemWindow
* Bug 1167077 - Disable all System app's audio channels when AudioChannelService is initializing.
* Bug 1166172 - Send MozContentEvent to gecko after the `system-first-paint` event is received.
* Bug 1165850 - Refactor: Remove the `_destroyed` event listener in AudioChannelController
* Bug 1158716 - The module name of AudioChannelManager is illegal

Comment 3

[Evelyn Hung]

Attachment: AudioChannel.patch

WIP patch, also need to apply the following one first
https://patch-diff.githubusercontent.com/raw/mozilla-b2g/gaia/pull/31966.patch

Comment 4

[Evelyn Hung]

After checking the bug list on comment 2, it looks like we need almost all of them since they were fixing common logic in the System. Then it's better to backporting the current version of all Audio Channel related modules, instead of cherry-pick one by one. I will start working on cutting a snapshot of these audio_channel_* files and makes them work on TV.

Comment 5

[Evelyn Hung]

Attachment: WIP patch running on TV

This patch backport the whole audio channel work to v2.1, and could be applied on partner side. We are testing it.

Comment 6

[Evelyn Hung]

@Luke, please follow my work. My local branch is here:
https://github.com/evelynhung/gaia/tree/issue-1206621

Comment 7

[Evelyn Hung]

Attachment: WIP v2 running on TV

Comment 8

[Josh Cheng [:josh]]

Hi Evelyn,
Will you continue work on this or Luke will help here?

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

Just discussed with SWU, I'll help check if the back-porting patches could work or if there's any thing missing from gecko side.  But still need Luke's or Evelyn's help.

Comment 10

[Luke Wagner [:luke]]

(You probably meant one of the other Lukes.)

Comment 11

[Evelyn Hung]

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #9)
> Just discussed with SWU, I'll help check if the back-porting patches could
> work or if there's any thing missing from gecko side.  But still need Luke's
> or Evelyn's help.

Kilik, please try the patch in comment 7 and it should already on the development TV in our office. According to Kusaba's words in a private mail, it works but I didn't see it working on our side. Let me know the result, thanks!

Comment 12

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Evelyn Hung [:evelyn] from comment #11)
> (In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #9)
> > Just discussed with SWU, I'll help check if the back-porting patches could
> > work or if there's any thing missing from gecko side.  But still need Luke's
> > or Evelyn's help.
> 
> Kilik, please try the patch in comment 7 and it should already on the
> development TV in our office. According to Kusaba's words in a private mail,
> it works but I didn't see it working on our side. Let me know the result,
> thanks!

Update : 
Since we don't have real tv source input in the test env.
I see TV app will be set muted via a BrowserElementAudioChannel which is not added into |systemAppFrame.allowedAudioChannels|, and this is why no "activestatechanged" event is received.

I need to understand what's the machinery for BrowserElementAudioChannel to be added into systemAppFrame.allowedAudioChannels.  Also need to check if |setMute| is done by app itself.
Will consult with Alastor for further information.

Comment 13

[Evelyn Hung]

Update my branch, now it should align to the version of attachment 8665993 [details] [diff] [review].
https://github.com/evelynhung/gaia/commits/issue-1206621

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I tried this patch on b2g desktop but still not working as expected. We'll need Alastor's help on getting the whole picture of current audio channel control mechanism. This will reduce the overhead of trial-and-error and future bug.

Hi Alastor, the only document I found for audio channel is https://wiki.mozilla.org/WebAPI/AudioChannels but it doesn't have the detailed information for implementing the control flow in TV system app. Could you help explain the mechanism, dos and don'ts to Evelyn and me. Thanks!

Comment 15

[Luke Chang [:lchang] (inactive)]

I've updated the patch and tried to fix a potential bug regarding the `SystemWindow` module. Please refer to the GitHub branch [1].

[1] https://github.com/luke-chang/gaia/commits/1206621_tv_audio_channel_manager

Comment 16

[Alastor Wu [:alwu]]

Hi, all,
Sorry about that, now we don't have a complete document for the new audio channel architecture, because we just finished the refactoring.
If you have any questions, we can talk face to face, I think it's more efficiently :)

Comment 17

[Luke Chang [:lchang] (inactive)]

We may need bug 1199420 to backport as well.

Comment 18

[GitHub Autolander Bot]

Attachment: [gaia] evelynhung:issue-1206621 > mozilla-b2g:master

Comment 19

[Evelyn Hung]

Comment on attachment 8675596 [details] [review]
[gaia] evelynhung:issue-1206621 > mozilla-b2g:master

in this patch, I backported latest version of audio channel management, except bug 1190434 (I haven't figured out its impact). 

The major difference of TV version and phone version are two:

1. how to get top most window: On phone, we listen to `hierarchytopmostwindowchanged` and use |Service.query('getTopMostWindow')| to get top most window. However, on TV, we don't want to backport hierarchy manager because it's not trivial and we had focus manager to provide similar information. Therefore we listen to `focuschanged` event and use |AppWindowManager.getActiveApp().getTopMostWindow()| to get top most window.

2. when/how to bring up audio channel service: on phone, module loading sequence has been moved into app_core.js, but TV doesn't have corresponding changes, therefore we initial `AudioChannelService` in core.js.

@Evan, as the owner of audio channel management, we need your feedback here. It's sad that we have to fork implementation in this way. We are also working on both system app merge so hopefully we could remove these duplications soon. Before that, please also help making corresponding changes on TV version if it's a common issue - we don't want them out of sync. I've list those unresolved issues on this bug as a note. I'm so sorry for the double effort. Let me know if there is a way to make our live easier. Thanks.

Comment 20

[Evelyn Hung]

Per offline talk with Evan, it looks like bug 1190434 is a short-term solution and default playing any normal channel in background sounds incorrect on TV. We won't introduce this fix for now.

Comment 21

[Evelyn Hung]

I just updated my PR and add the latest landed bug 1180618 to make sure both phone and tv version are still synced.

Comment 22

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8675596 [details] [review]
[gaia] evelynhung:issue-1206621 > mozilla-b2g:master

Since the files are basically copied from phone system app, I only reviewed the modified part (such as "focuschanged" and "_getTopMostWindow") and it looks good to me, except two nits on it.

However, it would be still better to hear Evan's feedback before landing. Thanks.

Comment 23

[Evan Tseng [:evanxd]]

Comment on attachment 8675596 [details] [review]
[gaia] evelynhung:issue-1206621 > mozilla-b2g:master

LGTM.

Comment 24

[Evelyn Hung]

Thank you guys! I will land it when the light is green.

Comment 25

[Evelyn Hung]

Merged into master
https://github.com/mozilla-b2g/gaia/commit/47c668e1988b7341b6411b16776d736586a6779d

Comment 26

[Kevin Grandon :kgrandon]

This broke the linter, I pushed a follow-up to fix it: https://github.com/mozilla-b2g/gaia/commit/9ab82aa9852a2dc26f6ec62d42488777f86f485f

Comment 27

[Evelyn Hung]

Sorry I didn't notice it. Thank you Kevin!