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?
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.
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)
And you could check the discussion to know why we need to fix this. : https://bugzilla.mozilla.org/show_bug.cgi?id=1113086#c159
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 2. this.publish('audio-channel-changed', this.currentChannel); : https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/playing_icon.js#L11 : https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/sound_manager.js#L81
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'
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 . The current channel is got from the sound_manager::currentChannel , and it is set by receiving the "audio-channel-changed" event . 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!  https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/videoplayer/test_play_ogv_video.py#L50  https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L520  https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/sound_manager.js#L360
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+
Dominic, thanks for the review. Updated patch for the comments. Once the CI is good, we could land the patch. : https://github.com/evanxd/gaia/commit/02de426c583b3d8d31988a918e0601ee4d4db978 : https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=02de426c583b3d8d31988a918e0601ee4d4db978
Once the CI is good, let's land the code. : https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=4de39935842f84610dc599398b5ca8a356b5fac6
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.