1177254 - Move the 'audio-channel-changed' and 'visible-audio-channel-changed' events to Gaia::AudioChannelService

Status: RESOLVED FIXED

(Firefox OS Graveyard :: AudioChannel, defect)

Description

[Alastor Wu [:alwu]]

Because the system app can know all audio information about every iframes, we can remove the codes which handle the 'audio-channel-changed' and 'visible-audio-channel-changed' events.

# audio-channel-changed
In present codebase, the sound_manager uses 'audio-channel-changed' event to know the active audio channel. This can be stored in Gaia::AudioChannelManager.

# visible-audio-channel-changed
The visibility_manager uses this event to monitor the new visible channel, and check its channel type to do something. I think we could also get these information from Gaia::AudioChannelManager.

Comment 1

[Alastor Wu [:alwu]]

After discuss with Evan, we still need these events, but move them to Gaia::AudioChannelService.

# audio-channel-changed
The sound_manager handles hardware volume key events, bluetooth volume changes, and volume/channel change events. It uses this event to know the current audio channel in order to send the correct volume setting event. ex. audio.volume.[channel_name]

Seems that the sound_manager have different job with the audio channel competing, so we should not change the volume control handling into Gaia::AudioChannelService. We need to send the event from the Gaia::AudioChannelService.

# visible-audio-channel-changed
I think that this event can be removed or moved to Gaia::AudioChannelService. Both solutions are well.

---

Evan will take this bug, very appreciate!

Comment 2

[Andrea Marchesini [:baku]]

Thanks Alastor and Evan for fixing this bug. When do you think this is going to be done?
Do you think it's possible to disable the test and land the new AudioChannelService in the meantime?

Comment 3

[Evan Tseng [:evanxd]]

Hi Alastor and baku,

Sorry, I don't think we can disable the tests. It will make regression problems.

I'm working on this now, but I have a WoT section later. However, I think we can have a patch for this bug today.

Then I'll find a reviewer, maybe Tim. He is very busy during the workweek. Honestly, I'm not sure when will be done now. But I'll discuss this with Tim, or find another people to help this.

Comment 4

[Evan Tseng [:evanxd]]

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

Hi Dominic,

Could you help to review the patch?

Thanks.

Comment 5

[Evan Tseng [:evanxd]]

And you could check the discussion[1] to know why we need to fix this.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1113086#c159

Comment 6

[Evan Tseng [:evanxd]]

The CI: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=58c5cd0a613c75e8abe4924bae3dc42844735844

Comment 7

[Evan Tseng [:evanxd]]

And honestly, I don't know why we should have the below code.

1. // The camera recording fires a audio-channel-changed event to kill[1]

2. this.publish('audio-channel-changed', this.currentChannel);[2]

[1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/playing_icon.js#L11
[2]: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/sound_manager.js#L81

Comment 8

[Evan Tseng [:evanxd]]

A new try: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=31850a75734bab557d6eda1340485eea070c2390

Comment 9

[Evan Tseng [:evanxd]]

Fix bugs: https://github.com/evanxd/gaia/commit/df27f5267e18a507240174a371e31c0f137696d8

Comment 10

[Evan Tseng [:evanxd]]

We should disable the below three tests before Bug 1113086 is landed. Because the tests are just depends on the bug.
* TEST-UNEXPECTED-FAIL | test_play_ogv_video.py TestPlayOgvVideo.test_play_ogv_video | AssertionError: 'content' != u'none'
* TEST-UNEXPECTED-FAIL | test_play_webm_video.py TestPlayWebMVideo.test_play_webm_video | AssertionError: 'content' != u'none'
* TEST-UNEXPECTED-FAIL | test_play_ogg_video.py TestPlayOGGVideo.test_play_ogg_video | AssertionError: 'content' != u'none'

Comment 11

[Evan Tseng [:evanxd]]

We'll re-enable them after Bug 1113086 is landed.

Comment 12

[Alastor Wu [:alwu]]

Hi, Even,
These test failures were also in the Gaia test of the Bug1113086. 

In these failures, the current audio channel is different as our expected [1]. The current channel is got from the sound_manager::currentChannel [2], and it is set by receiving the "audio-channel-changed" event [3]. 

As my understanding, these tests failed due to we didn't send the "audio-channel-changed" event, so that the current channel didn't be changed. That is why I file this bug, and I think this bug might solve these failures.

Do you have any idea about these? Or our assumption is wrong?

Very appreciate!

[1] https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/videoplayer/test_play_ogv_video.py#L50
[2] https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L520
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/sound_manager.js#L360

Comment 13

[Dominic Kuo [:dkuo]]

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

Evan,

Overall the patch looks good, but please make sure to keep the original 'mozChromeEvent: audio-channel-changed' cases on master branch, because we shouldn't break the existing functionalities before landing the new audio channel api, let's have these two logics(mozChromeEvent and Audio-channel-service) on master branch and remove the mozChromeEvent one after gecko landed.

Comment 14

[Evan Tseng [:evanxd]]

Dominic, thanks for the review. Updated patch for the comments[1].

Once the CI[2] is good, we could land the patch.

[1]: https://github.com/evanxd/gaia/commit/02de426c583b3d8d31988a918e0601ee4d4db978
[2]: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=02de426c583b3d8d31988a918e0601ee4d4db978

Comment 15

[Evan Tseng [:evanxd]]

Once the CI[1] is good, let's land the code.

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

Comment 16

[Evan Tseng [:evanxd]]

New try: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=401b4921079fed3c372d4a9415789517c755f15a

Comment 17

[Evan Tseng [:evanxd]]

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