Do not create a temporary transport channel when using max-bundle

webrtc/api/webrtcsession.cc

 /*
  *  Copyright 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 1709 matching lines @@
   }
 
   const cricket::ContentInfo* data_info =
       cricket::GetFirstDataContent(desc);
   if ((!data_info || data_info->rejected) && data_channel_) {
     SignalDataChannelDestroyed();
     channel_manager_->DestroyDataChannel(data_channel_.release());
   }
 }
 
+// Returns the name of the transport channel when BUNDLE is enabled.
+const std::string& WebRtcSession::GetTransportForChannel(

[pthatcher1, 2016/05/11 06:20:39]

Why not, instead, return a const pointer that is NULL when it's not bundled?

Also, should we call this GetBundleTransportName?

+    const cricket::ContentInfo* content,
+    const cricket::ContentGroup* bundle) {
+  if (!bundle) {
+    return content->name;
+  }
+  const std::string* first_content_name = bundle->FirstContentName();
+  if (!first_content_name) {
+    LOG(LS_WARNING) << "Tried to BUNDLE with no contents.";
+    return content->name;
+  }
+
+  const std::string& transport_name = *first_content_name;
+  if (bundle->HasContentName(content->name)) {
+    LOG(LS_INFO) << "Bundling " << content->name << " on " << transport_name;
+    return transport_name;
+  }
+  return content->name;
+}
+
 // TODO(mallinath) - Add a correct error code if the channels are not created

[skvlad, 2016/05/11 22:24:19]

This TODO seems to not apply - there is a check for this condition in
ValidateSessionDescription(). We should be rejecting descriptions which have
bundled channels without RTCP mux. 

There doesn't seem to be a check for the case where the offerer requires
RTCP-mux and the answer does not enable it. I've added a TODO to add one.

 // due to BUNDLE is enabled but rtcp-mux is disabled.
 bool WebRtcSession::CreateChannels(const SessionDescription* desc) {
+  const cricket::ContentGroup* bundle_group = nullptr;
+  if (bundle_policy_ == PeerConnectionInterface::kBundlePolicyMaxBundle) {

[pthatcher1, 2016/05/11 06:20:39]

If the policy is max bundle, then all the channels must be in the bundle and
that bundle must have a content name.  So you could probably just use
bundle->FirstContentName()  in all of the calls below and you won't need
GetTranpsortForChannel.

[Taylor Brandstetter, 2016/05/11 15:32:23]

JSEP does say the application can't munge BUNDLE groups or "a=bundle-only" (once
we implement that). But these channels could be created from a *remote*
description, and a remote peer doesn't have the same restrictions as a JSEP
implementation (it could decide to use bundle-only on a subset of media
descriptions). So I think this should be left as-is. Unless we start rejecting
non-BUNDLE media descriptions (working group hasn't decided on that yet).

[pthatcher1, 2016/05/11 22:23:47]

If we are in max-bundle mode, we already reject descriptions that don't have a
bundle group.  But you're right that we don't verify that all the m-lines are in
the max-bundle.  But I think we should reject descriptions that don't have
everything bundled when in max-bundle mode, unless the standards body decides
otherwise (and I'll argue in the group that max-bundle should mean we reject
non-bundled m-lines).

[skvlad, 2016/05/11 22:24:19]

Added a TODO to reject the offer in this case when the JSEP change is accepted.

[Taylor Brandstetter, 2016/05/11 22:35:55]

I'm of the opposite opinion... I think we should do what the standard says until
the standard is changed. Here's a PR where you can argue, though:
https://github.com/rtcweb-wg/jsep/pull/279

+    bundle_group = desc->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
+    if (!bundle_group) {
+      LOG(LS_WARNING) << "max-bundle specified without BUNDLE specified";
+      return false;
+    }
+  }
+
   // Creating the media channels and transport proxies.
   const cricket::ContentInfo* voice = cricket::GetFirstAudioContent(desc);
   if (voice && !voice->rejected && !voice_channel_) {
-    if (!CreateVoiceChannel(voice)) {
+    if (!CreateVoiceChannel(voice,
+                            GetTransportForChannel(voice, bundle_group))) {
       LOG(LS_ERROR) << "Failed to create voice channel.";
       return false;
     }
   }
 
   const cricket::ContentInfo* video = cricket::GetFirstVideoContent(desc);
   if (video && !video->rejected && !video_channel_) {
-    if (!CreateVideoChannel(video)) {
+    if (!CreateVideoChannel(video,
+                            GetTransportForChannel(video, bundle_group))) {
       LOG(LS_ERROR) << "Failed to create video channel.";
       return false;
     }
   }
 
   const cricket::ContentInfo* data = cricket::GetFirstDataContent(desc);
   if (data_channel_type_ != cricket::DCT_NONE &&
       data && !data->rejected && !data_channel_) {
-    if (!CreateDataChannel(data)) {
+    if (!CreateDataChannel(data, GetTransportForChannel(data, bundle_group))) {
       LOG(LS_ERROR) << "Failed to create data channel.";
       return false;
     }
   }
 
-  if (rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire) {
-    if (voice_channel()) {
-      voice_channel()->ActivateRtcpMux();
-    }
-    if (video_channel()) {
-      video_channel()->ActivateRtcpMux();
-    }
-    if (data_channel()) {
-      data_channel()->ActivateRtcpMux();
-    }
-  }
-
-  // Enable BUNDLE immediately when kBundlePolicyMaxBundle is in effect.
-  if (bundle_policy_ == PeerConnectionInterface::kBundlePolicyMaxBundle) {
-    const cricket::ContentGroup* bundle_group = desc->GetGroupByName(
-        cricket::GROUP_TYPE_BUNDLE);
-    if (!bundle_group) {
-      LOG(LS_WARNING) << "max-bundle specified without BUNDLE specified";
-      return false;
-    }
-    if (!EnableBundle(*bundle_group)) {
-      LOG(LS_WARNING) << "max-bundle failed to enable bundling.";
-      return false;
-    }
-  }
-
   return true;
 }
 
-bool WebRtcSession::CreateVoiceChannel(const cricket::ContentInfo* content) {
+bool WebRtcSession::CreateVoiceChannel(const cricket::ContentInfo* content,
+                                       const std::string& transport_name) {
   voice_channel_.reset(channel_manager_->CreateVoiceChannel(
-      media_controller_, transport_controller_.get(), content->name, true,
+      media_controller_, transport_controller_.get(), content->name,
+      transport_name,
+      rtcp_mux_policy_ !=
+          PeerConnectionInterface::kRtcpMuxPolicyRequire /* rtcp */,

