It's broken, not sure why yet, but it seems to add quite a lot of latency. I'm going to back it out and rewrite all that properly. In particular, we should not use a dedicated thread for the processing, now that it's all on the same thread. We can now do the echo cancelling (if needed) and synchronously insert in the graph, which gets us lower latency and probably less CPU usage anyways. Also the code using the packetizer continuously allocates, and packetizing is not needed anyway when not using processing, plus various other issues.

It's adding latency for an unknown yet reason.

This patch is just a backout of bug 1268428.

part 1, removing AudioGUM thread

This what was meant to happen, but it didn't work because `mInputBufferLen` was never set. An nsTArray prevents this to happen. MozReview-Commit-ID: EZ4RG4XofX1

Comment on attachment 8750758 [details] [diff] [review] Remove AudioGUM thread from MediaEngine getUserMedia input Review of attachment 8750758 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp @@ +740,5 @@ > (i+1 < len) ? 0 : 1, insertTime); > > // This is safe from any thread, and is safe if the track is Finished > // or Destroyed. > + mSources[i]->AppendToTrack(mTrackID, segment, (AudioSegment *) nullptr); The third argument default to `nullptr` already.

Moved all the audio insertion in the pipeline to a shared thread, to replace the AudioGUM thread in GUM and avoid doing Opus/etc encodes on the audio input callback thread

Comment on attachment 8750953 [details] [diff] [review] Proxy audio data to a separate thread for encoding Review of attachment 8750953 [details] [diff] [review]: ----------------------------------------------------------------- I'll just note that if you had used MozReview it'd have figured out what you had changed and what had just moved. Now I just assume the big chunks of added/removed code is old code that was moved. ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp @@ +479,5 @@ > +// An async inserter for audio data, to avoid running audio codec encoders > +// on the MSG/input audio thread. Basically just bounces all the audio > +// data to a single audio processing/input queue. We could if we wanted to > +// use multiple threads and a TaskQueue. > +// nit: delete this line

This is not optimal in the sense that it's keeping webrtc.org objects around, but it's seems hard to deallocate them properly, it's a bit entangled.

