Accept SDP with TRANSPORT attributes missing from bundled m= sections.

webrtc/pc/mediasession.cc

Index: webrtc/pc/mediasession.cc
diff --git a/webrtc/pc/mediasession.cc b/webrtc/pc/mediasession.cc
index 5e6942384ad734c0f8bfaa6be338bf8028e75540..2056c9800b60ff0f5d22a1cb1fb72772ec9c5e24 100644
--- a/webrtc/pc/mediasession.cc
+++ b/webrtc/pc/mediasession.cc
@@ -1448,6 +1448,9 @@ SessionDescription* MediaSessionDescriptionFactory::CreateOffer(
 SessionDescription* MediaSessionDescriptionFactory::CreateAnswer(
     const SessionDescription* offer, const MediaSessionOptions& options,
     const SessionDescription* current_description) const {
+  if (!offer) {
+    return nullptr;
+  }
   // The answer contains the intersection of the codecs in the offer with the
   // codecs we support. As indicated by XEP-0167, we retain the same payload ids
   // from the offer in the answer.
@@ -1456,55 +1459,63 @@ SessionDescription* MediaSessionDescriptionFactory::CreateAnswer(
   StreamParamsVec current_streams;
   GetCurrentStreamParams(current_description, &current_streams);
 
-  if (offer) {
-    ContentInfos::const_iterator it = offer->contents().begin();
-    for (; it != offer->contents().end(); ++it) {
-      if (IsMediaContentOfType(&*it, MEDIA_TYPE_AUDIO)) {
-        if (!AddAudioContentForAnswer(offer, options, current_description,
-                                  &current_streams, answer.get())) {
-          return NULL;
-        }
-      } else if (IsMediaContentOfType(&*it, MEDIA_TYPE_VIDEO)) {
-        if (!AddVideoContentForAnswer(offer, options, current_description,
-                                      &current_streams, answer.get())) {
-          return NULL;
-        }
-      } else {
-        RTC_DCHECK(IsMediaContentOfType(&*it, MEDIA_TYPE_DATA));
-        if (!AddDataContentForAnswer(offer, options, current_description,
-                                     &current_streams, answer.get())) {
-          return NULL;
-        }
-      }
-    }
-  }
-
   // If the offer supports BUNDLE, and we want to use it too, create a BUNDLE
   // group in the answer with the appropriate content names.
-  if (offer->HasGroup(GROUP_TYPE_BUNDLE) && options.bundle_enabled) {
-    const ContentGroup* offer_bundle = offer->GetGroupByName(GROUP_TYPE_BUNDLE);
-    ContentGroup answer_bundle(GROUP_TYPE_BUNDLE);
-    for (ContentInfos::const_iterator content = answer->contents().begin();
-       content != answer->contents().end(); ++content) {
-      if (!content->rejected && offer_bundle->HasContentName(content->name)) {
-        answer_bundle.AddContentName(content->name);
+  const ContentGroup* offer_bundle = offer->GetGroupByName(GROUP_TYPE_BUNDLE);
+  ContentGroup answer_bundle(GROUP_TYPE_BUNDLE);
+  // Transport info shared by the bundle group.
+  std::unique_ptr<TransportInfo> bundle_transport;
+
+  ContentInfos::const_iterator it = offer->contents().begin();
+  for (; it != offer->contents().end(); ++it) {
+    if (IsMediaContentOfType(&*it, MEDIA_TYPE_AUDIO)) {
+      if (!AddAudioContentForAnswer(offer, options, current_description,
+                                    bundle_transport.get(), &current_streams,
+                                    answer.get())) {
+        return NULL;
       }
-    }
-    if (answer_bundle.FirstContentName()) {
-      answer->AddGroup(answer_bundle);
-
-      // Share the same ICE credentials and crypto params across all contents,
-      // as BUNDLE requires.
-      if (!UpdateTransportInfoForBundle(answer_bundle, answer.get())) {
-        LOG(LS_ERROR) << "CreateAnswer failed to UpdateTransportInfoForBundle.";
+    } else if (IsMediaContentOfType(&*it, MEDIA_TYPE_VIDEO)) {
+      if (!AddVideoContentForAnswer(offer, options, current_description,
+                                    bundle_transport.get(), &current_streams,
+                                    answer.get())) {
         return NULL;
       }
-
-      if (!UpdateCryptoParamsForBundle(answer_bundle, answer.get())) {
-        LOG(LS_ERROR) << "CreateAnswer failed to UpdateCryptoParamsForBundle.";
+    } else {
+      RTC_DCHECK(IsMediaContentOfType(&*it, MEDIA_TYPE_DATA));
+      if (!AddDataContentForAnswer(offer, options, current_description,
+                                   bundle_transport.get(), &current_streams,
+                                   answer.get())) {
         return NULL;
       }
     }
+    // See if we can add the newly generated m= section to the BUNDLE group in
+    // the answer.
+    if (options.bundle_enabled) {
+      ContentInfo& added = answer->contents().back();
+      if (!added.rejected && offer_bundle &&
+          offer_bundle->HasContentName(added.name)) {
+        answer_bundle.AddContentName(added.name);
+        bundle_transport.reset(
+            new TransportInfo(*answer->GetTransportInfoByName(added.name)));

[pthatcher1, 2017/02/17 19:17:32]

Why do you need to set the bundle_transport here?  Why not do it after the for
loop, like so?

if (answer_bundle.FirstContentName()) {
  bundle_transport.reset(new
TransportInfo(*answer->GetTransportInfoByName(answer_bundle.FirstContentName))
}

That would put less code into the for loop, which would, I think, make it more
clear.  And you  might not need the std::unique_ptr.

[Taylor Brandstetter, 2017/02/17 21:49:15]

Because it's needed inside the loop, for AddXContentForAnswer.
What's the alternative? I think it's most clean with unique_ptr.

+      }
+    }

[pthatcher1, 2017/02/17 19:17:32]

Might be more clear with one if statement:

ContentInfo& added = answer->contents().back();
if (!added.rejected && options.bundled_enabled && 
    offer_bundle && offer_bundle->HasContentName(added.name)) {
  answer_bundle.AddContentName(added.name);
  bundle_transport.reset(new
TransportInfo(*answer->GetTransportInfoByName(added.name));
}

[Taylor Brandstetter, 2017/02/17 21:49:15]

Done.

+  }

[pthatcher1, 2017/02/17 19:17:32]

If the offer has this:
a=bundle b a
m=
a=mid:a
m=
a=mid:b


Do we response with this?
a=bundle a b
m=
a=mid:a
m=
a=mid:b


It looks like we already do that, but it seems wrong to me now.  Is it wrong?

[Taylor Brandstetter, 2017/02/17 21:49:15]

Yes we already do that. Yes, it's technically wrong, though I didn't intend to
change that in this CL; this CL is just about parsing spec-compliant SDP, not
generating it.

+
+  // Only put BUNDLE group in answer if nonempty.
+  if (answer_bundle.FirstContentName()) {
+    answer->AddGroup(answer_bundle);
+
+    // Share the same ICE credentials and crypto params across all contents,
+    // as BUNDLE requires.
+    if (!UpdateTransportInfoForBundle(answer_bundle, answer.get())) {
+      LOG(LS_ERROR) << "CreateAnswer failed to UpdateTransportInfoForBundle.";
+      return NULL;
+    }
+
+    if (!UpdateCryptoParamsForBundle(answer_bundle, answer.get())) {
+      LOG(LS_ERROR) << "CreateAnswer failed to UpdateCryptoParamsForBundle.";
+      return NULL;
+    }
   }
 
   return answer.release();
@@ -1652,16 +1663,16 @@ TransportDescription* MediaSessionDescriptionFactory::CreateTransportAnswer(
     const std::string& content_name,
     const SessionDescription* offer_desc,
     const TransportOptions& transport_options,
-    const SessionDescription* current_desc) const {
+    const SessionDescription* current_desc,
+    bool bundled) const {
   if (!transport_desc_factory_)
     return NULL;
   const TransportDescription* offer_tdesc =
       GetTransportDescription(content_name, offer_desc);
   const TransportDescription* current_tdesc =
       GetTransportDescription(content_name, current_desc);
-  return
-      transport_desc_factory_->CreateAnswer(offer_tdesc, transport_options,
-                                            current_tdesc);
+  return transport_desc_factory_->CreateAnswer(offer_tdesc, transport_options,
+                                               current_tdesc, bundled);
 }
 
 bool MediaSessionDescriptionFactory::AddTransportAnswer(
@@ -1856,15 +1867,17 @@ bool MediaSessionDescriptionFactory::AddAudioContentForAnswer(
     const SessionDescription* offer,
     const MediaSessionOptions& options,
     const SessionDescription* current_description,
+    const TransportInfo* bundle_transport,
     StreamParamsVec* current_streams,
     SessionDescription* answer) const {
   const ContentInfo* audio_content = GetFirstAudioContent(offer);
   const AudioContentDescription* offer_audio =
       static_cast<const AudioContentDescription*>(audio_content->description);
 
-  std::unique_ptr<TransportDescription> audio_transport(CreateTransportAnswer(
-      audio_content->name, offer,
-      GetTransportOptions(options, audio_content->name), current_description));
+  std::unique_ptr<TransportDescription> audio_transport(
+      CreateTransportAnswer(audio_content->name, offer,
+                            GetTransportOptions(options, audio_content->name),
+                            current_description, bundle_transport != nullptr));
   if (!audio_transport) {
     return false;
   }
@@ -1903,10 +1916,11 @@ bool MediaSessionDescriptionFactory::AddAudioContentForAnswer(
     return false;  // Fails the session setup.
   }
 
+  bool secure = bundle_transport ? bundle_transport->description.secure()
+                                 : audio_transport->secure();
   bool rejected = !options.has_audio() || audio_content->rejected ||
-      !IsMediaProtocolSupported(MEDIA_TYPE_AUDIO,
-                                audio_answer->protocol(),
-                                audio_transport->secure());
+                  !IsMediaProtocolSupported(MEDIA_TYPE_AUDIO,
+                                            audio_answer->protocol(), secure);
   if (!rejected) {
     AddTransportAnswer(audio_content->name, *(audio_transport.get()), answer);
   } else {
@@ -1924,12 +1938,14 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer(
     const SessionDescription* offer,
     const MediaSessionOptions& options,
     const SessionDescription* current_description,
+    const TransportInfo* bundle_transport,
     StreamParamsVec* current_streams,
     SessionDescription* answer) const {
   const ContentInfo* video_content = GetFirstVideoContent(offer);
-  std::unique_ptr<TransportDescription> video_transport(CreateTransportAnswer(
-      video_content->name, offer,
-      GetTransportOptions(options, video_content->name), current_description));
+  std::unique_ptr<TransportDescription> video_transport(
+      CreateTransportAnswer(video_content->name, offer,
+                            GetTransportOptions(options, video_content->name),
+                            current_description, bundle_transport != nullptr));
   if (!video_transport) {
     return false;
   }
@@ -1955,10 +1971,11 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer(
           video_answer.get())) {
     return false;
   }
+  bool secure = bundle_transport ? bundle_transport->description.secure()
+                                 : video_transport->secure();
   bool rejected = !options.has_video() || video_content->rejected ||
-      !IsMediaProtocolSupported(MEDIA_TYPE_VIDEO,
-                                video_answer->protocol(),
-                                video_transport->secure());
+                  !IsMediaProtocolSupported(MEDIA_TYPE_VIDEO,
+                                            video_answer->protocol(), secure);
   if (!rejected) {
     if (!AddTransportAnswer(video_content->name, *(video_transport.get()),
                             answer)) {
@@ -1979,12 +1996,14 @@ bool MediaSessionDescriptionFactory::AddDataContentForAnswer(
     const SessionDescription* offer,
     const MediaSessionOptions& options,
     const SessionDescription* current_description,
+    const TransportInfo* bundle_transport,
     StreamParamsVec* current_streams,
     SessionDescription* answer) const {
   const ContentInfo* data_content = GetFirstDataContent(offer);
-  std::unique_ptr<TransportDescription> data_transport(CreateTransportAnswer(
-      data_content->name, offer,
-      GetTransportOptions(options, data_content->name), current_description));
+  std::unique_ptr<TransportDescription> data_transport(
+      CreateTransportAnswer(data_content->name, offer,
+                            GetTransportOptions(options, data_content->name),
+                            current_description, bundle_transport != nullptr));
   if (!data_transport) {
     return false;
   }
@@ -2014,10 +2033,11 @@ bool MediaSessionDescriptionFactory::AddDataContentForAnswer(
     return false;  // Fails the session setup.
   }
 
+  bool secure = bundle_transport ? bundle_transport->description.secure()
+                                 : data_transport->secure();
   bool rejected = !options.has_data() || data_content->rejected ||
-      !IsMediaProtocolSupported(MEDIA_TYPE_DATA,
-                                data_answer->protocol(),
-                                data_transport->secure());
+                  !IsMediaProtocolSupported(MEDIA_TYPE_DATA,
+                                            data_answer->protocol(), secure);
   if (!rejected) {
     data_answer->set_bandwidth(options.data_bandwidth);
     if (!AddTransportAnswer(data_content->name, *(data_transport.get()),