Merge AudioChannelManager to TV System App

RESOLVED FIXED in FxOS-S9 (16Oct)

Status

defect
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: schien, Assigned: jj.evelyn)

Tracking

unspecified
FxOS-S9 (16Oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.5+)

Details

(Whiteboard: [ft:conndevices][partner-blocker][partner-cherry-picked<2015/11/4>])

Attachments

(2 attachments, 2 obsolete attachments)

[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.
(Assignee)

Updated

4 years ago
Depends on: 1206641
(Assignee)

Comment 1

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

we also need bug 1206641 fixed for testing this patch.

Updated

4 years ago
Blocks: 1206805
blocking-b2g: 2.5? → 2.5+

Updated

4 years ago
Priority: -- → P1
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
(Assignee)

Comment 3

4 years ago
Posted patch AudioChannel.patch (obsolete) — Splinter Review
WIP patch, also need to apply the following one first
https://patch-diff.githubusercontent.com/raw/mozilla-b2g/gaia/pull/31966.patch

Updated

4 years ago
Target Milestone: --- → FxOS-S8 (02Oct)

Updated

4 years ago
Assignee: nobody → ehung
Status: NEW → ASSIGNED
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
Posted patch WIP patch running on TV (obsolete) — Splinter Review
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
(Assignee)

Comment 6

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

Comment 7

4 years ago
Attachment #8665899 - Attachment is obsolete: true

Updated

4 years ago
Blocks: 1199985
Hi Evelyn,
Will you continue work on this or Luke will help here?
Flags: needinfo?(luke)
Flags: needinfo?(ehung)
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.)
Flags: needinfo?(luke)

Updated

4 years ago
Flags: needinfo?(lchang)

Updated

4 years ago
Target Milestone: FxOS-S8 (02Oct) → FxOS-S9 (16Oct)
(Assignee)

Comment 11

4 years ago
(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.
Flags: needinfo?(kikuo)
(Assignee)

Comment 13

4 years ago
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!
Flags: needinfo?(alwu)
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
Flags: needinfo?(lchang)
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 :)

Updated

4 years ago
Flags: needinfo?(alwu)

Updated

4 years ago
Depends on: 1214148
We may need bug 1199420 to backport as well.
Depends on: 1199420
(Assignee)

Updated

4 years ago
(Assignee)

Comment 19

4 years ago
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.
Attachment #8675596 - Flags: review?(lchang)
Attachment #8675596 - Flags: feedback?(evan)

Updated

4 years ago
Depends on: 1216417
(Assignee)

Comment 20

4 years ago
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.
(Assignee)

Comment 21

4 years ago
I just updated my PR and add the latest landed bug 1180618 to make sure both phone and tv version are still synced.
(Assignee)

Updated

4 years ago
Whiteboard: [ft:conndevices][partner-blocker] → [ft:conndevices][partner-blocker][partner-cherry-pick]
(Assignee)

Updated

4 years ago
Blocks: 1201451
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+
(Assignee)

Comment 24

4 years ago
Thank you guys! I will land it when the light is green.
(Assignee)

Comment 25

4 years ago
Merged into master
https://github.com/mozilla-b2g/gaia/commit/47c668e1988b7341b6411b16776d736586a6779d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 27

4 years ago
Sorry I didn't notice it. Thank you Kevin!
Duplicate of this bug: 1214520

Updated

4 years ago
Whiteboard: [ft:conndevices][partner-blocker][partner-cherry-pick] → [ft:conndevices][partner-blocker][partner-cherry-picked<2015/11/5>]

Updated

4 years ago
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.