Closed Bug 957439 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla29
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: posidron, Assigned: shelly)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(1 file, 7 obsolete files)

Attached file callstack (obsolete) —
Assignee: nobody → rlin
Assignee: rlin → slin
Has discussed with Randy about the solution, transfer the issue to me since he is blocked by something else.
Keywords: regression
blocking-b2g: --- → 1.4?
The pause() Should allow to execute until mTrackUnionStream finish setup.
Attached patch Assertion fail at Pause (obsolete) — Splinter Review
Attached patch Assertion fail at Pause (obsolete) — Splinter Review
Attachment #8359675 - Attachment is obsolete: true
Attachment #8359676 - Flags: review?(roc)
Attachment #8359676 - Flags: feedback?(rlin)
(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.
(In reply to Shelly Lin [:shelly] from comment #5)
> Created attachment 8359676 [details] [diff] [review]
> Assertion fail at Pause

Looks good for me
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 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.
(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.
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.
Attachment #8359676 - Flags: feedback?(rlin) → feedback+
(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.
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)
This bug would fix c12 comment. 
Bug 960075 - [Media Encoder] Encoder should produce video only clips when media stream without audio tracks.
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!
Attachment #8356932 - Attachment is obsolete: true
Attachment #8356933 - Attachment is obsolete: true
Attachment #8359676 - Attachment is obsolete: true
Attachment #8363574 - Flags: review?(roc)
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.
Attachment #8363574 - Attachment description: bug-957439-assertion-fail-at-pause.patch → Assertion fail at Pause if unsupported input stream.
Attachment #8363574 - Flags: review?(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.
Attachment #8363574 - Flags: review?(jsmith) → review-
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?
(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.
Update the test case.
Attachment #8363574 - Attachment is obsolete: true
Attachment #8363574 - Flags: review?(roc)
Attachment #8364838 - Flags: review?(roc)
Attachment #8364838 - Flags: review?(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.
Attachment #8364838 - Flags: review?(jsmith) → review-
Attachment #8364838 - Attachment is obsolete: true
Attachment #8364838 - Flags: review?(roc)
Attachment #8364907 - Flags: review?(roc)
Comment on attachment 8364907 [details] [diff] [review]
Assertion fail at Pause, w/ test case updated

Update test case.
Attachment #8364907 - Flags: review?(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
Attachment #8364907 - Flags: review?(jsmith) → review+
Attachment #8364907 - Attachment is obsolete: true
Attachment #8365751 - Flags: review?(jsmith)
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().
Attachment #8365751 - Flags: review?(jsmith) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4680e4f0354b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
blocking-b2g: 1.4? → 1.4+
Verified via clean landing with mochitest.
Status: RESOLVED → VERIFIED
Why did you remove the threadsafety assertions?
Flags: needinfo?(slin)
Gosh....I should just remove the check of mTrackUnionStream in previous assertions.
Flags: needinfo?(slin)
Follow-up bug 971664.
Component: Video/Audio → Video/Audio: Recording
No longer blocks: MediaRecording
You need to log in before you can comment on or make changes to this bug.