863306 - Signaling RTCP-Mux value from SDP negotiation should be used.

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P2)

Description

[Ethan Hugg [:ehugg]]

In Bug 777524 negotiation support for a=rtcp-mux was added.

Bug 786307 deals with the support in MediaPipeline.

However it appears that the two are not hooked up.  The code in VcmSIPCCBinding.cpp that creates the MediaPipelines creates a new TransportFlow for rtcp in every case.  It also appears to not yet have access to the negotiated rtcp-mux value stored in the fsmdef_media_t.  We should use this value, send in NULL for the rtcp TransportFlow to the creation of the MediaPipeline to make it mux, and add some unittests that verify the mux and non-mux cases.

Comment 1

[Suhas]

Attachment: Patch to link rtcpmux within VCM to pipeline - (WIP)

Propagates RTCP Mux status to Mediapipeline via VCM. (WIP)

Comment 2

[Suhas]

Attachment: Link RTCP_MUX in VCM to Pipeline.

Patch does 3 things
 1. RTCP flow creation is made dependent on RTCP_MUX setting. 
 2. RTCP_MUX is enabled in the config.
 3. In order to avoid creating new parameter to VCM Apis to just indicate RTCP_MUX value, set of place-holder structures are defined to encapsulate RTCP_MUX and media group semantics.

Comment 3

[Suhas]

Attachment: Propagate RTCP_MUX Status to pipeline via VCM.

Same patch after white-space removal

Comment 4

[Ethan Hugg [:ehugg]]

Comment on attachment 749692 [details] [diff] [review]
Propagate RTCP_MUX Status to pipeline via VCM.

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

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +1353,5 @@
>  
> +  //TODO: Only RTCP MUX is supported. Group semantics specific
> +  // handling should be done once implemented.
> +  mozilla::RefPtr<TransportFlow> rtcp_flow = nullptr;
> +  if(group_semantics && group_semantics->rtcp_mux == FALSE) {

Mozilla style would have !group_semantics->rtcp_mux instead of == FALSE

@@ +2006,5 @@
> +
> +  //TODO: Only RTCP MUX is supported. Group semantics specific
> +  // handling should be done once implemented.
> +  mozilla::RefPtr<TransportFlow> rtcp_flow = nullptr;
> +  if(group_semantics && group_semantics->rtcp_mux == FALSE) {

same - '== FALSE'

::: media/webrtc/signaling/src/sipcc/core/includes/config.h
@@ +178,5 @@
>  static const int gDscpCallControl = 1;
>  static const int gSpeakerEnabled = 1;
>  static const char gExternalNumberMask[] = "";
>  static const char gVersion[] = "0.1";
> +static const boolean gRTCPMUX = TRUE;

I assume this means we will default to mux from now on.

::: media/webrtc/signaling/src/sipcc/include/vcm.h
@@ +271,5 @@
> +  vcm_media_group_semantic_info_t* groups;
> +
> +} vcm_media_mux_group_semantics_info_t;
> +
> +/**

Not critical, but I would've put these struct defs in fsm.h instead.  I think of vcm.h as only having what's needed to call into vcmSIPCCBinding and not having so much SDP construction bits.  I see there's precedence for putting it here so you can decide what you want to do.

Comment 5

[Suhas]

Attachment: Propagate RTCP_MUX Status to pipeline via VCM.

Comment 6

[Suhas]

Attachment: Propagate RTCP_MUX Status to pipeline via VCM.

Comment 7

[Suhas]

Comment on attachment 768685 [details] [diff] [review]
Propagate RTCP_MUX Status to pipeline via VCM.

Taking R+ from Ethan with following changes from the previous version
  1. Made global gRTCPMUX as default true
  2. Propogate rtcp mux value to VCM via media_attributes structure
  3. Fix few bugs in rtcp mux handling in the gsm_sdp
  4. updated VCM to create/not to create RTCP Flow based on rtcp-mux setting
  5. Added new test-cases to verify rtcp flow creation in the case of Answerer not supporting Mux.

Comment 8

[Ethan Hugg [:ehugg]]

Comment on attachment 768685 [details] [diff] [review]
Propagate RTCP_MUX Status to pipeline via VCM.

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

Bringing forward my r+ based on diffs from the earlier reviewed version.

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +983,5 @@
>                  } else {
>                      media->src_port = open_rcv.port;
>                  }
>  
> +                // Set RTP/RTCP Mux.

nit: We have been generally using /* */ in .c files even though this file already has mixed style.

Comment 9

[Ethan Hugg [:ehugg]]

Attachment: Propagate RTCP_MUX Status to pipeline via VCM.

Comment 10

[Ethan Hugg [:ehugg]]

Comment on attachment 780553 [details] [diff] [review]
Propagate RTCP_MUX Status to pipeline via VCM.


Un-bitrot and reformatted a few comments.

Comment 11

[Eric Rescorla (:ekr)]

Comment on attachment 780553 [details] [diff] [review]
Propagate RTCP_MUX Status to pipeline via VCM.

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

re-setting to ABR, as he knows the SIPCC code better than I do.

Comment 12

[Adam Roach [:abr]]

Comment on attachment 780553 [details] [diff] [review]
Propagate RTCP_MUX Status to pipeline via VCM.

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

r- for a bunch of minor but functional issues with the unit tests. I also think we need a short discussion on the behavior for "remote party offers mux but our configuration has it disabled."

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2050,5 @@
>    }
> +  mozilla::RefPtr<TransportFlow> rtcp_flow = nullptr;
> +  if(!attrs->rtcp_mux) {
> +    rtcp_flow = vcmCreateTransportFlow(pc.impl(), level, true,
> +                                fingerprint_alg, fingerprint);

Fix the indenting on this line, please.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +4734,1 @@
>                                                0, SDP_ATTR_RTCP_MUX, 1, &rtcp_mux);

