957439 - MediaRecorder: "Assertion failure: NS_IsMainThread() && mTrackUnionStream" [@mozilla::dom::MediaRecorder::Session::Pause]

Status: VERIFIED FIXED

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

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/23e682c34d75

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Shelly Lin [:shelly]]

Has discussed with Randy about the solution, transfer the issue to me since he is blocked by something else.

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The pause() Should allow to execute until mTrackUnionStream finish setup.

Comment 4

[Shelly Lin [:shelly]]

Attachment: Assertion fail at Pause

Comment 5

[Shelly Lin [:shelly]]

Attachment: Assertion fail at Pause

Comment 6

[Jason Smith [:jsmith]]

(In reply to Shelly Lin [:shelly] from comment #5)
> Created attachment 8359676 [details] [diff] [review]
> Assertion fail at Pause

Note - we should really include a mochitest with this patch following in alignment with the reduced test case included on this bug.

Comment 7

[u459114]

(In reply to Shelly Lin [:shelly] from comment #5)
> Created attachment 8359676 [details] [diff] [review]
> Assertion fail at Pause

Looks good for me

Comment 8

[Shelly Lin [:shelly]]

The actual problem in this case is that it is trying to record a pure video, the TrackType in this test case is DOMMediaStream::HINT_CONTENTS_VIDEO only, making it fail to create mEncoder at Start. And because of the error in Start(), we clear the mTrackUnionStream, triggers the assertion in Pause.

I think we should just by pass and not changing the recorder state to pause if this happened, although the ultimate solution is having an onstart() and only allow Pause and Resume being called inside?

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Comment on attachment 8359676 [details] [diff] [review]
Assertion fail at Pause

For the Fail case of MediaRecorder::Resume/Resume. 
We should throw onError event to UA to notify something wrong.
BTW, the error object interface is under discussing. So suggest sent GenericError to UA first.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Shelly Lin [:shelly] from comment #8)
> The actual problem in this case is that it is trying to record a pure video,
> the TrackType in this test case is DOMMediaStream::HINT_CONTENTS_VIDEO only,
> making it fail to create mEncoder at Start.

Why's that? We should be able to record a pure video.

Comment 11

[Shelly Lin [:shelly]]

http://dxr.mozilla.org/mozilla-central/source/content/media/encoder/MediaEncoder.cpp#123

Because the source video of test case is not .ogg and neither the track type contains ContainerWriter::HAS_AUDIO.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Shelly Lin [:shelly] from comment #11)
> Because the source video of test case is not .ogg and neither the track type
> contains ContainerWriter::HAS_AUDIO.

Why isn't the WebM encoder used? http://dxr.mozilla.org/mozilla-central/source/content/media/encoder/MediaEncoder.cpp#97

Also I can see some bugs in the logic there:
  else if (MediaDecoder::IsWebMEnabled() &&
          (aMIMEType.EqualsLiteral(VIDEO_WEBM) ||
          (aTrackTypes & ContainerWriter::HAS_VIDEO))) {
The second line should be indented by 1 extra space, and the third line by 2 extra spaces. Also, we should be checking aMIMEType.IsEmpty() in there, otherwise if we support both OMX and WebM, anything with a video track will only use WebM.

Comment 13

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Thanks, let me change it.
the WebM encoder build flag isn't enable yet and the module is still under-construction.
I think we can land it within two week. (Before 1/24)

Comment 14

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

This bug would fix c12 comment. 
Bug 960075 - [Media Encoder] Encoder should produce video only clips when media stream without audio tracks.

Comment 15

[Shelly Lin [:shelly]]

Attachment: Assertion fail at Pause if unsupported input stream.

Added a test case.

Hi Roc, not much change here, Randy suggested I should notify error on Pause and Resume, could you review this patch again? Thanks!

Comment 16

[Shelly Lin [:shelly]]

Comment on attachment 8363574 [details] [diff] [review]
Assertion fail at Pause if unsupported input stream.

Hi Jason, could you review the part of test case? Thanks.

Comment 17

[Jason Smith [:jsmith]]

Comment on attachment 8363574 [details] [diff] [review]
Assertion fail at Pause if unsupported input stream.

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

review- - this isn't going to work on a b2g emulator. There's also some additional checks that need to be added in the test.

::: content/media/test/test_mediarecorder_unsupported_src.html
@@ +6,5 @@
> +  <script type="text/javascript" src="manifest.js"></script>
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=957439">Mozill
> +a Bug 957439</a>

Nit - it's probably okay here to break the 80 char limit, as it will be more readable. I'd just put this all on one line.

@@ +10,5 @@
> +a Bug 957439</a>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +function startTest() {

So I notice the test case here is different than the attached test case on the bug. We've confirmed that the assertion still fires without the bug fix here, right?

@@ +11,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +function startTest() {
> +  // In order to generate an "unsupported stream", perf off video encoding to

Nit - pref, not perf

@@ +13,5 @@
> +
> +function startTest() {
> +  // In order to generate an "unsupported stream", perf off video encoding to
> +  // make the platform support audio encoding only.
> +  SpecialPowers.setBoolPref("media.encoder.webm.enabled", false);

This function call is async, so we can't assume the pref is on after this line of code. We need to wait until the preference is confirmed to be on. 

Note - this will fail on b2g emulators. See bug 962883 as an example of a test that already fails for this reason.

@@ +18,5 @@
> +
> +  navigator.mozGetUserMedia({audio: false, video: true, fake: true},
> +    function(stream) {
> +      var onerrorCalled = false;
> +      var mediaRecorder = new MediaRecorder(stream);

There's two additional event handlers we need to handle here:

* ondataavailable - should fire
* onwarning - shouldn't fire

We also need to check media recorder state in relevant event handlers. See http://hg.mozilla.org/mozilla-central/file/b9d9649e7ec0/content/media/test/test_mediarecorder_creation_fail.html for an example of things to check here.

Comment 18

[Shelly Lin [:shelly]]

Comment on attachment 8363574 [details] [diff] [review]
Assertion fail at Pause if unsupported input stream.

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

::: content/media/test/test_mediarecorder_unsupported_src.html
@@ +10,5 @@
> +a Bug 957439</a>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +function startTest() {

Yes, I've tested it without the fix, and it fired the assertion as expected.

@@ +13,5 @@
> +
> +function startTest() {
> +  // In order to generate an "unsupported stream", perf off video encoding to
> +  // make the platform support audio encoding only.
> +  SpecialPowers.setBoolPref("media.encoder.webm.enabled", false);

Thanks for the heads-up, I'll use PushPrefEnv instead. Btw, can I use #define such as #define MOZ_WEBM_ENCODER to turn off different flag in the test file?

Comment 19

[Jason Smith [:jsmith]]

(In reply to Shelly Lin [:shelly] from comment #18)
> @@ +13,5 @@
> > +
> > +function startTest() {
> > +  // In order to generate an "unsupported stream", perf off video encoding to
> > +  // make the platform support audio encoding only.
> > +  SpecialPowers.setBoolPref("media.encoder.webm.enabled", false);
> 
> Thanks for the heads-up, I'll use PushPrefEnv instead. Btw, can I use
> #define such as #define MOZ_WEBM_ENCODER to turn off different flag in the
> test file?

I don't know - I haven't used an approach like that before in a mochitest to flip a pref.

Comment 20

[Shelly Lin [:shelly]]

Attachment: Assertion fail at Pause, w/ test case updated

Update the test case.

Comment 21

[Jason Smith [:jsmith]]

Comment on attachment 8364838 [details] [diff] [review]
Assertion fail at Pause, w/ test case updated

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

r- - still need to add a few more checks to the test here.

::: content/media/test/test_mediarecorder_unsupported_src.html
@@ +5,5 @@
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +  <script type="text/javascript" src="manifest.js"></script>
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=957439">Mozill Bug 957439</a>

Nit - Mozilla, not Mozill

@@ +13,5 @@
> +function startTest() {
> +  navigator.mozGetUserMedia({audio: false, video: true, fake: true},
> +    function(stream) {
> +      var onerrorCalled = false;
> +      var mediaRecorder = new MediaRecorder(stream);

There's still some missing pieces here for attribute checks. We need to check things similar to what's seen in test_mediarecorder_creation_fail.html such as:

* mediaRecorder.mimeType
* mediaRecorder.state
* ensure the events fire in the right order
* ensure the blob generated by ondataavailable has the correct size & type

@@ +61,5 @@
> +// make the platform support audio encoding only.
> +SpecialPowers.pushPrefEnv({
> +    "set": [
> +      ["media.encoder.webm.enabled", false],
> +      ["media.encoder.omx.enabled", false],

Nit - this isn't necessary, as gUM streams don't deal with the OMX encoder.

Comment 22

[Shelly Lin [:shelly]]

Attachment: Assertion fail at Pause, w/ test case updated

Comment 23

[Shelly Lin [:shelly]]

Comment on attachment 8364907 [details] [diff] [review]
Assertion fail at Pause, w/ test case updated

Update test case.

Comment 24

[Jason Smith [:jsmith]]

Comment on attachment 8364907 [details] [diff] [review]
Assertion fail at Pause, w/ test case updated

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

r+ with questions & comments addressed

::: content/media/test/test_mediarecorder_unsupported_src.html
@@ +15,5 @@
> +    function(stream) {
> +
> +      // Expected callback sequence should be:
> +      // 1. onerror
> +      // 2. onerror

Why are 2 onerror callbacks firing here? Shouldn't there only be one onerror callback?

@@ +19,5 @@
> +      // 2. onerror
> +      // 3. ondataavailable
> +      // 4. onstop
> +      var callbackStep = 0;
> +      var mediaRecorder = new MediaRecorder(stream);

Somewhere in here we should check that mediaRecorder.stream matches the stream object returned in the success callback.

@@ +75,5 @@
> +SimpleTest.waitForExplicitFinish();
> +
> +// In order to generate an "unsupported stream", pref off video encoding to
> +// make the platform support audio encoding only.
> +SpecialPowers.pushPrefEnv({

Nit - indentation looks off here

Comment 25

[Shelly Lin [:shelly]]

Attachment: Assertion fail at Pause, w/ test case updated

Comment 26

[Shelly Lin [:shelly]]

Comment on attachment 8365751 [details] [diff] [review]
Assertion fail at Pause, w/ test case updated

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

::: content/media/test/test_mediarecorder_unsupported_src.html
@@ +15,5 @@
> +    function(stream) {
> +
> +      // Expected callback sequence should be:
> +      // 1. onerror (from start)
> +      // 2. onerror (from pause)

There will be two onerror calls in this case, one from start() which fails at creating the MediaEncoder, the other from pause() which fails at validating mTrackUnionStream, which is destroyed due to the failure of start().

Comment 27

[Shelly Lin [:shelly]]

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=3ed8bcf362ed

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4680e4f0354b

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4680e4f0354b

Comment 30

[Jason Smith [:jsmith]]

Verified via clean landing with mochitest.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Why did you remove the threadsafety assertions?

Comment 32

[Shelly Lin [:shelly]]

Gosh....I should just remove the check of mTrackUnionStream in previous assertions.

Comment 33

[Shelly Lin [:shelly]]

Follow-up bug 971664.