Closed Bug 1302350 Opened 3 years ago Closed 3 years ago

Refactor the block-media-element feature.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: kaku, Assigned: alwu)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

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.
Blocks: 1244768
Depends on: 1262053
Assignee: nobody → alwu
Depends on: 1308119
Blocks: 1308154
Status: NEW → ASSIGNED
Comment on attachment 8797416 [details]
Bug 1302350 - part1 : create audio channel agent in the beginning.

https://reviewboard.mozilla.org/r/83036/#review87584
Attachment #8797416 - Flags: review?(amarchesini) → review+
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
Attachment #8797417 - Flags: review?(amarchesini) → review+
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() ?
Attachment #8797418 - Flags: review?(amarchesini) → review+
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
Attachment #8799635 - Flags: review?(amarchesini) → review+
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 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 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.
Attachment #8797419 - Flags: review?(jwwang) → review-
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 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();
Attachment #8797419 - Flags: review?(jwwang) → review+
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
Attachment #8806242 - Flags: review?(jwwang) → review+
Pushed by alwu@mozilla.com:
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
Depends on: 1315551
Depends on: 1315521
Depends on: 1315564
Depends on: 1317167
Blocks: 1321196
Blocks: 1322384
You need to log in before you can comment on or make changes to this bug.