I believe the AEC is off by default currently. We're probably going to want to turn it on by default for FF20 release, and perhaps expose an API to turn it on and off (see W3C bug DB)

Comment on attachment 706412 [details] [diff] [review] merge AudioConduits to allow AEC to work (WIP, working it appears) I realize this may not be the "right" design going forward, but it makes AEC work. (Though given the way Vcm interfaces, this may actually be reasonable.) The 150ms constant is evil. I made it adjustable via about:config until we can figure out a better solution.

Comment on attachment 706411 [details] [diff] [review] Enable AEC in PeerConnection, AGC/NoiseSuppression in gUM Review of attachment 706411 [details] [diff] [review]: ----------------------------------------------------------------- Changes should be easy, but I think I'd like to see this again. ::: content/media/webrtc/MediaEngineWebRTC.h @@ +181,5 @@ > , mCapIndex(aIndex) > , mChannel(-1) > , mInitDone(false) > + , mEchoOn(false), mAgcOn(false), mNoiseOn(false) > + , mEchoCancel(webrtc::kEcAec /*kEcUnchanged*/) What's with all the commented-out "Unchanged" constants? ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp @@ +49,5 @@ > nsresult > +MediaEngineWebRTCAudioSource::Config(bool aEchoOn, uint32_t aEcho, > + bool aAgcOn, uint32_t aAGC, > + bool aNoiseOn, uint32_t aNoise) > +{ Don't we need to at least check mInitDone before accessing mVoEProcessing? @@ +60,5 @@ > + mVoEProcessing->SetEcStatus(mEchoOn, mEchoCancel); > +#endif > + mAGC = (webrtc::AgcModes) aAGC; > + mAgcOn = aAgcOn; > + mVoEProcessing->SetAgcStatus(mAgcOn, mAGC); This will reset all the current capture levels, even if the mode is unchanged. Since you have an omnibus API that doesn't allow these settings to be changed individually, I think we should wrap this in an if() to avoid calling SetAgcStatus() if the old mode is the same as the new one @@ +128,5 @@ > return NS_OK; > } > mState = kStarted; > > + int error; I don't like this at all. We should set these from one place (e.g., Config()), and do the same logging there we would do here (I don't understand why you'd want to log in one place and not the other). If there's some reason you have to wait until Start() to call them, then we shouldn't be allowed to call Config() when mState < kStarted. If you follow my suggestion above and make setting the AGC mode conditional on it actually changing, you'll need to make sure to change the mAgcMode initializer to WEBRTC_VOICE_ENGINE_AGC_DEFAULT_MODE... this is different on Android vs. everything else. I'm also not a fan of merging the error checking here. If something does fail, we won't actually have any idea which call it was. ::: dom/media/MediaManager.cpp @@ +401,5 @@ > + > + LOG(("Audio config: aec: %d, agc: %d, noise: %d", > + aec_on ? aec : -1, > + agc_on ? agc : -1, > + noise_on ? noise : -1)); Shouldn't we do this logging from MediaEngineWebRTCAudioSource::Config() instead of just one of its callers? ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +251,5 @@ > > return kMediaConduitUnknownError; > } > > + // TEMPORARY Temporary until what? A bug number would be ideal here. @@ +258,5 @@ > + if (NS_SUCCEEDED(rv)) { > + nsCOMPtr<nsIPrefBranch> branch = do_QueryInterface(prefs); > + > + if (branch) { > + int32_t aec = 0; // 0 == unchanged Why can't we use the actual named constants here? @@ +272,5 @@ > + branch->GetIntPref("media.peerconnection.capture_delay", &mCaptureDelay); > + } > + } > + > + if (0 != (error = mPtrVoEProcessing->SetEcStatus(true, mEchoCancel))) { You're hard-coding AEC on, ignoring the pref you just read.

(In reply to Timothy B. Terriberry (:derf) from comment #7) > Comment on attachment 706411 [details] [diff] [review] > Enable AEC in PeerConnection, AGC/NoiseSuppression in gUM > > Review of attachment 706411 [details] [diff] [review]: > ----------------------------------------------------------------- > > Changes should be easy, but I think I'd like to see this again. > > ::: content/media/webrtc/MediaEngineWebRTC.h > @@ +181,5 @@ > > , mCapIndex(aIndex) > > , mChannel(-1) > > , mInitDone(false) > > + , mEchoOn(false), mAgcOn(false), mNoiseOn(false) > > + , mEchoCancel(webrtc::kEcAec /*kEcUnchanged*/) > > What's with all the commented-out "Unchanged" constants? Was working with hard-coded versions. The values are largely irrelevant given the defaults are 'off'. To be canonically correct, maybe kXxDefault is more descriptive, but it really doesn't matter. I can clean it up. > ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp > @@ +49,5 @@ > > nsresult > > +MediaEngineWebRTCAudioSource::Config(bool aEchoOn, uint32_t aEcho, > > + bool aAgcOn, uint32_t aAGC, > > + bool aNoiseOn, uint32_t aNoise) > > +{ > > Don't we need to at least check mInitDone before accessing mVoEProcessing? Probably. I"ll add that. > > @@ +60,5 @@ > > + mVoEProcessing->SetEcStatus(mEchoOn, mEchoCancel); > > +#endif > > + mAGC = (webrtc::AgcModes) aAGC; > > + mAgcOn = aAgcOn; > > + mVoEProcessing->SetAgcStatus(mAgcOn, mAGC); > > This will reset all the current capture levels, even if the mode is > unchanged. Since you have an omnibus API that doesn't allow these settings > to be changed individually, I think we should wrap this in an if() to avoid > calling SetAgcStatus() if the old mode is the same as the new one Well, in theory you could set it to "off, kNsHeavy" (or whatever) and then to "on, kNsUnchanged" and you should get Heavy, so we likely want to pass it on. We could avoid calling if both enable and mode are unchanged, true. > @@ +128,5 @@ > > return NS_OK; > > } > > mState = kStarted; > > > > + int error; > > I don't like this at all. We should set these from one place (e.g., > Config()), and do the same logging there we would do here (I don't > understand why you'd want to log in one place and not the other). > > If there's some reason you have to wait until Start() to call them, then we > shouldn't be allowed to call Config() when mState < kStarted. No reason we have to wait until start, but they have no effect until Start. ::Config before Start (or before InitDone) can just defer until Start. Though the main reason was writing it first as a quick test, then generalizing. I'll refactor. > > If you follow my suggestion above and make setting the AGC mode conditional > on it actually changing, you'll need to make sure to change the mAgcMode > initializer to WEBRTC_VOICE_ENGINE_AGC_DEFAULT_MODE... this is different on > Android vs. everything else. Good point; I was just thinking about that earlier. > > I'm also not a fan of merging the error checking here. If something does > fail, we won't actually have any idea which call it was. I copied that from elsewhere. I'll separate them. > ::: dom/media/MediaManager.cpp > @@ +401,5 @@ > > + > > + LOG(("Audio config: aec: %d, agc: %d, noise: %d", > > + aec_on ? aec : -1, > > + agc_on ? agc : -1, > > + noise_on ? noise : -1)); > > Shouldn't we do this logging from MediaEngineWebRTCAudioSource::Config() > instead of just one of its callers? Sure. Lack of going back and cleaning up. > > ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp > @@ +251,5 @@ > > > > return kMediaConduitUnknownError; > > } > > > > + // TEMPORARY > > Temporary until what? A bug number would be ideal here. I'll file or select one. > > @@ +258,5 @@ > > + if (NS_SUCCEEDED(rv)) { > > + nsCOMPtr<nsIPrefBranch> branch = do_QueryInterface(prefs); > > + > > + if (branch) { > > + int32_t aec = 0; // 0 == unchanged > > Why can't we use the actual named constants here? Sure (with a cast, since our prefs are int32_t). > @@ +272,5 @@ > > + branch->GetIntPref("media.peerconnection.capture_delay", &mCaptureDelay); > > + } > > + } > > + > > + if (0 != (error = mPtrVoEProcessing->SetEcStatus(true, mEchoCancel))) { > > You're hard-coding AEC on, ignoring the pref you just read. Thanks!

Comment on attachment 706848 [details] [diff] [review] merge AudioConduits to allow AEC to work (WIP, working it appears) re-added init of mOtherDirection (got lost in a merge I think) Added assertions about mainthread as we're not locking the pairing of conduits.

cleaned up. Made the prefs visible since I think we'll want the user to be able to set up defaults for these if the app didn't choose. kXxxDefault selects the correct mode for Android.

Comment on attachment 706901 [details] [diff] [review] merge AudioConduits to allow AEC to work (WIP, working it appears) un-bitrotted

Comment on attachment 706901 [details] [diff] [review] merge AudioConduits to allow AEC to work (WIP, working it appears) Review of attachment 706901 [details] [diff] [review]: ----------------------------------------------------------------- At minimum please fix the race condition below. If you try a different approach than the one I suggest below, please run it by me or derf. ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +54,5 @@ > } > > delete mCurSendCodecConfig; > > + if (mOtherDirection) I think my taste would be to re-acquire the references here rather than to have this asymmetry. @@ +74,5 @@ > > + //Deal with the transport > + if(mPtrVoENetwork) > + { > + mPtrVoENetwork->DeRegisterExternalTransport(mChannel); You have a race condition here that could cause use of stale memory. The issue is that only one of the conduits is registered as the external transport, but it's not necesssarily the second conduit to be destroyed. If the registered conduit is the one that is destroyed first, then we'll be trying to send packets on a conduit which is dead. I would advise having the first conduit to be destroyed do the deregistration. @@ +210,5 @@ > + MOZ_ASSERT(!other->mOtherDirection); > + other->mOtherDirection = this; > + mOtherDirection = other; > + > + // These will be released by the last to die See above wrt re-acquiring these. ::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h @@ +225,5 @@ > * Factory function to create and initialize a Video Conduit Session > * return: Concrete VideoSessionConduitObject or NULL in the case > * of failure > */ > + static mozilla::RefPtr<AudioSessionConduit> Create(AudioSessionConduit *aOther = nullptr); Is there a reason for this to be a default? I realize it will break the test harness callers, but that seems good. ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp @@ +74,5 @@ > > +static mozilla::RefPtr<AudioSessionConduit> vcmFindConduit(sipcc::PeerConnectionImpl *pc, > + int level, bool receive); > + > +static void vcmAddConduit(sipcc::PeerConnectionImpl *pc, If these functions are called on the main thread, they should have a _m suffix. @@ +2594,5 @@ > } > > } // extern "C" > > +static mozilla::RefPtr<AudioSessionConduit> Is there any reason to have these be functions rather than just inlining the relevant one-liner? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h @@ +296,5 @@ > mTransportFlows[index_inner] = aFlow; > } > > + mozilla::RefPtr<mozilla::AudioSessionConduit> GetConduit(int aStreamIndex, bool aReceive) { > + int index_inner = aStreamIndex * 2 + aReceive ? 0 : 1; I feel like these would be better if these were unsigned values. I realize we have other code that isn't, but,... @@ +299,5 @@ > + mozilla::RefPtr<mozilla::AudioSessionConduit> GetConduit(int aStreamIndex, bool aReceive) { > + int index_inner = aStreamIndex * 2 + aReceive ? 0 : 1; > + > + if (mConduits.find(index_inner) == mConduits.end()) > + return NULL; nullptr, no? @@ +306,5 @@ > + } > + > + // Add a conduit > + void AddConduit(int aIndex, bool aReceive, > + mozilla::AudioSessionConduit *aConduit) { I would suggest const mozilla::RefPtr<mozilla::AudioSessionConduit>* aConduit. @@ +308,5 @@ > + // Add a conduit > + void AddConduit(int aIndex, bool aReceive, > + mozilla::AudioSessionConduit *aConduit) { > + int index_inner = aIndex * 2 + aReceive ? 0 : 1; > + An assert that this conduit didn't exist seems useful. @@ +310,5 @@ > + mozilla::AudioSessionConduit *aConduit) { > + int index_inner = aIndex * 2 + aReceive ? 0 : 1; > + > + mConduits[index_inner] = aConduit; > + } This function would be better with an error return.

Comment on attachment 706856 [details] [diff] [review] Enable AEC in PeerConnection, AGC/NoiseSuppression in gUM Review of attachment 706856 [details] [diff] [review]: ----------------------------------------------------------------- r=me with a few nits ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp @@ +75,5 @@ > + mEchoCancel = (webrtc::EcModes) aEcho; > + mVoEProcessing->SetEcStatus(mEchoOn, aEcho); > +#endif > + > + if (0 != (error = mVoEProcessing->SetAgcStatus(mAgcOn, (webrtc::AgcModes) aAGC))) { I still think we should only call this if aAgcOn and aAGC actually differ from the old settings. You already went through the trouble of only updating mAGC when it's not kAgcUnchanged. @@ +146,5 @@ > } > mState = kStarted; > > + // Configure audio processing in webrtc code > + Config(mEchoOn, webrtc::kEcUnchanged, Trailing whitespace. ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +251,5 @@ > > return kMediaConduitUnknownError; > } > > + // TEMPORARY Don't forget to add the reference to bug 694814 comment 2 here. ::: modules/libpref/src/init/all.js @@ +178,5 @@ > pref("media.navigator.enabled", true); > pref("media.peerconnection.enabled", false); > pref("media.navigator.permission.disabled", false); > +pref("media.peerconnection.aec_enabled", true); > +pref("media.peerconnection.aec", 1); // kXxxDefault This seems likely to confuse someone who doesn't know the GIPS backend, which seems likely for someone reading all.js. I'd list out the full constant names for all 3 settings here (this will also avoid problems if the three settings are changed/moved independently of each other later).

(In reply to Eric Rescorla (:ekr) from comment #14) > Comment on attachment 706901 [details] [diff] [review] > merge AudioConduits to allow AEC to work (WIP, working it appears) > > Review of attachment 706901 [details] [diff] [review]: > ----------------------------------------------------------------- > > At minimum please fix the race condition below. If you try a different > approach than > the one I suggest below, please run it by me or derf. > > ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp > @@ +54,5 @@ > > } > > > > delete mCurSendCodecConfig; > > > > + if (mOtherDirection) > > I think my taste would be to re-acquire the references here rather than to > have this asymmetry. > > @@ +74,5 @@ > > > > + //Deal with the transport > > + if(mPtrVoENetwork) > > + { > > + mPtrVoENetwork->DeRegisterExternalTransport(mChannel); > > You have a race condition here that could cause use of stale memory. > > The issue is that only one of the conduits is registered as the external > transport, but it's not necesssarily the second conduit to be destroyed. If > the registered conduit is the one that is destroyed first, then we'll be > trying to send packets on a conduit which is dead. > > I would advise having the first conduit to be destroyed do the > deregistration. Ok revised patch to address that and the registration issue. Note that mVoiceEngine is special, only one should call ::Delete(). > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h > @@ +296,5 @@ > > mTransportFlows[index_inner] = aFlow; > > } > > > > + mozilla::RefPtr<mozilla::AudioSessionConduit> GetConduit(int aStreamIndex, bool aReceive) { > > + int index_inner = aStreamIndex * 2 + aReceive ? 0 : 1; > > I feel like these would be better if these were unsigned values. I realize > we have other code that isn't, but,... I'm mirroring the other code that uses int in the file, and it devolves from use of int in sipcc. > > @@ +299,5 @@ > > + mozilla::RefPtr<mozilla::AudioSessionConduit> GetConduit(int aStreamIndex, bool aReceive) { > > + int index_inner = aStreamIndex * 2 + aReceive ? 0 : 1; > > + > > + if (mConduits.find(index_inner) == mConduits.end()) > > + return NULL; > > nullptr, no? I dislike changing one use in a file full of the other style; it just confuses things. It can change when the entire file is reworked. Work in the style of the file, or things become a confusing mismash of styles. Or clean up the entire file, but this isn't the bug to do that on. > > @@ +306,5 @@ > > + } > > + > > + // Add a conduit > > + void AddConduit(int aIndex, bool aReceive, > > + mozilla::AudioSessionConduit *aConduit) { > > I would suggest > const mozilla::RefPtr<mozilla::AudioSessionConduit>* aConduit. > > @@ +308,5 @@ > > + // Add a conduit > > + void AddConduit(int aIndex, bool aReceive, > > + mozilla::AudioSessionConduit *aConduit) { > > + int index_inner = aIndex * 2 + aReceive ? 0 : 1; > > + > > An assert that this conduit didn't exist seems useful. > > @@ +310,5 @@ > > + mozilla::AudioSessionConduit *aConduit) { > > + int index_inner = aIndex * 2 + aReceive ? 0 : 1; > > + > > + mConduits[index_inner] = aConduit; > > + } > > This function would be better with an error return. I suppose it can (though the mirrored function doesn't, but what's the error? At best, conduit_already_in_use (and that should be a MOZ_ASSERT).

Interdiffs for update for ekr's review

Changes are significant enough for a quick re-review of the Init()/delete code. Interdiffs posted, though for the Init/Delete code this might be better. Note that VoiceEngine doesn't like multiple direct openers.

Comment on attachment 707297 [details] [diff] [review] Updated conduit patch for ekr's comments Review of attachment 707297 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/AudioConduit.h @@ +143,5 @@ > > > WebrtcAudioConduit(): > + mOtherDirection(NULL), > + mShutDown(false), I would name this mHasShutdown ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp @@ +71,5 @@ > const char *fingerprint_alg, > const char *fingerprint > ); > > +static mozilla::RefPtr<AudioSessionConduit> vcmFindConduit(sipcc::PeerConnectionImpl *pc, Is this used? @@ +1310,3 @@ > // Instantiate an appropriate conduit > + mozilla::RefPtr<mozilla::AudioSessionConduit> tx_conduit = > + pc.impl()->media()->GetConduit(level, true); Shouldn't this second argument be false? This seems like a bug. I.e., shouldn't any given function Get and Add with different send/recv values. This also seems like an argument for not using booleans. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h @@ +307,5 @@ > + } > + > + // Add a conduit > + void AddConduit(int aIndex, bool aReceive, > + const mozilla::RefPtr<mozilla::AudioSessionConduit> aConduit) { Make this const & to avoid a spurious increment/decrement

Comment on attachment 707625 [details] [diff] [review] interdiffs for final review Review of attachment 707625 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

https://hg.mozilla.org/integration/mozilla-inbound/rev/19e164f7d88d https://hg.mozilla.org/integration/mozilla-inbound/rev/df75a87cce60 NOTE! currently there's a magic constant of (default) 150ms you can modify by adding a pref (not visible by default) called media.peerconnection.capture_delay. At minimum we'll likely need to make the default system-specific, but we likely will need to do something more adaptive (as well as reduce audio delays). On my linux system the *first* call gets ~400ms total out+in delay, and the second and subsequent calls get ~200ms. the AEC cannot handle 250ms past the capture delay and so will not cancel the first call.

Backed out for: https://tbpl.mozilla.org/php/getParsedLog.php?id=19243424&tree=Mozilla-Inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/37c311484185

With bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/40f09f7bc670 https://hg.mozilla.org/integration/mozilla-inbound/rev/fc262e3c635f Try: https://tbpl.mozilla.org/?tree=Try&rev=1ffd1b91729b

Backed out for causing frequent (50-75% occurence) Fedora64 mochitest-3 (debug) leaks, eg: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Rev3%20Fedora%2012x64%20mozilla-inbound%20debug%20test%20mochitest-3&rev=fc262e3c635f https://hg.mozilla.org/integration/mozilla-inbound/rev/054718506d8c

https://hg.mozilla.org/mozilla-central/rev/40f09f7bc670 https://hg.mozilla.org/mozilla-central/rev/fc262e3c635f

Ryan missed the later backout (which will be merged soon).

Backout: https://hg.mozilla.org/mozilla-central/rev/054718506d8c

Comment on attachment 708241 [details] [diff] [review] ensure pipelinelistener doesn't release conduit off main thread https://tbpl.mozilla.org/?tree=Try&rev=c7a16a4d82f6 Please assume I'll add the MOZ_OVERRIDE's to the NotifyXxxx's in the second listener.

Comment on attachment 708241 [details] [diff] [review] ensure pipelinelistener doesn't release conduit off main thread Review of attachment 708241 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

https://hg.mozilla.org/integration/mozilla-inbound/rev/83305a2fa224 https://hg.mozilla.org/integration/mozilla-inbound/rev/c859a9098db0 https://hg.mozilla.org/integration/mozilla-inbound/rev/4ee75ced109c Green try with multiple re-triggers of linux64 M3

https://hg.mozilla.org/mozilla-central/rev/83305a2fa224 https://hg.mozilla.org/mozilla-central/rev/c859a9098db0 https://hg.mozilla.org/mozilla-central/rev/4ee75ced109c

Comment on attachment 707297 [details] [diff] [review] Updated conduit patch for ekr's comments Review of attachment 707297 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h @@ +302,5 @@ > + > + if (mAudioConduits.find(index_inner) == mAudioConduits.end()) > + return NULL; > + > + return mAudioConduits[index_inner]; It seems to me that we could find the conduit via the mLocal/RemoteSourceStreams data member which has mPipelines stored by track which each have a data member conduit_. Is there a reason we need to store the conduits again here? Perhaps I'm missing something.

Comment on attachment 707297 [details] [diff] [review] Updated conduit patch for ekr's comments cleaning up old requests - ekr r+'d the interdiff