Add firing of OnRemoveTrack and OnRenegotationNeeded during rollback
Bug: chromium:980875
Change-Id: I71439cea4c79e4a8dae6488404b0c303a9c33a97
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/157581
Commit-Queue: Eldar Rello <elrello@microsoft.com>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29563}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index f019ec9..494a649 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -2264,7 +2264,7 @@
// For SLD we support only explicit rollback.
if (desc->GetType() == SdpType::kRollback) {
if (IsUnifiedPlan()) {
- RTCError error = Rollback();
+ RTCError error = Rollback(desc->GetType());
if (error.ok()) {
PostSetSessionDescriptionSuccess(observer);
} else {
@@ -2654,12 +2654,12 @@
if (configuration_.enable_implicit_rollback) {
if (desc->GetType() == SdpType::kOffer &&
signaling_state() == kHaveLocalOffer) {
- Rollback();
+ Rollback(desc->GetType());
}
}
// Explicit rollback.
if (desc->GetType() == SdpType::kRollback) {
- observer->OnSetRemoteDescriptionComplete(Rollback());
+ observer->OnSetRemoteDescriptionComplete(Rollback(desc->GetType()));
return;
}
} else if (desc->GetType() == SdpType::kRollback) {
@@ -7610,7 +7610,7 @@
return false;
}
-RTCError PeerConnection::Rollback() {
+RTCError PeerConnection::Rollback(SdpType sdp_type) {
auto state = signaling_state();
if (state != PeerConnectionInterface::kHaveLocalOffer &&
state != PeerConnectionInterface::kHaveRemoteOffer) {
@@ -7630,6 +7630,10 @@
transport_controller_->RollbackTransportForMid(mid);
DestroyTransceiverChannel(transceiver);
+ if (signaling_state() == PeerConnectionInterface::kHaveRemoteOffer &&
+ transceiver->receiver()) {
+ Observer()->OnRemoveTrack(transceiver->receiver());
+ }
if (state.newly_created()) {
// Remove added transceivers with no added track.
if (transceiver->internal()->sender()->track()) {
@@ -7654,6 +7658,15 @@
pending_local_description_.reset();
pending_remote_description_.reset();
ChangeSignalingState(PeerConnectionInterface::kStable);
+
+ // The assumption is that in case of implicit rollback UpdateNegotiationNeeded
+ // gets called in SetRemoteDescription.
+ if (sdp_type == SdpType::kRollback) {
+ UpdateNegotiationNeeded();
+ if (is_negotiation_needed_) {
+ Observer()->OnRenegotiationNeeded();
+ }
+ }
return RTCError::OK();
}
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 428c2e8..baaa14d 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -1193,7 +1193,9 @@
void UpdateNegotiationNeeded();
bool CheckIfNegotiationIsNeeded();
- RTCError Rollback();
+
+ // | sdp_type | is the type of the SDP that caused the rollback.
+ RTCError Rollback(SdpType sdp_type);
sigslot::signal1<DataChannel*> SignalDataChannelCreated_
RTC_GUARDED_BY(signaling_thread());
diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc
index 514374b..bb1039c 100644
--- a/pc/peer_connection_jsep_unittest.cc
+++ b/pc/peer_connection_jsep_unittest.cc
@@ -1786,7 +1786,7 @@
EXPECT_EQ(callee->pc()->pending_remote_description(), nullptr);
}
-TEST_F(PeerConnectionJsepTest, RollbackLocalOfferImplicitly) {
+TEST_F(PeerConnectionJsepTest, RollbackImplicitly) {
RTCConfiguration config;
config.sdp_semantics = SdpSemantics::kUnifiedPlan;
config.enable_implicit_rollback = true;
@@ -1796,9 +1796,48 @@
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
EXPECT_EQ(callee->signaling_state(),
PeerConnectionInterface::kHaveRemoteOffer);
+ EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal());
+ EXPECT_FALSE(callee->observer()->negotiation_needed());
}
-TEST_F(PeerConnectionJsepTest, AttemptToRollbackLocalOfferImplicitly) {
+TEST_F(PeerConnectionJsepTest, RollbackImplicitlyNegotatiationNotNeeded) {
+ RTCConfiguration config;
+ config.sdp_semantics = SdpSemantics::kUnifiedPlan;
+ config.enable_implicit_rollback = true;
+ auto caller = CreatePeerConnection(config);
+ auto callee = CreatePeerConnection(config);
+ caller->AddAudioTrack("a");
+ callee->AddAudioTrack("b");
+ EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
+ callee->observer()->clear_negotiation_needed();
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+ EXPECT_EQ(callee->signaling_state(),
+ PeerConnectionInterface::kHaveRemoteOffer);
+ EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal());
+ // No negotiation needed as track got attached in the answer.
+ EXPECT_FALSE(callee->observer()->negotiation_needed());
+ EXPECT_EQ(callee->observer()->remove_track_events_.size(), 0u);
+}
+
+TEST_F(PeerConnectionJsepTest, RollbackImplicitlyAndNegotiationNeeded) {
+ RTCConfiguration config;
+ config.sdp_semantics = SdpSemantics::kUnifiedPlan;
+ config.enable_implicit_rollback = true;
+ auto caller = CreatePeerConnection(config);
+ auto callee = CreatePeerConnection(config);
+ callee->AddAudioTrack("a");
+ EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
+ callee->observer()->clear_negotiation_needed();
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+ EXPECT_EQ(callee->signaling_state(),
+ PeerConnectionInterface::kHaveRemoteOffer);
+ EXPECT_FALSE(callee->observer()->negotiation_needed());
+ EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal());
+ EXPECT_TRUE(callee->observer()->negotiation_needed());
+ EXPECT_EQ(callee->observer()->remove_track_events_.size(), 0u);
+}
+
+TEST_F(PeerConnectionJsepTest, AttemptToRollbackImplicitly) {
RTCConfiguration config;
config.sdp_semantics = SdpSemantics::kUnifiedPlan;
config.enable_implicit_rollback = true;
@@ -1816,9 +1855,10 @@
caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
auto callee = CreatePeerConnection();
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
- EXPECT_EQ(callee->pc()->GetTransceivers().size(), size_t{1});
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateRollback()));
- EXPECT_EQ(callee->pc()->GetTransceivers().size(), size_t{0});
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 0u);
+ EXPECT_EQ(callee->observer()->remove_track_events_.size(), 1u);
}
TEST_F(PeerConnectionJsepTest, RollbackKeepsTransceiverAndClearsMid) {
@@ -1827,15 +1867,16 @@
auto callee = CreatePeerConnection();
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
callee->AddAudioTrack("a");
- EXPECT_EQ(callee->pc()->GetTransceivers().size(), size_t{1});
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateRollback()));
// Transceiver can't be removed as track was added to it.
- EXPECT_EQ(callee->pc()->GetTransceivers().size(), size_t{1});
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
// Mid got cleared to make it reusable.
EXPECT_EQ(callee->pc()->GetTransceivers()[0]->mid(), absl::nullopt);
// Transceiver should be counted as addTrack-created after rollback.
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
- EXPECT_EQ(callee->pc()->GetTransceivers().size(), size_t{1});
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
+ EXPECT_EQ(callee->observer()->remove_track_events_.size(), 1u);
}
TEST_F(PeerConnectionJsepTest, RollbackRestoresMid) {
@@ -1845,7 +1886,7 @@
callee->AddAudioTrack("a");
auto offer = callee->CreateOffer();
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
- EXPECT_EQ(callee->pc()->GetTransceivers().size(), size_t{1});
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
EXPECT_NE(callee->pc()->GetTransceivers()[0]->mid(), absl::nullopt);
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateRollback()));
EXPECT_EQ(callee->pc()->GetTransceivers()[0]->mid(), absl::nullopt);
@@ -1861,13 +1902,38 @@
caller->AddVideoTrack("c");
auto mid = callee->pc()->GetTransceivers()[0]->mid();
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
- EXPECT_EQ(callee->pc()->GetTransceivers().size(), size_t{2});
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 2u);
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateRollback()));
- EXPECT_EQ(callee->pc()->GetTransceivers().size(), size_t{1});
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
EXPECT_EQ(callee->pc()->GetTransceivers()[0]->mid(), mid);
EXPECT_EQ(callee->pc()->GetTransceivers()[0]->media_type(),
cricket::MEDIA_TYPE_VIDEO);
EXPECT_TRUE(callee->SetLocalDescription(std::move(offer)));
+ EXPECT_EQ(callee->observer()->remove_track_events_.size(),
+ callee->observer()->add_track_events_.size());
+}
+
+TEST_F(PeerConnectionJsepTest, RollbackHasNoEffectOnStableTransceivers) {
+ auto callee = CreatePeerConnection();
+ callee->AddVideoTrack("a");
+ auto caller = CreatePeerConnection();
+ caller->AddAudioTrack("b");
+ caller->AddVideoTrack("c");
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+ EXPECT_TRUE(
+ caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
+ // In stable don't add or remove anything.
+ callee->observer()->clear_negotiation_needed();
+ size_t transceiver_count = callee->pc()->GetTransceivers().size();
+ auto mid_0 = callee->pc()->GetTransceivers()[0]->mid();
+ auto mid_1 = callee->pc()->GetTransceivers()[1]->mid();
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateRollback()));
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), transceiver_count);
+ EXPECT_EQ(callee->pc()->GetTransceivers()[0]->mid(), mid_0);
+ EXPECT_EQ(callee->pc()->GetTransceivers()[1]->mid(), mid_1);
+ EXPECT_EQ(callee->observer()->remove_track_events_.size(), 0u);
+ EXPECT_FALSE(callee->observer()->negotiation_needed());
}
TEST_F(PeerConnectionJsepTest, ImplicitlyRollbackTransceiversWithSameMids) {
@@ -1881,7 +1947,7 @@
EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
auto initial_mid = callee->pc()->GetTransceivers()[0]->mid();
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
- EXPECT_EQ(callee->pc()->GetTransceivers().size(), size_t{2});
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 2u);
EXPECT_EQ(callee->pc()->GetTransceivers()[0]->mid(), absl::nullopt);
EXPECT_EQ(callee->pc()->GetTransceivers()[1]->mid(),
caller->pc()->GetTransceivers()[0]->mid());
@@ -1902,7 +1968,7 @@
caller->AddVideoTrack("a");
callee->AddVideoTrack("b");
EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
- EXPECT_EQ(callee->pc()->GetTransceivers().size(), size_t{2});
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 2u);
auto audio_transport =
callee->pc()->GetTransceivers()[0]->sender()->dtls_transport();
EXPECT_EQ(callee->pc()->GetTransceivers()[0]->sender()->dtls_transport(),
@@ -1920,14 +1986,68 @@
audio_transport); // Audio transport is still the same.
}
+TEST_F(PeerConnectionJsepTest, RollbackLocalDirectionChange) {
+ auto caller = CreatePeerConnection();
+ caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
+ auto callee = CreatePeerConnection();
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+ EXPECT_TRUE(
+ caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
+ callee->AddAudioTrack("a");
+ callee->pc()->GetTransceivers()[0]->SetDirection(
+ RtpTransceiverDirection::kSendOnly);
+ EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
+ auto audio_transport =
+ callee->pc()->GetTransceivers()[0]->receiver()->dtls_transport();
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateRollback()));
+ EXPECT_EQ(callee->pc()->GetTransceivers()[0]->direction(),
+ RtpTransceiverDirection::kSendOnly);
+ // One way audio must remain working after rollback as local direction change
+ // comes in effect after completing full negotiation round.
+ EXPECT_EQ(callee->pc()->GetTransceivers()[0]->receiver()->dtls_transport(),
+ audio_transport);
+}
+
+TEST_F(PeerConnectionJsepTest, RollbackRemoteDirectionChange) {
+ auto caller = CreatePeerConnection();
+ auto caller_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
+ auto callee = CreatePeerConnection();
+ callee->AddAudioTrack("a");
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+ EXPECT_TRUE(
+ caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
+ // In stable make remote audio receive only.
+ caller_transceiver->SetDirection(RtpTransceiverDirection::kRecvOnly);
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
+ // The direction attribute is not modified by the offer.
+ EXPECT_EQ(callee->pc()->GetTransceivers()[0]->direction(),
+ RtpTransceiverDirection::kSendRecv);
+ auto audio_transport =
+ callee->pc()->GetTransceivers()[0]->sender()->dtls_transport();
+ EXPECT_EQ(callee->observer()->add_track_events_.size(), 1u);
+ EXPECT_EQ(callee->observer()->remove_track_events_.size(), 1u);
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateRollback()));
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u);
+ EXPECT_EQ(callee->pc()->GetTransceivers()[0]->direction(),
+ RtpTransceiverDirection::kSendRecv);
+ // One way audio must remain working after rollback.
+ EXPECT_EQ(callee->pc()->GetTransceivers()[0]->sender()->dtls_transport(),
+ audio_transport);
+ EXPECT_EQ(callee->observer()->remove_track_events_.size(), 1u);
+}
+
TEST_F(PeerConnectionJsepTest, RollbackAfterMultipleSLD) {
auto callee = CreatePeerConnection();
callee->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
callee->AddTransceiver(cricket::MEDIA_TYPE_VIDEO);
EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
+ callee->observer()->clear_negotiation_needed();
EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback()));
- EXPECT_EQ(callee->pc()->GetTransceivers().size(), size_t{2});
+ EXPECT_TRUE(callee->observer()->negotiation_needed());
+ EXPECT_EQ(callee->pc()->GetTransceivers().size(), 2u);
EXPECT_EQ(callee->pc()->GetTransceivers()[0]->mid(), absl::nullopt);
EXPECT_EQ(callee->pc()->GetTransceivers()[1]->mid(), absl::nullopt);
}