[pthatcher1, 2016/05/11 06:20:39]

Can you put this on a separate line?

bool rtcp_mux = (rtcp_mux_policy_ ==
PeerConnectionInterface::kRtcpMuxPolicyPeerConnectionInterface::kRtcpMuxPolicyRequire);

Then you could use it here and below.

[Taylor Brandstetter, 2016/05/11 15:32:23]

This means that if we receive a remote description with no "a=rtcp-mux" and our
policy is "require", RTCP will silently fail to work as opposed to us rejecting
the media description (which JSEP says we should do). Could you add a TODO
comment to reject media descriptions without "a=rtcp-mux" when in "require"
mode?

Oh, and the "bool rtcp" parameter tells whether RTCP is enabled *at all*. So
this would turn off RTCP for anyone not using "require mux". I think there need
to be two separate parameters: "RTCP enabled" and "RTCP mux required".

[pthatcher1, 2016/05/11 22:23:47]

The "rtcp" parameter here ultimately gets passed into the BaseChannel parameters
"rtcp_tranpsort_enabled", so it might make sense to call it that here as well
and everywhere from here to there (instead of calling it "rtcp").  That would
make it more clear what this value means.  

And having rtcp_transport_enabled = (rtcp_mux_policy_ ==
PeerConnectionInterface::kRtcpMuxPolicyNegotiate), I believe, works.

