Move AEC from WebRTC code to our audio drivers, so that we can both echo-cancel any other audio we're generating from the browser, and to allow us to use a system or hardware echo-canceller, if they exist. -> kinetik based on discussions at All-Hands Not immediately critical; primarily to improve the performance and user-experience.

Putting it in the audio drivers is critical if we're going to allow any audio processing (especially webaudio) between the output of the PeerConnection and the speakers.

So, as an outgrowth of bug 818670 where we're enabling the AEC, here are some of our options: In increasing order of complexity and utility: 1) do AEC on all PeerConnections and streams in a peerconnection instead of just individual ones We pull audio data from separate channels, but that means the audio far-end reference data for each one is separate. With two PC's, each would have it's own echo cancelled, but not the echo of the other (and you use N times the CPU resource to do the AEC, which is expensive). We could use as a far-end signal the output to cubeb of MediaStreamGraph. That would cancel all MSG-sourced echoes. 2) Combined with 1), the cancellation can be moved entirely to occur at the microphone input in getUserMedia(). This avoids mis-cancelling when the audio source isn't a microphone, and also provides cancelling for non-peerconnection use-cases (gUM audio loopback, though it's not a super-useful usecase). 3) A better version of this would combine all MSG-sourced audio with all other audio output to the system audio drivers by the browser; this would cancel any other sources of audio that don't go through MSG (Youtube videos, etc). 4) Hooking into system-provided AECs, and/or finding a way to get access to all system audio, not just the browser's. This may only be possible on B2G, though. Note that some minor API changes will need to be made to allow feeding the reference signal into the webrtc audio channel from the 'outside'. We'll want to do them in a way that will allow the changes to be upstreamed to webrtc.org.

Plus this one for v1.3 since it blocks the webRTC committed feature.

We are planning to do this for 28, to improve AEC performance, especially in cases where we're in a mesh conference. Since landing bug 884365, the input latency is far more stable, and gives the AEC a chance to function. However, there are still issues with cancelling in mesh conferences, and on mobile we don't want to run multiple copies of the AEC if we can avoid it. Also, if we combine WebAudio with WebRTC, it will only work (outside of a few simple cases) if the AEC is before insertion to the MediaStream. Audio-only 1-1 peerconnection calls on mobile should work without this. This may slightly improve them. The big improvements will be be for other cases, per above. I would not block 1.3 on this, though I believe it will make 28/1.3.