Can we line these up more sensibly? Something like:

>              sdp_res = sdp_attr_get_rtcp_mux_attribute (sdp_p->dest_sdp, i,
>                                                         0, SDP_ATTR_RTCP_MUX,
>                                                         1, &rtcp_mux);

@@ +4734,4 @@
>                                                0, SDP_ATTR_RTCP_MUX, 1, &rtcp_mux);
>  
> +              if (SDP_SUCCESS == sdp_res) {
> +            	  media->rtcp_mux = TRUE;

Remove tab character

@@ +4749,5 @@
>                                                sdp_p->src_sdp, media->candidatesp[j]);
>                    }
>  
> +                  /* Set RTCPMux if negotiated */
> +                  if (media->rtcp_mux) {

Don't we want to also consult the configuration here? In other words: if the remote party has offered MUX, but our configuration has turned it off, what is the expected behavior? I would *think* that the answer is "don't do MUX," but I could be off in the weeds...

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +983,5 @@
>                  } else {
>                      media->src_port = open_rcv.port;
>                  }
>  
> +                /* Set RTP/RTCP Mux */

This comment is probably overkill in terms of the "avoid comment the obvious" category.

@@ +1222,5 @@
>              media->xmit_chan = TRUE;
>  
>              attrs.mute = FALSE;
> +
> +            /* Set RTP/RTCP Mux. */

