1302350 - Refactor the block-media-element feature.

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Tzuhao Kuo [:kaku]]

The current implementation has to call PlayInternal() to register audio channel agent so that we could know whether to block the play operation or not. Once we know that the element must be blocked, we suspend the following actions of play operation.

The current approach works but leave the media element at a temporary state, we should be able to implement the block-media-element feature by pending the play command without invoking the PlayInternal() method.

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1302350 - part1 : create audio channel agent in the beginning.

Review commit: https://reviewboard.mozilla.org/r/83036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83036/

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1302350 - part2 : provide a method to check whether the agent was started.

Review commit: https://reviewboard.mozilla.org/r/83038/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83038/

Comment 3

[Alastor Wu [:alwu]]

Attachment: Bug 1302350 - part3 : provide a method to know the media-block-state before connecting to the AudioChannelService

Review commit: https://reviewboard.mozilla.org/r/83040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83040/

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1302350 - part4 : refactor the media-blocking mechanism.

Review commit: https://reviewboard.mozilla.org/r/83042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83042/

Comment 5

[Alastor Wu [:alwu]]

Comment on attachment 8797416 [details]
Bug 1302350 - part1 : create audio channel agent in the beginning.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83036/diff/1-2/

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8797417 [details]
Bug 1302350 - part2 : provide a method to check whether the agent was started.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83038/diff/1-2/

Comment 7

[Alastor Wu [:alwu]]

Comment on attachment 8797418 [details]
Bug 1302350 - part3 : provide a method to know the media-block-state before connecting to the AudioChannelService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83040/diff/1-2/

Comment 8

[Alastor Wu [:alwu]]

Comment on attachment 8797419 [details]
Bug 1302350 - part4 : refactor the media-blocking mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83042/diff/1-2/

Comment 9

[Alastor Wu [:alwu]]

Attachment: Bug 1302350 - part5 : modify test to make sure blocked media won't dispatch 'play' event.

Review commit: https://reviewboard.mozilla.org/r/84778/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84778/

Comment 10

[Alastor Wu [:alwu]]

Comment on attachment 8797416 [details]
Bug 1302350 - part1 : create audio channel agent in the beginning.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83036/diff/2-3/

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 8797417 [details]
Bug 1302350 - part2 : provide a method to check whether the agent was started.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83038/diff/2-3/

Comment 12

[Alastor Wu [:alwu]]

Comment on attachment 8797418 [details]
Bug 1302350 - part3 : provide a method to know the media-block-state before connecting to the AudioChannelService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83040/diff/2-3/

Comment 13

[Alastor Wu [:alwu]]

Comment on attachment 8797419 [details]
Bug 1302350 - part4 : refactor the media-blocking mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83042/diff/2-3/

Comment 14

[Alastor Wu [:alwu]]

Comment on attachment 8797416 [details]
Bug 1302350 - part1 : create audio channel agent in the beginning.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83036/diff/3-4/

Comment 15

[Alastor Wu [:alwu]]

Comment on attachment 8797417 [details]
Bug 1302350 - part2 : provide a method to check whether the agent was started.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83038/diff/3-4/

Comment 16

[Alastor Wu [:alwu]]

Comment on attachment 8797418 [details]
Bug 1302350 - part3 : provide a method to know the media-block-state before connecting to the AudioChannelService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83040/diff/3-4/

Comment 17

[Alastor Wu [:alwu]]

Comment on attachment 8797419 [details]
Bug 1302350 - part4 : refactor the media-blocking mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83042/diff/3-4/

Comment 18

[Alastor Wu [:alwu]]

Comment on attachment 8799635 [details]
Bug 1302350 - part5 : modify test to make sure blocked media won't dispatch 'play' event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84778/diff/1-2/

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8797416 [details]
Bug 1302350 - part1 : create audio channel agent in the beginning.

https://reviewboard.mozilla.org/r/83036/#review87584

Comment 20

[Andrea Marchesini [:baku]]

Comment on attachment 8797417 [details]
Bug 1302350 - part2 : provide a method to check whether the agent was started.

https://reviewboard.mozilla.org/r/83038/#review87586

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8797418 [details]
Bug 1302350 - part3 : provide a method to know the media-block-state before connecting to the AudioChannelService

https://reviewboard.mozilla.org/r/83040/#review87588

::: dom/audiochannel/AudioChannelAgent.h:52
(Diff revision 4)
>  
>    uint64_t WindowID() const;
>    uint64_t InnerWindowID() const;
>  
>    bool IsStartedPlaying() const;
> +  bool IsMediaShouldBeBlocked() const;

