Closed Bug 1578615 Opened 1 year ago Closed 5 months ago

Separate the logic for delaying autoplay from HTMLMediaElement::AudioChannelAgentCallback

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

Attachments

(12 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

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.

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.

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

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.

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.

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_*".

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

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

Blocks: 1579588
Depends on: 1580659

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

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

Attachment #9090549 - Attachment description: Bug 1578615 - part1 : provide IsAudible() on media element. → Bug 1578615 - part1 : provide function GetAudibleState() on media element.
Blocks: 1583978

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.

Pushed by alwu@mozilla.com:
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
Regressions: 1584106
Regressions: 1590579
Duplicate of this bug: 1579627
See Also: → 1595603
Regressions: 1611748
You need to log in before you can comment on or make changes to this bug.