Update bundle behavior to match BundlePolicy spec in http://rtcweb-wg.github.io/jsep/.

BUG=1574
R=juberti@webrtc.org, pthatcher@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/36659004

Cr-Commit-Position: refs/heads/master@{#8851}
diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc
index d0c4cc2..46cae4c 100644
--- a/talk/app/webrtc/peerconnection.cc
+++ b/talk/app/webrtc/peerconnection.cc
@@ -339,8 +339,7 @@
   // To handle both internal and externally created port allocator, we will
   // enable BUNDLE here.
   int portallocator_flags = port_allocator_->flags();
-  portallocator_flags |= cricket::PORTALLOCATOR_ENABLE_BUNDLE |
-                         cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
+  portallocator_flags |= cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
                          cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET |
                          cricket::PORTALLOCATOR_ENABLE_IPV6;
   bool value;
diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc
index 1af0e19..9d0a8e4 100644
--- a/talk/app/webrtc/webrtcsession.cc
+++ b/talk/app/webrtc/webrtcsession.cc
@@ -1571,13 +1571,6 @@
 // TODO(mallinath) - Add a correct error code if the channels are not creatued
 // due to BUNDLE is enabled but rtcp-mux is disabled.
 bool WebRtcSession::CreateChannels(const SessionDescription* desc) {
-  // Disabling the BUNDLE flag in PortAllocator if offer disabled it.
-  bool bundle_enabled = desc->HasGroup(cricket::GROUP_TYPE_BUNDLE);
-  if (state() == STATE_INIT && !bundle_enabled) {
-    port_allocator()->set_flags(port_allocator()->flags() &
-                                ~cricket::PORTALLOCATOR_ENABLE_BUNDLE);
-  }
-
   // Creating the media channels and transport proxies.
   const cricket::ContentInfo* voice = cricket::GetFirstAudioContent(desc);
   if (voice && !voice->rejected && !voice_channel_) {
@@ -1604,6 +1597,21 @@
     }
   }
 
+  // Enable bundle before when kMaxBundle policy is in effect.
+  if (bundle_policy_ == PeerConnectionInterface::kBundlePolicyMaxBundle) {
+    const cricket::ContentGroup* local_bundle_group =
+        BaseSession::local_description()->GetGroupByName(
+            cricket::GROUP_TYPE_BUNDLE);
+    if (!local_bundle_group) {
+      LOG(LS_WARNING) << "max-bundle specified without BUNDLE specified";
+      return false;
+    }
+    if (!BaseSession::BundleContentGroup(local_bundle_group)) {
+      LOG(LS_WARNING) << "max-bundle failed to enable bundling.";
+      return false;
+    }
+  }
+
   return true;
 }
 
diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc
index 9267c4c..4e0066b 100644
--- a/talk/app/webrtc/webrtcsession_unittest.cc
+++ b/talk/app/webrtc/webrtcsession_unittest.cc
@@ -371,7 +371,6 @@
         SocketAddress(), SocketAddress(), SocketAddress()));
     allocator_->set_flags(cricket::PORTALLOCATOR_DISABLE_TCP |
                          cricket::PORTALLOCATOR_DISABLE_RELAY |
-                         cricket::PORTALLOCATOR_ENABLE_BUNDLE |
                          cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG);
     EXPECT_TRUE(channel_manager_->Init());
     desc_factory_->set_add_legacy_streams(false);
@@ -1248,7 +1247,6 @@
     allocator_->AddRelay(relay_server);
     allocator_->set_step_delay(cricket::kMinimumStepDelay);
     allocator_->set_flags(cricket::PORTALLOCATOR_DISABLE_TCP |
-                          cricket::PORTALLOCATOR_ENABLE_BUNDLE |
                           cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG);
   }
 
@@ -2597,39 +2595,97 @@
   EXPECT_FALSE(session_->SetRemoteDescription(modified_offer, &error));
 }
 
