Assertion failure: meta, at content/media/encoder/MediaEncoder.cpp:167

Karl, can you look at this?

At first glance, this looks more likely a problem with an encoder or MediaRecorder. Is there someone more familiar that code to have a look? Throw back to me if MediaStreamDestination is lying about what it is providing.

(In reply to Karl Tomlinson (:karlt) from comment #3) > At first glance, this looks more likely a problem with an encoder or > MediaRecorder. > Is there someone more familiar that code to have a look? > Throw back to me if MediaStreamDestination is lying about what it is > providing. Thanks for looking, Karl. JW -- Do you feel familiar enough with this code to look into this bug?

I am trying to repro this bug. What is the hg revision number?

Never mind. I just pull the latest Mozilla-Central and the code looks the same now.

This may related to fail to init the opus encoder and fail to retrieve metadata.

http://dxr.mozilla.org/mozilla-central/source/content/media/encoder/OpusTrackEncoder.cpp#l136 Init() fails for aChannels = 3 while Opus only allows 1 or 2 channels. We might need some error handling for OpusTrackEncoder.

Turns out this isn't just an assertion - the same test case is causing a reproducible crash. https://crash-stats.mozilla.com/report/index/6d90145a-fe9b-4c7e-879c-1c3662131110

This is a regression from bug 919905.

fire onerror to UA if failed to init encoder, also provide test case.

Comment on attachment 829834 [details] [diff] [review] patch v1 + test case Review of attachment 829834 [details] [diff] [review]: ----------------------------------------------------------------- The test result suggested by this mochitest doesn't align with the implementation in Firefox 26 - Firefox 26 actually indicates this is allowed, while the current mochitest indicates this isn't allowed. I need clarity around what the spec argues for here to know what's the right behavior that this mochitest should be checking. Clearing review to get some questions answered. When these questions get clarified, then I'll take a look at the mochitest again. ::: content/media/test/test_mediarecorder_record_wrong_audiocontext.html @@ +27,5 @@ > + ok(e.name == 'GenericError', 'onerror fired'); > + SimpleTest.finish(); > + }; > + > + mMediaRecorder.onstop = function() { This seems a bit off - if I fire onerror, why would I fire onstop as well? Is onstop expected to even fire in this test case? @@ +33,5 @@ > + is(mMediaRecorder.state, 'inactive', 'check recording status is inactive'); > + }; > + > + mMediaRecorder.ondataavailable = function (e) { > + info ('ondataavailable fired'); If this shouldn't be firing, then you should be including a test failure if this event unexpectedly fires. @@ +37,5 @@ > + info ('ondataavailable fired'); > + }; > + > + try { > + mMediaRecorder.start(1000); Note - the test case in question here uses 0 as a parameter, not 1000.

From the spec: https://dvcs.w3.org/hg/dap/raw-file/tip/media-stream-capture/MediaRecorder.html#widl-MediaRecorder-onerror (change with time...) start: If the UA is unable to start recording or at any point is unable to contine recording, it must raise a DOMError event, followed by a dataavailable event containing the Blob it has gathered, follwed by the stop event. It seems I should change to follow this one. Another way to record this kind media object is down-mixing audio channel to 2 channel. It can be another bug for this purpose.

(In reply to Randy Lin [:rlin] from comment #13) > From the spec: > https://dvcs.w3.org/hg/dap/raw-file/tip/media-stream-capture/MediaRecorder. > html#widl-MediaRecorder-onerror (change with time...) > > start: If the UA is unable to start recording or at any point is unable to > contine recording, it must raise a DOMError event, followed by a > dataavailable event containing the Blob it has gathered, follwed by the stop > event. > It seems I should change to follow this one. > > Another way to record this kind media object is down-mixing audio channel to > 2 channel. > It can be another bug for this purpose. I think the bigger question I'm still trying to understand here is why is the behavior in Firefox 26 different than what's proposed here in this patch. What's the right behavior for the reduced test case included here? Rob - What do you think?

I think we should take the code change, but the test is incorrect. We should definitely handle 3-channel output with no error. If the codec only supported 2 channels, we should downmix to 2 channels. However, Opus actually supports up to 255 channels.

Comment on attachment 829834 [details] [diff] [review] patch v1 + test case Review of attachment 829834 [details] [diff] [review]: ----------------------------------------------------------------- r+, but without the test.

Hi Monty, I checked the opusenc tool and says it can support the downmix function.(3-7ch->2ch, >8ch->mono) Is it suitable to use the tool's implementation to do the downmix stuff?

I will try to use AudioStream::DownmixAndInterleave for downmix process.

The codebase already has the downmix processing. Remove the opus encoder init channels check and downmix the pcm data.

Comment on attachment 830044 [details] [diff] [review] patch v2 Review of attachment 830044 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/OpusTrackEncoder.cpp @@ +133,3 @@ > OpusTrackEncoder::Init(int aChannels, int aSamplingRate) > { > // The track must have 1 or 2 channels. Remove this comment. @@ +145,4 @@ > if (!((aSamplingRate >= 8000) && (kOpusSamplingRate / aSamplingRate) * > aSamplingRate == kOpusSamplingRate)) { > int error; > + int outputChannels = mChannels > 2 ? 2 : mChannels; I don't understand why we're limiting to 2 channels. Opus in Ogg supports more than 2 channels.

Comment on attachment 830044 [details] [diff] [review] patch v2 Review of attachment 830044 [details] [diff] [review]: ----------------------------------------------------------------- review- only cause I think you should expand on the tests in ondataavailable. ::: content/media/test/test_mediarecorder_record_3ch_audiocontext.html @@ +25,5 @@ > + mMediaRecorder.onerror = function() { > + ok(false, 'onerror unexpectedly fired'); > + }; > + mMediaRecorder.ondataavailable = function(e) { > + ok(true, 'ondataavailable fired'); Instead of just asserting that the event fired, you should check for event blob's size, event blob's mime type, and media recorder mime type. See test_media_record_no_timeslice.html for an example of the checks needed here. @@ +26,5 @@ > + ok(false, 'onerror unexpectedly fired'); > + }; > + mMediaRecorder.ondataavailable = function(e) { > + ok(true, 'ondataavailable fired'); > + if (mMediaRecorder.state == 'recording') { Nit - You shouldn't need to check this if statement in ondataavailable in this test case context @@ +40,5 @@ > + mMediaRecorder.start(0); > + is('recording', mMediaRecorder.state, "check record state recording"); > + mMediaRecorder.requestData(); > + } catch (e) { > + ok(false, 'Can t record audio context' + e); Nit - spelling on can't & add whitespace after context @@ +45,5 @@ > + } > +} > + > +startTest(); > +SimpleTest.waitForExplicitFinish(); Nit - I'd call waitForExplicitFinish before calling startTest, just to ensure that the expectation is set before starting to execute test code.

Hi roc, Form the opus API document, if we want to support more then 2 channels, we should use multistream api to do encoding... http://www.opus-codec.org/docs/html_api-1.0.2/group__opus__encoder.html int opus_encoder_init [in] channels int: Number of channels (1 or 2) in input signal

(In reply to Jason Smith [:jsmith] from comment #21) > Comment on attachment 830044 [details] [diff] [review] > patch v2 > > Review of attachment 830044 [details] [diff] [review]: > ----------------------------------------------------------------- > > review- only cause I think you should expand on the tests in ondataavailable. > > ::: content/media/test/test_mediarecorder_record_3ch_audiocontext.html > @@ +25,5 @@ > > + mMediaRecorder.onerror = function() { > > + ok(false, 'onerror unexpectedly fired'); > > + }; > > + mMediaRecorder.ondataavailable = function(e) { > > + ok(true, 'ondataavailable fired'); > > Instead of just asserting that the event fired, you should check for event > blob's size, event blob's mime type, and media recorder mime type. See > test_media_record_no_timeslice.html for an example of the checks needed here. > > @@ +26,5 @@ > > + ok(false, 'onerror unexpectedly fired'); > > + }; > > + mMediaRecorder.ondataavailable = function(e) { > > + ok(true, 'ondataavailable fired'); > > + if (mMediaRecorder.state == 'recording') { > > Nit - You shouldn't need to check this if statement in ondataavailable in > this test case context > > @@ +40,5 @@ > > + mMediaRecorder.start(0); > > + is('recording', mMediaRecorder.state, "check record state recording"); > > + mMediaRecorder.requestData(); > > + } catch (e) { > > + ok(false, 'Can t record audio context' + e); > > Nit - spelling on can't & add whitespace after context > > @@ +45,5 @@ > > + } > > +} > > + > > +startTest(); > > +SimpleTest.waitForExplicitFinish(); > > Nit - I'd call waitForExplicitFinish before calling startTest, just to > ensure that the expectation is set before starting to execute test code. OK, I will add it.

Plus this one based one comment 10

Open another following bug https://bugzilla.mozilla.org/show_bug.cgi?id=937460 Bug 937460 - Media Recording - Change opus encoder to use Opus Multistream API For this one, after discuss with Ralph, We would downmix to 2ch first to solve this bug.

OK.

Gecko patch, fix nits.

Test case for this one. Due to this sample don't fill any pcm buffer in it, so the blob provide by media Recorder can't set the mimetype & size in blob. Just check the ondataavailable /onstop /state translate in this one.

Comment on attachment 830705 [details] [diff] [review] patch v2 Review of attachment 830705 [details] [diff] [review]: ----------------------------------------------------------------- Where are we doing the downmixing to 2 channels?

My bad....need more fix....also need > 2 ch workable test case. The PCM data should go through AudioTrackEncoder::InterleaveTrackData. ->DownmixAndInterleave function call.

test case for crash and record 4 channels audio context.

Directly change to mChannels to 2 if input channels over the encoder capacity. The downmix process can be performed when chunks contain audio channels > mChannels, then it will call DownmixAndInterleave to downmix.

Comment on attachment 830898 [details] [diff] [review] test case Review of attachment 830898 [details] [diff] [review]: ----------------------------------------------------------------- Close, but still needs some cleanup. Btw - you need to update you commit message to indicate what tests are being added here with r= line included. ::: content/media/test/test_mediarecorder_record_4ch_audiocontext.html @@ +1,4 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Test MediaRecorder Record AudioContext</title> Nit - You might want to change the title to the 4 channels piece you mention in the filename. @@ +11,5 @@ > +<script class="testbody" type="text/javascript"> > + > +function startTest() { > + var context = new AudioContext(); > + var hasonstop = false; Nit - unused variable @@ +44,5 @@ > + ok(true, 'onstop fired successfully'); > + is(mMediaRecorder.state, 'inactive', 'check recording status is inactive'); > + SimpleTest.finish(); > + }; > + mMediaRecorder.ondataavailable = function (e) { You might want to also check the blob's mime type here as well. @@ +45,5 @@ > + is(mMediaRecorder.state, 'inactive', 'check recording status is inactive'); > + SimpleTest.finish(); > + }; > + mMediaRecorder.ondataavailable = function (e) { > + if (mMediaRecorder.state == 'recording') { Nit - you've already checked this at this point, so I don't think you need this if statement ::: content/media/test/test_mediarecorder_record_crash_audiocontext.html @@ +5,5 @@ > + <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" /> > + <script type="text/javascript" src="manifest.js"></script> > +</head> > +This test case is for capturing the recorder crash when media stream has more than 2 channels. Nit - this needs to be captured in a code comment. @@ +29,5 @@ > + }; > + mediaRecorder.onerror = function() { > + ok(false, 'onerror unexpectedly fired'); > + }; > + mediaRecorder.ondataavailable = function (evt) { If the sample doesn't fill the pcm buffer, shouldn't we be checking that the mime type isn't populated & the size is zero? @@ +35,5 @@ > + 'Events fired from ondataavailable should be BlobEvent'); > + is(evt.type, 'dataavailable', > + 'Event type should dataavailable'); > + > + if (onStopFired) { I'm unsure this if check will actually catch this failure, as onstop issues a SimpleTest.finish. You probably need to do the check in onstop instead to ensure that ondataavailable fires before onstop. @@ +39,5 @@ > + if (onStopFired) { > + ok(false, 'onstop should be the last event!'); > + } > + // onstop should not have fired before ondataavailable > + if (stoptriggered == false) { I don't think you need this if check here - the important piece is that we at least fail the test.

Fix nits, also add more check for expect behavior.

Add header

Comment on attachment 831319 [details] [diff] [review] test case v4 Review of attachment 831319 [details] [diff] [review]: ----------------------------------------------------------------- review+ with nits Note - you need to add a commit message indicating what mochitests you are adding & r=jsmith on it. ::: content/media/test/test_mediarecorder_record_4ch_audiocontext.html @@ +47,5 @@ > + SimpleTest.finish(); > + }; > + mMediaRecorder.ondataavailable = function (e) { > + ok(e.data.size > 0, 'check blob has data'); > + ok(mMediaRecorder.mimeType == expectedMimeType, 'blob should has mimetype, return ' + mMediaRecorder.mimeType); Nit - use the is statement here @@ +51,5 @@ > + ok(mMediaRecorder.mimeType == expectedMimeType, 'blob should has mimetype, return ' + mMediaRecorder.mimeType); > + if (!stopTriggered) { > + mMediaRecorder.stop(); > + stopTriggered = true; > + } Nit - I'd add an else clause here to ensure that a test failure is fired if ondataavailable managed to fire twice. ::: content/media/test/test_mediarecorder_record_crash_audiocontext.html @@ +36,5 @@ > + is(evt.type, 'dataavailable', > + 'Event type should dataavailable'); > + > + // stop media recorder after first blob comes > + if (stoptriggered == false) { Nit - stoptriggered needs to be written as stopTriggered. Also, use !stopTriggered instead of direct comparison to false @@ +39,5 @@ > + // stop media recorder after first blob comes > + if (stoptriggered == false) { > + // reqeustData after data could not get any data. > + ok(evt.data.size == 0, 'blob should has no data len=' + evt.data.size); > + ok(mediaRecorder.mimeType == '', 'blob should has mimetype, return ' + mediaRecorder.mimeType); Nit - the two lines above are duplicated in both if & else clauses - move this out out of the if-else clause. Also, use the is statement instead of direct comparison. @@ +43,5 @@ > + ok(mediaRecorder.mimeType == '', 'blob should has mimetype, return ' + mediaRecorder.mimeType); > + // call stop, expect one ondataavailable event, then onstop event > + mediaRecorder.stop(); > + is(mediaRecorder.state, 'inactive', > + 'Media recorder should be Inactive'); Nit - move this to the same line as above @@ +46,5 @@ > + is(mediaRecorder.state, 'inactive', > + 'Media recorder should be Inactive'); > + stoptriggered = true; > + } else { > + is(onStopFired, false, 'ondataavailable should not follow by onstop'); Nit - I'd use ok(false) to declare a test failure here @@ +52,5 @@ > + ok(mediaRecorder.mimeType == expectedMimeType, 'blob should has mimetype, return ' + mediaRecorder.mimeType); > + } > + }; > + mediaRecorder.onstop = function () { > + onStopFired = true; Nit - you don't need this variable

Comment on attachment 831320 [details] [diff] [review] gecko patch Review of attachment 831320 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/OpusTrackEncoder.cpp @@ +137,5 @@ > ReentrantMonitorAutoEnter mon(mReentrantMonitor); > + // This version of encoder API only support 1 or 2 channels, > + // So set the mChannels less or equal 2 and > + // let InterleaveTrackData downmix pcm data. > + mChannels = aChannels > 2 ? 2 : aChannels; You should still throw an error for zero channels I think.

Add error handle by roc's suggestion. BTW. I found I can't set dest.channelCount = 0. It would throw exception, Is any way to test this case?

http://www.w3.org/TR/webaudio/#AudioNode-section channelCount The number of channels used when up-mixing and down-mixing connections to any inputs to the node. The default value is 2 except for specific nodes where its value is specially determined. This attribute has no effect for nodes with no inputs. If this value is set to zero, the implementation MUST throw a NOT_SUPPORTED_ERR exception. You might want to try something other than web audio nodes to provide zero channels.

(In reply to Randy Lin [:rlin] from comment #38) > BTW. I found I can't set dest.channelCount = 0. > It would throw exception, Is any way to test this case? I don't know. It doesn't matter.

Comment on attachment 832054 [details] [diff] [review] gecko patch v3 Review of attachment 832054 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/OpusTrackEncoder.cpp @@ +142,2 @@ > > + if (aChannels == 0) { aChannels <= 0

fix some nits. carry reviewer.

carry reviewer. I create another Bug 940933 to land the test case inorder to caupute this problem. It will land once 927579 got solved. Test with loading 828330 js code and it can pass.

try result: https://tbpl.mozilla.org/?tree=Try&rev=19d5e80eca9a

https://hg.mozilla.org/integration/b2g-inbound/rev/c9a22296f311 https://hg.mozilla.org/integration/b2g-inbound/rev/efb217f1897a

https://hg.mozilla.org/mozilla-central/rev/c9a22296f311 https://hg.mozilla.org/mozilla-central/rev/efb217f1897a

Sorry, this had to be backed out due to perma-fail on all platforms that started occurring after we merged mozilla-inbound to m-c today. Unfortunately, the range is huge due to yesterday's nearly-constant stream of bustage :(. Really sorry I don't have more for you to go off. https://hg.mozilla.org/mozilla-central/pushloghtml?startID=25683&endID=25684 Backout: https://hg.mozilla.org/mozilla-central/rev/c7cbfa315d46 Log: https://tbpl.mozilla.org/php/getParsedLog.php?id=30896532&tree=Mozilla-Central 07:36:45 INFO - 156268 INFO TEST-START | /tests/content/media/test/test_mediarecorder_record_4ch_audiocontext.html 07:36:46 INFO - 156269 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_4ch_audiocontext.html | check record state recording 07:36:47 INFO - 156270 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_4ch_audiocontext.html | check blob has data 07:36:47 INFO - 156271 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_4ch_audiocontext.html | blob should has mimetype, return audio/ogg 07:36:47 INFO - 156272 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_4ch_audiocontext.html | onerror unexpectedly fired 07:36:47 INFO - 156273 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_mediarecorder_record_4ch_audiocontext.html | check blob has data 07:36:47 INFO - 156274 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_4ch_audiocontext.html | blob should has mimetype, return audio/ogg 07:36:47 INFO - 156275 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_4ch_audiocontext.html | onstop fired successfully 07:36:47 INFO - 156276 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_4ch_audiocontext.html | check recording status is inactive 07:36:47 INFO - 156277 INFO TEST-INFO | MEMORY STAT vsize after test: 3858771968 07:36:47 INFO - 156278 INFO TEST-INFO | MEMORY STAT residentFast after test: 334757888 07:36:47 INFO - 156279 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 89242136 07:36:47 INFO - 156280 INFO TEST-END | /tests/content/media/test/test_mediarecorder_record_4ch_audiocontext.html | finished in 1229ms

OK, I will check it again.

check-in patch with test case, fix try server error.

try result: https://tbpl.mozilla.org/?tree=Try&rev=e225db2b292e

Verified on trunk - the test case no longer crashes Firefox on a 11/26 build.