As above.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +459,5 @@
> +  {
> +    std::multimap<std::string, SdpLine>::iterator it;
> +    it = sdp_map_.find(objType);
> +    if(it != sdp_map_.end()) {
> +      SdpLine sdp_line_pair = (*it).second;

Why are we copying the SdpLine object here? Remove or explain in a code comment.

@@ +474,5 @@
>        int line_no = sdp_line_pair.first;
>        sdp_map_.erase(it);
> +      if(content.empty()) {
> +        return;
> +      }

We seem to be defining two interfaces for removing a line: DeleteLine("foo") and ReplaceLine("foo",""). Is there a rationale here?

Also, this behavior is sufficiently unintuitive that we definitely want to add a comment above the method to explain the special handling for empty strings.

@@ +2305,5 @@
> +{
> +  sipcc::MediaConstraints constraints;
> +
> +  a1_.CreateOffer(constraints, OFFER_AUDIO, SHOULD_SENDRECV_AUDIO);
> +  a1_.SetLocal(TestObserver::OFFER, a1_.offer(), true);

We don't expect this to generate an error, do we? I think the "true" flag needs to be omitted.

@@ +2310,5 @@
> +  ParsedSDP sdpWrapper(a1_.offer());
> +  sdpWrapper.DeleteLine("a=rtcp-mux");
> +  std::cout << "Modified SDP " << std::endl
> +            << indent(sdpWrapper.getSdp()) << std::endl;
> +  a2_.SetRemote(TestObserver::OFFER, sdpWrapper.getSdp(), true);

Same comment as above regarding the ignoreError parameter

@@ +2311,5 @@
> +  sdpWrapper.DeleteLine("a=rtcp-mux");
> +  std::cout << "Modified SDP " << std::endl
> +            << indent(sdpWrapper.getSdp()) << std::endl;
> +  a2_.SetRemote(TestObserver::OFFER, sdpWrapper.getSdp(), true);
> +  a2_.CreateAnswer(constraints, sdpWrapper.getSdp(), OFFER_AUDIO | ANSWER_AUDIO);

Wrap to 80 columns.

Also, shouldn't we verify that the answer doesn't contain an rtcp-mux attribute?

@@ +2312,5 @@
> +  std::cout << "Modified SDP " << std::endl
> +            << indent(sdpWrapper.getSdp()) << std::endl;
> +  a2_.SetRemote(TestObserver::OFFER, sdpWrapper.getSdp(), true);
> +  a2_.CreateAnswer(constraints, sdpWrapper.getSdp(), OFFER_AUDIO | ANSWER_AUDIO);
> +  a2_.SetLocal(TestObserver::ANSWER, a2_.answer(), true);

Same comment as above regarding the ignoreError parameter

@@ +2313,5 @@
> +            << indent(sdpWrapper.getSdp()) << std::endl;
> +  a2_.SetRemote(TestObserver::OFFER, sdpWrapper.getSdp(), true);
> +  a2_.CreateAnswer(constraints, sdpWrapper.getSdp(), OFFER_AUDIO | ANSWER_AUDIO);
> +  a2_.SetLocal(TestObserver::ANSWER, a2_.answer(), true);
> +  a1_.SetRemote(TestObserver::ANSWER, a2_.answer(), true);

Same comment as above regarding the ignoreError parameter

@@ +2321,5 @@
> +
> +  PR_Sleep(kDefaultTimeout * 2); // Wait for some data to get written
> +
> +  a1_.CloseSendStreams();
> +  a2_.CloseReceiveStreams();

The point, I believe, of the preceding wait is to allows us to count the number of packets send and received so as to ensure data can actually flow. I think you want to perform the same verifications here. If, for whatever reason, this proves infeasible, then I think we *really* want to remove the PR_Sleep() above.

Is there some way to verify whether muxing actually occured? Otherwise, this test doesn't seem to actually verify the functionality implemented by the rest of the patch.

@@ +2329,5 @@
> +{
> +  sipcc::MediaConstraints constraints;
> +
> +  a1_.CreateOffer(constraints, OFFER_AV, SHOULD_SENDRECV_AV);
> +  a1_.SetLocal(TestObserver::OFFER, a1_.offer(), true);

Same comment as above regarding the ignoreError parameter.

@@ +2334,5 @@
> +  ParsedSDP sdpWrapper(a1_.offer());
> +  sdpWrapper.DeleteLine("a=rtcp-mux");
> +  std::cout << "Modified SDP " << std::endl
> +            << indent(sdpWrapper.getSdp()) << std::endl;
> +  a2_.SetRemote(TestObserver::OFFER, sdpWrapper.getSdp(), true);

Same comment as above regarding the ignoreError parameter

@@ +2335,5 @@
> +  sdpWrapper.DeleteLine("a=rtcp-mux");
> +  std::cout << "Modified SDP " << std::endl
> +            << indent(sdpWrapper.getSdp()) << std::endl;
> +  a2_.SetRemote(TestObserver::OFFER, sdpWrapper.getSdp(), true);
> +  a2_.CreateAnswer(constraints, sdpWrapper.getSdp(), OFFER_AV | ANSWER_AV);

Check that the answer has rtcp-mux in the video section, but not the audio section.

@@ +2336,5 @@
> +  std::cout << "Modified SDP " << std::endl
> +            << indent(sdpWrapper.getSdp()) << std::endl;
> +  a2_.SetRemote(TestObserver::OFFER, sdpWrapper.getSdp(), true);
> +  a2_.CreateAnswer(constraints, sdpWrapper.getSdp(), OFFER_AV | ANSWER_AV);
> +  a2_.SetLocal(TestObserver::ANSWER, a2_.answer(), true);

Same comment as above regarding the ignoreError parameter

@@ +2337,5 @@
> +            << indent(sdpWrapper.getSdp()) << std::endl;
> +  a2_.SetRemote(TestObserver::OFFER, sdpWrapper.getSdp(), true);
> +  a2_.CreateAnswer(constraints, sdpWrapper.getSdp(), OFFER_AV | ANSWER_AV);
> +  a2_.SetLocal(TestObserver::ANSWER, a2_.answer(), true);
> +  a1_.SetRemote(TestObserver::ANSWER, a2_.answer(), true);

Same comment as above regarding the ignoreError parameter

@@ +2345,5 @@
> +
> +  PR_Sleep(kDefaultTimeout * 2); // Wait for some data to get written
> +
> +  a1_.CloseSendStreams();
> +  a2_.CloseReceiveStreams();

Same comments about checking packet counts and actual MUX behavior as above.

Comment 13

[Ethan Hugg [:ehugg]]

Comment on attachment 780553 [details] [diff] [review]
Propagate RTCP_MUX Status to pipeline via VCM.

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

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +4749,5 @@
>                                                sdp_p->src_sdp, media->candidatesp[j]);
>                    }
>  
> +                  /* Set RTCPMux if negotiated */
> +                  if (media->rtcp_mux) {

This is an interesting question.  Currently the configuration parameter only affects what we initially offer.  If we insist on not doing mux when the config is off, then would be also insist on doing mux when the config is on?  We can't do both.  Do you think off is the best choice if the endpoints disagree?

Comment 14

[Adam Roach [:abr]]

(In reply to Ethan Hugg [:ehugg] from comment #13)

> This is an interesting question.  Currently the configuration parameter only
> affects what we initially offer.  If we insist on not doing mux when the
> config is off, then would be also insist on doing mux when the config is on?
> We can't do both.  Do you think off is the best choice if the endpoints
> disagree?

I think the behavior we want is: the configuration option controls whether we *support* RTCP mux. If we don't *support* it, then it never gets used, regardless of what the other side says.

If we do support it, then there's a second issue of whether we *use* it. That comes about as the result of SDP negotiation, and is specified in RFCs. That behavior does boil down to "if the endpoints don't both signal support for a feature, then the feature is off."

Comment 15

[Suhas]

RTCWeb mandates RTCP Mux , BUNDLE as well too. We mandate offering RTCP Mux option in the offer.

Case 1: I think we need to handle the above case where we offer say BUNDLE and RTCP Mux for a given m-line, but the other end doesn't support either, we reject the m=line.

Case 2: On the other hand, we offer only RTCP-MUX (non bundled) m=line and the other ends answers with no RTCP-MUX, we do 2 flows (one rtp and one rtcp). For this case, we need to make a decision onto accept the m=line as we do today or we reject the m=line.

I expect Case1 will be handled in the BUNDLE implementation. For Case 2, we need a call to decide the implementation.

Comment 16

[Suhas]

For the case 2 above, our current implementation mimics RFC5761 behavior for rtcp-mux attribute.

Comment 17

[Eric Rescorla (:ekr)]

(In reply to Suhas from comment #15)
> RTCWeb mandates RTCP Mux , BUNDLE as well too. We mandate offering RTCP Mux
> option in the offer.

Sorry, where do you think the RTCWeb specifications require offering
RTCP-Mux?


> Case 1: I think we need to handle the above case where we offer say BUNDLE
> and RTCP Mux for a given m-line, but the other end doesn't support either,
> we reject the m=line.

Huh? It's totally legitimate for the other side to reject both of
these, if, for instance, it's a legacy endpoint. Why would we
reject the m-line?


> Case 2: On the other hand, we offer only RTCP-MUX (non bundled) m=line and
> the other ends answers with no RTCP-MUX, we do 2 flows (one rtp and one
> rtcp). For this case, we need to make a decision onto accept the m=line as
> we do today or we reject the m=line.

Whhy would we reject in this case? This is also totally legit.


> I expect Case1 will be handled in the BUNDLE implementation. For Case 2, we
> need a call to decide the implementation.

Comment 18

[Suhas]

I agree .. I need to correct and clarify myself here ... (response inline)

(In reply to Eric Rescorla (:ekr) from comment #17)
> (In reply to Suhas from comment #15)
> > RTCWeb mandates RTCP Mux , BUNDLE as well too. We mandate offering RTCP Mux
> > option in the offer.
> 
> Sorry, where do you think the RTCWeb specifications require offering
> RTCP-Mux?
> 
I was referring to this section in my mind : http://tools.ietf.org/html/draft-ietf-rtcweb-rtp-usage-07#section-4.5
As an RTCWeb end-point we support multiplexing of RTP and RTCP , but the other end might not do the same.
> 
> > Case 1: I think we need to handle the above case where we offer say BUNDLE
> > and RTCP Mux for a given m-line, but the other end doesn't support either,
> > we reject the m=line.
> 
> Huh? It's totally legitimate for the other side to reject both of
> these, if, for instance, it's a legacy endpoint. Why would we
> reject the m-line?

Agree completely .. I was more thinking of the other having BUNDLE indicated without RTCP-MUX scenario. In this case, we reject the m=line since it is wrongly constructed BUNDLE.
> 
> > Case 2: On the other hand, we offer only RTCP-MUX (non bundled) m=line and
> > the other ends answers with no RTCP-MUX, we do 2 flows (one rtp and one
> > rtcp). For this case, we need to make a decision onto accept the m=line as
> > we do today or we reject the m=line.
> 
> Whhy would we reject in this case? This is also totally legit.

Yes . I updated this point on the comment that followed my original comment.. 
> 
> > I expect Case1 will be handled in the BUNDLE implementation. For Case 2, we
> > need a call to decide the implementation.

Comment 19

[Ethan Hugg [:ehugg]]

Attachment: Propagate RTCP_MUX Status to pipeline via VCM.

Comment 20

[Ethan Hugg [:ehugg]]

Comment on attachment 787227 [details] [diff] [review]
Propagate RTCP_MUX Status to pipeline via VCM.

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

Uploaded this as work in progress.  The one outstanding issue is how to end-end test rtcp-mux with the fake media streams.

Comment 21

[Ethan Hugg [:ehugg]]

Attachment: Propagate RTCP_MUX Status to pipeline via VCM.

Comment 22

[Ethan Hugg [:ehugg]]

Comment on attachment 788360 [details] [diff] [review]
Propagate RTCP_MUX Status to pipeline via VCM.

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

This patch should address the review comments.  It has some parts added that enable the signaling unit test to query the media pipelines to get info on mux and media sent/recv.  I also added a superclass for Local/RemoteStreamSourceInfo.  Not the full refactor mentioned in the comments, but I wanted to avoid duplicating some code.

In the unit test you'll see now that FullCall checks for rtcp-mux and rtcp sent/recv.  AudioOnlyCalleeNoRtcpMux checks for rtcp-mux off and rtcp send/recv, and FullCallAudioNoMuxVideoMux checks for mux on video but not audio.  We are not yet able to test video flows for lack of fake video for unit tests.

abr: I am on PTO all next week so no hurry on this one.

Comment 23

[Ethan Hugg [:ehugg]]

I should also mention that bug 786307 is on M-I, but if it's not on M-C by the time you try this out you will need to add the patch from it before you apply this one.

Comment 24

[Ethan Hugg [:ehugg]]

Attachment: Interdiff between latest and previously reviewed patch

Comment 25

[Adam Roach [:abr]]

Comment on attachment 788360 [details] [diff] [review]
Propagate RTCP_MUX Status to pipeline via VCM.

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

Thanks for the interdiff; it was very helpful.

The new patch looks good to me. I found some minor style nits in the unittests, plus one functional typo.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +951,5 @@
> +      sipcc::LocalSourceStreamInfo *localStream =
> +        pc->media()->GetLocalStream(stream);
> +
> +      if (!localStream) {
> +        return NULL;

return nullptr;

@@ +960,5 @@
> +      sipcc::RemoteSourceStreamInfo *remoteStream =
> +        pc->media()->GetRemoteStream(stream);
> +
> +      if (!remoteStream) {
> +        return NULL;

return nullptr;

@@ +965,5 @@
> +      }
> +
> +      return remoteStream->GetPipeline(track);
> +    }
> +  }

With your refactor of the pipeline classes, wouldn't this be cleaner as something like:

> mozilla::RefPtr<mozilla::MediaPipeline> GetMediaPipeline(
>   bool local, int stream, int track) {
>   sipcc::SourceStreamInfo *stream = local ?
>                                     pc->media()->GetLocalStream(stream) : 
>                                     pc->media()->GetRemoteStream(stream);
>   if (!stream) {
>     return nullptr;
>   }
> 
>   return stream->GetPipeline(track);
> }

@@ +968,5 @@
> +    }
> +  }
> +
> +  void CheckMediaPipeline(bool isLocal, int stream, int track,
> +    bool isRtcpMux, bool isSend, bool isVideo) {

You might consider defining a bitmap enum that makes this slightly easier to read in the testcases, similar to what this file does with sdpTestFlags and offerAnswerFlags. Consider the readability differences between:

> a2_.CheckMediaPipeline(true, 0, 1, true, true, false);

and

> a2_.CheckMediaPipeline(0, 1, PIPELINE_LOCAL | PIPELINE_RTCP_MUX | PIPELINE_SEND);

@@ +2406,5 @@
> +
> +  // Answer should have only one a=rtcp-mux line
> +  size_t match = a2_.getLocalDescription().find("\r\na=rtcp-mux");
> +  ASSERT_NE(match, std::string::npos);
> +  match = a2_.getLocalDescription().find("\r\aa=rtcp-mux", match + 1);

Typo: \r\n instead of \r\a

Comment 26

[Ethan Hugg [:ehugg]]

Attachment: Propagate RTCP_MUX Status to pipeline via VCM.

Comment 27

[Ethan Hugg [:ehugg]]

Comment on attachment 792442 [details] [diff] [review]
Propagate RTCP_MUX Status to pipeline via VCM.


Bringing forward r+ from ABR.

Comment 28

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6263d08cbb87

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6263d08cbb87

Comment 30

[Ethan Hugg [:ehugg]]

OK, this is not quite right.  It appears that when we mux we still create two ICE components.  You can see their a=candidate lines in the SDP with a '2' as the second parameter. 

While this works with all versions of Firefox because the latest connect both and the previous ones negotiate rtcp-mux off, it doesn't work with Chrome.  It appears we wait for both ICE components to connect and Chrome knows to only use the first one.

I propose doing this:

1. Instead of backing out, pushing a new patch on this bug that turns gRTCPMux to FALSE in config.h so that we stop negotiating rtcp-mux.

2. Create a new bug for this ICE component issue and turn rtcp-mux back on when it has been fixed.  Currently I don't think there is a way of disabling the second ICE component after negotiation so we may need support for this from the transport.

Other ideas/comments?

Comment 31

[Ethan Hugg [:ehugg]]

Attachment: Turn off rtcp-mux in config

Comment 32

[Ethan Hugg [:ehugg]]

Comment on attachment 793024 [details] [diff] [review]
Turn off rtcp-mux in config

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

Patch to turn off the config setting for rtcp-mux.  This worked again when tested against Chrome 31

Comment 33

[Ethan Hugg [:ehugg]]

Added this bug to fix and turn back on.

Bug 907353 - RTCP-Mux in Nightly is incompatible with Chrome

Comment 34

[Ethan Hugg [:ehugg]]

Attachment: Turn off rtcp-mux in config

Comment 35

[Ethan Hugg [:ehugg]]

Comment on attachment 793068 [details] [diff] [review]
Turn off rtcp-mux in config

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

Including config.h directly created somewhat of an include storm, so I went to using a separate bool in signaling_unittest.cpp.  To turn it back on we'll only need to flip the two bools.

Comment 36

[Adam Roach [:abr]]

Comment on attachment 793068 [details] [diff] [review]
Turn off rtcp-mux in config

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

Looks good to me.

Regarding the include storm: we might consider (later, not in this patch) having both variables set off of a common boolean preference, drawn from those available via about:config. This would allow shipping the feature in a field-configurable fashion so we can test without necessarily deploying widely.

Comment 37

[Adam Roach [:abr]]

(Scratch my comment about basing the unit test behavior on about:config -- that's wrong for obvious reasons. :) )

Comment 38

[Ethan Hugg [:ehugg]]

I have thought about whether we should put things like rtcp-mux and bundle as options in the about:config but I'm not sure we're ready to test all the permutations.

Comment 39

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36a2061cff79

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe01e7ee9f2 for breaking the build like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=26786385&tree=Mozilla-Inbound&full=1

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #40)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe01e7ee9f2 for
> breaking the build like this:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26786385&tree=Mozilla-
> Inbound&full=1

I think you just need to touch the CLOBBER file and include that when you reland this bug once the tree reopens:

[16:24]	<philor>  the clobbers will be fine, I failed to recognize it as something we've had before, where we apparently somehow configure as though it's not android, and then leave that broken configure around waiting to bust the next push that recompiles webrtc

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And until bug 907473 is resolved, it'd probably be a good idea to touch CLOBBER any time webrtc is changed, to help prevent this in the future. :)

Comment 43

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b13d1bd2e73

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0b13d1bd2e73