You're right that we should reject the remote description if a=rtcp-mux is not
in it and rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire. 
We should fix that in another CL.  I don't think it impacts this CL.

[skvlad, 2016/05/11 22:24:19]

Done.

[skvlad, 2016/05/11 22:24:20]

Added a comment in ValidateSessionDescription. 
If RTCP mux is enabled, CreateVoiceChannel calls ActivateRtcpMux right after
creating the channel. This creates the RtcpMuxFilter which appears to be enough
to send and receive muxed RTCP.

Setting rtcp=false - if my understanding is correct - only prevents us from
creating and then destroying the non-muxed RTCP transport channel.

       audio_options_));
   if (!voice_channel_) {
     return false;
   }
+  if (rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire) {
+    voice_channel_->ActivateRtcpMux();
+  }
 
   voice_channel_->SignalDtlsSetupFailure.connect(
       this, &WebRtcSession::OnDtlsSetupFailure);
 
   SignalVoiceChannelCreated();
   voice_channel_->transport_channel()->SignalSentPacket.connect(
       this, &WebRtcSession::OnSentPacket_w);
   return true;
 }
 
-bool WebRtcSession::CreateVideoChannel(const cricket::ContentInfo* content) {
+bool WebRtcSession::CreateVideoChannel(const cricket::ContentInfo* content,
+                                       const std::string& transport_name) {
   video_channel_.reset(channel_manager_->CreateVideoChannel(
-      media_controller_, transport_controller_.get(), content->name, true,
+      media_controller_, transport_controller_.get(), content->name,
+      transport_name,
+      rtcp_mux_policy_ !=
+          PeerConnectionInterface::kRtcpMuxPolicyRequire /* rtcp */,

[pthatcher1, 2016/05/11 06:20:39]

Same thing here.

[skvlad, 2016/05/11 22:24:20]

Acknowledged.

       video_options_));
   if (!video_channel_) {
     return false;
   }
-
+  if (rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire) {
+    video_channel_->ActivateRtcpMux();
+  }
   video_channel_->SignalDtlsSetupFailure.connect(
       this, &WebRtcSession::OnDtlsSetupFailure);
 
   SignalVideoChannelCreated();
   video_channel_->transport_channel()->SignalSentPacket.connect(
       this, &WebRtcSession::OnSentPacket_w);
   return true;
 }
 
-bool WebRtcSession::CreateDataChannel(const cricket::ContentInfo* content) {
+bool WebRtcSession::CreateDataChannel(const cricket::ContentInfo* content,
+                                      const std::string& transport_name) {
   bool sctp = (data_channel_type_ == cricket::DCT_SCTP);
   data_channel_.reset(channel_manager_->CreateDataChannel(
-      transport_controller_.get(), content->name, !sctp, data_channel_type_));
+      transport_controller_.get(), content->name, transport_name,
+      !sctp &&
+          rtcp_mux_policy_ !=
+              PeerConnectionInterface::kRtcpMuxPolicyRequire /* rtcp */,
+      data_channel_type_));

[pthatcher1, 2016/05/11 06:20:39]

And here.

[skvlad, 2016/05/11 22:24:19]

Done.

   if (!data_channel_) {
     return false;
   }
+  if (rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire) {
+    data_channel_->ActivateRtcpMux();
+  }
 
   if (sctp) {
     data_channel_->SignalDataReceived.connect(
         this, &WebRtcSession::OnDataChannelMessageReceived);
   }
 
   data_channel_->SignalDtlsSetupFailure.connect(
       this, &WebRtcSession::OnDtlsSetupFailure);
 
   SignalDataChannelCreated();
@@ skipping 316 matching lines @@
   }
 }
 
 void WebRtcSession::OnSentPacket_w(cricket::TransportChannel* channel,
                                    const rtc::SentPacket& sent_packet) {
   RTC_DCHECK(worker_thread()->IsCurrent());
   media_controller_->call_w()->OnSentPacket(sent_packet);
 }
 
 }  // namespace webrtc