Closed
Bug 926684
Opened 11 years ago
Closed 10 years ago
MediaRecorder: "Assertion failure: mEncoder (CreateEncoder failed)"
Categories
(Core :: Audio/Video: Recording, defect)
Tracking
()
VERIFIED
FIXED
mozilla29
People
(Reporter: jruderman, Assigned: rlin)
Details
(Keywords: assertion, testcase)
Attachments
(4 files, 3 obsolete files)
179 bytes,
text/html
|
Details | |
8.18 KB,
text/plain
|
Details | |
3.72 KB,
patch
|
jsmith
:
review+
|
Details | Diff | Splinter Review |
3.73 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
Updated•11 years ago
|
Blocks: MediaRecording
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → rlin
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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-
Assignee | ||
Comment 4•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
Use crashtest to capture this problem.
Attachment #8351162 -
Attachment is obsolete: true
Attachment #8355853 -
Flags: review?(jsmith)
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
ah, It cause crash :) I will back to write the mochitest.
Assignee | ||
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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-
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8357540 [details] [diff] [review] patch v4 Fix nits :) Thanks.
Attachment #8357540 -
Flags: review?(jsmith)
Assignee | ||
Updated•10 years ago
|
Attachment #8356999 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8357540 -
Flags: review?(jsmith) → review+
Assignee | ||
Comment 12•10 years ago
|
||
check-in patch, try result https://tbpl.mozilla.org/?tree=Try&rev=e180046006a9
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 13•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/528f6a845df6
Flags: in-testsuite+
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/528f6a845df6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 15•10 years ago
|
||
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
Updated•10 years ago
|
Component: Video/Audio → Video/Audio: Recording
Updated•10 years ago
|
No longer blocks: MediaRecording
You need to log in
before you can comment on or make changes to this bug.
Description
•