Closed Bug 1192733 Opened 6 years ago Closed 6 years ago

Test case for SetDormant() in MDSM

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ayang, Assigned: bechen)

References

Details

Attachments

(2 files, 4 obsolete files)

A following bug of bug 1192694.

It needs to make sure MSDM can enter/leave dormant normally before or after decoding first frame.
Flags: needinfo?(bechen)
I'm going to implement some testcases about the dormant for all platforms and ensure comment 0 will be tested.

There are some ways to trigger dormant / DocumentActivityChange.
1. UnbindFromTree/BindToTree in HTMLMediaElement
2. Minimize the current document?
3. Go to another URL so the origin one wil go into BFCache?
4. ...
Assignee: nobody → bechen
Flags: needinfo?(bechen)
Awesome! We really need some test case to test the dormant mechanism!
tips:
see bug 1181864, EME's dormant is disable.
See Also: → 1181864
http://mxr.mozilla.org/mozilla-central/source/dom/media/test/test_reactivate.html should cover dormant mode. Can you extend or copy this test to cover the extra cases you need to test?
Attached patch bug-1192733.part1.v01.patch (obsolete) — Splinter Review
This testcase copy from test_playback.html.

The difference are:
1. PARALLEL_TESTS = 1
2. remove the v.play() when testcase start
3. when receive loadedmetadata, remove the element and set a timer to append the element and call v.play().
//
 var check = function(test, v) { return function() {
    document.body.removeChild(v);
    var appendAndPlayElement = function() {
      document.body.appendChild(v);
      info("Element play: " + v.name);
      v.play();
    }
    setTimeout(appendAndPlayElement, 2000);
  }}(test, v);
//

And now the tescase timeout at my emulator-kk build...
(In reply to Benjamin Chen [:bechen] from comment #5)
> Created attachment 8646824 [details] [diff] [review]
> bug-1192733.part1.v01.patch
> And now the tescase timeout at my emulator-kk build...

Oh I see, the timeout is caused by Bug 1192694.
Attached patch bug-1192733.part1.fix.v01.patch (obsolete) — Splinter Review
After apply bug 1192694, the new testcase still timeout.
It is because the |EnsureDecodersInitialized| return true to tell us the metadata is ready, but we forgot to resolve the promise.
Attachment #8647363 - Flags: review?(cpearce)
This testcase copy from test_playback.html.
The difference are:
1. PARALLEL_TESTS = 1
2. remove the v.play() when testcase start
3. when receive loadedmetadata, remove the element and set a timer to append the element and call v.play().
//
 var check = function(test, v) { return function() {
    document.body.removeChild(v);
    var appendAndPlayElement = function() {
      document.body.appendChild(v);
      info("Element play: " + v.name);
      v.play();
    }
    setTimeout(appendAndPlayElement, 2000);
  }}(test, v);
//

4. using gSmallTests as test files. |manager.runTests(gSmallTests, startTest);|
Attachment #8646824 - Attachment is obsolete: true
Attachment #8647379 - Flags: review?(jwwang)
Comment on attachment 8647379 [details] [diff] [review]
bug-1192733.part1.addtestcase.patch

Review of attachment 8647379 [details] [diff] [review]:
-----------------------------------------------------------------

Please state the purpose and the criteria to pass/fail the test case. It is not very useful to tell the difference between test_playback.html which is not for dormant test.

::: dom/media/test/test_playback_reactivate.html
@@ +16,5 @@
> +  SimpleTest.requestCompleteLog();
> +}
> +
> +var manager = new MediaTestManager;
> +PARALLEL_TESTS = 1;

Why setting 1?

@@ +17,5 @@
> +}
> +
> +var manager = new MediaTestManager;
> +PARALLEL_TESTS = 1;
> +SimpleTest.requestFlakyTimeout("setTimeout for append the element");

This is already called in manifest.js.

