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

RESOLVED FIXED

Status

Firefox OS
AudioChannel
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: alwu, Assigned: evanxd)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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!
Assignee: nobody → evanxd
Summary: Remove the 'audio-channel-changed' and 'visible-audio-channel-changed' events → Move the 'audio-channel-changed' and 'visible-audio-channel-changed' events to Gaia::AudioChannelService
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?
Flags: needinfo?(alwu)
(Assignee)

Comment 3

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

Comment 4

3 years ago
Created attachment 8626367 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/30717

Hi Dominic,

Could you help to review the patch?

Thanks.
Attachment #8626367 - Flags: review?(dkuo)
(Assignee)

Comment 5

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

Updated

3 years ago
Flags: needinfo?(alwu)
(Assignee)

Comment 7

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

Comment 10

3 years ago
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'
(Assignee)

Comment 11

3 years ago
We'll re-enable them after Bug 1113086 is landed.
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

3 years ago
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.
Attachment #8626367 - Flags: review?(dkuo) → review+
(Assignee)

Comment 14

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

Comment 17

3 years ago
master: https://github.com/mozilla-b2g/gaia/commit/7bf44f76a50187977277e9b123a074af06792468
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.