Only create a datagram RTP transport if the datagram transport should be used for RTP.

This was an oversight when integrating datagram-based data channels into
JsepTransportController.  If a DatagramTransport exists, but only to be
used for data channels, JsepTransportController will still create an RTP
transport for it and use it for RTP.

Bug: webrtc:9719
Change-Id: I93cdb8bfc03159882a83a9f5097d3ef99fed215f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/150241
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Commit-Queue: Bjorn Mellem <mellem@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28993}

pc/jsep_transport.cc

@@ -769,8 +769,8 @@ void JsepTransport::OnStateChanged(webrtc::MediaTransportState state) {
 void JsepTransport::NegotiateDatagramTransport(SdpType type) {
   RTC_DCHECK(type == SdpType::kAnswer || type == SdpType::kPrAnswer);
   rtc::CritScope lock(&accessor_lock_);
-  if (!composite_rtp_transport_) {
-    return;  // No need to negotiate which RTP transport to use.
+  if (!datagram_transport_) {
+    return;  // No need to negotiate the use of datagram transport.
   }
 
   bool use_datagram_transport =
@@ -778,14 +778,16 @@ void JsepTransport::NegotiateDatagramTransport(SdpType type) {
       remote_description_->transport_desc.opaque_parameters ==
           local_description_->transport_desc.opaque_parameters;
 
+  RTC_LOG(LS_INFO) << "Negotiating datagram transport, use_datagram_transport="
+                   << use_datagram_transport << " answer type="
+                   << (type == SdpType::kAnswer ? "answer" : "pr_answer");
+
   // A provisional or full or answer lets the peer start sending on one of the
   // transports.
-  if (use_datagram_transport) {
-    RTC_LOG(INFO) << "Datagram transport provisionally activated";
-    composite_rtp_transport_->SetSendTransport(datagram_rtp_transport_.get());
-  } else {
-    RTC_LOG(INFO) << "Datagram transport provisionally rejected";
-    composite_rtp_transport_->SetSendTransport(default_rtp_transport());
+  if (composite_rtp_transport_) {
+    composite_rtp_transport_->SetSendTransport(
+        use_datagram_transport ? datagram_rtp_transport_.get()
+                               : default_rtp_transport());
   }
 
   if (type != SdpType::kAnswer) {
@@ -805,18 +807,22 @@ void JsepTransport::NegotiateDatagramTransport(SdpType type) {
       /*provisional=*/false);
 
   if (use_datagram_transport) {
-    RTC_LOG(INFO) << "Datagram transport activated";
-    composite_rtp_transport_->RemoveTransport(default_rtp_transport());
-    if (unencrypted_rtp_transport_) {
-      unencrypted_rtp_transport_ = nullptr;
-    } else if (sdes_transport_) {
-      sdes_transport_ = nullptr;
-    } else {
-      dtls_srtp_transport_ = nullptr;
+    if (composite_rtp_transport_) {
+      // Remove and delete the non-datagram RTP transport.
+      composite_rtp_transport_->RemoveTransport(default_rtp_transport());
+      if (unencrypted_rtp_transport_) {
+        unencrypted_rtp_transport_ = nullptr;
+      } else if (sdes_transport_) {
+        sdes_transport_ = nullptr;
+      } else {
+        dtls_srtp_transport_ = nullptr;
+      }
     }
   } else {
-    RTC_LOG(INFO) << "Datagram transport rejected";
-    composite_rtp_transport_->RemoveTransport(datagram_rtp_transport_.get());
+    // Remove and delete the datagram transport.
+    if (composite_rtp_transport_) {
+      composite_rtp_transport_->RemoveTransport(datagram_rtp_transport_.get());
+    }
     datagram_rtp_transport_ = nullptr;
     datagram_transport_ = nullptr;
   }

pc/jsep_transport_controller.cc

@@ -1196,7 +1196,9 @@ RTCError JsepTransportController::MaybeCreateJsepTransport(
                                               /*datagram_transport=*/nullptr);
   }
 
-  if (datagram_transport) {
+  // Only create a datagram RTP transport if the datagram transport should be
+  // used for RTP.
+  if (datagram_transport && config_.use_datagram_transport) {
     // TODO(sukhanov): We use unencrypted RTP transport over DatagramTransport,
     // because MediaTransport encrypts. In the future we may want to
     // implement our own version of RtpTransport over MediaTransport, because

pc/jsep_transport_controller_unittest.cc

@@ -463,6 +463,56 @@ TEST_F(JsepTransportControllerTest,
          "should be created.";
 }
 
+TEST_F(JsepTransportControllerTest,
+       DtlsIsStillCreatedIfDatagramTransportIsOnlyUsedForDataChannels) {
+  FakeMediaTransportFactory fake_media_transport_factory("transport_params");
+  JsepTransportController::Config config;
+
+  config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyRequire;
+  config.bundle_policy = PeerConnectionInterface::kBundlePolicyMaxBundle;
+  config.media_transport_factory = &fake_media_transport_factory;
+  config.use_datagram_transport_for_data_channels = true;
+  CreateJsepTransportController(config);
+
+  auto description = CreateSessionDescriptionWithBundleGroup();
+  AddCryptoSettings(description.get());
+  absl::optional<cricket::OpaqueTransportParameters> params =
+      transport_controller_->GetTransportParameters(kAudioMid1);
+  for (auto& info : description->transport_infos()) {
+    info.description.opaque_parameters = params;
+  }
+
+  EXPECT_TRUE(transport_controller_
+                  ->SetLocalDescription(SdpType::kOffer, description.get())
+                  .ok());
+  EXPECT_TRUE(transport_controller_
+                  ->SetRemoteDescription(SdpType::kAnswer, description.get())
+                  .ok());
+
+  FakeDatagramTransport* datagram_transport =
+      static_cast<FakeDatagramTransport*>(
+          transport_controller_->GetDataChannelTransport(kAudioMid1));
+
+  ASSERT_NE(nullptr, datagram_transport);
+
+  EXPECT_EQ(cricket::ICE_CANDIDATE_COMPONENT_RTP,
+            transport_controller_->GetDtlsTransport(kAudioMid1)->component())
+      << "Datagram transport for media was not enabled, and so DTLS transport "
+         "should be created.";
+
+  // Datagram transport is not used for media, so no max packet size is
+  // specified.
+  EXPECT_EQ(transport_controller_->GetMediaTransportConfig(kAudioMid1)
+                .rtp_max_packet_size,
+            absl::nullopt);
+
+  // Since datagram transport is not used for RTP, setting it to writable should
+  // not make the RTP transport writable.
+  datagram_transport->set_state(MediaTransportState::kWritable);
+  EXPECT_FALSE(transport_controller_->GetRtpTransport(kAudioMid1)
+                   ->IsWritable(/*rtcp=*/false));
+}
+
 TEST_F(JsepTransportControllerTest, GetMediaTransportInCaller) {
   FakeMediaTransportFactory fake_media_transport_factory;
   JsepTransportController::Config config;