From 30952b460ff6e6e0fada8015c362064fac7ba8af Mon Sep 17 00:00:00 2001 From: deadbeef Date: Fri, 21 Apr 2017 02:41:29 -0700 Subject: [PATCH] Add "ice-option:trickle" to generated offers/answers. BUG=webrtc:7443 Review-Url: https://codereview.webrtc.org/2808913003 Cr-Commit-Position: refs/heads/master@{#17809} --- webrtc/p2p/base/transportdescription.h | 10 ++++++-- .../p2p/base/transportdescriptionfactory.cc | 6 +++-- .../transportdescriptionfactory_unittest.cc | 14 +++++++++-- webrtc/pc/mediasession_unittest.cc | 4 +-- webrtc/pc/peerconnection_integrationtest.cc | 16 ++++++------ webrtc/pc/peerconnectioninterface_unittest.cc | 25 +++++++++++++++++++ 6 files changed, 59 insertions(+), 16 deletions(-) diff --git a/webrtc/p2p/base/transportdescription.h b/webrtc/p2p/base/transportdescription.h index 6f4e9a4bf1..22d054d3b1 100644 --- a/webrtc/p2p/base/transportdescription.h +++ b/webrtc/p2p/base/transportdescription.h @@ -87,7 +87,8 @@ extern const char CONNECTIONROLE_PASSIVE_STR[]; extern const char CONNECTIONROLE_ACTPASS_STR[]; extern const char CONNECTIONROLE_HOLDCONN_STR[]; -constexpr auto ICE_RENOMINATION_STR = "renomination"; +constexpr auto ICE_OPTION_TRICKLE = "trickle"; +constexpr auto ICE_OPTION_RENOMINATION = "renomination"; bool StringToConnectionRole(const std::string& role_str, ConnectionRole* role); bool ConnectionRoleToString(const ConnectionRole& role, std::string* role_str); @@ -140,6 +141,7 @@ struct TransportDescription { return *this; } + // TODO(deadbeef): Rename to HasIceOption, etc. bool HasOption(const std::string& option) const { return (std::find(transport_options.begin(), transport_options.end(), option) != transport_options.end()); @@ -150,7 +152,8 @@ struct TransportDescription { bool secure() const { return identity_fingerprint != nullptr; } IceParameters GetIceParameters() { - return IceParameters(ice_ufrag, ice_pwd, HasOption(ICE_RENOMINATION_STR)); + return IceParameters(ice_ufrag, ice_pwd, + HasOption(ICE_OPTION_RENOMINATION)); } static rtc::SSLFingerprint* CopyFingerprint( @@ -161,6 +164,9 @@ struct TransportDescription { return new rtc::SSLFingerprint(*from); } + // These are actually ICE options (appearing in the ice-options attribute in + // SDP). + // TODO(deadbeef): Rename to ice_options. std::vector transport_options; std::string ice_ufrag; std::string ice_pwd; diff --git a/webrtc/p2p/base/transportdescriptionfactory.cc b/webrtc/p2p/base/transportdescriptionfactory.cc index b431eaaa1b..856ad46126 100644 --- a/webrtc/p2p/base/transportdescriptionfactory.cc +++ b/webrtc/p2p/base/transportdescriptionfactory.cc @@ -37,8 +37,9 @@ TransportDescription* TransportDescriptionFactory::CreateOffer( desc->ice_ufrag = current_description->ice_ufrag; desc->ice_pwd = current_description->ice_pwd; } + desc->AddOption(ICE_OPTION_TRICKLE); if (options.enable_ice_renomination) { - desc->AddOption(ICE_RENOMINATION_STR); + desc->AddOption(ICE_OPTION_RENOMINATION); } // If we are trying to establish a secure transport, add a fingerprint. @@ -75,8 +76,9 @@ TransportDescription* TransportDescriptionFactory::CreateAnswer( desc->ice_ufrag = current_description->ice_ufrag; desc->ice_pwd = current_description->ice_pwd; } + desc->AddOption(ICE_OPTION_TRICKLE); if (options.enable_ice_renomination) { - desc->AddOption(ICE_RENOMINATION_STR); + desc->AddOption(ICE_OPTION_RENOMINATION); } // Negotiate security params. diff --git a/webrtc/p2p/base/transportdescriptionfactory_unittest.cc b/webrtc/p2p/base/transportdescriptionfactory_unittest.cc index 195ccd239b..83bc072ae4 100644 --- a/webrtc/p2p/base/transportdescriptionfactory_unittest.cc +++ b/webrtc/p2p/base/transportdescriptionfactory_unittest.cc @@ -127,8 +127,7 @@ class TransportDescriptionFactoryTest : public testing::Test { bool renomination_expected) { ASSERT_TRUE(desc != nullptr); std::vector& options = desc->transport_options; - auto iter = std::find(options.begin(), options.end(), - cricket::ICE_RENOMINATION_STR); + auto iter = std::find(options.begin(), options.end(), "renomination"); EXPECT_EQ(renomination_expected, iter != options.end()); } @@ -300,3 +299,14 @@ TEST_F(TransportDescriptionFactoryTest, TestIceRenomination) { TEST_F(TransportDescriptionFactoryTest, TestIceRenominationWithDtls) { TestIceRenomination(true); } + +// Test that offers and answers have ice-option:trickle. +TEST_F(TransportDescriptionFactoryTest, AddsTrickleIceOption) { + cricket::TransportOptions options; + std::unique_ptr offer( + f1_.CreateOffer(options, nullptr)); + EXPECT_TRUE(offer->HasOption("trickle")); + std::unique_ptr answer( + f2_.CreateAnswer(offer.get(), options, true, nullptr)); + EXPECT_TRUE(answer->HasOption("trickle")); +} diff --git a/webrtc/pc/mediasession_unittest.cc b/webrtc/pc/mediasession_unittest.cc index 7fe4b51fa8..a1d6a6d0c3 100644 --- a/webrtc/pc/mediasession_unittest.cc +++ b/webrtc/pc/mediasession_unittest.cc @@ -262,8 +262,8 @@ class MediaSessionDescriptionFactoryTest : public testing::Test { bool GetIceRenomination(const TransportInfo* transport_info) { const std::vector& ice_options = transport_info->description.transport_options; - auto iter = std::find(ice_options.begin(), ice_options.end(), - cricket::ICE_RENOMINATION_STR); + auto iter = + std::find(ice_options.begin(), ice_options.end(), "renomination"); return iter != ice_options.end(); } diff --git a/webrtc/pc/peerconnection_integrationtest.cc b/webrtc/pc/peerconnection_integrationtest.cc index 3823f8b8ab..f8126501b6 100644 --- a/webrtc/pc/peerconnection_integrationtest.cc +++ b/webrtc/pc/peerconnection_integrationtest.cc @@ -2508,17 +2508,17 @@ TEST_F(PeerConnectionIntegrationTest, EndToEndCallWithIceRenomination) { const cricket::SessionDescription* desc = caller()->pc()->local_description()->description(); for (const cricket::TransportInfo& info : desc->transport_infos()) { - ASSERT_NE(info.description.transport_options.end(), - std::find(info.description.transport_options.begin(), - info.description.transport_options.end(), - cricket::ICE_RENOMINATION_STR)); + ASSERT_NE( + info.description.transport_options.end(), + std::find(info.description.transport_options.begin(), + info.description.transport_options.end(), "renomination")); } desc = callee()->pc()->local_description()->description(); for (const cricket::TransportInfo& info : desc->transport_infos()) { - ASSERT_NE(info.description.transport_options.end(), - std::find(info.description.transport_options.begin(), - info.description.transport_options.end(), - cricket::ICE_RENOMINATION_STR)); + ASSERT_NE( + info.description.transport_options.end(), + std::find(info.description.transport_options.begin(), + info.description.transport_options.end(), "renomination")); } ExpectNewFramesReceivedWithWait( kDefaultExpectedAudioFrameCount, kDefaultExpectedVideoFrameCount, diff --git a/webrtc/pc/peerconnectioninterface_unittest.cc b/webrtc/pc/peerconnectioninterface_unittest.cc index e7636e024f..91c79ebb76 100644 --- a/webrtc/pc/peerconnectioninterface_unittest.cc +++ b/webrtc/pc/peerconnectioninterface_unittest.cc @@ -3165,6 +3165,31 @@ TEST_F(PeerConnectionInterfaceTest, pc_->StopRtcEventLog(); } +// Test that generated offers/answers include "ice-option:trickle". +TEST_F(PeerConnectionInterfaceTest, OffersAndAnswersHaveTrickleIceOption) { + CreatePeerConnection(); + + // First, create an offer with audio/video. + FakeConstraints constraints; + constraints.SetMandatoryReceiveAudio(true); + constraints.SetMandatoryReceiveVideo(true); + std::unique_ptr offer; + ASSERT_TRUE(DoCreateOffer(&offer, &constraints)); + cricket::SessionDescription* desc = offer->description(); + ASSERT_EQ(2u, desc->transport_infos().size()); + EXPECT_TRUE(desc->transport_infos()[0].description.HasOption("trickle")); + EXPECT_TRUE(desc->transport_infos()[1].description.HasOption("trickle")); + + // Apply the offer as a remote description, then create an answer. + EXPECT_TRUE(DoSetRemoteDescription(offer.release())); + std::unique_ptr answer; + ASSERT_TRUE(DoCreateAnswer(&answer, &constraints)); + desc = answer->description(); + ASSERT_EQ(2u, desc->transport_infos().size()); + EXPECT_TRUE(desc->transport_infos()[0].description.HasOption("trickle")); + EXPECT_TRUE(desc->transport_infos()[1].description.HasOption("trickle")); +} + // Test that ICE renomination isn't offered if it's not enabled in the PC's // RTCConfiguration. TEST_F(PeerConnectionInterfaceTest, IceRenominationNotOffered) {