Do not create a temporary transport channel when using max-bundle
With this change, when max-bundle and rtcp-mux are both enabled, we no
longer create and destroy a temporary transport channel when a media
channel gets added. Instead, the media channel uses the correct bundled
transport channel from the start.
This fixes a bug where adding a media type would cause the ICE state to
briefly become Disconnected and then immediately recover. The temporary
channel was created in a non-writable state, which caused the
TransportController to declare the ICE state to be Disconnected (as not
all transport channels were writable). Right after creation, the
temporary channel was then destroyed and the ICE state went back to the
correct one.
BUG=webrtc:5856
Review-Url: https://codereview.webrtc.org/1972493002
Cr-Commit-Position: refs/heads/master@{#12781}
diff --git a/webrtc/api/webrtcsession.cc b/webrtc/api/webrtcsession.cc
index e7d9b14..80352cf 100644
--- a/webrtc/api/webrtcsession.cc
+++ b/webrtc/api/webrtcsession.cc
@@ -1739,13 +1739,41 @@
}
}
-// TODO(mallinath) - Add a correct error code if the channels are not created
-// due to BUNDLE is enabled but rtcp-mux is disabled.
+// Returns the name of the transport channel when BUNDLE is enabled, or nullptr
+// if the channel is not part of any bundle.
+const std::string* WebRtcSession::GetBundleTransportName(
+ const cricket::ContentInfo* content,
+ const cricket::ContentGroup* bundle) {
+ if (!bundle) {
+ return nullptr;
+ }
+ const std::string* first_content_name = bundle->FirstContentName();
+ if (!first_content_name) {
+ LOG(LS_WARNING) << "Tried to BUNDLE with no contents.";
+ return nullptr;
+ }
+ if (!bundle->HasContentName(content->name)) {
+ LOG(LS_WARNING) << content->name << " is not part of any bundle group";
+ return nullptr;
+ }
+ LOG(LS_INFO) << "Bundling " << content->name << " on " << *first_content_name;
+ return first_content_name;
+}
+
bool WebRtcSession::CreateChannels(const SessionDescription* desc) {
+ const cricket::ContentGroup* bundle_group = nullptr;
+ if (bundle_policy_ == PeerConnectionInterface::kBundlePolicyMaxBundle) {
+ bundle_group = desc->GetGroupByName(cricket::GROUP_TYPE_BUNDLE);
+ if (!bundle_group) {
+ LOG(LS_WARNING) << "max-bundle specified without BUNDLE specified";
+ return false;
+ }
+ }
// Creating the media channels and transport proxies.
const cricket::ContentInfo* voice = cricket::GetFirstAudioContent(desc);
if (voice && !voice->rejected && !voice_channel_) {
- if (!CreateVoiceChannel(voice)) {
+ if (!CreateVoiceChannel(voice,
+ GetBundleTransportName(voice, bundle_group))) {
LOG(LS_ERROR) << "Failed to create voice channel.";
return false;
}
@@ -1753,7 +1781,8 @@
const cricket::ContentInfo* video = cricket::GetFirstVideoContent(desc);
if (video && !video->rejected && !video_channel_) {
- if (!CreateVideoChannel(video)) {
+ if (!CreateVideoChannel(video,
+ GetBundleTransportName(video, bundle_group))) {
LOG(LS_ERROR) << "Failed to create video channel.";
return false;
}
@@ -1762,48 +1791,29 @@
const cricket::ContentInfo* data = cricket::GetFirstDataContent(desc);
if (data_channel_type_ != cricket::DCT_NONE &&
data && !data->rejected && !data_channel_) {
- if (!CreateDataChannel(data)) {
+ if (!CreateDataChannel(data, GetBundleTransportName(data, bundle_group))) {
LOG(LS_ERROR) << "Failed to create data channel.";
return false;
}
}
- if (rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire) {
- if (voice_channel()) {
- voice_channel()->ActivateRtcpMux();
- }
- if (video_channel()) {
- video_channel()->ActivateRtcpMux();
- }
- if (data_channel()) {
- data_channel()->ActivateRtcpMux();
- }
- }
-
- // Enable BUNDLE immediately when kBundlePolicyMaxBundle is in effect.
- if (bundle_policy_ == PeerConnectionInterface::kBundlePolicyMaxBundle) {
- const cricket::ContentGroup* bundle_group = desc->GetGroupByName(
- cricket::GROUP_TYPE_BUNDLE);
- if (!bundle_group) {
- LOG(LS_WARNING) << "max-bundle specified without BUNDLE specified";
- return false;
- }
- if (!EnableBundle(*bundle_group)) {
- LOG(LS_WARNING) << "max-bundle failed to enable bundling.";
- return false;
- }
- }
-
return true;
}
-bool WebRtcSession::CreateVoiceChannel(const cricket::ContentInfo* content) {
+bool WebRtcSession::CreateVoiceChannel(const cricket::ContentInfo* content,
+ const std::string* bundle_transport) {
+ bool require_rtcp_mux =
+ rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire;
+ bool create_rtcp_transport_channel = !require_rtcp_mux;
voice_channel_.reset(channel_manager_->CreateVoiceChannel(
- media_controller_, transport_controller_.get(), content->name, true,
- audio_options_));
+ media_controller_, transport_controller_.get(), content->name,
+ bundle_transport, create_rtcp_transport_channel, audio_options_));
if (!voice_channel_) {
return false;
}
+ if (require_rtcp_mux) {
+ voice_channel_->ActivateRtcpMux();
+ }
voice_channel_->SignalDtlsSetupFailure.connect(
this, &WebRtcSession::OnDtlsSetupFailure);
@@ -1814,14 +1824,20 @@
return true;
}
-bool WebRtcSession::CreateVideoChannel(const cricket::ContentInfo* content) {
+bool WebRtcSession::CreateVideoChannel(const cricket::ContentInfo* content,
+ const std::string* bundle_transport) {
+ bool require_rtcp_mux =
+ rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire;
+ bool create_rtcp_transport_channel = !require_rtcp_mux;
video_channel_.reset(channel_manager_->CreateVideoChannel(
- media_controller_, transport_controller_.get(), content->name, true,
- video_options_));
+ media_controller_, transport_controller_.get(), content->name,
+ bundle_transport, create_rtcp_transport_channel, video_options_));
if (!video_channel_) {
return false;
}
-
+ if (require_rtcp_mux) {
+ video_channel_->ActivateRtcpMux();
+ }
video_channel_->SignalDtlsSetupFailure.connect(
this, &WebRtcSession::OnDtlsSetupFailure);
@@ -1831,13 +1847,21 @@
return true;
}
-bool WebRtcSession::CreateDataChannel(const cricket::ContentInfo* content) {
+bool WebRtcSession::CreateDataChannel(const cricket::ContentInfo* content,
+ const std::string* bundle_transport) {
bool sctp = (data_channel_type_ == cricket::DCT_SCTP);
+ bool require_rtcp_mux =
+ rtcp_mux_policy_ == PeerConnectionInterface::kRtcpMuxPolicyRequire;
+ bool create_rtcp_transport_channel = !sctp && !require_rtcp_mux;
data_channel_.reset(channel_manager_->CreateDataChannel(
- transport_controller_.get(), content->name, !sctp, data_channel_type_));
+ transport_controller_.get(), content->name, bundle_transport,
+ create_rtcp_transport_channel, data_channel_type_));
if (!data_channel_) {
return false;
}
+ if (require_rtcp_mux) {
+ data_channel_->ActivateRtcpMux();
+ }
if (sctp) {
data_channel_->SignalDataReceived.connect(
@@ -1950,6 +1974,9 @@
return BadSdp(source, type, kBundleWithoutRtcpMux, err_desc);
}
+ // TODO(skvlad): When the local rtcp-mux policy is Require, reject any
+ // m-lines that do not rtcp-mux enabled.
+
// Verify m-lines in Answer when compared against Offer.
if (action == kAnswer) {
const cricket::SessionDescription* offer_desc =
diff --git a/webrtc/api/webrtcsession.h b/webrtc/api/webrtcsession.h
index a97bd73..98217bf 100644
--- a/webrtc/api/webrtcsession.h
+++ b/webrtc/api/webrtcsession.h
@@ -386,6 +386,12 @@
const std::string& content_name,
cricket::TransportDescription* info);
+ // Returns the name of the transport channel when BUNDLE is enabled, or
+ // nullptr if the channel is not part of any bundle.
+ const std::string* GetBundleTransportName(
+ const cricket::ContentInfo* content,
+ const cricket::ContentGroup* bundle);
+
// Cause all the BaseChannels in the bundle group to have the same
// transport channel.
bool EnableBundle(const cricket::ContentGroup& bundle);
@@ -412,9 +418,12 @@
bool CreateChannels(const cricket::SessionDescription* desc);
// Helper methods to create media channels.
- bool CreateVoiceChannel(const cricket::ContentInfo* content);
- bool CreateVideoChannel(const cricket::ContentInfo* content);
- bool CreateDataChannel(const cricket::ContentInfo* content);
+ bool CreateVoiceChannel(const cricket::ContentInfo* content,
+ const std::string* bundle_transport);
+ bool CreateVideoChannel(const cricket::ContentInfo* content,
+ const std::string* bundle_transport);
+ bool CreateDataChannel(const cricket::ContentInfo* content,
+ const std::string* bundle_transport);
// Listens to SCTP CONTROL messages on unused SIDs and process them as OPEN
// messages.
diff --git a/webrtc/api/webrtcsession_unittest.cc b/webrtc/api/webrtcsession_unittest.cc
index ea8b3c5..4207c24 100644
--- a/webrtc/api/webrtcsession_unittest.cc
+++ b/webrtc/api/webrtcsession_unittest.cc
@@ -163,6 +163,7 @@
void OnIceConnectionChange(
PeerConnectionInterface::IceConnectionState new_state) override {
ice_connection_state_ = new_state;
+ ice_connection_state_history_.push_back(new_state);
}
void OnIceGatheringChange(
PeerConnectionInterface::IceGatheringState new_state) override {
@@ -202,6 +203,8 @@
std::vector<cricket::Candidate> mline_1_candidates_;
PeerConnectionInterface::IceConnectionState ice_connection_state_;
PeerConnectionInterface::IceGatheringState ice_gathering_state_;
+ std::vector<PeerConnectionInterface::IceConnectionState>
+ ice_connection_state_history_;
size_t num_candidates_removed_ = 0;
};
@@ -3263,6 +3266,60 @@
session_->video_rtp_transport_channel());
}
+// Adding a new channel to a BUNDLE which is already connected should directly
+// assign the bundle transport to the channel, without first setting a
+// disconnected non-bundle transport and then replacing it. The application
+// should not receive any changes in the ICE state.
+TEST_F(WebRtcSessionTest, TestAddChannelToConnectedBundle) {
+ LoopbackNetworkConfiguration config;
+ LoopbackNetworkManager loopback_network_manager(this, config);
+ // Both BUNDLE and RTCP-mux need to be enabled for the ICE state to remain
+ // connected. Disabling either of these two means that we need to wait for the
+ // answer to find out if more transports are needed.
+ configuration_.bundle_policy =
+ PeerConnectionInterface::kBundlePolicyMaxBundle;
+ configuration_.rtcp_mux_policy =
+ PeerConnectionInterface::kRtcpMuxPolicyRequire;
+ options_.disable_encryption = true;
+ Init();
+
+ // Negotiate an audio channel with MAX_BUNDLE enabled.
+ SendAudioOnlyStream2();
+ SessionDescriptionInterface* offer = CreateOffer();
+ SetLocalDescriptionWithoutError(offer);
+ EXPECT_EQ_WAIT(PeerConnectionInterface::kIceGatheringComplete,
+ observer_.ice_gathering_state_, kIceCandidatesTimeout);
+ std::string sdp;
+ offer->ToString(&sdp);
+ SessionDescriptionInterface* answer = webrtc::CreateSessionDescription(
+ JsepSessionDescription::kAnswer, sdp, nullptr);
+ ASSERT_TRUE(answer != NULL);
+ SetRemoteDescriptionWithoutError(answer);
+
+ // Wait for the ICE state to stabilize.
+ EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
+ observer_.ice_connection_state_, kIceCandidatesTimeout);
+ observer_.ice_connection_state_history_.clear();
+
+ // Now add a video channel which should be using the same bundle transport.
+ SendAudioVideoStream2();
+ offer = CreateOffer();
+ offer->ToString(&sdp);
+ SetLocalDescriptionWithoutError(offer);
+ answer = webrtc::CreateSessionDescription(JsepSessionDescription::kAnswer,
+ sdp, nullptr);
+ ASSERT_TRUE(answer != NULL);
+ SetRemoteDescriptionWithoutError(answer);
+
+ // Wait for ICE state to stabilize
+ rtc::Thread::Current()->ProcessMessages(0);
+ EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
+ observer_.ice_connection_state_, kIceCandidatesTimeout);
+
+ // No ICE state changes are expected to happen.
+ EXPECT_EQ(0, observer_.ice_connection_state_history_.size());
+}
+
TEST_F(WebRtcSessionTest, TestRequireRtcpMux) {
InitWithRtcpMuxPolicy(PeerConnectionInterface::kRtcpMuxPolicyRequire);
SendAudioVideoStream1();