(In reply to Randell Jesup [:jesup] from comment #4) > We are planning to do this for 28, to improve AEC performance, especially in > cases where we're in a mesh conference. Since landing bug 884365, the input > latency is far more stable, and gives the AEC a chance to function. > However, there are still issues with cancelling in mesh conferences, and on > mobile we don't want to run multiple copies of the AEC if we can avoid it. > Also, if we combine WebAudio with WebRTC, it will only work (outside of a > few simple cases) if the AEC is before insertion to the MediaStream. > > Audio-only 1-1 peerconnection calls on mobile should work without this. > This may slightly improve them. The big improvements will be be for other > cases, per above. I would not block 1.3 on this, though I believe it will > make 28/1.3. Sounds good. Moving to blocking- then.

On reviewing cubeb, I find that there's no mixed stream available currently (some OS's can provide a 'loopback' copy of what did play out (modulo issues with protected content), but not all, and we don't implement that currently). Either we'll need to mix in cubeb, or we'll need to get audio back from the OS, or we'll need to use OS AEC capabilities, if they're good enough (and don't glitch enabling and disabling, etc). The fallback will be to use AEC in the peerconnection as we do today, and cancel only one stream. :-(

WIP patch that uses an AudioStream for the remote source. Note: assumes there is only ONE audiostream in use! Note also some SDP logging stuff is mixed in, sorry

If it made sense as part of this patch to make mixed audio (from the OS or otherwise) available to web content, that would be tremendously useful for writing functional tests of a whole variety of audio-using apps. If not, I can file a separate bug.

The APIs here wouldn't directly lend themselves to web content getting access - but you could write something to use these APIs to provide that. I'd say file a separate bug, and we'll keep that in mind designing this API/patch

Thanks, Jesup. Bug 957725 filed on exposing mixed audio somehow.

This assumed a patch queue of: Bug 818822, Bug 982490 (3 patches including my fixes), then these 3. One would probably also want to flip the aec debug setting in build/gyp.mozbuild

FYI, while there are plenty of rough edges and hacks to deal with in this, this does appear to work and cancel in GUM (use http://mozilla.github.com/webrtc-landing/gum_test.html to test). The Audio test here is a cruel test, as it loops back gUM audio to the speakers, which normally would lead to an uncontrolled feedback loop (and not something someone would normally do - to note, this test does NOT need to work well; it's not a real usecase). Real usecases are a normal call in speakerphone mode, especially at higher volumes. Note also that very high volumes may cause rattling, clipping or other non-linear effects which an AEC cannot correct for.

Comment on attachment 8395049 [details] [diff] [review] Patch 2: modifications to webrtc.org single_rw_fifo Review of attachment 8395049 [details] [diff] [review]: ----------------------------------------------------------------- ted for build side, bsmedberg for the memory barrier. I'm not certain if I could hoist the MemoryBarrier out of the loop, but since Clear is almost never called I wanted to be safe.

Comment on attachment 8395049 [details] [diff] [review] Patch 2: modifications to webrtc.org single_rw_fifo Switching to glandium for review; he did the previous change to this file (__sync_synchronize)

Comment on attachment 8395158 [details] [diff] [review] Patch 1: Add farend input to webrtc.org upstream Review of attachment 8395158 [details] [diff] [review]: ----------------------------------------------------------------- note: for reference, the actual analysis is done in AudioProcessingImpl::AnalyzeReverseStream() in audio_processing_impl.cc

Comment on attachment 8395160 [details] [diff] [review] Patch 3 - add far-end output observers to getUserMedia Review of attachment 8395160 [details] [diff] [review]: ----------------------------------------------------------------- Note that this uses a singleton global observer in place of the TBD API in MSG to register (the XXX comments are where it needs insertion)

Comment on attachment 8395049 [details] [diff] [review] Patch 2: modifications to webrtc.org single_rw_fifo Review of attachment 8395049 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/webrtc/modules/audio_device/android/single_rw_fifo.cc @@ +83,5 @@ > + while (size() > 0) { > + // Memory barrier ensures that |size_| is updated after the size has changed. > + subtle::MemoryBarrier(); > + --size_; // can't do size_ = 0 > + } Seems to me you should use CompareExchange instead of decreasing size one by one.

Comment on attachment 8395158 [details] [diff] [review] Patch 1: Add farend input to webrtc.org upstream Review of attachment 8395158 [details] [diff] [review]: ----------------------------------------------------------------- This looks like it could work, but I'm not familiar with this code. rs=padenot

Comment on attachment 8395165 [details] [diff] [review] Patch 4 - Add audio playout delay config var Review of attachment 8395165 [details] [diff] [review]: ----------------------------------------------------------------- Did you forget to set a sensible default in all.js? Or maybe if you pass in 0 to the AEC, it will do something smart?

Comment on attachment 8394952 [details] [diff] [review] Patch 5 - enable AEC in getUserMedia instead of PeerConnection Review of attachment 8394952 [details] [diff] [review]: ----------------------------------------------------------------- Same thing, I'm not familiar with this code, but it seems correct. rs=padenot

Comment on attachment 8395165 [details] [diff] [review] Patch 4 - Add audio playout delay config var Review of attachment 8395165 [details] [diff] [review]: ----------------------------------------------------------------- 0 should be ok (we could use 10 as well), but likely we'll need to add platform-specific output delays similar to current "capture_delay" settings, until we add a followup to get the delay from cubeb. I'll add a separate patch to set the defaults.

Comment on attachment 8398371 [details] [diff] [review] Patch 6 - Only enable AEC when directly hooked into a PeerConnection/MediaPipeline r? to jib, as mostly this is plumbing changes

move Clear() into the output observer to reduce upstream changes

Comment on attachment 8398685 [details] [diff] [review] Patch 2: modifications to webrtc.org single_rw_fifo Removed the Clear() function; setting up a cross-platform solution for that was a bit of a pain and might not be taken upstream; I made the caller instead just Pop() until empty.

Comment on attachment 8398371 [details] [diff] [review] Patch 6 - Only enable AEC when directly hooked into a PeerConnection/MediaPipeline Review of attachment 8398371 [details] [diff] [review]: ----------------------------------------------------------------- Some questions. ::: dom/media/MediaManager.cpp @@ +450,5 @@ > mSourceStream->AddDirectListener(aListener); > + // Tell GUM to start echo-cancelling > + // XXX Only do if the app didn't specify on or off > + if (mEchoOn) { > + RUN_ON_THREAD(MediaManager::GetThread(), Should this be #ifdef MOZ_WEBRTC ? I don't have enough context, I'm just guessing from similar code in MediaManager.h @@ +591,5 @@ > + int32_t aec = (int32_t) webrtc::kEcUnchanged; > + int32_t agc = (int32_t) webrtc::kAgcUnchanged; > + int32_t noise = (int32_t) webrtc::kNsUnchanged; > + bool aec_on = false, agc_on = false, noise_on = false; > + int32_t playout_delay = 0; These could be located closer to their use. @@ +669,5 @@ > new TracksAvailableCallback(mManager, mSuccess, mWindowID, trackunion); > > #ifdef MOZ_WEBRTC > + // leave AEC off until it's connected to a PeerConnection > + mListener->AudioConfig(false, (uint32_t) aec, Previously, AudioConfig() was only called here if both do_GetService() and do_QueryInterface() succeeded, and only #ifdef MOZ_WEBRTC, whereas now it is called all the time. Is this intentional? ::: modules/libpref/src/init/all.js @@ +284,5 @@ > +pref("media.getusermedia.capture_delay", 10); > +#endif > +// Adjustments for OS-specific AudioStream+cubeb+output delay (lower bound) > +#if defined(XP_MACOSX) > +pref("media.getusermedia.playout_delay", 10); Just for readability, maybe lump playout_delay in with capture_delay to cut the number of #ifdefs in half? The structure seems identical.

Comment on attachment 8398684 [details] [diff] [review] Patch 3 - WIP patch to add far-end output observers Review of attachment 8398684 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/MediaStreamGraph.cpp @@ +1191,5 @@ > // xxx > + if (aFrames > 0 && aChannels > 0) { > + // XXX need Observer base class and registration API > + if (gFarendObserver) { > + gFarendObserver->InsertFarEnd(aMixedBuffer, aFrames, 44100, aChannels, AUDIO_FORMAT_FLOAT32); s/44100/IdealAudioRate()/, so it works on all configurations. s/AUDIO_FORMAT_FLOAT32/aFormat/, because the buffer will be integers on mobile (and we can save a float -> int16 conversion). ::: content/media/webrtc/AudioOutputObserver.h @@ +12,5 @@ > +} > + > +namespace mozilla { > + > +// Fifo for cubeb to dump data into that's sent to the speakers. This is not accurate anymore, since we take the MSG output rather than cubeb's. @@ +25,5 @@ > + > + void Clear(); > + // XXX Needs a "underran" boolean > + void InsertFarEnd(const void *aBuffer, uint32_t aSamples, > + int aFreq, int aChannels, AudioSampleFormat aFormat); The msg can certainly tell you if the input side it too slow at feeding data (because the MediaStream is blocked). @@ +27,5 @@ > + // XXX Needs a "underran" boolean > + void InsertFarEnd(const void *aBuffer, uint32_t aSamples, > + int aFreq, int aChannels, AudioSampleFormat aFormat); > + > + int8_t *Pop(); Pop() should return short*, to make it clear that the consumer will get 16-bits integer audio samples. @@ +31,5 @@ > + int8_t *Pop(); > + uint32_t Size(); > + > + uint32_t mPlayoutFreq; > + uint32_t mPlayoutChannels; No need for this to be public, right? Just write two inline members that get the value for you. That way, we can also make sure the values are valid when getting them. @@ +38,5 @@ > + nsAutoPtr<webrtc::SingleRwFifo> mPlayoutFifo; > + uint32_t mChunkSize; > + > + // chunking to 10ms support > + nsAutoPtr<int16_t> mSaved; nsTArray, please. Also, we tend to use "short" (and not int16_t) for integer audio samples in content/media. ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp @@ +20,5 @@ > #define SAMPLE_LENGTH ((SAMPLE_FREQUENCY*10)/1000) > > +// FIX! get from cubeb > +#define MAX_CHANNELS 2 > +#define MAX_SAMPLING_FREQ 48000 // Hz - multiple of 100 Either this restriction is arbitrary, and it should not exist, or it comes from the AEC code, and we should get the values from there. Also, we should not make assumptions regarding the ideal sample rate of the MSG: The MSG get the information for the OS. If the user has set a samplerate of 96kHz, say in the little windows configuration panel, then it'll be the MediaStreamGraph::IdealSampleRate(), and this will assert. @@ +21,5 @@ > > +// FIX! get from cubeb > +#define MAX_CHANNELS 2 > +#define MAX_SAMPLING_FREQ 48000 // Hz - multiple of 100 > +#define MAX_AEC_FIFO_DEPTH 200 // ms - multiple of 10 MOZ_STATIC_ASSERT(!(MAX_AEC_FIFO_DEPTH % 10), "MAX_AEC_FIFO_DEPTH should be a multiple of 10"); @@ +47,5 @@ > + > +AudioOutputObserver::AudioOutputObserver() : mPlayoutFreq(0), > + mPlayoutChannels(0), > + mChunkSize(0), > + mSamplesSaved(0) We usually put the initializer lists at column 1. @@ +97,5 @@ > + } > + } else { > + MOZ_ASSERT(aFreq <= MAX_SAMPLING_FREQ); > + mPlayoutFreq = aFreq; > + mChunkSize = aFreq/100; // 10ms MOZ_ASSERT(!(aFreq % 100), "Sampling rate for far end data should be multiple of 100."); Also, spaces around binary operators. @@ +112,5 @@ > + const float *src_float = (const float *) aBuffer; > + > + while (aSamples) { > + if (!mSaved) { > + mSaved = new int16_t[1 + mChunkSize * aChannels]; I'd be more comfortable with nsTArrays, here, so you don't need to do this little dance with the length. @@ +117,5 @@ > + // put the length in the first two bytes as an uint16_t > + mSaved[0] = mChunkSize; > + } > + uint32_t to_copy = mSaved[0] - mSamplesSaved; > + if (to_copy > mChunkSize - mSamplesSaved) Always brace your ifs, and prefer camelCase, please. @@ +138,5 @@ > + break; > + default: > + MOZ_ASSERT(false, "Unsupported audio format for AEC far end"); > + break; > + } content/media/AudioSampleFormat.h so we don't reinvent the wheel here. You can even drop the switch case because of the template magic. @@ +149,5 @@ > + aSamples -= to_copy; > + mSamplesSaved += to_copy; > + > + if (mSamplesSaved >= mChunkSize) { > + int free_slots = mPlayoutFifo->capacity() - mPlayoutFifo->size(); camelCase @@ +508,5 @@ > + // input code with "old" audio. > + if (!mStarted) { > + mStarted = true; > + while (gFarendObserver->Size() > 1) { > + int8_t *buffer = gFarendObserver->Pop(); // only call if size() > 0 MOZ_ASSERT(gFarendObserver->Size > 0);

(In reply to Paul Adenot (:padenot) from comment #48) > Comment on attachment 8398684 [details] [diff] [review] > Patch 3 - WIP patch to add far-end output observers > > Review of attachment 8398684 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ +38,5 @@ > > + nsAutoPtr<webrtc::SingleRwFifo> mPlayoutFifo; > > + uint32_t mChunkSize; > > + > > + // chunking to 10ms support > > + nsAutoPtr<int16_t> mSaved; > > nsTArray, please. Also, we tend to use "short" (and not int16_t) for integer > audio samples in content/media. mSaved is a single buffer, not an array. And we need to switch to int16_t at some point in this process as that's what this feed into in the WebRTC code. > ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp > @@ +20,5 @@ > > #define SAMPLE_LENGTH ((SAMPLE_FREQUENCY*10)/1000) > > > > +// FIX! get from cubeb > > +#define MAX_CHANNELS 2 > > +#define MAX_SAMPLING_FREQ 48000 // Hz - multiple of 100 > > Either this restriction is arbitrary, and it should not exist, or it comes > from the AEC code, and we should get the values from there. Also, we should > not make assumptions regarding the ideal sample rate of the MSG: The MSG get > the information for the OS. If the user has set a samplerate of 96kHz, say > in the little windows configuration panel, then it'll be the > MediaStreamGraph::IdealSampleRate(), and this will assert. The WebRTC AEC code doesn't handle >48000Hz currently. Also, I believe it handles at most stereo, but I'll check. > > @@ +21,5 @@ > > > > +// FIX! get from cubeb > > +#define MAX_CHANNELS 2 > > +#define MAX_SAMPLING_FREQ 48000 // Hz - multiple of 100 > > +#define MAX_AEC_FIFO_DEPTH 200 // ms - multiple of 10 > > MOZ_STATIC_ASSERT(!(MAX_AEC_FIFO_DEPTH % 10), "MAX_AEC_FIFO_DEPTH should be > a multiple of 10"); ok > @@ +47,5 @@ > > + > > +AudioOutputObserver::AudioOutputObserver() : mPlayoutFreq(0), > > + mPlayoutChannels(0), > > + mChunkSize(0), > > + mSamplesSaved(0) > > We usually put the initializer lists at column 1. All the WebRTC code I think has it indented, but I'll check. > > @@ +97,5 @@ > > + } > > + } else { > > + MOZ_ASSERT(aFreq <= MAX_SAMPLING_FREQ); > > + mPlayoutFreq = aFreq; > > + mChunkSize = aFreq/100; // 10ms > > MOZ_ASSERT(!(aFreq % 100), "Sampling rate for far end data should be > multiple of 100."); ok > > Also, spaces around binary operators. The rest of the file's style is no spaces (again I'll check). Mixing styles in a file is bad, and restyling generally to be avoided (messes up blame, etc). > > @@ +112,5 @@ > > + const float *src_float = (const float *) aBuffer; > > + > > + while (aSamples) { > > + if (!mSaved) { > > + mSaved = new int16_t[1 + mChunkSize * aChannels]; > > I'd be more comfortable with nsTArrays, here, so you don't need to do this > little dance with the length. We need to pass these as int16_t *'s to the AEC code. What I *really* want to do is replace the lockless-rw-fifo of pointers with a lockless-rw-circular-buffer of data to avoid having to allocate for every 10ms chunk, so switching to nsTArray will simply make that harder. > > @@ +117,5 @@ > > + // put the length in the first two bytes as an uint16_t > > + mSaved[0] = mChunkSize; > > + } > > + uint32_t to_copy = mSaved[0] - mSamplesSaved; > > + if (to_copy > mChunkSize - mSamplesSaved) > > Always brace your ifs, and prefer camelCase, please. Bracing - sure, missed that one I guess. local_var style in this file isn't camelCase. > > @@ +138,5 @@ > > + break; > > + default: > > + MOZ_ASSERT(false, "Unsupported audio format for AEC far end"); > > + break; > > + } > > content/media/AudioSampleFormat.h so we don't reinvent the wheel here. You > can even drop the switch case because of the template magic. I'll check that.

jib: I'll revise that patch; I think we're not going to need the "enable AEC once connected to PeerConnection part of it" (though this adds more pressure to be able to enable/disable it on the fly)

with interdiffs folded in

Comment on attachment 8400075 [details] [diff] [review] Patch 3 - WIP patch to add far-end output observers Interdiffs are uploaded as a separate patch

back to turning on AEC by default. Changed config names to reduce future confusion. Keeps access to change AEC settings in-call, though we haven't hooked that up yet

Comment on attachment 8400126 [details] [diff] [review] Patch 3 - WIP patch to add far-end output observers Added a 'break' if the rw_fifo is full to avoid ilooping (this may be fallout of the restructuring I just did).

Comment on attachment 8400086 [details] [diff] [review] Patch 6 - Only enable AEC when directly hooked into a PeerConnection/MediaPipeline Review of attachment 8400086 [details] [diff] [review]: ----------------------------------------------------------------- r=me. ::: dom/media/MediaManager.cpp @@ +599,5 @@ > +#ifdef MOZ_WEBRTC > + // Right now these configs are only of use if webrtc is available > + nsresult rv; > + nsCOMPtr<nsIPrefService> prefs = do_GetService("@mozilla.org/preferences-service;1", &rv); > + if (NS_SUCCEEDED(rv)) { One point from comment 47 is left unaddressed: if this fails... @@ +623,5 @@ > LOG(("Returning error for getUserMedia() - no stream")); > error->OnError(NS_LITERAL_STRING("NO_STREAM")); > return NS_OK; > } > + trackunion->AudioConfig(aec_on, (uint32_t) aec, ...you now set AudioConfig to hard-coded values instead of values from about:config. This is a change, but it seems reasonable.

Forgot to qref and pick up the ConvertAudioSamples change

Comment on attachment 8400452 [details] [diff] [review] Patch 3 - WIP patch to add far-end output observers Review of attachment 8400452 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp @@ +121,5 @@ > + // samples per call. Annoying... > + while (aSamples) { > + if (!mSaved) { > + mSaved = (FarEndAudioChunk *) moz_xmalloc(sizeof(FarEndAudioChunk) + > + (mChunkSize * aChannels - 1)*sizeof(int16_t)); remove the -1, right? @@ +122,5 @@ > + while (aSamples) { > + if (!mSaved) { > + mSaved = (FarEndAudioChunk *) moz_xmalloc(sizeof(FarEndAudioChunk) + > + (mChunkSize * aChannels - 1)*sizeof(int16_t)); > + // put the length in the first two bytes as an uint16_t this is obsolete @@ +127,5 @@ > + mSaved->mSamples = mChunkSize; > + mSaved->mOverrun = aOverran; > + aOverran = false; > + } > + uint32_t to_copy = mChunkSize - mSamplesSaved; camelCase

(In reply to Paul Adenot (:padenot) from comment #59) > Comment on attachment 8400452 [details] [diff] [review] > Patch 3 - WIP patch to add far-end output observers > > Review of attachment 8400452 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp > @@ +121,5 @@ > > + // samples per call. Annoying... > > + while (aSamples) { > > + if (!mSaved) { > > + mSaved = (FarEndAudioChunk *) moz_xmalloc(sizeof(FarEndAudioChunk) + > > + (mChunkSize * aChannels - 1)*sizeof(int16_t)); > > remove the -1, right? Nope. mData[1]. (Last I recall we can't use [0]) > @@ +127,5 @@ > > + mSaved->mSamples = mChunkSize; > > + mSaved->mOverrun = aOverran; > > + aOverran = false; > > + } > > + uint32_t to_copy = mChunkSize - mSamplesSaved; > > camelCase The rest of the file isn't camelCase for locals.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8cb15625567 https://hg.mozilla.org/integration/mozilla-inbound/rev/34bc46be6f67 https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4db991bf74 https://hg.mozilla.org/integration/mozilla-inbound/rev/fe548073933e https://hg.mozilla.org/integration/mozilla-inbound/rev/03402caf2023 https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce56c66a536

Backed out for perma-orange on b2g emulator M10: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=cb894b5d342f (likely the MSG changes) http://hg.mozilla.org/integration/mozilla-inbound/rev/c0eaa844f2f0

https://hg.mozilla.org/mozilla-central/rev/f8cb15625567 https://hg.mozilla.org/mozilla-central/rev/34bc46be6f67 https://hg.mozilla.org/mozilla-central/rev/1e4db991bf74 https://hg.mozilla.org/mozilla-central/rev/fe548073933e https://hg.mozilla.org/mozilla-central/rev/03402caf2023 https://hg.mozilla.org/mozilla-central/rev/5ce56c66a536

backed out of central: https://hg.mozilla.org/mozilla-central/rev/ac6cbaa47f34

https://hg.mozilla.org/integration/mozilla-inbound/rev/89a615263614 https://hg.mozilla.org/integration/mozilla-inbound/rev/6922b1261595 https://hg.mozilla.org/integration/mozilla-inbound/rev/e3664615ecbf https://hg.mozilla.org/integration/mozilla-inbound/rev/74e5c32c6fa2 https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc08e9fc7e8 https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf98d1c6b75

backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/b4cc87b3eeac https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5f6bc7cdec https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae7d42531c7 https://hg.mozilla.org/integration/mozilla-inbound/rev/20aea86b3432 https://hg.mozilla.org/integration/mozilla-inbound/rev/63be52cd09c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/df77827fc918

Third time's the charm! https://hg.mozilla.org/integration/mozilla-inbound/rev/4a27b8086916 https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5de7e21de8 https://hg.mozilla.org/integration/mozilla-inbound/rev/cb7e248e9ccf https://hg.mozilla.org/integration/mozilla-inbound/rev/263d6539e0a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/38cdf299eda8 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c233a3ad819

https://hg.mozilla.org/mozilla-central/rev/4a27b8086916 https://hg.mozilla.org/mozilla-central/rev/3f5de7e21de8 https://hg.mozilla.org/mozilla-central/rev/cb7e248e9ccf https://hg.mozilla.org/mozilla-central/rev/263d6539e0a9 https://hg.mozilla.org/mozilla-central/rev/38cdf299eda8 https://hg.mozilla.org/mozilla-central/rev/3c233a3ad819

Can somebody confirm this does break b2g build, when WEBRTC is disabled? I am on an older version of code, and just backported this patches, but I think it should break anyway, as webrtc:: code inside dom/media/MediaManager.cpp is not wrapped with ifdefs.

(In reply to Nikola from comment #69) > Can somebody confirm this does break b2g build, when WEBRTC is disabled? I > am on an older version of code, and just backported this patches, but I > think it should break anyway, as webrtc:: code inside > dom/media/MediaManager.cpp is not wrapped with ifdefs. This breaks all builds with webrtc disabled, as reported in 996433.

(In reply to Landry Breuil (:gaston) from comment #70) > (In reply to Nikola from comment #69) > > Can somebody confirm this does break b2g build, when WEBRTC is disabled? I > > am on an older version of code, and just backported this patches, but I > > think it should break anyway, as webrtc:: code inside > > dom/media/MediaManager.cpp is not wrapped with ifdefs. > > This breaks all builds with webrtc disabled, as reported in 996433. Sorry, did not see that one. Thank you for your prompt reply.