Closed Bug 1206621 Opened 5 years ago Closed 5 years ago
Channel Manager to TV System App
[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.
WIP: https://github.com/evelynhung/gaia/commit/433a39567393db2910ea2c97b2fa19ca7d4f7b9c we also need bug 1206641 fixed for testing this patch.
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
WIP patch, also need to apply the following one first https://patch-diff.githubusercontent.com/raw/mozilla-b2g/gaia/pull/31966.patch
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.
This patch backport the whole audio channel work to v2.1, and could be applied on partner side. We are testing it.
Attachment #8664180 - Attachment is obsolete: true
@Luke, please follow my work. My local branch is here: https://github.com/evelynhung/gaia/tree/issue-1206621
Hi Evelyn, Will you continue work on this or Luke will help here?
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.
(You probably meant one of the other Lukes.)
(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!
Flags: needinfo?(ehung) → needinfo?(kikuo)
(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.
Update my branch, now it should align to the version of attachment 8665993 [details] [diff] [review]. https://github.com/evelynhung/gaia/commits/issue-1206621
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!
I've updated the patch and tried to fix a potential bug regarding the `SystemWindow` module. Please refer to the GitHub branch .  https://github.com/luke-chang/gaia/commits/1206621_tv_audio_channel_manager
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 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.
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.
I just updated my PR and add the latest landed bug 1180618 to make sure both phone and tv version are still synced.
Whiteboard: [ft:conndevices][partner-blocker] → [ft:conndevices][partner-blocker][partner-cherry-pick]
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.
Attachment #8675596 - Flags: review?(lchang) → review+
Comment on attachment 8675596 [details] [review] [gaia] evelynhung:issue-1206621 > mozilla-b2g:master LGTM.
Attachment #8675596 - Flags: feedback?(evan) → feedback+
Thank you guys! I will land it when the light is green.
Merged into master https://github.com/mozilla-b2g/gaia/commit/47c668e1988b7341b6411b16776d736586a6779d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
This broke the linter, I pushed a follow-up to fix it: https://github.com/mozilla-b2g/gaia/commit/9ab82aa9852a2dc26f6ec62d42488777f86f485f
Sorry I didn't notice it. Thank you Kevin!
Whiteboard: [ft:conndevices][partner-blocker][partner-cherry-pick] → [ft:conndevices][partner-blocker][partner-cherry-picked<2015/11/5>]
Whiteboard: [ft:conndevices][partner-blocker][partner-cherry-picked<2015/11/5>] → [ft:conndevices][partner-blocker][partner-cherry-picked<2015/11/4>]
You need to log in before you can comment on or make changes to this bug.