Closed Bug 926684 Opened 11 years ago Closed 10 years ago

MediaRecorder: "Assertion failure: mEncoder (CreateEncoder failed)"

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla29

People

(Reporter: jruderman, Assigned: rlin)

Details

(Keywords: assertion, testcase)

Attachments

(4 files, 3 obsolete files)

Attached file j-1.html
With:
  user_pref("media.ogg.enabled", false);

Assertion failure: mEncoder (CreateEncoder failed), at content/media/MediaRecorder.cpp:294

This assertion was added in bug 803414 part 4, and moved in bug 909670.
Attached file stack
Assignee: nobody → rlin
Attached patch test case v1 (obsolete) — Splinter Review
Hi Jason,
This bug has been fixed with 879669's patch.
So I wrote a test case for capturing the encoder creating fail case.
Attachment #8351162 - Flags: review?(jsmith)
Comment on attachment 8351162 [details] [diff] [review]
test case v1

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

review- - this test isn't extensible for other mime types, so we need to ensure this only runs with ogg files. Also, left some cleanup comments below.

::: content/media/test/test_mediarecorder_creation_fail.html
@@ +20,5 @@
> +
> +  element.src = test.name;
> +  element.test = test;
> +  element.stream = element.mozCaptureStream();
> +  var callbackCount = 0;

Nit - I'd call this callbackStep, rather than callbackCount to signify what event handler step you are on.

@@ +23,5 @@
> +  element.stream = element.mozCaptureStream();
> +  var callbackCount = 0;
> +  var mediaRecorder = new MediaRecorder(element.stream);
> +
> +  mediaRecorder.onerror = function () {

Shouldn't there be an event fired here with an error code? What would be the error code fired here in the event?

@@ +24,5 @@
> +  var callbackCount = 0;
> +  var mediaRecorder = new MediaRecorder(element.stream);
> +
> +  mediaRecorder.onerror = function () {
> +    info('onerror callback fired');

I think this should be is(callbackCount, 0, 'should fired onerror callback') instead of info to handle the case if the events don't fire in the right order.

@@ +25,5 @@
> +  var mediaRecorder = new MediaRecorder(element.stream);
> +
> +  mediaRecorder.onerror = function () {
> +    info('onerror callback fired');
> +    SpecialPowers.setBoolPref("media.ogg.enabled", true);

So if I understand this correctly - this code does the following:

If an error is generated, then recording will stop with the following events fired in this order:

1. onerror
2. ondataavailable
3. onstop

Is that right?

@@ +27,5 @@
> +  mediaRecorder.onerror = function () {
> +    info('onerror callback fired');
> +    SpecialPowers.setBoolPref("media.ogg.enabled", true);
> +    callbackCount = 1;
> +  };

What would be the value of mediaRecorder.mimeType & mediaRecorder.state at this point?

@@ +40,5 @@
> +    manager.finished(token);
> +  };
> +
> +  // This handler fires every 250ms to generate a blob.
> +  mediaRecorder.ondataavailable = function (evt) {

What values would we expect the evt to hold here - specifically evt.data.size & evt.data.type?

@@ +48,5 @@
> +  };
> +
> +  // Start recording once canplaythrough fires
> +  element.oncanplaythrough = function() {
> +    SpecialPowers.setBoolPref("media.ogg.enabled", false);

This isn't going to work when these tests expand to handle multiple mime types, which is coming soon when a new mime type encoder lands. I think you need to ensure that this test is only ran with an ogg file.
Attachment #8351162 - Flags: review?(jsmith) → review-
You got it, the event sequence is if Media Recorder can't start.
1. onerror
2. ondataavailable
3. onstop

BTW, we don't allow UA to assign specific encoder(mimeType) to encoder. The mimeType would be chosen by platform.
So for audio only stream case, it should output opus, for video, it should be mp4 in b2g or WebM for others.
Attached patch patch v2 (obsolete) — Splinter Review
Use crashtest to capture this problem.
Attachment #8351162 - Attachment is obsolete: true
Attachment #8355853 - Flags: review?(jsmith)
Comment on attachment 8355853 [details] [diff] [review]
patch v2

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

Why switch to a crashtest? A mochitest is going to get more value here overall, as we're testing functional flows along with verifying that the assertion no longer fires.
Attachment #8355853 - Flags: review?(jsmith)
ah, It cause crash  :)
I will back to write the mochitest.
Attached patch patch v3 (obsolete) — Splinter Review
Back to original one, fix nits. 
For audio only case, we only produce opus in ogg (audio/opus mimeType) blob as our default choose. If we add another mimeType, we should add another type to capture this one problem also.
Attachment #8355853 - Attachment is obsolete: true
Attachment #8356999 - Flags: review?(jsmith)
Comment on attachment 8356999 [details] [diff] [review]
patch v3

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

Close. The main change that needs to happen here is that you need to move away from using the manager.runTests & instead use a single execution of startTest against the opus file under test.

::: content/media/test/test_mediarecorder_creation_fail.html
@@ +54,5 @@
> +    info('ondataavailable callback fired');
> +    is(callbackStep, 1, 'should fired ondataavailable callback');
> +    is(evt.data.size, 0, 'data size should be zero');
> +    ok(evt instanceof BlobEvent,
> +      'Events fired from ondataavailable should be BlobEvent');

I'd also add a check here for evt.data.type to ensure that it matches an empty string mime type.

@@ +70,5 @@
> +
> +  element.play();
> +}
> +
> +manager.runTests(gMediaRecorderTests, startTest);

Since we're only intending to run this test with the opus file, we don't need to use the manager.runTests function here. Instead, you can follow a typical SimpleTest.waitForExplicitFinish & startTest function call with an audio element with the source file you are intending to use for the test.

<audio id="testAudio" src="dedotos.opus"></audio>

function startTest() {
   // do test logic here using audio element testAudio above
}

SimpleTest.waitForExplicitFinish();
startTest();
Attachment #8356999 - Flags: review?(jsmith) → review-
Comment on attachment 8357540 [details] [diff] [review]
patch v4

Fix nits :) Thanks.
Attachment #8357540 - Flags: review?(jsmith)
Attachment #8356999 - Attachment is obsolete: true
Attachment #8357540 - Flags: review?(jsmith) → review+
https://hg.mozilla.org/mozilla-central/rev/528f6a845df6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Reproduced 2013-10-14-mozilla-central-debug Mac OS X 10.9.
Verified fixed FF 29.0a1 2014-01-13-mozilla-central-debug.
Status: RESOLVED → VERIFIED
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.

Attachment

General

Creator:
Created:
Updated:
Size: