907353 - RTCP-Mux in Nightly is incompatible with Chrome

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Ethan Hugg [:ehugg]]

Today's Nightly has the patch from bug 863306 which turns rtcp-mux negotiation on.  While this works with FF, it does not work with Chrome.

It appears the problem is that we are creating two ICE components and expecting them both to connect when only one is needed.  That may not be the only reason it doesn't connect though.

rtcp-mux will be turned off in a second patch to bug 863306, but if we can make it compatible with Chrome we should turn it back on in this bug.

Comment 1

[Ethan Hugg [:ehugg]]

I just ran a test with gRTCPMUX == TRUE and the calls to IceCtx->CreateStream set to one component instead of two and it worked on apprtc with Chrome 31.  I think if we are able to disable the second component when muxing then this will work.

Comment 2

[Suhas]

Probably a good idea to also consider implications of the following as a wholistic solution
, say, 
when a set of m=lines are part of BUNDLE group, we need to just create one component for all of them VS 
 
non bundled rtcp-mux m=lines VS 
 
non bundled non rtcp-mux m=lines  [ we do this today]

So we need a way to assign membership of each MediaStream (ICE) to be part of some multiplexing scheme or not [ such as BUNDLE or RTCP Mux]

Not sure if this should be part of this bug or not though

Comment 3

[Ethan Hugg [:ehugg]]

Attachment: Disable second component when rtcp-mux

Comment 4

[Ethan Hugg [:ehugg]]

Attachment: Disable second component when rtcp-mux

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 796770 [details] [diff] [review]
Disable second component when rtcp-mux

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

This patch requires the patch from bug 909179 which in turn requires the patch from bug 842549.

Tested with Canary version 31 and unlike the first patch does not call disable twice on the same component.

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 796770 [details] [diff] [review]
Disable second component when rtcp-mux

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

The code looks fine, but I would like to see tests that verify that
we haven't broken the non-mux case. Can you add some with fRtcpMux == false?

Comment 7

[Ethan Hugg [:ehugg]]

Take a look at the last two signaling unittests, TEST_F(SignalingTest, AudioOnlyCalleeNoRtcpMux) and TEST_F(SignalingTest, FullCallAudioNoMuxVideoMux).

They were added in bug 863306 and should test what you want.

Comment 8

[Eric Rescorla (:ekr)]

Comment on attachment 796770 [details] [diff] [review]
Disable second component when rtcp-mux

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

with minor comments

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2865,5 @@
> + * If we are doing rtcp-mux we need to disable component number 2 in the ICE
> + * layer.  Otherwise we will wait for it to connect when it is unused
> + */
> +static int vcmDisableRtcpComponent_m(const char *peerconnection, int level) {
> +  sipcc::PeerConnectionWrapper pc(peerconnection);

Assert that on the right thread.

@@ +2868,5 @@
> +static int vcmDisableRtcpComponent_m(const char *peerconnection, int level) {
> +  sipcc::PeerConnectionWrapper pc(peerconnection);
> +  ENSURE_PC(pc, VCM_ERROR);
> +
> +  CSFLogDebug( logTag, "%s: disabling rtcp component %d", __FUNCTION__, level);

Can you assert that level >0

Comment 9

[Ethan Hugg [:ehugg]]

Attachment: Disable second component when rtcp-mux

Comment 10

[Ethan Hugg [:ehugg]]

Comment on attachment 799000 [details] [diff] [review]
Disable second component when rtcp-mux


Fixed nits and retested.  Bringing forward r+ from EKR.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cdb9d968f557