1578615 - Separate the logic for delaying autoplay from HTMLMediaElement::AudioChannelAgentCallback

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, task, P2)

Description

[Alastor Wu [:alwu]]

We usually start AudioChannelAgent when media playback starts and stop it when media playback stops, however, there is a special case where we would start AudioChannelAgent even if media playback doesn't start, which is "delaying autoplay for unvisited tab".

That makes the working flow in HTMLMediaElement::AudioChannelAgentCallback messy, so I would like to separate this special case to another dedicated class so that we can keep the usage of AudioChannelAgent in HTMLMediaElement::AudioChannelAgentCallback consistent with other use cases.

And this bug is something I have to refactor before fixing other media controller bugs.

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part1 : provide function GetAudibleState() on media element.

As we have another usage to know whether media is audible or not, we remove this function from AudioChannelWrapper and make it as media element's function.

In addition, it also makes sense to have this function for media element, because the result is decided by media element's own attributes.

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part2 : make the pref 'media.block-autoplay-until-in-foreground' to a static pref.

Comment 3

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part3 : implement delay media playback policy.

Separate the logic of delaying media playback from HTMLMediaElement's mAudioChannelWrapper to a new class, because delaying media playback is a special usage of using AudioChannelAgent.

It would make the code cleaner and separate normal usage and special usage, to reduce confusion.

We usually start AudioChannelAgent when media playback starts and stop it when media playback stops, however, in this case, we would start the agent even if media hasn't be started yet, because we would like to use the agent as a connection with AudioChannelService in order to receive the resume command when we're able to play delayed playback.

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part4 : remove SUSPENDED_BLOCK related logic in AudioChannelWrapper.

Now AudioChannelWrapper would only start AudioChannelAgent when media starts and stop the agent when media stops. It won't handle any case with SUSPENDED_BLOCK.

It's clearer to know when we would start or stop the agent in AudioChannelWrapper.

In addition, in bug1577890, we will remove more unnecessary logic which is used for Fennec only in order to make AudioChannelWrapper cleane

Comment 5

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part5 : notify media block when we append agent.

As we would show delay media playback icon for eNotAudible and eMaybeAudible, we should call AudioChannelWindow::AudioAudibleChanged() when we register the agent in order to notify a delaying start.

In the past, as we use AudioChannelWrapper to handle delaying, which would call HTMLMediaElement::AudioChannelWrapper::WindowSuspendChanged() immediately when we delay autoplay, and finally triggers AudioChannelWindow::AudioAudibleChanged() to notify sending block-start event. It's very unclear, so we should set it in the beginning if we have already know the audible state of delayed media.

Comment 6

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part6 : modify tests.

We could simply check TabBlockEvent to know whether we starts delaying media playback or not. We don't need to check media element's computedSuspended.

Comment 7

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part7 : rename tests 'browser_block_*' to 'browser_delay_autoplay_*'.

As the things we do in those tests are not really "block" autoplay, which is not to allow autoplay starts, what we do is actually delaying them until tab goes to foreground or user clicking play tab icon on the tab.

In order to distinguish them from the real blocking-autoplay, and reflect what we really do in those tests, rename all of them with prefix "browser_delay_autoplay_*".

Comment 8

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part8 : add test case for delaying media with autoplay keyword.

Current test cases are all about calling video.play(), we don't have test case for media with autoplay keyword.

Comment 9

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part9 : refactor browser_delay_autoplay_media.js

Modify test and some description naming from "block autoplay" to "delay autoplay" and split the blocking autoplay test to new independent task.

Comment 10

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part10 : do not enable delaying autoplay during test by default.

Tests which use this feature on testing should enable the pref explicitly.

Comment 11

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part0.5 : simply the logic of "IsOwnerAudible()".

There are some checking conditions can be reordered or simplified in order to remove some unnecessary check.

Comment 12

[Alastor Wu [:alwu]]

Attachment: Bug 1578615 - part11 : remove usages of 'AudibleState::eMaybeAudible' in media element.

In bug1583978, we are going to remove AudibleState::eMaybeAudible. But before that, as AudibleState::eMaybeAudible is only used for delaying media playback, so we can remove its usage from media element and move it to the dedicated class to reduce confusion about how and when we should use AudibleState::eMaybeAudible or AudibleState::eNotAudible.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/750bba466592
part0.5 : simply the logic of "IsOwnerAudible()". r=chunmin
https://hg.mozilla.org/integration/autoland/rev/6690d621d726
part1 : provide function GetAudibleState() on media element. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/b38eec7c0555
part2 : make the pref 'media.block-autoplay-until-in-foreground' to a static pref. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/d7933b57b9a5
part3 : implement delay media playback policy. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/7ade7615fccf
part4 : remove SUSPENDED_BLOCK related logic in AudioChannelWrapper. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/34b256af59d6
part5 : notify media block when we append agent. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/2c7f0d54cda8
part6 : modify tests. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/263512ebd456
part7 : rename tests 'browser_block_*' to 'browser_delay_autoplay_*'. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/ed86e6ff19a7
part8 : add test case for delaying media with autoplay keyword. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/c430a883a4f6
part9 : refactor browser_delay_autoplay_media.js r=chunmin
https://hg.mozilla.org/integration/autoland/rev/a8deaa2603d3
part10 : do not enable delaying autoplay during test by default. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/aeec93cc3005
part11 : remove usages of 'AudibleState::eMaybeAudible' in media element. r=chunmin

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/750bba466592
https://hg.mozilla.org/mozilla-central/rev/6690d621d726
https://hg.mozilla.org/mozilla-central/rev/b38eec7c0555
https://hg.mozilla.org/mozilla-central/rev/d7933b57b9a5
https://hg.mozilla.org/mozilla-central/rev/7ade7615fccf
https://hg.mozilla.org/mozilla-central/rev/34b256af59d6
https://hg.mozilla.org/mozilla-central/rev/2c7f0d54cda8
https://hg.mozilla.org/mozilla-central/rev/263512ebd456
https://hg.mozilla.org/mozilla-central/rev/ed86e6ff19a7
https://hg.mozilla.org/mozilla-central/rev/c430a883a4f6
https://hg.mozilla.org/mozilla-central/rev/a8deaa2603d3
https://hg.mozilla.org/mozilla-central/rev/aeec93cc3005