From dbf9d03204f0dbd6136024ce1e97acef3316e1a6 Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Fri, 19 Jan 2018 15:23:40 -0800 Subject: [PATCH] Parameterize PeerConnection data channel tests for Unified Plan Bug: webrtc:8765 Change-Id: Ifac06b2f36230adb093169af0a88dda5463a1216 Reviewed-on: https://webrtc-review.googlesource.com/40503 Commit-Queue: Steve Anton Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#21704} --- pc/peerconnection.cc | 14 ++-- pc/peerconnection_datachannel_unittest.cc | 78 ++++++++++++++++------- 2 files changed, 62 insertions(+), 30 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 2d41d912fa..8431707b66 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -2246,8 +2246,9 @@ RTCError PeerConnection::UpdateDataChannel( const cricket::ContentInfo& content, const cricket::ContentGroup* bundle_group) { if (data_channel_type_ == cricket::DCT_NONE) { - LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, - "Data channels are not supported."); + // If data channels are disabled, ignore this media section. CreateAnswer + // will take care of rejecting it. + return RTCError::OK(); } if (content.rejected) { DestroyDataChannel(); @@ -3404,10 +3405,11 @@ void PeerConnection::GetOptionsForUnifiedPlanAnswer( GetMediaDescriptionOptionsForTransceiver(transceiver, content.name)); } else { RTC_CHECK_EQ(cricket::MEDIA_TYPE_DATA, media_type); - RTC_CHECK(GetDataMid()); - // Always reject a data section if it has already been rejected. - // Additionally, reject all data sections except for the first one. - if (content.rejected || content.name != *GetDataMid()) { + // Reject all data sections if data channels are disabled. + // Reject a data section if it has already been rejected. + // Reject all data sections except for the first one. + if (data_channel_type_ == cricket::DCT_NONE || content.rejected || + content.name != *GetDataMid()) { session_options->media_description_options.push_back( GetMediaDescriptionOptionsForRejectedData(content.name)); } else { diff --git a/pc/peerconnection_datachannel_unittest.cc b/pc/peerconnection_datachannel_unittest.cc index ca3a3f11ca..db6c6568bd 100644 --- a/pc/peerconnection_datachannel_unittest.cc +++ b/pc/peerconnection_datachannel_unittest.cc @@ -16,6 +16,7 @@ #include "pc/peerconnection.h" #include "pc/peerconnectionfactory.h" #include "pc/peerconnectionwrapper.h" +#include "pc/sdputils.h" #ifdef WEBRTC_ANDROID #include "pc/test/androidtestinitializer.h" #endif @@ -84,12 +85,14 @@ class PeerConnectionWrapperForDataChannelTest : public PeerConnectionWrapper { FakeSctpTransportFactory* sctp_transport_factory_ = nullptr; }; -class PeerConnectionDataChannelTest : public ::testing::Test { +class PeerConnectionDataChannelBaseTest : public ::testing::Test { protected: typedef std::unique_ptr WrapperPtr; - PeerConnectionDataChannelTest() - : vss_(new rtc::VirtualSocketServer()), main_(vss_.get()) { + explicit PeerConnectionDataChannelBaseTest(SdpSemantics sdp_semantics) + : vss_(new rtc::VirtualSocketServer()), + main_(vss_.get()), + sdp_semantics_(sdp_semantics) { #ifdef WEBRTC_ANDROID InitializeAndroidObjects(); #endif @@ -112,8 +115,10 @@ class PeerConnectionDataChannelTest : public ::testing::Test { pc_factory->SetOptions(factory_options); RTC_CHECK(pc_factory->Initialize()); auto observer = rtc::MakeUnique(); - auto pc = pc_factory->CreatePeerConnection(config, nullptr, nullptr, - observer.get()); + RTCConfiguration modified_config = config; + modified_config.sdp_semantics = sdp_semantics_; + auto pc = pc_factory->CreatePeerConnection(modified_config, nullptr, + nullptr, observer.get()); if (!pc) { return nullptr; } @@ -153,9 +158,18 @@ class PeerConnectionDataChannelTest : public ::testing::Test { std::unique_ptr vss_; rtc::AutoSocketServerThread main_; + const SdpSemantics sdp_semantics_; }; -TEST_F(PeerConnectionDataChannelTest, +class PeerConnectionDataChannelTest + : public PeerConnectionDataChannelBaseTest, + public ::testing::WithParamInterface { + protected: + PeerConnectionDataChannelTest() + : PeerConnectionDataChannelBaseTest(GetParam()) {} +}; + +TEST_P(PeerConnectionDataChannelTest, NoSctpTransportCreatedIfRtpDataChannelEnabled) { RTCConfiguration config; config.enable_rtp_data_channel = true; @@ -165,7 +179,7 @@ TEST_F(PeerConnectionDataChannelTest, EXPECT_FALSE(caller->sctp_transport_factory()->last_fake_sctp_transport()); } -TEST_F(PeerConnectionDataChannelTest, +TEST_P(PeerConnectionDataChannelTest, RtpDataChannelCreatedEvenIfSctpAvailable) { RTCConfiguration config; config.enable_rtp_data_channel = true; @@ -179,7 +193,7 @@ TEST_F(PeerConnectionDataChannelTest, // Test that sctp_content_name/sctp_transport_name (used for stats) are correct // before and after BUNDLE is negotiated. -TEST_F(PeerConnectionDataChannelTest, SctpContentAndTransportNameSetCorrectly) { +TEST_P(PeerConnectionDataChannelTest, SctpContentAndTransportNameSetCorrectly) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); @@ -193,12 +207,24 @@ TEST_F(PeerConnectionDataChannelTest, SctpContentAndTransportNameSetCorrectly) { caller->AddAudioTrack("a"); caller->AddVideoTrack("v"); caller->pc()->CreateDataChannel("dc", nullptr); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + auto offer = caller->CreateOffer(); + const auto& offer_contents = offer->description()->contents(); + ASSERT_EQ(cricket::MEDIA_TYPE_AUDIO, + offer_contents[0].media_description()->type()); + std::string audio_mid = offer_contents[0].name; + ASSERT_EQ(cricket::MEDIA_TYPE_DATA, + offer_contents[2].media_description()->type()); + std::string data_mid = offer_contents[2].name; + + ASSERT_TRUE( + caller->SetLocalDescription(CloneSessionDescription(offer.get()))); + ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer))); ASSERT_TRUE(caller->sctp_content_name()); - EXPECT_EQ(cricket::CN_DATA, *caller->sctp_content_name()); + EXPECT_EQ(data_mid, *caller->sctp_content_name()); ASSERT_TRUE(caller->sctp_transport_name()); - EXPECT_EQ(cricket::CN_DATA, *caller->sctp_transport_name()); + EXPECT_EQ(data_mid, *caller->sctp_transport_name()); // Create answer that finishes BUNDLE negotiation, which means everything // should be bundled on the first transport (audio). @@ -208,12 +234,12 @@ TEST_F(PeerConnectionDataChannelTest, SctpContentAndTransportNameSetCorrectly) { caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); ASSERT_TRUE(caller->sctp_content_name()); - EXPECT_EQ(cricket::CN_DATA, *caller->sctp_content_name()); + EXPECT_EQ(data_mid, *caller->sctp_content_name()); ASSERT_TRUE(caller->sctp_transport_name()); - EXPECT_EQ(cricket::CN_AUDIO, *caller->sctp_transport_name()); + EXPECT_EQ(audio_mid, *caller->sctp_transport_name()); } -TEST_F(PeerConnectionDataChannelTest, +TEST_P(PeerConnectionDataChannelTest, CreateOfferWithNoDataChannelsGivesNoDataSection) { auto caller = CreatePeerConnection(); auto offer = caller->CreateOffer(); @@ -222,7 +248,7 @@ TEST_F(PeerConnectionDataChannelTest, EXPECT_FALSE(offer->description()->GetTransportInfoByName(cricket::CN_DATA)); } -TEST_F(PeerConnectionDataChannelTest, +TEST_P(PeerConnectionDataChannelTest, CreateAnswerWithRemoteSctpDataChannelIncludesDataSection) { auto caller = CreatePeerConnectionWithDataChannel(); auto callee = CreatePeerConnection(); @@ -231,14 +257,14 @@ TEST_F(PeerConnectionDataChannelTest, auto answer = callee->CreateAnswer(); ASSERT_TRUE(answer); - auto* data_content = - answer->description()->GetContentByName(cricket::CN_DATA); + auto* data_content = cricket::GetFirstDataContent(answer->description()); ASSERT_TRUE(data_content); EXPECT_FALSE(data_content->rejected); - EXPECT_TRUE(answer->description()->GetTransportInfoByName(cricket::CN_DATA)); + EXPECT_TRUE( + answer->description()->GetTransportInfoByName(data_content->name)); } -TEST_F(PeerConnectionDataChannelTest, +TEST_P(PeerConnectionDataChannelTest, CreateDataChannelWithDtlsDisabledSucceeds) { RTCConfiguration config; config.enable_dtls_srtp.emplace(false); @@ -247,7 +273,7 @@ TEST_F(PeerConnectionDataChannelTest, EXPECT_TRUE(caller->pc()->CreateDataChannel("dc", nullptr)); } -TEST_F(PeerConnectionDataChannelTest, CreateDataChannelWithSctpDisabledFails) { +TEST_P(PeerConnectionDataChannelTest, CreateDataChannelWithSctpDisabledFails) { PeerConnectionFactoryInterface::Options options; options.disable_sctp_data_channels = true; auto caller = CreatePeerConnection(RTCConfiguration(), options); @@ -258,7 +284,7 @@ TEST_F(PeerConnectionDataChannelTest, CreateDataChannelWithSctpDisabledFails) { // Test that if a callee has SCTP disabled and receives an offer with an SCTP // data channel, the data section is rejected and no SCTP transport is created // on the callee. -TEST_F(PeerConnectionDataChannelTest, +TEST_P(PeerConnectionDataChannelTest, DataSectionRejectedIfCalleeHasSctpDisabled) { auto caller = CreatePeerConnectionWithDataChannel(); PeerConnectionFactoryInterface::Options options; @@ -270,13 +296,12 @@ TEST_F(PeerConnectionDataChannelTest, EXPECT_FALSE(callee->sctp_transport_factory()->last_fake_sctp_transport()); auto answer = callee->CreateAnswer(); - auto* data_content = - answer->description()->GetContentByName(cricket::CN_DATA); + auto* data_content = cricket::GetFirstDataContent(answer->description()); ASSERT_TRUE(data_content); EXPECT_TRUE(data_content->rejected); } -TEST_F(PeerConnectionDataChannelTest, SctpPortPropagatedFromSdpToTransport) { +TEST_P(PeerConnectionDataChannelTest, SctpPortPropagatedFromSdpToTransport) { constexpr int kNewSendPort = 9998; constexpr int kNewRecvPort = 7775; @@ -298,4 +323,9 @@ TEST_F(PeerConnectionDataChannelTest, SctpPortPropagatedFromSdpToTransport) { EXPECT_EQ(kNewRecvPort, callee_transport->local_port()); } +INSTANTIATE_TEST_CASE_P(PeerConnectionDataChannelTest, + PeerConnectionDataChannelTest, + Values(SdpSemantics::kPlanB, + SdpSemantics::kUnifiedPlan)); + } // namespace webrtc