Audio recorded with MediaRecorder is crackling really badly... but only some times. I can't find what's exactly the difference, but when it crackles, this example always crackles: https://mozdevs.github.io/MediaRecorder-examples/record-live-audio.html Whereas this example never produces the crackles, whether using audio only or audio and video: https://mozdevs.github.io/MediaRecorder-examples/media-recorder-mimetypes.html I've tried to discard if it's due to * specifying or not specifying video: false when requesting the media stream in getUserMedia * specifying, or not, the mime type when creating the MediaRecorder instance * trying different mime types (audio/ogg vs video/webm) but I can't find any definitive difference that makes this reproducible. When it crackles it crackles, and maybe in the same session it starts fine and then goes on to getting 'crackling'. Then I close the browser and reopen it and it's fine again or perhaps it goes crackling from the beginning right after starting the browser. Happening both with today's Nightly and the latest Dev Edition, on Mac using 10.10.5.

Sole -- Can you upload a copy of the recorded file that has the crackling? That would help a lot.

Sole -- One other request: can you disable e10s and try to reproduce the crackling? I'm very curious if e10s is involved here. Thanks.

Tried without e10s-crackling is still present even right after restarting the browser. I can't download the generated blob - the file list always says "failed" instead of actually downloading the file. Not sure if it's because the same blob that is used as src for an audio tag cannot be used as href of a file for download? (maybe I have found ANOTHER bug?!?) In any case I have recorded this screencast that shows the crackling. Perhaps that will guide the highly trained ears of your team of experts towards the solution! Please have a listen: http://soledadpenades.com/tmp/crackle.mp4

