Closed Bug 1284177 Opened 8 years ago Closed 8 years ago

Add tests for video suspend in background

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: u480271, Assigned: u480271)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1284177 - P1: Provide observable notification for video suspend.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1284177 - P2: Video decode suspend mochitests.
58 bytes, text/x-review-board-request
jwwang
: review+
Details 
We don't have any automated tests to check that the functionality works. This needs to change.
Assignee: nobody → dglastonbury
Blocks: 1275665
To support mochitests, report change in video decode suspend state via
nsIObserverService, so tests can use priviledged calls to respond to
these events.

Review commit: https://reviewboard.mozilla.org/r/62012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62012/
Attachment #8767555 - Flags: review?(cpearce)
Attachment #8767556 - Flags: review?(cpearce)
Attachment #8767557 - Flags: review?(jwwang)
Attachment #8767557 - Flags: review?(cpearce)
Test:
- That video decode suspends when enabled and delay is reached.
- That video decode doesn't suspend when disabled.
- That video decode doesn't suspend when video finishes before suspend delay.

These tests need to run from content process to observe the suspend
notifications via nsIObserverService, but access to gBrowser is in
chrome process in e10s. Thus, the reason for loading
background_video_chrome.js into chrome process and invoking functions
via async messages.

Review commit: https://reviewboard.mozilla.org/r/62014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62014/
Suspended videos were blocking the video from completing.