-TEST_F(WebRtcSessionTest, VerifyBundleFlagInPA) {
-  // This test verifies BUNDLE flag in PortAllocator, if BUNDLE information in
-  // local description is removed by the application, BUNDLE flag should be
-  // disabled in PortAllocator. By default BUNDLE is enabled in the WebRtc.
-  Init();
-  EXPECT_TRUE((cricket::PORTALLOCATOR_ENABLE_BUNDLE &
-      allocator_->flags()) == cricket::PORTALLOCATOR_ENABLE_BUNDLE);
-  rtc::scoped_ptr<SessionDescriptionInterface> offer(CreateOffer());
-
-  cricket::SessionDescription* offer_copy =
-      offer->description()->Copy();
-  offer_copy->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE);
-  JsepSessionDescription* modified_offer =
-      new JsepSessionDescription(JsepSessionDescription::kOffer);
-  modified_offer->Initialize(offer_copy, "1", "1");
-
-  SetLocalDescriptionWithoutError(modified_offer);
-  EXPECT_FALSE(allocator_->flags() & cricket::PORTALLOCATOR_ENABLE_BUNDLE);
-}
-
-TEST_F(WebRtcSessionTest, TestDisabledBundleInAnswer) {
-  Init();
+// kBundlePolicyBalanced bundle policy with and answer contains BUNDLE.
+TEST_F(WebRtcSessionTest, TestBalancedBundleInAnswer) {
+  InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyBalanced);
   mediastream_signaling_.SendAudioVideoStream1();
-  EXPECT_TRUE((cricket::PORTALLOCATOR_ENABLE_BUNDLE &
-      allocator_->flags()) == cricket::PORTALLOCATOR_ENABLE_BUNDLE);
 
   PeerConnectionInterface::RTCOfferAnswerOptions options;
   options.use_rtp_mux = true;
 
   SessionDescriptionInterface* offer = CreateOffer(options);
-
   SetLocalDescriptionWithoutError(offer);
+
+  EXPECT_NE(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
+
   mediastream_signaling_.SendAudioVideoStream2();
+  SessionDescriptionInterface* answer =
+      CreateRemoteAnswer(session_->local_description());
+  SetRemoteDescriptionWithoutError(answer);
+
+  EXPECT_EQ(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
+}
+
+// kBundlePolicyBalanced bundle policy but no BUNDLE in the answer.
+TEST_F(WebRtcSessionTest, TestBalancedNoBundleInAnswer) {
+  InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyBalanced);
+  mediastream_signaling_.SendAudioVideoStream1();
+  PeerConnectionInterface::RTCOfferAnswerOptions options;
+  options.use_rtp_mux = true;
+
+  SessionDescriptionInterface* offer = CreateOffer(options);
+  SetLocalDescriptionWithoutError(offer);
+
+  EXPECT_NE(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
+
+  mediastream_signaling_.SendAudioVideoStream2();
+
+  // Remove BUNDLE from the answer.
+  rtc::scoped_ptr<SessionDescriptionInterface> answer(
+      CreateRemoteAnswer(session_->local_description()));
+  cricket::SessionDescription* answer_copy = answer->description()->Copy();
+  answer_copy->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  JsepSessionDescription* modified_answer =
+      new JsepSessionDescription(JsepSessionDescription::kAnswer);
+  modified_answer->Initialize(answer_copy, "1", "1");
+  SetRemoteDescriptionWithoutError(modified_answer);  //
+
+  EXPECT_NE(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
+}
+
+// kBundlePolicyMaxBundle policy with BUNDLE in the answer.
+TEST_F(WebRtcSessionTest, TestMaxBundleBundleInAnswer) {
+  InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyMaxBundle);
+  mediastream_signaling_.SendAudioVideoStream1();
+
+  PeerConnectionInterface::RTCOfferAnswerOptions options;
+  options.use_rtp_mux = true;
+
+  SessionDescriptionInterface* offer = CreateOffer(options);
+  SetLocalDescriptionWithoutError(offer);
+
+  EXPECT_EQ(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
+
+  mediastream_signaling_.SendAudioVideoStream2();
+  SessionDescriptionInterface* answer =
+      CreateRemoteAnswer(session_->local_description());
+  SetRemoteDescriptionWithoutError(answer);
+
+  EXPECT_EQ(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
+}
+
+// kBundlePolicyMaxBundle policy but no BUNDLE in the answer.
+TEST_F(WebRtcSessionTest, TestMaxBundleNoBundleInAnswer) {
+  InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyMaxBundle);
+  mediastream_signaling_.SendAudioVideoStream1();
+  PeerConnectionInterface::RTCOfferAnswerOptions options;
+  options.use_rtp_mux = true;
+
+  SessionDescriptionInterface* offer = CreateOffer(options);
+  SetLocalDescriptionWithoutError(offer);
+
+  EXPECT_EQ(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
+
+  mediastream_signaling_.SendAudioVideoStream2();
+
+  // Remove BUNDLE from the answer.
   rtc::scoped_ptr<SessionDescriptionInterface> answer(
       CreateRemoteAnswer(session_->local_description()));
   cricket::SessionDescription* answer_copy = answer->description()->Copy();
@@ -2638,22 +2694,63 @@
       new JsepSessionDescription(JsepSessionDescription::kAnswer);
   modified_answer->Initialize(answer_copy, "1", "1");
   SetRemoteDescriptionWithoutError(modified_answer);
-  EXPECT_TRUE((cricket::PORTALLOCATOR_ENABLE_BUNDLE &
-      allocator_->flags()) == cricket::PORTALLOCATOR_ENABLE_BUNDLE);
 
-  video_channel_ = media_engine_->GetVideoChannel(0);
-  voice_channel_ = media_engine_->GetVoiceChannel(0);
+  EXPECT_EQ(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
+}
 
-  ASSERT_EQ(1u, video_channel_->recv_streams().size());
-  EXPECT_TRUE(kVideoTrack2 == video_channel_->recv_streams()[0].id);
+// kBundlePolicyMaxCompat bundle policy with and answer contains BUNDLE.
+TEST_F(WebRtcSessionTest, TestMaxCompatBundleInAnswer) {
+  InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyMaxCompat);
+  mediastream_signaling_.SendAudioVideoStream1();
 
-  ASSERT_EQ(1u, voice_channel_->recv_streams().size());
-  EXPECT_TRUE(kAudioTrack2 == voice_channel_->recv_streams()[0].id);
+  PeerConnectionInterface::RTCOfferAnswerOptions options;
+  options.use_rtp_mux = true;
 
-  ASSERT_EQ(1u, video_channel_->send_streams().size());
-  EXPECT_TRUE(kVideoTrack1 == video_channel_->send_streams()[0].id);
-  ASSERT_EQ(1u, voice_channel_->send_streams().size());
-  EXPECT_TRUE(kAudioTrack1 == voice_channel_->send_streams()[0].id);
+  SessionDescriptionInterface* offer = CreateOffer(options);
+  SetLocalDescriptionWithoutError(offer);
+
+  EXPECT_NE(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
+
+  mediastream_signaling_.SendAudioVideoStream2();
+  SessionDescriptionInterface* answer =
+      CreateRemoteAnswer(session_->local_description());
+  SetRemoteDescriptionWithoutError(answer);
+
+  // This should lead to an audio-only call but isn't implemented
+  // correctly yet.
+  EXPECT_EQ(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
+}
+
+// kBundlePolicyMaxCompat bundle policy but no BUNDLE in the answer.
+TEST_F(WebRtcSessionTest, TestMaxCompatNoBundleInAnswer) {
+  InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyMaxCompat);
+  mediastream_signaling_.SendAudioVideoStream1();
+  PeerConnectionInterface::RTCOfferAnswerOptions options;
+  options.use_rtp_mux = true;
+
+  SessionDescriptionInterface* offer = CreateOffer(options);
+  SetLocalDescriptionWithoutError(offer);
+
+  EXPECT_NE(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
+
+  mediastream_signaling_.SendAudioVideoStream2();
+
+  // Remove BUNDLE from the answer.
+  rtc::scoped_ptr<SessionDescriptionInterface> answer(
+      CreateRemoteAnswer(session_->local_description()));
+  cricket::SessionDescription* answer_copy = answer->description()->Copy();
+  answer_copy->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE);
+  JsepSessionDescription* modified_answer =
+      new JsepSessionDescription(JsepSessionDescription::kAnswer);
+  modified_answer->Initialize(answer_copy, "1", "1");
+  SetRemoteDescriptionWithoutError(modified_answer);  //
+
+  EXPECT_NE(session_->GetTransportProxy("audio")->impl(),
+            session_->GetTransportProxy("video")->impl());
 }
 
 // This test verifies that SetLocalDescription and SetRemoteDescription fails
@@ -2661,8 +2758,6 @@
 TEST_F(WebRtcSessionTest, TestDisabledRtcpMuxWithBundleEnabled) {
   Init();
   mediastream_signaling_.SendAudioVideoStream1();
-  EXPECT_TRUE((cricket::PORTALLOCATOR_ENABLE_BUNDLE &
-      allocator_->flags()) == cricket::PORTALLOCATOR_ENABLE_BUNDLE);
 
   PeerConnectionInterface::RTCOfferAnswerOptions options;
   options.use_rtp_mux = true;
@@ -3153,7 +3248,6 @@
 // Runs the loopback call test with BUNDLE and STUN enabled.
 TEST_F(WebRtcSessionTest, TestIceStatesBundle) {
   allocator_->set_flags(cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG |
-                       cricket::PORTALLOCATOR_ENABLE_BUNDLE |
                        cricket::PORTALLOCATOR_DISABLE_TCP |
                        cricket::PORTALLOCATOR_DISABLE_RELAY);
   TestLoopbackCall();
diff --git a/webrtc/p2p/base/session.cc b/webrtc/p2p/base/session.cc
index 4da6519..136f391 100644
--- a/webrtc/p2p/base/session.cc
+++ b/webrtc/p2p/base/session.cc
@@ -537,23 +537,6 @@
   return (iter != transports_.end()) ? iter->second : NULL;
 }
 
-TransportProxy* BaseSession::GetTransportProxy(const Transport* transport) {
-  for (TransportMap::iterator iter = transports_.begin();
-       iter != transports_.end(); ++iter) {
-    TransportProxy* transproxy = iter->second;
-    if (transproxy->impl() == transport) {
-      return transproxy;
-    }
-  }
-  return NULL;
-}
-
-TransportProxy* BaseSession::GetFirstTransportProxy() {
-  if (transports_.empty())
-    return NULL;
-  return transports_.begin()->second;
-}
-
 void BaseSession::DestroyTransportProxy(
     const std::string& content_name) {
   TransportMap::iterator iter = transports_.find(content_name);
@@ -643,8 +626,9 @@
   for (TransportMap::iterator iter = transports_.begin();
        iter != transports_.end(); ++iter) {
     ASSERT(iter->second->negotiated());
-    if (!iter->second->negotiated())
+    if (!iter->second->negotiated()) {
       return false;
+    }
   }
 
   // If both sides agree to BUNDLE, mux all the specified contents onto the
@@ -654,56 +638,63 @@
   // BUNDLE the same way?
   bool candidates_allocated = IsCandidateAllocationDone();
   const ContentGroup* local_bundle_group =
-      local_description()->GetGroupByName(GROUP_TYPE_BUNDLE);
+      local_description_->GetGroupByName(GROUP_TYPE_BUNDLE);
   const ContentGroup* remote_bundle_group =
-      remote_description()->GetGroupByName(GROUP_TYPE_BUNDLE);
-  if (local_bundle_group && remote_bundle_group &&
-      local_bundle_group->FirstContentName()) {
-    const std::string* content_name = local_bundle_group->FirstContentName();
-    const ContentInfo* content =
-        local_description_->GetContentByName(*content_name);
-    if (!content) {
-      LOG(LS_WARNING) << "Content \"" << *content_name
-                      << "\" referenced in BUNDLE group is not present";
-      return false;
-    }
-
-    if (!SetSelectedProxy(content->name, local_bundle_group)) {
+      remote_description_->GetGroupByName(GROUP_TYPE_BUNDLE);
+  if (local_bundle_group && remote_bundle_group) {
+    if (!BundleContentGroup(local_bundle_group)) {
       LOG(LS_WARNING) << "Failed to set up BUNDLE";
       return false;
     }
 
     // If we weren't done gathering before, we might be done now, as a result
     // of enabling mux.
-    LOG(LS_INFO) << "Enabling BUNDLE, bundling onto transport: "
-                 << *content_name;
     if (!candidates_allocated) {
       MaybeCandidateAllocationDone();
     }
   } else {
-    LOG(LS_INFO) << "No BUNDLE information, not bundling.";
+    LOG(LS_INFO) << "BUNDLE group missing from remote or local description.";
   }
   return true;
 }
 
-bool BaseSession::SetSelectedProxy(const std::string& content_name,
-                                   const ContentGroup* muxed_group) {
-  TransportProxy* selected_proxy = GetTransportProxy(content_name);
-  if (!selected_proxy) {
+bool BaseSession::BundleContentGroup(const ContentGroup* bundle_group) {
+  const std::string* content_name = bundle_group->FirstContentName();
+  if (!content_name) {
+    LOG(LS_INFO) << "No content names specified in BUNDLE group.";
+    return true;
+  }
+
+  const ContentInfo* content =
+      local_description_->GetContentByName(*content_name);
+  if (!content) {
+    LOG(LS_WARNING) << "Content \"" << *content_name
+                    << "\" referenced in BUNDLE group"
+                    << " not present in local description";
     return false;
   }
 
-  ASSERT(selected_proxy->negotiated());
+  TransportProxy* selected_proxy = GetTransportProxy(*content_name);
+  if (!selected_proxy) {
+    LOG(LS_WARNING) << "No transport found for content \""
+                    << *content_name << "\".";
+    return false;
+  }
+
   for (TransportMap::iterator iter = transports_.begin();
        iter != transports_.end(); ++iter) {
     // If content is part of the mux group, then repoint its proxy at the
     // transport object that we have chosen to mux onto. If the proxy
     // is already pointing at the right object, it will be a no-op.
-    if (muxed_group->HasContentName(iter->first) &&
+    if (bundle_group->HasContentName(iter->first) &&
         !iter->second->SetupMux(selected_proxy)) {
+      LOG(LS_WARNING) << "Failed to bundle " << iter->first << " to "
+                      << *content_name;
       return false;
     }
+    LOG(LS_INFO) << "Bundling " << iter->first << " to " << *content_name;
   }
+
   return true;
 }
 
diff --git a/webrtc/p2p/base/session.h b/webrtc/p2p/base/session.h
index cdee801..ba4206a 100644
--- a/webrtc/p2p/base/session.h
+++ b/webrtc/p2p/base/session.h
@@ -331,8 +331,6 @@
   const TransportMap& transport_proxies() const { return transports_; }
   // Get a TransportProxy by content_name or transport. NULL if not found.
   TransportProxy* GetTransportProxy(const std::string& content_name);
-  TransportProxy* GetTransportProxy(const Transport* transport);
-  TransportProxy* GetFirstTransportProxy();
   void DestroyTransportProxy(const std::string& content_name);
   // TransportProxy is owned by session.  Return proxy just for convenience.
   TransportProxy* GetOrCreateTransportProxy(const std::string& content_name);
@@ -411,6 +409,10 @@
 
   // Fires the new description signal according to the current state.
   virtual void SignalNewDescription();
+  // This method will delete the Transport and TransportChannelImpls
+  // and replace those with the Transport object of the first
+  // MediaContent in bundle_group.
+  bool BundleContentGroup(const ContentGroup* bundle_group);
 
  private:
   // Helper methods to push local and remote transport descriptions.
@@ -423,12 +425,6 @@
 
   void MaybeCandidateAllocationDone();
 
-  // This method will delete the Transport and TransportChannelImpls and
-  // replace those with the selected Transport objects. Selection is done
-  // based on the content_name and in this case first MediaContent information
-  // is used for mux.
-  bool SetSelectedProxy(const std::string& content_name,
-                        const ContentGroup* muxed_group);
   // Log session state.
   void LogState(State old_state, State new_state);