It's odd.... I was getting 100% repro on mac in e10s, and none in non-e10s (same browser). Then it stopped happening, and wouldn't happen again. I did modify the demo to save the blob; it of course shows the same thing. I pushed a try that saves the input to mediarecorder (as raw mono audio data) to /tmp/mediarecorder.raw (just "mediarecorder.raw" on windows). https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d698e60de79 I play it in Audacity via "File->Import->Raw", and set it to signed 16-bit mono 44100Khz (since that's what the natural output frequency is for my system). Sounds perfect the in test I did, but the test wasn't failing. I've tried linux and mac; only reproed on mac so far. As for the difference - the other test starts the recorder immediately on getting the stream. I tried that; it works, but my system had stopped reproducing so who knows if that solves it. It may make sense in any case.

zip of a modified copy of the recorder test that allows saving the data to a file.

With my Try on Mac, I was able to repro. Input is 100% clean. Also, a modified version that forces waiting on onmetadataloaded (like in the "test that never fails" above) also showed the problem, so I don't think that alone is the issue.

Some more info: I restarted my system multiple times just in case it was a case of "need to turn it off and on again", but it doesn't make a difference on the reproducibility (or lack thereof)

null chunks are causing 99 or 100 sample stretches of silence to be inserted into audio recordings (stretching and distorting the input data). Nice and clean with the 'if (!chunk.IsNull())'

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6865ccd6ec8 -- This sounds *so* much better. Still need to figure why it was so bad without it (underruns...) but DirectListeners for a Recorder make a lot of sense. Note this will NOT affect things like WebAudio sources.

Comment on attachment 8732395 [details] [diff] [review] Move MediaRecorder to use DirectListeners wherever possible Review of attachment 8732395 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/encoder/MediaEncoder.cpp @@ +38,5 @@ > > void > +MediaEncoder::SetDirectConnect(bool aConnected) > +{ > + fprintf(stderr, "****** direct connect: %s\n", aConnected ? "true" : "false"); Remove this

Simple fix for pause()/resume(); tested and works

This should resolve any issues with lost duration after Resume causing sync issues; the duration given the first video frame after Resume will be appended to a video frame we get from the DirectListener; this should ensure the a/v sync remains good.

MediaRecorder depends on TRACK_EVENT_ENDED events in NotifyQueuedTrackChanges to terminate recording on stream.stop(); this passed through the events and avoids intermittent failures.

We don't want to re-use an existing TrackUnion since we may Suspend on it - simplifies things too

Try is green

Comment on attachment 8732485 [details] [diff] [review] Patch 2 - Make recorder.pause()/resume() work with DirectListeners Though I'll take other reviewers too! The point here is to deal with roc's comments in IRC about pause()/resume() causing a/v misalignment if we don't capture the duration of the partial frame that's sent right after Resume(). We capture the duration of that frame, and (effectively) add it to the next frame that gets pushed in via a DirectListener. AppendVideoSegments() in TrackEncoder.cpp accumulates duration of Null segments and gives them to the next real frame.

Comment on attachment 8732522 [details] [diff] [review] Patch 3 - Pass TRACK_EVENT_ENDED events through to the TrackEncoders MediaRecorder relies on TRACK_EVENT_ENDED,but if we ignore the NotifyQueuedTrackChanges() calls entirely we might miss it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bd7b787162 is green, btw

Comment on attachment 8732485 [details] [diff] [review] Patch 2 - Make recorder.pause()/resume() work with DirectListeners Review of attachment 8732485 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/encoder/MediaEncoder.h @@ +85,5 @@ > + RECORD_NOT_SUSPENDED, > + RECORD_SUSPENDED, > + RECORD_RESUMED > + }; > + nit: trailing space. @@ +97,5 @@ > + { > + // Note - called from control code, not on MSG threads > + if (mSuspended == RECORD_SUSPENDED) { > + // Arm to collect the Duration of the next video frame and give it > + // to the next frame, in order to avoid any possible loss of sync Can you put the comments for these two functions on top of the functions in the style of the surrounding code?

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e90b54f195de974aebc54bc3de7c119ba28947 Bug 1257318: Move MediaRecorder to use DirectListeners wherever possible. r=roc https://hg.mozilla.org/integration/mozilla-inbound/rev/106213af2460a8e34ba10cd251326884b5020127 Bug 1257318: Make recorder.pause()/resume() work with DirectListeners r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e7ee6970dd9ab8b11e18dcce8e328deb708add Bug 1257318: Pass TRACK_EVENT_ENDED events through to the TrackEncoders r=padenot

Comment on attachment 8732485 [details] [diff] [review] Patch 2 - Make recorder.pause()/resume() work with DirectListeners This is for all three patches in this series. Approval Request Comment [Feature/regressing bug #]: The bug was present but very rare in non-e10s situations [User impact if declined]: Corrupted audio when using MediaRecorder in certain load situations. Can be easily reproduced on all platforms. [Describe test coverage new/current, TreeHerder]: It was always clean on the infra, because the machines are very different in terms of speed and load. We're investigating writing more quality (vs. correctness) tests, but it looks challenging in the current setup. [Risks and why]: Can be backed-out cleanly if really needed, but this is really just re-routing data into an heavily used and tested code path. [String/UUID change made/needed]: none.

https://hg.mozilla.org/mozilla-central/rev/c2e90b54f195 https://hg.mozilla.org/mozilla-central/rev/106213af2460 https://hg.mozilla.org/mozilla-central/rev/f8e7ee6970dd

Hi Jesup, have we used the examples from comment 0 to verify the fix works? I am not comfortable uplifting based on the test coverage mentioned in comment 21.

(In reply to Ritu Kothari (:ritu) from comment #23) > Hi Jesup, have we used the examples from comment 0 to verify the fix works? > I am not comfortable uplifting based on the test coverage mentioned in > comment 21. We've verified this quite well manually, using the examples in comment 0 (and others). Most of our mochitests don't test media quality directly, since the VMs they use are inherently highly variable (and low) performance, which can cause quality problems you'd never see on a real machine. Also, quality measurement for media is actually a very tough thing to do reasonably. On top of that, when certain problems happen (like the underruns here) the system tries to hide them (by replacing them with silence), so it's hard to discern if a failure occurs without careful examination of the media when played back. This problem turns out to be easy for a bunch of us to reproduce locally, and thus to test manually, and we all see it fixed in our testing. We'd like to get this in the next spin of dev edition - when is that? We're also planning a blog posting (which is where the examples in comment 0 came from).

Comment on attachment 8732485 [details] [diff] [review] Patch 2 - Make recorder.pause()/resume() work with DirectListeners Was verified locally using media samples, seems safe to uplift to Aurora47

(In reply to Randell Jesup [:jesup] from comment #24) > (In reply to Ritu Kothari (:ritu) from comment #23) > > Hi Jesup, have we used the examples from comment 0 to verify the fix works? > > I am not comfortable uplifting based on the test coverage mentioned in > > comment 21. > > We've verified this quite well manually, using the examples in comment 0 > (and others). Most of our mochitests don't test media quality directly, > since the VMs they use are inherently highly variable (and low) performance, > which can cause quality problems you'd never see on a real machine. Also, > quality measurement for media is actually a very tough thing to do > reasonably. On top of that, when certain problems happen (like the > underruns here) the system tries to hide them (by replacing them with > silence), so it's hard to discern if a failure occurs without careful > examination of the media when played back. > > This problem turns out to be easy for a bunch of us to reproduce locally, > and thus to test manually, and we all see it fixed in our testing. We'd > like to get this in the next spin of dev edition - when is that? We're also > planning a blog posting (which is where the examples in comment 0 came from). Thanks Jesup for a detailed reply. I understand the media playback tests are hard to automate and therefore appreciate the extra due diligence. I have A+'d all 3 patches and if these land today, they should be part of the 3/26 Aurora build here: https://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora/

https://hg.mozilla.org/releases/mozilla-aurora/rev/49207e06538f https://hg.mozilla.org/releases/mozilla-aurora/rev/99dc7c2f51eb https://hg.mozilla.org/releases/mozilla-aurora/rev/d9c81b59d63c

Another demo https://jsfiddle.net/7jLjgg3d/3 from the dupe which is fixed in the latest Nightly.