Closed
Bug 1302350
Opened 8 years ago
Closed 8 years ago
Refactor the block-media-element feature.
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: kaku, Assigned: alwu)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
58 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jwwang
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
baku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jwwang
:
review+
|
Details |
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.
Reporter | ||
Updated•8 years ago
|
Updated•8 years ago
|
Priority: -- → P3
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → alwu
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Blocks: delay-autoplay
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•8 years ago
|
||
mozreview-review |
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 20•8 years ago
|
||
mozreview-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 21•8 years ago
|
||
mozreview-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 22•8 years ago
|
||
mozreview-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 23•8 years ago
|
||
mozreview-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 24•8 years ago
|
||
mozreview-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/#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•8 years ago
|
||
mozreview-review |
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 26•8 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 33•8 years ago
|
||
mozreview-review |
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 34•8 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f2abaa33e0f5fc99a83e6ea7fa500c951c26a8f
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 48•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6e07f8ad5462c6d945c0ee8cb488a5d65e9fe54
Comment 49•8 years ago
|
||
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
Comment 50•8 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•