ShouldMediaBlocked() ?

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 8799635 [details]
Bug 1302350 - part5 : modify test to make sure blocked media won't dispatch 'play' event.

https://reviewboard.mozilla.org/r/84778/#review87590

Comment 23

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8797417 [details]
Bug 1302350 - part2 : provide a method to check whether the agent was started.

https://reviewboard.mozilla.org/r/83038/#review87872

::: dom/audiochannel/AudioChannelAgent.cpp:359
(Diff revision 4)
>  
>    callback->WindowAudioCaptureChanged(aCapture);
>  }
> +
> +bool
> +AudioChannelAgent::IsStartedPlaying() const

"IsPlayingStarted"?

::: dom/html/HTMLMediaElement.cpp:6373
(Diff revision 4)
>  }
>  
>  void
>  HTMLMediaElement::SetAudibleState(bool aAudible)
>  {
> -  if (mIsAudioTrackAudible != aAudible) {
> +  if (mIsAudioTrackAudible != aAudible && mAudioChannelAgent) {

Why checking mAudioChannelAgent whose lifetime equals to that of HTMLMediaElement?

Comment 24

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8797418 [details]
Bug 1302350 - part3 : provide a method to know the media-block-state before connecting to the AudioChannelService

https://reviewboard.mozilla.org/r/83040/#review87878

::: dom/audiochannel/AudioChannelAgent.h:52
(Diff revision 4)
>  
>    uint64_t WindowID() const;
>    uint64_t InnerWindowID() const;
>  
>    bool IsStartedPlaying() const;
> +  bool IsMediaShouldBeBlocked() const;

ShouldBlockMedia()?

Comment 25

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8797419 [details]
Bug 1302350 - part4 : refactor the media-blocking mechanism.

https://reviewboard.mozilla.org/r/83042/#review87890

::: dom/html/HTMLMediaElement.cpp:5899
(Diff revision 4)
>  HTMLMediaElement::ResumeFromAudioChannelBlocked()
>  {
>    MOZ_ASSERT(mAudioChannelSuspended == nsISuspendedTypes::SUSPENDED_BLOCK);
>  
>    SetAudioChannelSuspended(nsISuspendedTypes::NONE_SUSPENDED);
> -  mPaused = false;
> +  Play();

This strikes me as wrong. We should call Play() only when the script calls play() while being blocked.

Comment 26

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8797419 [details]
Bug 1302350 - part4 : refactor the media-blocking mechanism.

https://reviewboard.mozilla.org/r/83042/#review88260

::: dom/html/HTMLMediaElement.cpp:3016
(Diff revision 4)
>  }
>  
>  nsresult
>  HTMLMediaElement::PlayInternal()
>  {
> -  if (!IsAllowedToPlay()) {
> +  if (!CanActivatePlay()) {

This should be checked before calling PlayInternal() so PlayInternal() has no policy code.

Comment 27

[Alastor Wu [:alwu]]

Attachment: Bug 1302350 - part6 : ensure loading process doens't be interrupted even media element can't be played.

We don't want to stop the loading process even we canceled the play operation.

Review commit: https://reviewboard.mozilla.org/r/89726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89726/

Comment 28

[Alastor Wu [:alwu]]

Comment on attachment 8797416 [details]
Bug 1302350 - part1 : create audio channel agent in the beginning.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83036/diff/4-5/

Comment 29

[Alastor Wu [:alwu]]

Comment on attachment 8797417 [details]
Bug 1302350 - part2 : provide a method to check whether the agent was started.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83038/diff/4-5/

Comment 30

[Alastor Wu [:alwu]]

Comment on attachment 8797418 [details]
Bug 1302350 - part3 : provide a method to know the media-block-state before connecting to the AudioChannelService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83040/diff/4-5/

Comment 31

[Alastor Wu [:alwu]]

Comment on attachment 8797419 [details]
Bug 1302350 - part4 : refactor the media-blocking mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83042/diff/4-5/

Comment 32

[Alastor Wu [:alwu]]

Comment on attachment 8799635 [details]
Bug 1302350 - part5 : modify test to make sure blocked media won't dispatch 'play' event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84778/diff/2-3/

Comment 33

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8797419 [details]
Bug 1302350 - part4 : refactor the media-blocking mechanism.

https://reviewboard.mozilla.org/r/83042/#review89162

::: dom/html/HTMLMediaElement.cpp:5785
(Diff revision 5)
>  }
>  
>  void
> -HTMLMediaElement::UpdateAudioChannelPlayingState()
> +HTMLMediaElement::UpdateAudioChannelPlayingState(bool aForcePlaying)
>  {
> -  bool playingThroughTheAudioChannel = IsPlayingThroughTheAudioChannel();
> +  bool playingThroughTheAudioChannel = aForcePlaying ?

bool playingThroughTheAudioChannel = aForcePlaying || IsPlayingThroughTheAudioChannel();

Comment 34

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8806242 [details]
Bug 1302350 - part6 : ensure loading process doens't be interrupted even media element can't be played.

https://reviewboard.mozilla.org/r/89726/#review89164

Comment 35

[Alastor Wu [:alwu]]

Comment on attachment 8797416 [details]
Bug 1302350 - part1 : create audio channel agent in the beginning.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83036/diff/5-6/

Comment 36

[Alastor Wu [:alwu]]

Comment on attachment 8797417 [details]
Bug 1302350 - part2 : provide a method to check whether the agent was started.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83038/diff/5-6/

Comment 37

[Alastor Wu [:alwu]]

Comment on attachment 8797418 [details]
Bug 1302350 - part3 : provide a method to know the media-block-state before connecting to the AudioChannelService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83040/diff/5-6/

Comment 38

[Alastor Wu [:alwu]]

Comment on attachment 8797419 [details]
Bug 1302350 - part4 : refactor the media-blocking mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83042/diff/5-6/

Comment 39

[Alastor Wu [:alwu]]

Comment on attachment 8799635 [details]
Bug 1302350 - part5 : modify test to make sure blocked media won't dispatch 'play' event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84778/diff/3-4/

Comment 40

[Alastor Wu [:alwu]]

Comment on attachment 8806242 [details]
Bug 1302350 - part6 : ensure loading process doens't be interrupted even media element can't be played.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89726/diff/1-2/

Comment 41

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f2abaa33e0f5fc99a83e6ea7fa500c951c26a8f

Comment 42

[Alastor Wu [:alwu]]

Comment on attachment 8797416 [details]
Bug 1302350 - part1 : create audio channel agent in the beginning.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83036/diff/6-7/

Comment 43

[Alastor Wu [:alwu]]

Comment on attachment 8797417 [details]
Bug 1302350 - part2 : provide a method to check whether the agent was started.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83038/diff/6-7/

Comment 44

[Alastor Wu [:alwu]]

Comment on attachment 8797418 [details]
Bug 1302350 - part3 : provide a method to know the media-block-state before connecting to the AudioChannelService

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83040/diff/6-7/

Comment 45

[Alastor Wu [:alwu]]

Comment on attachment 8797419 [details]
Bug 1302350 - part4 : refactor the media-blocking mechanism.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83042/diff/6-7/

Comment 46

[Alastor Wu [:alwu]]

Comment on attachment 8799635 [details]
Bug 1302350 - part5 : modify test to make sure blocked media won't dispatch 'play' event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84778/diff/4-5/

Comment 47

[Alastor Wu [:alwu]]

Comment on attachment 8806242 [details]
Bug 1302350 - part6 : ensure loading process doens't be interrupted even media element can't be played.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89726/diff/2-3/

Comment 48

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6e07f8ad5462c6d945c0ee8cb488a5d65e9fe54

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c4a212295b6
part1 : create audio channel agent in the beginning. r=baku
https://hg.mozilla.org/integration/autoland/rev/b797bf4330eb
part2 : provide a method to check whether the agent was started. r=baku
https://hg.mozilla.org/integration/autoland/rev/ddf60739dfd7
part3 : provide a method to know the media-block-state before connecting to the AudioChannelService r=baku
https://hg.mozilla.org/integration/autoland/rev/4cdb9e2c9595
part4 : refactor the media-blocking mechanism. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/d94c3a6c1ad6
part5 : modify test to make sure blocked media won't dispatch 'play' event. r=baku
https://hg.mozilla.org/integration/autoland/rev/10d414508e17
part6 : ensure loading process doens't be interrupted even media element can't be played. r=jwwang

Comment 50

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7c4a212295b6
https://hg.mozilla.org/mozilla-central/rev/b797bf4330eb
https://hg.mozilla.org/mozilla-central/rev/ddf60739dfd7
https://hg.mozilla.org/mozilla-central/rev/4cdb9e2c9595
https://hg.mozilla.org/mozilla-central/rev/d94c3a6c1ad6
https://hg.mozilla.org/mozilla-central/rev/10d414508e17