@@ +40,5 @@
> +
> +  var check = function(test, v) { return function() {
> +    is(test.name, v.name, test.name + ": Name should match #1");
> +    checkMetadata(test.name, v, test);
> +    info("removeChild: " + v.name);

It is not consistent to mix Log() and info(). Just use Log() which also outputs timestamps.

@@ +51,5 @@
> +    }
> +    setTimeout(appendAndPlayElement, 2000);
> +  }}(test, v);
> +
> +  var noLoad = function(test, v) { return function() {

This is not related to the dormant test. Please remove it.

@@ +92,5 @@
> +    v.seenSuspend = true;
> +    mayFinish();
> +  }}(test, v);
> +
> +  var timeUpdate = function(test, v) { return function() {

This is not related to the dormant test.
Attachment #8647379 - Flags: review?(jwwang) → review-
Comment on attachment 8647363 [details] [diff] [review]
bug-1192733.part1.fix.v01.patch

Review of attachment 8647363 [details] [diff] [review]:
-----------------------------------------------------------------

This fixes EME decoders failing to recover from dormant mode (bug 1181864), which I was trying to figure out yesterday, so f+ from me. But I think jya should review this too.
Attachment #8647363 - Flags: review?(jyavenard)
Attachment #8647363 - Flags: review?(cpearce)
Attachment #8647363 - Flags: feedback+
Comment on attachment 8647363 [details] [diff] [review]
bug-1192733.part1.fix.v01.patch

Review of attachment 8647363 [details] [diff] [review]:
-----------------------------------------------------------------

good spotting

::: dom/media/MediaFormatReader.cpp
@@ +270,5 @@
>  MediaFormatReader::AsyncReadMetadata()
>  {
>    MOZ_ASSERT(OnTaskQueue());
>  
>    nsRefPtr<MetadataPromise> p = mMetadataPromise.Ensure(__func__);

please leave the extra line.
Attachment #8647363 - Flags: review?(jyavenard) → review+
regression was introduced by bug 1146086
Depends on: 1146086
(In reply to JW Wang [:jwwang] from comment #9)
> Comment on attachment 8647379 [details] [diff] [review]
> bug-1192733.part1.addtestcase.patch
> 
> Review of attachment 8647379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please state the purpose and the criteria to pass/fail the test case. It is
> not very useful to tell the difference between test_playback.html which is
> not for dormant test.

This testcase wants to test a video element's playback is not break by dormant.
When the metadata is loaded, we remove the video element to trigger dormant. Then set a timer to append the video element back and play it.
Test pass if the video plays to the end.

> > +PARALLEL_TESTS = 1;
> 
> Why setting 1?

I think serial testing is better if the testcase runs on b2g platform.
Attachment #8647379 - Attachment is obsolete: true
Attachment #8647885 - Flags: review?(jwwang)
Address comment 11.
r=jya
Attachment #8647363 - Attachment is obsolete: true
Attachment #8647886 - Flags: review+
(In reply to Benjamin Chen [:bechen] from comment #13)
> This testcase wants to test a video element's playback is not break by
> dormant.
> When the metadata is loaded, we remove the video element to trigger dormant.
> Then set a timer to append the video element back and play it.
> Test pass if the video plays to the end.
Please put this comment in the test case so everyone can have a clear understanding when reading the test case.

> I think serial testing is better if the testcase runs on b2g platform.
The reason is not sufficient for me. Did you meet any problem when setting the value to 2 in running the test case?
Comment on attachment 8647885 [details] [diff] [review]
bug-1192733.part1.addtestcase.patch

Review of attachment 8647885 [details] [diff] [review]:
-----------------------------------------------------------------

Please don't change the default value of PARALLEL_TESTS. If you meet problems, please file a new bug for it.

::: dom/media/test/test_playback_reactivate.html
@@ +44,5 @@
> +  v.name = test.name;
> +
> +  var check = function(test, v) { return function() {
> +    is(test.name, v.name, test.name + ": Name should match #1");
> +    checkMetadata(test.name, v, test);

checkMetadata() is not related to this test.

@@ +96,5 @@
> +  v.addEventListener("loadedmetadata", check, false);
> +
> +  // We should get "ended" and "suspend" events for every resource
> +  v.addEventListener("ended", checkEnded, false);
> +  v.addEventListener("suspend", checkSuspended, false);

Checking suspend status is not relevant to this test.
Attachment #8647885 - Flags: review?(jwwang) → review+
See Also: → 1195164
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ec1a567031b3
https://hg.mozilla.org/mozilla-central/rev/27e51c924a74
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Blocks: 1197083
No longer blocks: 1197083
You need to log in before you can comment on or make changes to this bug.