Closed Bug 818670 Opened 11 years ago Closed 11 years ago

Turn on echo canceller by default

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Whiteboard: [WebRTC][blocking-webrtc+][qa-])

Attachments

(6 files, 6 obsolete files)

24.37 KB, patch
derf
: review+
Details | Diff | Splinter Review
17.06 KB, patch
ekr
: review+
Details | Diff | Splinter Review
20.23 KB, patch
Details | Diff | Splinter Review
21.99 KB, patch
jesup
: review+
Details | Diff | Splinter Review
5.21 KB, patch
ekr
: review+
Details | Diff | Splinter Review
4.01 KB, patch
derf
: review+
Details | Diff | Splinter Review
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)
Whiteboard: [WebRTC][blocking-webrtc+]
Assignee: nobody → rjesup
Blocks: 694814
Priority: -- → P2
Blocks: 796463
Attachment #705563 - Attachment is obsolete: true
Attachment #705973 - Attachment is obsolete: true
Attachment #705975 - Attachment is obsolete: true
Attachment #706411 - Flags: review?(tterribe)
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.
Attachment #706412 - Flags: review?(ekr)
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.
Attachment #706411 - Flags: review?(tterribe) → review-
(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!
Attachment #706412 - Attachment is obsolete: true
Attachment #706412 - Flags: review?(ekr)
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.
Attachment #706848 - Flags: review?(ekr)
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.
Attachment #706411 - Attachment is obsolete: true
Attachment #706856 - Flags: review?(tterribe)
Attachment #706848 - Attachment is obsolete: true
Attachment #706848 - Flags: review?(ekr)
Comment on attachment 706901 [details] [diff] [review]
merge AudioConduits to allow AEC to work (WIP, working it appears)

un-bitrotted
Attachment #706901 - Flags: review?(ekr)
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.
Attachment #706901 - Flags: review?(ekr) → review+
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).
Attachment #706856 - Flags: review?(tterribe) → review+
(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.
Attachment #707297 - Flags: review?(ekr)
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
Attachment #707625 - Flags: review?(ekr)
Comment on attachment 707625 [details] [diff] [review]
interdiffs for final review

Review of attachment 707625 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm
Attachment #707625 - Flags: review?(ekr) → review+
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.
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/40f09f7bc670
https://hg.mozilla.org/mozilla-central/rev/fc262e3c635f
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Ryan missed the later backout (which will be merged soon).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #708241 - Flags: review?(tterribe)
Comment on attachment 708241 [details] [diff] [review]
ensure pipelinelistener doesn't release conduit off main thread

Review of attachment 708241 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #708241 - Flags: review?(tterribe) → review+
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.
Keywords: verifyme
Blocks: 843929
Keywords: verifyme
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc+][qa-]
Comment on attachment 707297 [details] [diff] [review]
Updated conduit patch for ekr's comments

cleaning up old requests - ekr r+'d the interdiff
Attachment #707297 - Flags: review?(ekr) → review+
You need to log in before you can comment on or make changes to this bug.