Review commit: https://reviewboard.mozilla.org/r/62016/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62016/
(In reply to Dan Glastonbury :kamidphish from comment #3)
> Bug 1284177 - P3: Fix ended event reporting for suspended video decode.

It doesn't look right to me. Suspended video decoding can be resumed but it isn't so for the completely decoded video.
Blocks: 1276556
No longer blocks: 1275665
Attachment #8767555 - Flags: review?(cpearce) → review?(jwwang)
Attachment #8767556 - Flags: review?(cpearce) → review?(jwwang)
Attachment #8767557 - Flags: review?(cpearce)
(In reply to JW Wang [:jwwang] from comment #5)
> (In reply to Dan Glastonbury :kamidphish from comment #3)
> > Bug 1284177 - P3: Fix ended event reporting for suspended video decode.
> 
> It doesn't look right to me. Suspended video decoding can be resumed but it
> isn't so for the completely decoded video.

I'm confused by what you mean. Can explain with an example? (Maybe I need more tests to cover your comment)
Comment on attachment 8767555 [details]
Bug 1284177 - P1: Provide observable notification for video suspend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62012/diff/1-2/
Comment on attachment 8767556 [details]
Bug 1284177 - P2: Video decode suspend mochitests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62014/diff/1-2/
Comment on attachment 8767557 [details]
Bug 1284177 - P3: Fix ended event reporting for suspended video decode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62016/diff/1-2/
Blocks: 1284389
Need to include a test for a silent video and check that the video ends correctly.
(In reply to Dan Glastonbury :kamidphish from comment #6)
> I'm confused by what you mean. Can explain with an example? (Maybe I need
> more tests to cover your comment)

Suppose we have a file with 5s audio and 10s video. If we suspend the video decoding at 7s, do we want to fire the 'ended' event at 7s or 10s which is the end of the entire file?
Mass change P2 -> P3
Priority: P2 → P3
Attachment #8767555 - Flags: review?(jwwang) → review-
Comment on attachment 8767555 [details]
Bug 1284177 - P1: Provide observable notification for video suspend.

https://reviewboard.mozilla.org/r/62012/#review59318

::: dom/media/MediaDecoder.cpp:1996
(Diff revision 2)
>  }
>  
> +void
> +MediaDecoder::NotifyVideoDecodeSuspendedChanged()
> +{
> +  MOZ_ASSERT(!mShuttingDown);

SetStateMachine(nullptr) is called in FinishShutdown(). It is possible for this notification to come in between Shutdown() and FinishShutdown() to hit the assertion.

::: dom/media/MediaDecoder.cpp:2006
(Diff revision 2)
> +  if (NS_WARN_IF(!obs)) {
> +    return;
> +  }
> +
> +  MediaDecoderOwner* owner = GetOwner();
> +  HTMLMediaElement* mediaElement = owner ? owner->GetMediaElement() : nullptr;

Do we still want to send the notification when GetOwner() returns null?
Comment on attachment 8767555 [details]
Bug 1284177 - P1: Provide observable notification for video suspend.

https://reviewboard.mozilla.org/r/62012/#review59318

> SetStateMachine(nullptr) is called in FinishShutdown(). It is possible for this notification to come in between Shutdown() and FinishShutdown() to hit the assertion.

That's a good question. The value is tied to the suspend state in MDSM.

I think I want to keep the assert here because GetOwner() is invalid if Shutdown() has been called.

I note that VisibilityChanged() early exits if the MDSM has had Shutdown() called. Also, In MDSM::Shutdown(), any pending suspend timers are canceled, so once Shutdown() is called, NotifyVideoDecodeSuspendedChanged() should not be called. (Lending more credence to having the assertion)

> Do we still want to send the notification when GetOwner() returns null?

Good point. Probably not. There would a be a broadcast event, no way to determine for which media element the notification was for.
Comment on attachment 8767557 [details]
Bug 1284177 - P3: Fix ended event reporting for suspended video decode.

https://reviewboard.mozilla.org/r/62016/#review67110

::: dom/media/MediaDecoderStateMachine.cpp:846
(Diff revision 3)
>        mState == DECODER_STATE_COMPLETED) {
>      // Don't change our state if we've already been shutdown, or we're seeking,
>      // since we don't want to abort the shutdown or seek processes.
>      return;
>    }
> +  //  if ((!IsVideoDecoding() || IsVideoDecodeSuspended()) && !IsAudioDecoding()) {

Remove this line.
Attachment #8767557 - Flags: review?(jwwang) → review+
Comment on attachment 8767555 [details]
Bug 1284177 - P1: Provide observable notification for video suspend.

https://reviewboard.mozilla.org/r/62012/#review59318

> That's a good question. The value is tied to the suspend state in MDSM.
> 
> I think I want to keep the assert here because GetOwner() is invalid if Shutdown() has been called.
> 
> I note that VisibilityChanged() early exits if the MDSM has had Shutdown() called. Also, In MDSM::Shutdown(), any pending suspend timers are canceled, so once Shutdown() is called, NotifyVideoDecodeSuspendedChanged() should not be called. (Lending more credence to having the assertion)

https://hg.mozilla.org/try/diff/62d1f1c7fce2/dom/media/MediaDecoder.cpp
https://hg.mozilla.org/try/diff/62d1f1c7fce2/dom/media/MediaDecoderStateMachine.cpp
I think you can take this approach in which I added 2 events: "mozenterdormant" and "mozexitdormant".

It will be much easier for mochitests to register events with media.addEventListener(...).
https://hg.mozilla.org/mozilla-central/file/e78975b53563d80c99ebfbdf8a9fbf6b829a8a48/dom/webidl/HTMLMediaElement.webidl#l224

Per discussion, you can call this function to simulate visibility changes in the mochitest to avoid the complicated tab-switching tricks.
Attachment #8767557 - Attachment is obsolete: true
Comment on attachment 8767555 [details]
Bug 1284177 - P1: Provide observable notification for video suspend.

https://reviewboard.mozilla.org/r/62012/#review67992

Please update the commit message since nsIObserverService is no longer used.

::: dom/media/MediaDecoderStateMachine.h:134
(Diff revision 4)
>    The state machine class. This manages the decoding and seeking in the
>    MediaDecoderReader on the decode task queue, and A/V sync on the shared
>    state machine thread, and controls the audio "push" thread.
>  
>    All internal state is synchronised via the decoder monitor. State changes
> +

blank line?
Attachment #8767555 - Flags: review?(jwwang) → review+
Comment on attachment 8767556 [details]
Bug 1284177 - P2: Video decode suspend mochitests.

https://reviewboard.mozilla.org/r/62014/#review67994

::: dom/media/test/background_video.js:33
(Diff revision 4)
> +
> +/**
> + * @param {HTMLMediaElement} video Video element under test.
> + * @returns {Promise} Promise that is resolved when video 'playing' event fires and rejected on error.
> + */
> +function checkVideoStarts(video) {

Might be good to call it "waitUntilPlaying".

::: dom/media/test/background_video.js:35
(Diff revision 4)
> + * @param {HTMLMediaElement} video Video element under test.
> + * @returns {Promise} Promise that is resolved when video 'playing' event fires and rejected on error.
> + */
> +function checkVideoStarts(video) {
> +  return new Promise((resolve, reject) => {
> +    video.onplaying = (e) => { ok(true, "Video played."); resolve(video); };

This will overwrite the existing onplaying handler. Please use:

video.addEventListener('playing', handler, {once: true});

::: dom/media/test/background_video.js:45
(Diff revision 4)
> +
> +/**
> + * @param {HTMLMediaElement} video Video element under test.
> + * @returns {Promise} Promise which is resolved when video 'ended' event fires.
> + */
> +function checkVideoEnds(video) {

Might be better to call it "waitUntilEnded".

::: dom/media/test/background_video.js:48
(Diff revision 4)
> + * @returns {Promise} Promise which is resolved when video 'ended' event fires.
> + */
> +function checkVideoEnds(video) {
> +  return new Promise(resolve => {
> +    function success() {
> +      ok(true, 'Video ended');

It would be help debugging to print the URI or tag to know which video is ended.

::: dom/media/test/background_video.js:56
(Diff revision 4)
> +    // Maybe this should be an error. We want to check that ended happens
> +    // after this call?
> +    if (video.ended) {
> +      success();
> +    } else {
> +      video.onended = (e) => { success(); };

Don't overwrit the exsiting handler.

::: dom/media/test/background_video.js:68
(Diff revision 4)
> + * @returns {Promise} Promise that is resolved when video decode suspends.
> + */
> +function checkVideoSuspends(video) {
> +  return new Promise(resolve => {
> +    video.addEventListener('mozentervideosuspend', function suspendEvent() {
> +      video.removeEventListener('mozentervideosuspend', suspendEvent);

No need to remove the handler since we have bug 1287706.
Attachment #8767556 - Flags: review?(jwwang) → review-
Comment on attachment 8767556 [details]
Bug 1284177 - P2: Video decode suspend mochitests.

https://reviewboard.mozilla.org/r/62014/#review68232

::: dom/media/test/mochitest.ini:893
(Diff revision 5)
> +[test_background_video_suspend.html]
> +tags = suspend
> +[test_background_video_no_suspend_short_vid.html]
> +tags = suspend
> +[test_background_video_no_suspend_disabled.html]
> +tags = suspend djg

What does the "djg" tag mean?
Attachment #8767556 - Flags: review?(jwwang) → review+
Comment on attachment 8767556 [details]
Bug 1284177 - P2: Video decode suspend mochitests.

https://reviewboard.mozilla.org/r/62014/#review68232

> What does the "djg" tag mean?

It's my initials. Left over from testing, already removed in Diff 6. :-)
Pushed by dglastonbury@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24602315090f
P1: Provide observable notification for video suspend. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/4b946dd315c8
P2: Video decode suspend mochitests. r=jwwang
Blocks: 1294420
https://hg.mozilla.org/mozilla-central/rev/24602315090f
https://hg.mozilla.org/mozilla-central/rev/4b946dd315c8
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: