1192733 - Test case for SetDormant() in MDSM

Status: RESOLVED FIXED

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

Description

[Alfredo Yang (:alfredo)]

A following bug of bug 1192694.

It needs to make sure MSDM can enter/leave dormant normally before or after decoding first frame.

Comment 1

[Benjamin Chen [: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. ...

Comment 2

[Blake Wu [:bwu][:blakewu]]

Awesome! We really need some test case to test the dormant mechanism!

Comment 3

[Benjamin Chen [:bechen]]

tips:
see bug 1181864, EME's dormant is disable.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 5

[Benjamin Chen [:bechen]]

Attachment: bug-1192733.part1.v01.patch

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...

Comment 6

[Benjamin Chen [:bechen]]

(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.

Comment 7

[Benjamin Chen [:bechen]]

Attachment: bug-1192733.part1.fix.v01.patch

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.

Comment 8

[Benjamin Chen [:bechen]]

Attachment: bug-1192733.part1.addtestcase.patch

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);|

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 11

[Jean-Yves Avenard [:jya]]

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.

Comment 12

[Jean-Yves Avenard [:jya]]

regression was introduced by bug 1146086

Comment 13

[Benjamin Chen [:bechen]]

Attachment: bug-1192733.part1.addtestcase.patch

(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.

Comment 14

[Benjamin Chen [:bechen]]

Attachment: bug-1192733.part1.fix.v01.patch

Address comment 11.
r=jya

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

(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 16

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 17

[Benjamin Chen [:bechen]]

Attachment: bug-1192733.part1.addtestcase.patch

Address comment 16.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=018328a5fd48

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec1a567031b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/27e51c924a74

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec1a567031b3
https://hg.mozilla.org/mozilla-central/rev/27e51c924a74