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

webrtc/pc/mediasession.cc

 /*
  *  Copyright 2004 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 1430 matching lines @@
       return NULL;
     }
   }
 
   return offer.release();
 }
 
 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.
   std::unique_ptr<SessionDescription> answer(new SessionDescription());
 
   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.
+  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;
+      }
+    } else if (IsMediaContentOfType(&*it, MEDIA_TYPE_VIDEO)) {
+      if (!AddVideoContentForAnswer(offer, options, current_description,
+                                    bundle_transport.get(), &current_streams,
+                                    answer.get())) {
+        return NULL;
+      }
+    } 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.

 
-  // 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);
-      }
+  // 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 (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;
-      }
+    if (!UpdateCryptoParamsForBundle(answer_bundle, answer.get())) {
+      LOG(LS_ERROR) << "CreateAnswer failed to UpdateCryptoParamsForBundle.";
+      return NULL;
     }
   }
 
   return answer.release();
 }
 
 const AudioCodecs& MediaSessionDescriptionFactory::GetAudioCodecsForOffer(
     const RtpTransceiverDirection& direction) const {
   // If stream is inactive - generate list as if sendrecv.
   if (direction.send == direction.recv) {
@@ skipping 128 matching lines @@
     LOG(LS_ERROR)
         << "Failed to AddTransportOffer, content name=" << content_name;
   }
   return ret;
 }
 
 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(
     const std::string& content_name,
     const TransportDescription& transport_desc,
     SessionDescription* answer_desc) const {
   if (!answer_desc->AddTransportInfo(TransportInfo(content_name,
                                                    transport_desc))) {
     LOG(LS_ERROR)
         << "Failed to AddTransportAnswer, content name=" << content_name;
@@ skipping 174 matching lines @@
                          current_description, desc)) {
     return false;
   }
   return true;
 }
 
 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;
   }
 
   // Pick codecs based on the requested communications direction in the offer.
   const bool wants_send =
       options.HasSendMediaStream(MEDIA_TYPE_AUDIO) || add_legacy_;
   auto wants_rtd = RtpTransceiverDirection(wants_send, options.recv_audio);
   auto offer_rtd =
       RtpTransceiverDirection::FromMediaContentDirection(
@@ skipping 18 matching lines @@
           sdes_policy,
           GetCryptos(GetFirstAudioContentDescription(current_description)),
           audio_rtp_extensions_,
           current_streams,
           add_legacy_,
           bundle_enabled,
           audio_answer.get())) {
     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 {
     // RFC 3264
     // The answer MUST contain the same number of m-lines as the offer.
     LOG(LS_INFO) << "Audio is not supported in the answer.";
   }
 
   answer->AddContent(audio_content->name, audio_content->type, rejected,
                      audio_answer.release());
   return true;
 }
 
 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;
   }
 
   std::unique_ptr<VideoContentDescription> video_answer(
       new VideoContentDescription());
   // Do not require or create SDES cryptos if DTLS is used.
   cricket::SecurePolicy sdes_policy =
       video_transport->secure() ? cricket::SEC_DISABLED : secure();
   bool bundle_enabled =
       offer->HasGroup(GROUP_TYPE_BUNDLE) && options.bundle_enabled;
   if (!CreateMediaContentAnswer(
           static_cast<const VideoContentDescription*>(
               video_content->description),
           options,
           video_codecs_,
           sdes_policy,
           GetCryptos(GetFirstVideoContentDescription(current_description)),
           video_rtp_extensions_,
           current_streams,
           add_legacy_,
           bundle_enabled,
           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)) {
       return false;
     }
     video_answer->set_bandwidth(options.video_bandwidth);
   } else {
     // RFC 3264
     // The answer MUST contain the same number of m-lines as the offer.
     LOG(LS_INFO) << "Video is not supported in the answer.";
   }
   answer->AddContent(video_content->name, video_content->type, rejected,
                      video_answer.release());
   return true;
 }
 
 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;
   }
   bool is_sctp = (options.data_channel_type == DCT_SCTP);
   std::vector<DataCodec> data_codecs(data_codecs_);
   FilterDataCodecs(&data_codecs, is_sctp);
 
   std::unique_ptr<DataContentDescription> data_answer(
       new DataContentDescription());
   // Do not require or create SDES cryptos if DTLS is used.
   cricket::SecurePolicy sdes_policy =
       data_transport->secure() ? cricket::SEC_DISABLED : secure();
   bool bundle_enabled =
       offer->HasGroup(GROUP_TYPE_BUNDLE) && options.bundle_enabled;
   if (!CreateMediaContentAnswer(
           static_cast<const DataContentDescription*>(
               data_content->description),
           options,
           data_codecs_,
           sdes_policy,
           GetCryptos(GetFirstDataContentDescription(current_description)),
           RtpHeaderExtensions(),
           current_streams,
           add_legacy_,
           bundle_enabled,
           data_answer.get())) {
     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()),
                             answer)) {
       return false;
     }
   } else {
     // RFC 3264
     // The answer MUST contain the same number of m-lines as the offer.
     LOG(LS_INFO) << "Data is not supported in the answer.";
@@ skipping 156 matching lines @@
       GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_VIDEO));
 }
 
 DataContentDescription* GetFirstDataContentDescription(
     SessionDescription* sdesc) {
   return static_cast<DataContentDescription*>(
       GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_DATA));
 }
 
 }  // namespace cricket