Closed Bug 1322505 Opened 8 years ago Closed 7 years ago

Don't resume the video after calling video.pause()

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
Other Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- verified

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(3 files)

Bug 1322505 - part1 : rename audio channel wrapper's function.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1322505 - part2 : reset suspend type for blocked media after calling media.pause().
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1322505 - part3 : add test.
59 bytes, text/x-review-board-request
baku
: review+
jaws
: review+
Details 
Fork from bug1262053 comment97.

STR.
1. open an background tab
2. on inactive tab, JS calls video.play()
3. on inactive tab, JS calls video.pause()
4. change the tab to foreground

Expect.
5. The video shouldn't stop playback

Actual.
5. The video still starts playback
Attachment #8822115 - Flags: review?(amarchesini)
Attachment #8822116 - Flags: review?(amarchesini)
Attachment #8822117 - Flags: review?(amarchesini)
Comment on attachment 8822115 [details]
Bug 1322505 - part1 : rename audio channel wrapper's function.

https://reviewboard.mozilla.org/r/101114/#review105728
Attachment #8822115 - Flags: review?(amarchesini) → review+
Comment on attachment 8822116 [details]
Bug 1322505 - part2 : reset suspend type for blocked media after calling media.pause().

https://reviewboard.mozilla.org/r/101120/#review105730

::: dom/html/HTMLMediaElement.cpp:2777
(Diff revision 3)
>    mAutoplaying = false;
>    // We changed mPaused and mAutoplaying which can affect AddRemoveSelfReference
>    AddRemoveSelfReference();
>    UpdateSrcMediaStreamPlaying();
> -  UpdateAudioChannelPlayingState();
> +  if (mAudioChannelWrapper) {
> +    mAudioChannelWrapper->NotifyPlayStateChanged();

You should notify the playstate changed only if the suspend, or the playingThroughTheAudioChannel is changed.

Wondering if we can extend UpdateAudioChannelPlayingState() so that you check:

    if (playingThroughTheAudioChannel != mPlayingThroughTheAudioChannel || suspendState != mSuspendState) {
...
Attachment #8822116 - Flags: review?(amarchesini) → review+
Comment on attachment 8822117 [details]
Bug 1322505 - part3 : add test.

https://reviewboard.mozilla.org/r/101122/#review105732

This looks good to me, but you should ask a Browser/frontend team to take a look.
Attachment #8822117 - Flags: review?(amarchesini) → review+
Comment on attachment 8822116 [details]
Bug 1322505 - part2 : reset suspend type for blocked media after calling media.pause().

https://reviewboard.mozilla.org/r/101120/#review105730

> You should notify the playstate changed only if the suspend, or the playingThroughTheAudioChannel is changed.
> 
> Wondering if we can extend UpdateAudioChannelPlayingState() so that you check:
> 
>     if (playingThroughTheAudioChannel != mPlayingThroughTheAudioChannel || suspendState != mSuspendState) {
> ...

The NotifyPlayStateChanged() here won't directly notify the audio channel agent anything, it just tells the wrapper media element changed its playing state. 

The IsPlayingThroughTheAudioChannel() is response for deciding whether we really need to notify the audio channel agent. 

Since the |mSuspendState| would only be changed between pause (different kinds of pause, including block) and non-suspend, and this condition has already been included in the condition (condition2, IsSuspended()) in the IsPlayingThroughTheAudioChannel(). So when the suspend state changes (pause to non-suspend/non-suspend to pause), the playingThroughTheAudioChannel would also be changed.
Attachment #8822117 - Flags: review?(jaws)
Comment on attachment 8822117 [details]
Bug 1322505 - part3 : add test.

https://reviewboard.mozilla.org/r/101122/#review106704

::: toolkit/content/tests/browser/browser_block_autoplay_media_pausedAfterPlay.js:11
(Diff revision 3)
> +  SUSPENDED_PAUSE            : 1,
> +  SUSPENDED_BLOCK            : 2,
> +  SUSPENDED_PAUSE_DISPOSABLE : 3
> +};
> +
> +function* wait_for_tab_block_event(tab, expectBlocked) {

Is this function used in other tests? Can it be moved to head.js?
Attachment #8822117 - Flags: review?(jaws) → review+
Comment on attachment 8822117 [details]
Bug 1322505 - part3 : add test.

https://reviewboard.mozilla.org/r/101122/#review106704

> Is this function used in other tests? Can it be moved to head.js?

Yes, we can, and I'm planing to add other tests which would also use this function.
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/049ab1a92da6
part1 : rename audio channel wrapper's function. r=baku
https://hg.mozilla.org/integration/autoland/rev/ddf90342d71d
part2 : reset suspend type for blocked media after calling media.pause(). r=baku
https://hg.mozilla.org/integration/autoland/rev/861f01ee0e6e
part3 : add test. r=baku,jaws
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a8c89e67f9a
part1 : rename audio channel wrapper's function. r=baku
https://hg.mozilla.org/integration/autoland/rev/5e90eada67cf
part2 : reset suspend type for blocked media after calling media.pause(). r=baku
https://hg.mozilla.org/integration/autoland/rev/726ee922483f
part3 : add test. r=baku,jaws
Depends on: 1335063
Reproduced the issue on Nightly 53.0a1 (Build ID: 20170117030218) using the following video: 
https://people-mozilla.org/~alwu/Test2.html

I can confirm the fix using Nightly 54.0a1 and the same video as above on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11. When the tab with the video is in foreground, the video does not start playback and the Play Tab indicator is not displayed at all. 

But, there still are some issues related with the duplicate of this bug (bug 1328881) for which I filed Bug 1335063.

CoolCmd, could you please check this issue (reported in bug1262053 comment97) is fixed on your end?
Flags: needinfo?(CoolCmd)
Depends on: 1341062
(In reply to Simona B [:simonab ] from comment #35)
> CoolCmd, could you please check this issue (reported in bug1262053
> comment97) is fixed on your end?
fixed
Flags: needinfo?(CoolCmd)
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: