From 001fd2d5037cca62b717619827cde675ee35f470 Mon Sep 17 00:00:00 2001 From: "jiayl@webrtc.org" Date: Thu, 29 May 2014 15:31:11 +0000 Subject: [PATCH] Fire OnRenegotiationNeeded only for the first SCTP DataChannel. Subsequent DataChannels do not need renegotiation since SCTP data streams are not negotiated through SDP. BUG=2431 R=pthatcher@google.com, wu@webrtc.org Review URL: https://webrtc-codereview.appspot.com/12629004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@6268 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/app/webrtc/peerconnection.cc | 8 ++++++- .../peerconnectioninterface_unittest.cc | 22 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc index 37b2a3b32..0839977f1 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -482,6 +482,8 @@ talk_base::scoped_refptr PeerConnection::CreateDataChannel( const std::string& label, const DataChannelInit* config) { + bool first_datachannel = !mediastream_signaling_->HasDataChannels(); + talk_base::scoped_ptr internal_config; if (config) { internal_config.reset(new InternalDataChannelInit(*config)); @@ -491,7 +493,11 @@ PeerConnection::CreateDataChannel( if (!channel.get()) return NULL; - observer_->OnRenegotiationNeeded(); + // Trigger the onRenegotiationNeeded event for every new RTP DataChannel, or + // the first SCTP DataChannel. + if (session_->data_channel_type() == cricket::DCT_RTP || first_datachannel) { + observer_->OnRenegotiationNeeded(); + } return DataChannelProxy::Create(signaling_thread(), channel.get()); } diff --git a/talk/app/webrtc/peerconnectioninterface_unittest.cc b/talk/app/webrtc/peerconnectioninterface_unittest.cc index a605b0d30..5c9b826da 100644 --- a/talk/app/webrtc/peerconnectioninterface_unittest.cc +++ b/talk/app/webrtc/peerconnectioninterface_unittest.cc @@ -946,23 +946,28 @@ TEST_F(PeerConnectionInterfaceTest, CreateSctpDataChannel) { pc_->CreateDataChannel("1", &config); EXPECT_TRUE(channel != NULL); EXPECT_TRUE(channel->reliable()); + EXPECT_TRUE(observer_.renegotiation_needed_); + observer_.renegotiation_needed_ = false; config.ordered = false; channel = pc_->CreateDataChannel("2", &config); EXPECT_TRUE(channel != NULL); EXPECT_TRUE(channel->reliable()); + EXPECT_FALSE(observer_.renegotiation_needed_); config.ordered = true; config.maxRetransmits = 0; channel = pc_->CreateDataChannel("3", &config); EXPECT_TRUE(channel != NULL); EXPECT_FALSE(channel->reliable()); + EXPECT_FALSE(observer_.renegotiation_needed_); config.maxRetransmits = -1; config.maxRetransmitTime = 0; channel = pc_->CreateDataChannel("4", &config); EXPECT_TRUE(channel != NULL); EXPECT_FALSE(channel->reliable()); + EXPECT_FALSE(observer_.renegotiation_needed_); } // This tests that no data channel is returned if both maxRetransmits and @@ -1012,6 +1017,23 @@ TEST_F(PeerConnectionInterfaceTest, EXPECT_TRUE(channel == NULL); } +// This test verifies that OnRenegotiationNeeded is fired for every new RTP +// DataChannel. +TEST_F(PeerConnectionInterfaceTest, RenegotiationNeededForNewRtpDataChannel) { + FakeConstraints constraints; + constraints.SetAllowRtpDataChannels(); + CreatePeerConnection(&constraints); + + scoped_refptr dc1 = + pc_->CreateDataChannel("test1", NULL); + EXPECT_TRUE(observer_.renegotiation_needed_); + observer_.renegotiation_needed_ = false; + + scoped_refptr dc2 = + pc_->CreateDataChannel("test2", NULL); + EXPECT_TRUE(observer_.renegotiation_needed_); +} + // This test that a data channel closes when a PeerConnection is deleted/closed. TEST_F(PeerConnectionInterfaceTest, DataChannelCloseWhenPeerConnectionClose) { FakeConstraints constraints;