diff --git a/talk/app/webrtc/peerconnection_unittest.cc b/talk/app/webrtc/peerconnection_unittest.cc index bfee48cc9..f6003f6ab 100644 --- a/talk/app/webrtc/peerconnection_unittest.cc +++ b/talk/app/webrtc/peerconnection_unittest.cc @@ -1345,6 +1345,20 @@ TEST_F(JsepPeerConnectionP2PTestClient, AddDataChannelAfterRenegotiation) { kMaxWaitMs); } +// This test sets up a Jsep call with SCTP DataChannel and verifies the +// negotiation is completed without error. +#ifdef HAVE_SCTP +TEST_F(JsepPeerConnectionP2PTestClient, CreateOfferWithSctpDataChannel) { + MAYBE_SKIP_TEST(talk_base::SSLStreamAdapter::HaveDtlsSrtp); + FakeConstraints constraints; + constraints.SetMandatory( + MediaConstraintsInterface::kEnableDtlsSrtp, true); + ASSERT_TRUE(CreateTestClients(&constraints, &constraints)); + initializing_client()->CreateDataChannel(); + initializing_client()->Negotiate(false, false); +} +#endif + // This test sets up a call between two parties with audio, and video. // During the call, the initializing side restart ice and the test verifies that // new ice candidates are generated and audio and video still can flow. @@ -1406,6 +1420,4 @@ TEST_F(JsepPeerConnectionP2PTestClient, EnableVideoDecoderFactory(); LocalP2PTest(); } - #endif // if !defined(THREAD_SANITIZER) - diff --git a/talk/app/webrtc/webrtcsdp.cc b/talk/app/webrtc/webrtcsdp.cc index 2a773b63d..e726dabd9 100644 --- a/talk/app/webrtc/webrtcsdp.cc +++ b/talk/app/webrtc/webrtcsdp.cc @@ -207,7 +207,6 @@ static const char kIsacCodecName[] = "ISAC"; // From webrtcvoiceengine.cc static const int kIsacWbDefaultRate = 32000; // From acm_common_defs.h static const int kIsacSwbDefaultRate = 56000; // From acm_common_defs.h -static const int kDefaultSctpFmt = 5000; static const char kDefaultSctpmapProtocol[] = "webrtc-datachannel"; struct SsrcInfo { @@ -240,7 +239,7 @@ static void BuildMediaDescription(const ContentInfo* content_info, const TransportInfo* transport_info, const MediaType media_type, std::string* message); -static void BuildSctpContentAttributes(std::string* message); +static void BuildSctpContentAttributes(std::string* message, int sctp_port); static void BuildRtpContentAttributes( const MediaContentDescription* media_desc, const MediaType media_type, @@ -1166,6 +1165,7 @@ void BuildMediaDescription(const ContentInfo* content_info, ASSERT(media_desc != NULL); bool is_sctp = (media_desc->protocol() == cricket::kMediaProtocolDtlsSctp); + int sctp_port = cricket::kSctpDefaultPort; // RFC 4566 // m= @@ -1200,14 +1200,22 @@ void BuildMediaDescription(const ContentInfo* content_info, fmt.append(talk_base::ToString(it->id)); } } else if (media_type == cricket::MEDIA_TYPE_DATA) { + const DataContentDescription* data_desc = + static_cast(media_desc); if (is_sctp) { fmt.append(" "); - // TODO(jiayl): Replace the hard-coded string with the fmt read out of the - // ContentDescription. - fmt.append(talk_base::ToString(kDefaultSctpFmt)); + + for (std::vector::const_iterator it = + data_desc->codecs().begin(); + it != data_desc->codecs().end(); ++it) { + if (it->id == cricket::kGoogleSctpDataCodecId && + it->GetParam(cricket::kCodecParamPort, &sctp_port)) { + break; + } + } + + fmt.append(talk_base::ToString(sctp_port)); } else { - const DataContentDescription* data_desc = - static_cast(media_desc); for (std::vector::const_iterator it = data_desc->codecs().begin(); it != data_desc->codecs().end(); ++it) { @@ -1289,18 +1297,18 @@ void BuildMediaDescription(const ContentInfo* content_info, AddLine(os.str(), message); if (is_sctp) { - BuildSctpContentAttributes(message); + BuildSctpContentAttributes(message, sctp_port); } else { BuildRtpContentAttributes(media_desc, media_type, message); } } -void BuildSctpContentAttributes(std::string* message) { +void BuildSctpContentAttributes(std::string* message, int sctp_port) { // draft-ietf-mmusic-sctp-sdp-04 // a=sctpmap:sctpmap-number protocol [streams] std::ostringstream os; InitAttrLine(kAttributeSctpmap, &os); - os << kSdpDelimiterColon << kDefaultSctpFmt << kSdpDelimiterSpace + os << kSdpDelimiterColon << sctp_port << kSdpDelimiterSpace << kDefaultSctpmapProtocol << kSdpDelimiterSpace << (cricket::kMaxSctpSid + 1); AddLine(os.str(), message); @@ -2166,6 +2174,7 @@ bool ParseMediaDescription(const std::string& message, codec_port.SetParam(cricket::kCodecParamPort, fields[3]); LOG(INFO) << "ParseMediaDescription: Got SCTP Port Number " << fields[3]; + ASSERT(!desc->HasCodec(cricket::kGoogleSctpDataCodecId)); desc->AddCodec(codec_port); } diff --git a/talk/app/webrtc/webrtcsdp_unittest.cc b/talk/app/webrtc/webrtcsdp_unittest.cc index aec144745..e02b11f74 100644 --- a/talk/app/webrtc/webrtcsdp_unittest.cc +++ b/talk/app/webrtc/webrtcsdp_unittest.cc @@ -299,7 +299,7 @@ static const char kSdpSctpDataChannelWithCandidatesString[] = "a=mid:data_content_name\r\n" "a=sctpmap:5000 webrtc-datachannel 1024\r\n"; - static const char kSdpConferenceString[] = +static const char kSdpConferenceString[] = "v=0\r\n" "o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n" "s=-\r\n" @@ -1466,6 +1466,37 @@ TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithSctpDataChannel) { EXPECT_EQ(message, expected_sdp); } +TEST_F(WebRtcSdpTest, SerializeWithSctpDataChannelAndNewPort) { + AddSctpDataChannel(); + JsepSessionDescription jsep_desc(kDummyString); + + ASSERT_TRUE(jsep_desc.Initialize(desc_.Copy(), kSessionId, kSessionVersion)); + DataContentDescription* dcdesc = static_cast( + jsep_desc.description()->GetContentDescriptionByName(kDataContentName)); + + const int kNewPort = 1234; + cricket::DataCodec codec( + cricket::kGoogleSctpDataCodecId, cricket::kGoogleSctpDataCodecName, 0); + codec.SetParam(cricket::kCodecParamPort, kNewPort); + dcdesc->AddOrReplaceCodec(codec); + + std::string message = webrtc::SdpSerialize(jsep_desc); + + std::string expected_sdp = kSdpString; + expected_sdp.append(kSdpSctpDataChannelString); + + char default_portstr[16]; + char new_portstr[16]; + talk_base::sprintfn(default_portstr, sizeof(default_portstr), "%d", + kDefaultSctpPort); + talk_base::sprintfn(new_portstr, sizeof(new_portstr), "%d", kNewPort); + talk_base::replace_substrs(default_portstr, strlen(default_portstr), + new_portstr, strlen(new_portstr), + &expected_sdp); + + EXPECT_EQ(expected_sdp, message); +} + TEST_F(WebRtcSdpTest, SerializeSessionDescriptionWithDataChannelAndBandwidth) { AddRtpDataChannel(); data_desc_->set_bandwidth(100*1000); @@ -1898,6 +1929,7 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithSctpDataChannelAndNewPort) { talk_base::sprintfn(unusual_portstr, sizeof(unusual_portstr), "%d", kUnusualSctpPort); + // First setup the expected JsepSessionDescription. JsepSessionDescription jdesc(kDummyString); // take our pre-built session description and change the SCTP port. cricket::SessionDescription* mutant = desc_.Copy(); @@ -1907,11 +1939,13 @@ TEST_F(WebRtcSdpTest, DeserializeSdpWithSctpDataChannelAndNewPort) { EXPECT_EQ(codecs.size(), 1UL); EXPECT_EQ(codecs[0].id, cricket::kGoogleSctpDataCodecId); codecs[0].SetParam(cricket::kCodecParamPort, kUnusualSctpPort); + dcdesc->set_codecs(codecs); // note: mutant's owned by jdesc now. ASSERT_TRUE(jdesc.Initialize(mutant, kSessionId, kSessionVersion)); mutant = NULL; + // Then get the deserialized JsepSessionDescription. std::string sdp_with_data = kSdpString; sdp_with_data.append(kSdpSctpDataChannelString); talk_base::replace_substrs(default_portstr, strlen(default_portstr), diff --git a/talk/media/base/codec.h b/talk/media/base/codec.h index 56fc975ad..120c17b0f 100644 --- a/talk/media/base/codec.h +++ b/talk/media/base/codec.h @@ -120,6 +120,8 @@ struct Codec { name = c.name; clockrate = c.clockrate; preference = c.preference; + params = c.params; + feedback_params = c.feedback_params; return *this; } @@ -127,7 +129,9 @@ struct Codec { return this->id == c.id && // id is reserved in objective-c name == c.name && clockrate == c.clockrate && - preference == c.preference; + preference == c.preference && + params == c.params && + feedback_params == c.feedback_params; } bool operator!=(const Codec& c) const { diff --git a/talk/media/base/codec_unittest.cc b/talk/media/base/codec_unittest.cc index f0ffd8f5a..f2bf4c70d 100644 --- a/talk/media/base/codec_unittest.cc +++ b/talk/media/base/codec_unittest.cc @@ -40,6 +40,43 @@ class CodecTest : public testing::Test { CodecTest() {} }; +TEST_F(CodecTest, TestCodecOperators) { + Codec c0(96, "D", 1000, 0); + c0.SetParam("a", 1); + + Codec c1 = c0; + EXPECT_TRUE(c1 == c0); + + int param_value0; + int param_value1; + EXPECT_TRUE(c0.GetParam("a", ¶m_value0)); + EXPECT_TRUE(c1.GetParam("a", ¶m_value1)); + EXPECT_EQ(param_value0, param_value1); + + c1.id = 86; + EXPECT_TRUE(c0 != c1); + + c1 = c0; + c1.name = "x"; + EXPECT_TRUE(c0 != c1); + + c1 = c0; + c1.clockrate = 2000; + EXPECT_TRUE(c0 != c1); + + c1 = c0; + c1.preference = 1; + EXPECT_TRUE(c0 != c1); + + c1 = c0; + c1.SetParam("a", 2); + EXPECT_TRUE(c0 != c1); + + Codec c5; + Codec c6(0, "", 0, 0); + EXPECT_TRUE(c5 == c6); +} + TEST_F(CodecTest, TestAudioCodecOperators) { AudioCodec c0(96, "A", 44100, 20000, 2, 3); AudioCodec c1(95, "A", 44100, 20000, 2, 3); @@ -238,23 +275,6 @@ TEST_F(CodecTest, TestDataCodecMatches) { EXPECT_FALSE(c1.Matches(DataCodec(95, "D", 0))); } -TEST_F(CodecTest, TestDataCodecOperators) { - DataCodec c0(96, "D", 3); - DataCodec c1(95, "D", 3); - DataCodec c2(96, "x", 3); - DataCodec c3(96, "D", 1); - EXPECT_TRUE(c0 != c1); - EXPECT_TRUE(c0 != c2); - EXPECT_TRUE(c0 != c3); - - DataCodec c4; - DataCodec c5(0, "", 0); - DataCodec c6 = c0; - EXPECT_TRUE(c5 == c4); - EXPECT_TRUE(c6 != c4); - EXPECT_TRUE(c6 == c0); -} - TEST_F(CodecTest, TestSetParamAndGetParam) { AudioCodec codec; codec.SetParam("a", "1"); diff --git a/talk/media/sctp/sctpdataengine.cc b/talk/media/sctp/sctpdataengine.cc index 59e252aae..75c2a4cf5 100644 --- a/talk/media/sctp/sctpdataengine.cc +++ b/talk/media/sctp/sctpdataengine.cc @@ -105,12 +105,6 @@ namespace cricket { typedef talk_base::ScopedMessageData InboundPacketMessage; typedef talk_base::ScopedMessageData OutboundPacketMessage; -// This is the SCTP port to use. It is passed along the wire and the listener -// and connector must be using the same port. It is not related to the ports at -// the IP level. (Corresponds to: sockaddr_conn.sconn_port in usrsctp.h) -// -// TODO(ldixon): Allow port to be set from higher level code. -static const int kSctpDefaultPort = 5001; // TODO(ldixon): Find where this is defined, and also check is Sctp really // respects this. static const size_t kSctpMtu = 1280; @@ -277,10 +271,9 @@ SctpDataEngine::SctpDataEngine() { } usrsctp_engines_count++; - // We don't put in a codec because we don't want one offered when we - // use the hybrid data engine. - // codecs_.push_back(cricket::DataCodec( kGoogleSctpDataCodecId, - // kGoogleSctpDataCodecName, 0)); + cricket::DataCodec codec(kGoogleSctpDataCodecId, kGoogleSctpDataCodecName, 0); + codec.SetParam(kCodecParamPort, kSctpDefaultPort); + codecs_.push_back(codec); } SctpDataEngine::~SctpDataEngine() { @@ -308,8 +301,8 @@ DataMediaChannel* SctpDataEngine::CreateChannel( SctpDataMediaChannel::SctpDataMediaChannel(talk_base::Thread* thread) : worker_thread_(thread), - local_port_(-1), - remote_port_(-1), + local_port_(kSctpDefaultPort), + remote_port_(kSctpDefaultPort), sock_(NULL), sending_(false), receiving_(false), @@ -423,12 +416,6 @@ void SctpDataMediaChannel::CloseSctpSocket() { bool SctpDataMediaChannel::Connect() { LOG(LS_VERBOSE) << debug_name_ << "->Connect()."; - if (remote_port_ < 0) { - remote_port_ = kSctpDefaultPort; - } - if (local_port_ < 0) { - local_port_ = kSctpDefaultPort; - } // If we already have a socket connection, just return. if (sock_) { diff --git a/talk/media/sctp/sctpdataengine.h b/talk/media/sctp/sctpdataengine.h index f2322ab27..7561977c0 100644 --- a/talk/media/sctp/sctpdataengine.h +++ b/talk/media/sctp/sctpdataengine.h @@ -58,6 +58,12 @@ namespace cricket { // tell SCTP we're going to use. const uint32 kMaxSctpSid = 1023; +// This is the default SCTP port to use. It is passed along the wire and the +// connectee and connector must be using the same port. It is not related to the +// ports at the IP level. (Corresponds to: sockaddr_conn.sconn_port in +// usrsctp.h) +const int kSctpDefaultPort = 5000; + // A DataEngine that interacts with usrsctp. // // From channel calls, data flows like this: diff --git a/talk/session/media/mediasession.cc b/talk/session/media/mediasession.cc index 17f7a1a7b..8219ec884 100644 --- a/talk/session/media/mediasession.cc +++ b/talk/session/media/mediasession.cc @@ -308,6 +308,20 @@ static void GetCurrentStreamParams(const SessionDescription* sdesc, } } +// Filters the data codecs for the data channel type. +void FilterDataCodecs(std::vector* codecs, bool sctp) { + // Filter RTP codec for SCTP and vice versa. + int codec_id = sctp ? kGoogleRtpDataCodecId : kGoogleSctpDataCodecId; + for (std::vector::iterator iter = codecs->begin(); + iter != codecs->end();) { + if (iter->id == codec_id) { + iter = codecs->erase(iter); + } else { + ++iter; + } + } +} + template class UsedIds { public: @@ -1204,6 +1218,8 @@ SessionDescription* MediaSessionDescriptionFactory::CreateOffer( scoped_ptr data(new DataContentDescription()); bool is_sctp = (options.data_channel_type == DCT_SCTP); + FilterDataCodecs(&data_codecs, is_sctp); + cricket::SecurePolicy sdes_policy = IsDtlsActive(CN_DATA, current_description) ? cricket::SEC_DISABLED : secure(); @@ -1397,6 +1413,10 @@ SessionDescription* MediaSessionDescriptionFactory::CreateAnswer( if (!data_transport) { return NULL; } + bool is_sctp = (options.data_channel_type == DCT_SCTP); + std::vector data_codecs(data_codecs_); + FilterDataCodecs(&data_codecs, is_sctp); + scoped_ptr data_answer( new DataContentDescription()); // Do not require or create SDES cryptos if DTLS is used. diff --git a/talk/session/media/mediasession.h b/talk/session/media/mediasession.h index dff254f1a..a130b151f 100644 --- a/talk/session/media/mediasession.h +++ b/talk/session/media/mediasession.h @@ -318,6 +318,16 @@ class MediaContentDescriptionImpl : public MediaContentDescription { void AddCodec(const C& codec) { codecs_.push_back(codec); } + void AddOrReplaceCodec(const C& codec) { + for (typename std::vector::iterator iter = codecs_.begin(); + iter != codecs_.end(); ++iter) { + if (iter->id == codec.id) { + *iter = codec; + return; + } + } + AddCodec(codec); + } void AddCodecs(const std::vector& codecs) { typename std::vector::const_iterator codec; for (codec = codecs.begin(); codec != codecs.end(); ++codec) {