Comment on attachment 8751750 [details] [diff] [review] Part 2 - Synchronously insert audio frames from the microphone in the MSG if possible Review of attachment 8751750 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp @@ +774,5 @@ > > uint32_t len = mSources.Length(); > for (uint32_t i = 0; i < len; i++) { > + MOZ_ASSERT(!isStereo); > + InsertInGraph<int16_t>(audio10ms, length, 1); So this patch (here and InsertInGraph) appears to lose the ability to process stereo input, right? was stereo every actually working through gUM? (Note that we'd want it to work for things like audio-screencapture, and in some cases stereo mics, especially if you disable all procesing. At minimum we should verify if we can ever get a stereo signal here and trigger the assert, and file a followup and include hte bug # here, and have that block audio screencapture.

https://hg.mozilla.org/integration/mozilla-inbound/rev/01262a32c45f https://hg.mozilla.org/integration/mozilla-inbound/rev/3967d009efc0

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f52e4417aa2f for media test failures: https://treeherder.mozilla.org/logviewer.html#?job_id=27794517&repo=mozilla-inbound

This should fix the crash on packetizer_ - it's a timing issue on close/free of the pipelines; likely why it showed up on Android

https://hg.mozilla.org/integration/mozilla-inbound/rev/454ff63e5995 https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3e7b53306b

Backed out for crash in mediatest test_dataChannel_basicAudio.html on Android https://hg.mozilla.org/integration/mozilla-inbound/rev/10ea1cabf6d4 https://hg.mozilla.org/integration/mozilla-inbound/rev/349f5ef87e5b Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ba3e7b53306bbd0e1e758ce14073895ebd86ff5e Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=27939080&repo=mozilla-inbound 06:48:34 INFO - 3 INFO TEST-START | dom/media/tests/mochitest/test_dataChannel_basicAudio.html 06:51:58 INFO - INFO | automation.py | Application ran for: 0:04:17.513232 06:51:58 INFO - INFO | zombiecheck | Reading PID log: /tmp/tmpD5hJcqpidlog 06:51:59 INFO - /data/tombstones does not exist; tombstone check skipped 06:51:59 INFO - mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/CRT3wxqHRA2fGGSM_jv8rQ/artifacts/public/build/fennec-49.0a1.en-US.android-arm.crashreporter-symbols.zip 06:52:03 INFO - mozcrash Copy/paste: /builds/slave/test/build/linux64-minidump_stackwalk /tmp/tmpvN_8wQ/68db250b-9a26-9c16-180fb2ae-0d1b1baf.dmp /tmp/tmpWL1UhU 06:52:13 INFO - mozcrash Saved minidump as /builds/slave/test/build/blobber_upload_dir/68db250b-9a26-9c16-180fb2ae-0d1b1baf.dmp 06:52:13 INFO - mozcrash Saved app info as /builds/slave/test/build/blobber_upload_dir/68db250b-9a26-9c16-180fb2ae-0d1b1baf.extra 06:52:13 WARNING - PROCESS-CRASH | dom/media/tests/mochitest/test_dataChannel_basicAudio.html | application crashed [@ mozilla::AudioProxyThread::InternalProcessAudioChunk] 06:52:13 INFO - Crash dump filename: /tmp/tmpvN_8wQ/68db250b-9a26-9c16-180fb2ae-0d1b1baf.dmp 06:52:13 INFO - Operating system: Android 06:52:13 INFO - 0.0.0 Linux 2.6.29-gea477bb #1 Wed Sep 26 11:04:45 PDT 2012 armv7l 06:52:13 INFO - CPU: arm 06:52:13 INFO - ARMv7 ARM Cortex-A8 features: swp,half,thumb,fastmult,vfpv2,edsp,neon,vfpv3 06:52:13 INFO - 1 CPU 06:52:13 INFO - Crash reason: SIGSEGV 06:52:13 INFO - Crash address: 0xe5e5e63d 06:52:13 INFO - Process uptime: not available 06:52:13 INFO - Thread 55 (crashed) 06:52:13 INFO - 0 libxul.so!mozilla::AudioProxyThread::InternalProcessAudioChunk [MediaPipeline.cpp:ba3e7b53306b : 559 + 0xa] 06:52:13 INFO - r0 = 0x60ff9500 r1 = 0x65abee38 r2 = 0x000001b9 r3 = 0xe5e5e5e5 06:52:13 INFO - r4 = 0x000001b9 r5 = 0x64070040 r6 = 0x65abee34 r7 = 0x000001b9 06:52:13 INFO - r8 = 0x611a0820 r9 = 0x65abee38 r10 = 0x60ff9500 r12 = 0x00000527 06:52:13 INFO - fp = 0x0000ac44 sp = 0x65abee20 lr = 0x5bbe7587 pc = 0x5bbe7cee 06:52:13 INFO - Found by: given as instruction pointer in context 06:52:13 INFO - 1 libxul.so!mozilla::runnable_args_memfn<RefPtr<mozilla::AudioProxyThread>, void (mozilla::AudioProxyThread::*)(mozilla::AudioSessionConduit*, int, mozilla::AudioChunk&, bool), mozilla::AudioSessionConduit*, int, mozilla::AudioChunk, bool>::Run [runnable_utils.h:ba3e7b53306b : 102 + 0x7] 06:52:13 INFO - r4 = 0x60dc4ce0 r5 = 0x5bbe7afd r6 = 0x00000000 r7 = 0x6109eb30 06:52:13 INFO - r8 = 0x65abfda8 r9 = 0x65abfdb4 r10 = 0x65abfdac fp = 0x65abfdb0 06:52:13 INFO - sp = 0x65abfd60 pc = 0x5bbe68b5 06:52:13 INFO - Found by: call frame info 06:52:13 INFO - 2 libxul.so!nsThreadPool::Run [nsThreadPool.cpp:ba3e7b53306b : 227 + 0x3] 06:52:13 INFO - r4 = 0x6109eb20 r5 = 0x00000000 r6 = 0x00000000 r7 = 0x6109eb30 06:52:13 INFO - r8 = 0x65abfda8 r9 = 0x65abfdb4 r10 = 0x65abfdac fp = 0x65abfdb0 06:52:13 INFO - sp = 0x65abfd80 pc = 0x5b8b77fb 06:52:13 INFO - Found by: call frame info 06:52:13 INFO - 3 libxul.so!nsThread::ProcessNextEvent [nsThread.cpp:ba3e7b53306b : 1073 + 0x3] 06:52:13 INFO - r4 = 0x678d66d0 r5 = 0x00000000 r6 = 0x65abfe00 r7 = 0x65abfe0c 06:52:13 INFO - r8 = 0x00000000 r9 = 0x65abfe47 r10 = 0x00000000 fp = 0x678d6704 06:52:13 INFO - sp = 0x65abfde0 pc = 0x5b8b65e9 06:52:13 INFO - Found by: call frame info 06:52:13 INFO - 4 libxul.so!NS_ProcessNextEvent [nsThreadUtils.cpp:ba3e7b53306b : 290 + 0xb] 06:52:13 INFO - r4 = 0x00000000 r5 = 0x00000000 r6 = 0x6108c060 r7 = 0x678d66d0 06:52:13 INFO - r8 = 0x61d18440 r9 = 0x678d6704 r10 = 0x65abff00 fp = 0x2a366028 06:52:13 INFO - sp = 0x65abfe40 pc = 0x5b8c86f5 06:52:13 INFO - Found by: call frame info 06:52:13 INFO - 5 libxul.so!mozilla::ipc::MessagePumpForNonMainThreads::Run [MessagePump.cpp:ba3e7b53306b : 352 + 0x7] 06:52:13 INFO - r4 = 0x61d18430 r5 = 0x00000000 r6 = 0x6108c060 r7 = 0x678d66d0 06:52:13 INFO - r8 = 0x61d18440 r9 = 0x678d6704 r10 = 0x65abff00 fp = 0x2a366028 06:52:13 INFO - sp = 0x65abfe50 pc = 0x5ba2679d 06:52:13 INFO - Found by: call frame info 06:52:13 INFO - 6 libxul.so!MessageLoop::Run [message_loop.cc:ba3e7b53306b : 226 + 0x5] 06:52:13 INFO - r4 = 0x6108c060 r5 = 0x65abfea0 r6 = 0x65abfe9c r7 = 0x6108c060 06:52:13 INFO - r8 = 0x678d66dc r9 = 0x678d6704 r10 = 0x65abff00 fp = 0x2a366028 06:52:13 INFO - sp = 0x65abfe80 pc = 0x5ba17513 06:52:13 INFO - Found by: call frame info 06:52:13 INFO - 7 libxul.so!nsThread::ThreadFunc [nsThread.cpp:ba3e7b53306b : 465 + 0x5] 06:52:13 INFO - r4 = 0x678d66d0 r5 = 0x65abfea0 r6 = 0x65abfe9c r7 = 0x6108c060 06:52:13 INFO - r8 = 0x678d66dc r9 = 0x678d6704 r10 = 0x65abff00 fp = 0x2a366028 06:52:13 INFO - sp = 0x65abfe98 pc = 0x5b8b815b 06:52:13 INFO - Found by: call frame info 06:52:13 INFO - 8 libnss3.so!_pt_root [ptthread.c:ba3e7b53306b : 216 + 0x5] 06:52:13 INFO - r4 = 0x659f4500 r5 = 0x00000000 r6 = 0x52b096d0 r7 = 0x2a366028 06:52:13 INFO - r8 = 0x52b096d0 r9 = 0x65a80000 r10 = 0x65abff00 fp = 0x2a366028 06:52:13 INFO - sp = 0x65abfec8 pc = 0x52ac0ae9 06:52:13 INFO - Found by: call frame info 06:52:13 INFO - 9 libc.so!__thread_entry [pthread_create.cpp : 92 + 0x6] 06:52:13 INFO - r4 = 0x65abff00 r5 = 0x2a366028 r6 = 0x52ac0a2d r7 = 0x659f4500 06:52:13 INFO - r8 = 0x66f7f92c r9 = 0x65a80000 r10 = 0x65abff00 fp = 0x2a366028 06:52:13 INFO - sp = 0x65abfee8 pc = 0x40033a5c 06:52:13 INFO - Found by: call frame info 06:52:13 INFO - 10 libc.so!pthread_create [pthread_create.cpp : 201 + 0x16] 06:52:13 INFO - r3 = 0x659f4500 r4 = 0x00000000 r5 = 0x00040000 r6 = 0x659f4500 06:52:13 INFO - r7 = 0x00000078 r8 = 0x66f7f92c r9 = 0x65a80000 r10 = 0x65abff00 06:52:13 INFO - fp = 0x2a366028 sp = 0x65abff00 pc = 0x40033bd8 06:52:13 INFO - Found by: call frame info

We need to ensure the conduit sticks around as well, and we can't just make it a RefPtr<>& because it has to be released on MainThread. Simpler to encapsulate it in the AudioProxyThread object to start with. Try: (will check this time!) https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5D5Dc5477bd9e33

The try didn't work?

(In reply to Randell Jesup [:jesup] from comment #11) > Comment on attachment 8751750 [details] [diff] [review] > Part 2 - Synchronously insert audio frames from the microphone in the MSG if > possible > > Review of attachment 8751750 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp > @@ +774,5 @@ > > > > uint32_t len = mSources.Length(); > > for (uint32_t i = 0; i < len; i++) { > > + MOZ_ASSERT(!isStereo); > > + InsertInGraph<int16_t>(audio10ms, length, 1); > > So this patch (here and InsertInGraph) appears to lose the ability to > process stereo input, right? was stereo every actually working through gUM? > (Note that we'd want it to work for things like audio-screencapture, and in > some cases stereo mics, especially if you disable all procesing. No, it works with audio capture and web audio and HTMLMediaElement -> PeerConnection, but not with microphones, I think. It's easy to enable stereo mic in this processing code, but currently mono input is hard coded in GraphDriver.cpp.

Review commit: https://reviewboard.mozilla.org/r/56226/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56226/

Comment on attachment 8757822 [details] MozReview Request: Bug 1271585 - Part 2 - Synchronously insert audio frames from the microphone in the MSG if possible. r?pehrsons https://reviewboard.mozilla.org/r/56226/#review52850 Looks good. I only have minor issues and nits. ::: dom/media/webrtc/MediaEngine.h:64 (Diff revision 1) > static const int DEFAULT_SAMPLE_RATE = 16000; > #endif > + // This allows using whatever rate the graph is using for the > + // MediaStreamTrack. This is useful for microphone data, we know it's already > + // at the correct rate for insertion in the MSG. > + static const int USE_GRAPH_RATE = 0; In my book 0 should be avoided since it's the default value for int. Can we do with -1? ::: dom/media/webrtc/MediaEngineWebRTC.h:521 (Diff revision 1) > + bool PassThrough() { > + return mSkipProcessing; > + } > + template<typename T> > + void InsertInGraph(const T* aBuffer, > + size_t aFrames, > + uint32_t aChannels); > + void PacketizeAndProcess(MediaStreamGraph* aGraph, > + const AudioDataValue* aBuffer, > + size_t aFrames, > + TrackRate aRate, > + uint32_t aChannels); Empty line between methods for readability. ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:497 (Diff revision 1) > } > int16_t* packet = mInputBuffer.Elements(); > mPacketizer->Output(packet); > > - mVoERender->ExternalRecordingInsertData(packet, samplesPerPacket, aRate, 0); > + mVoERender->ExternalRecordingInsertData(packet, samplesPerPacket, > + aRate, 0); Indentation. ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:498 (Diff revision 1) > int16_t* packet = mInputBuffer.Elements(); > mPacketizer->Output(packet); > > - mVoERender->ExternalRecordingInsertData(packet, samplesPerPacket, aRate, 0); > + mVoERender->ExternalRecordingInsertData(packet, samplesPerPacket, > + aRate, 0); > + } Indentation. ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:511 (Diff revision 1) > + uint32_t len = mSources.Length(); > + for (uint32_t i = 0; i < len; i++) { Doesn't Length() return size_t? ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:524 (Diff revision 1) > + LogTime(AsyncLatencyLogger::AudioTrackInsertion, LATENCY_STREAM_ID(mSources[i].get(), mTrackID), > + (i+1 < len) ? 0 : 1, insertTime); Line length, indentation. ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:529 (Diff revision 1) > + LogTime(AsyncLatencyLogger::AudioTrackInsertion, LATENCY_STREAM_ID(mSources[i].get(), mTrackID), > + (i+1 < len) ? 0 : 1, insertTime); > + > + nsAutoPtr<AudioSegment> segment(new AudioSegment()); > + AutoTArray<const T*, 1> channels; > + // XXX handle stereo Add a bug number. ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:530 (Diff revision 1) > + (i+1 < len) ? 0 : 1, insertTime); > + > + nsAutoPtr<AudioSegment> segment(new AudioSegment()); > + AutoTArray<const T*, 1> channels; > + // XXX handle stereo > + MOZ_ASSERT(aChannels == 1); I think a message would be useful here! ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:538 (Diff revision 1) > + mPrincipalHandles[i]); > + segment->GetStartTime(insertTime); > + > + RUN_ON_THREAD(mThread, > + WrapRunnable(mSources[i], &SourceMediaStream::AppendToTrack, > + mTrackID, segment, (AudioSegment*)nullptr), Use c++-style static_cast instead. ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:554 (Diff revision 1) > + if (!PassThrough()) { > + PacketizeAndProcess(aGraph, aBuffer, aFrames, aRate, aChannels); > + } else { > + InsertInGraph<AudioDataValue>(aBuffer, aFrames, aChannels); > } Remove the negation in the conditional for readability.

https://hg.mozilla.org/integration/mozilla-inbound/rev/466534e08cbc https://hg.mozilla.org/integration/mozilla-inbound/rev/999676db297b

https://hg.mozilla.org/mozilla-central/rev/466534e08cbc https://hg.mozilla.org/mozilla-central/rev/999676db297b https://hg.mozilla.org/mozilla-central/rev/7a2d02375445

rebased the audio threading change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3c87509bf9b

Rebased patch, merges the conduit reference fix

Comment on attachment 8777322 [details] [diff] [review] Proxy audio data to a separate thread for encoding carry forward r=pehrsons

rebased removal of Audio GUM thread

Comment on attachment 8777328 [details] [diff] [review] Remove AudioGUM thread from MediaEngine getUserMedia input carry forward r=padenot

Updated try with both patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f351d714abf

To reduce load from running the checks

To reduce load, especially on VMs and doubly on android emulators

May lengthen the time to complete slightly

Comment on attachment 8778691 [details] [diff] [review] rework trackDisabling test to avoid running two analyzers at once Review of attachment 8778691 [details] [diff] [review]: ----------------------------------------------------------------- Perhaps splitting audio and video into separate tests is a better long-term solution. ::: dom/media/tests/mochitest/test_peerConnection_trackDisabling.html @@ +79,5 @@ > .then(() => checkAudioEnabled(localAnalyser)) > + .then(() => test.pcLocal._pc.getLocalStreams()[0].getAudioTracks()[0].enabled = false) > + .then(() => info("Checking local audio disabled")) > + .then(() => checkAudioDisabled(localAnalyser)) > + .then(() => localAnalyser = null) Does this rely on GC?

Comment on attachment 8778692 [details] [diff] [review] reduce load running clone trackDisabling tests by reducing the number of analyzers Review of attachment 8778692 [details] [diff] [review]: ----------------------------------------------------------------- Same thoughts here with splitting audio and video into separate tests.

Comment on attachment 8778693 [details] [diff] [review] Slow down webaudio+webrtc analysis result checks to reduce emulator test load Review of attachment 8778693 [details] [diff] [review]: ----------------------------------------------------------------- Would changing the analyser's fftSize change how much data to process?

Comment on attachment 8778689 [details] [diff] [review] Use longer retries for stats checks after the first one Review of attachment 8778689 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: dom/media/tests/mochitest/pc.js @@ +1464,3 @@ > .then(stats => hasFlow(stats)? ok(true, "RTP flowing for track " + track.id) : > + wait(delay).then(retry(1000))); > + return retry(200); Makes me wonder if this should get implemented as a kind of exponential back off...

Comment on attachment 8778693 [details] [diff] [review] Slow down webaudio+webrtc analysis result checks to reduce emulator test load Review of attachment 8778693 [details] [diff] [review]: ----------------------------------------------------------------- Just a clarification question, but otherwise LGTM ::: dom/media/tests/mochitest/head.js @@ +127,5 @@ > return; > } > // else, we need more time > + // would love to make it somewhat adaptive (backoff) > + setTimeout(analysisLoop, 500); Second place where an exponential back off appears to make sense. Maybe we should add some helper function to head.js for that. Did you on purpose drop the requestAnimationFrame() call? I guess it is pointless as you sleep 500ms now. Just wanted to make sure that you did not drop this by accident.

Comment on attachment 8778694 [details] [diff] [review] Disable a number of webrtc tests on android emulator Review of attachment 8778694 [details] [diff] [review]: ----------------------------------------------------------------- Would prefer if you would make the suggested change, but technically not needed before landing. ::: dom/media/tests/mochitest/mochitest.ini @@ +24,5 @@ > !/dom/media/test/gizmo.mp4 > > [test_a_noOp.html] > [test_dataChannel_basicAudio.html] > +skip-if = toolkit == 'gonk' || buildapp == 'mulet' || (android_version == '18' && debug) # Bug 962984 for debug, bug 963244 for opt I think you should replace the "android_version == '18'" checks in this patch with "toolkit == 'android'", as I don't see how these test failures are specific to a version of Android.

Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/eee98c38241e Remove AudioGUM thread from MediaEngine getUserMedia input r=padenot https://hg.mozilla.org/integration/mozilla-inbound/rev/e49e6e9abae3 Proxy audio data to a separate thread for encoding r=pehrsons https://hg.mozilla.org/integration/mozilla-inbound/rev/5315624028d5 Use longer retries for stats checks after the first one r=drno https://hg.mozilla.org/integration/mozilla-inbound/rev/a82c9e7d2030 Disable a number of webrtc tests on android emulator r=drno https://hg.mozilla.org/integration/mozilla-inbound/rev/6b36a3160529 Disable trackCloning tests on android and addtrack_removetrackevents r=pehrsons

Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/354e31887dbd Disable peerconnection_addtrack_removetrack_events, not getusermedia rs=jesup

https://hg.mozilla.org/mozilla-central/rev/eee98c38241e https://hg.mozilla.org/mozilla-central/rev/e49e6e9abae3 https://hg.mozilla.org/mozilla-central/rev/5315624028d5 https://hg.mozilla.org/mozilla-central/rev/a82c9e7d2030 https://hg.mozilla.org/mozilla-central/rev/6b36a3160529 https://hg.mozilla.org/mozilla-central/rev/354e31887dbd