[Perfect Negotiation] Fire onnegotiationneeded when chain is empty.
This CL generates "negotiationneeded" events if negotiation is needed
when the Operations Chain becomes empty. This is only implemented in
Unified Plan to avoid Plan B regressions (the event is pretty useless
in Plan B as it fires repeatedly).
In order to implement the spec-compliant behavior of only firing the
event when the chain is empty, this CL introduces
PeerConnectionObserver::OnNegotiationNeededEvent() and
PeerConnectionInterface::ShouldFireNegotiationNeededEvent() to allow
validating the event before firing it. This is needed because the event
must not be fired until a task has been posted and subsequently chained
operations could invalidate it in the meantime.
Test coverage is added for both legacy and modern "negotiationneeded"
events.
Bug: chromium:1060083
Change-Id: I1dbaa8f6ddb1c6e7c8abd8da3b92efcb64060383
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/180620
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31989}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 6a5ad95..34e638f 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -1038,7 +1038,14 @@
call_ptr_(call_.get()),
local_ice_credentials_to_replace_(new LocalIceCredentialsToReplace()),
data_channel_controller_(this),
- weak_ptr_factory_(this) {}
+ weak_ptr_factory_(this) {
+ operations_chain_->SetOnChainEmptyCallback(
+ [this_weak_ptr = weak_ptr_factory_.GetWeakPtr()]() {
+ if (!this_weak_ptr)
+ return;
+ this_weak_ptr->OnOperationsChainEmpty();
+ });
+}
PeerConnection::~PeerConnection() {
TRACE_EVENT0("webrtc", "PeerConnection::~PeerConnection");
@@ -2638,18 +2645,24 @@
ReportNegotiatedSdpSemantics(*local_description());
}
+ observer->OnSetLocalDescriptionComplete(RTCError::OK());
+ NoteUsageEvent(UsageEvent::SET_LOCAL_DESCRIPTION_SUCCEEDED);
+
+ // Check if negotiation is needed. We must do this after informing the
+ // observer that SetLocalDescription() has completed to ensure negotiation is
+ // not needed prior to the promise resolving.
if (IsUnifiedPlan()) {
bool was_negotiation_needed = is_negotiation_needed_;
UpdateNegotiationNeeded();
if (signaling_state() == kStable && was_negotiation_needed &&
is_negotiation_needed_) {
+ // Legacy version.
Observer()->OnRenegotiationNeeded();
+ // Spec-compliant version; the event may get invalidated before firing.
+ GenerateNegotiationNeededEvent();
}
}
- observer->OnSetLocalDescriptionComplete(RTCError::OK());
- NoteUsageEvent(UsageEvent::SET_LOCAL_DESCRIPTION_SUCCEEDED);
-
// MaybeStartGathering needs to be called after informing the observer so that
// we don't signal any candidates before signaling that SetLocalDescription
// completed.
@@ -3098,17 +3111,23 @@
ReportNegotiatedSdpSemantics(*remote_description());
}
+ observer->OnSetRemoteDescriptionComplete(RTCError::OK());
+ NoteUsageEvent(UsageEvent::SET_REMOTE_DESCRIPTION_SUCCEEDED);
+
+ // Check if negotiation is needed. We must do this after informing the
+ // observer that SetRemoteDescription() has completed to ensure negotiation is
+ // not needed prior to the promise resolving.
if (IsUnifiedPlan()) {
bool was_negotiation_needed = is_negotiation_needed_;
UpdateNegotiationNeeded();
if (signaling_state() == kStable && was_negotiation_needed &&
is_negotiation_needed_) {
+ // Legacy version.
Observer()->OnRenegotiationNeeded();
+ // Spec-compliant version; the event may get invalidated before firing.
+ GenerateNegotiationNeededEvent();
}
}
-
- observer->OnSetRemoteDescriptionComplete(RTCError::OK());
- NoteUsageEvent(UsageEvent::SET_REMOTE_DESCRIPTION_SUCCEEDED);
}
RTCError PeerConnection::ApplyRemoteDescription(
@@ -7310,9 +7329,22 @@
RTC_DCHECK_RUN_ON(signaling_thread());
if (!IsUnifiedPlan()) {
Observer()->OnRenegotiationNeeded();
+ GenerateNegotiationNeededEvent();
return;
}
+ // In the spec, a task is queued here to run the following steps - this is
+ // meant to ensure we do not fire onnegotiationneeded prematurely if multiple
+ // changes are being made at once. In order to support Chromium's
+ // implementation where the JavaScript representation of the PeerConnection
+ // lives on a separate thread though, the queuing of a task is instead
+ // performed by the PeerConnectionObserver posting from the signaling thread
+ // to the JavaScript main thread that negotiation is needed. And because the
+ // Operations Chain lives on the WebRTC signaling thread,
+ // ShouldFireNegotiationNeededEvent() must be called before firing the event
+ // to ensure the Operations Chain is still empty and the event has not been
+ // invalidated.
+
// If connection's [[IsClosed]] slot is true, abort these steps.
if (IsClosed())
return;
@@ -7331,6 +7363,9 @@
bool is_negotiation_needed = CheckIfNegotiationIsNeeded();
if (!is_negotiation_needed) {
is_negotiation_needed_ = false;
+ // Invalidate any negotiation needed event that may previosuly have been
+ // generated.
+ ++negotiation_needed_event_id_;
return;
}
@@ -7347,6 +7382,10 @@
// If connection's [[NegotiationNeeded]] slot is false, abort these steps.
// Fire an event named negotiationneeded at connection.
Observer()->OnRenegotiationNeeded();
+ // Fire the spec-compliant version; when ShouldFireNegotiationNeededEvent() is
+ // used in the task queued by the observer, this event will only fire when the
+ // chain is empty.
+ GenerateNegotiationNeededEvent();
}
bool PeerConnection::CheckIfNegotiationIsNeeded() {
@@ -7488,6 +7527,59 @@
return false;
}
+void PeerConnection::OnOperationsChainEmpty() {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ if (IsClosed() || !update_negotiation_needed_on_empty_chain_)
+ return;
+ update_negotiation_needed_on_empty_chain_ = false;
+ // Firing when chain is empty is only supported in Unified Plan to avoid Plan
+ // B regressions. (In Plan B, onnegotiationneeded is already broken anyway, so
+ // firing it even more might just be confusing.)
+ if (IsUnifiedPlan()) {
+ UpdateNegotiationNeeded();
+ }
+}
+
+bool PeerConnection::ShouldFireNegotiationNeededEvent(uint32_t event_id) {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ // Plan B? Always fire to conform with useless legacy behavior.
+ if (!IsUnifiedPlan()) {
+ return true;
+ }
+ // The event ID has been invalidated. Either negotiation is no longer needed
+ // or a newer negotiation needed event has been generated.
+ if (event_id != negotiation_needed_event_id_) {
+ return false;
+ }
+ // The chain is no longer empty, update negotiation needed when it becomes
+ // empty. This should generate a newer negotiation needed event, making this
+ // one obsolete.
+ if (!operations_chain_->IsEmpty()) {
+ // Since we just suppressed an event that would have been fired, if
+ // negotiation is still needed by the time the chain becomes empty again, we
+ // must make sure to generate another event if negotiation is needed then.
+ // This happens when |is_negotiation_needed_| goes from false to true, so we
+ // set it to false until UpdateNegotiationNeeded() is called.
+ is_negotiation_needed_ = false;
+ update_negotiation_needed_on_empty_chain_ = true;
+ return false;
+ }
+ // We must not fire if the signaling state is no longer "stable". If
+ // negotiation is still needed when we return to "stable", a new negotiation
+ // needed event will be generated, so this one can safely be suppressed.
+ if (signaling_state_ != PeerConnectionInterface::kStable) {
+ return false;
+ }
+ // All checks have passed - please fire "negotiationneeded" now!
+ return true;
+}
+
+void PeerConnection::GenerateNegotiationNeededEvent() {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ ++negotiation_needed_event_id_;
+ Observer()->OnNegotiationNeededEvent(negotiation_needed_event_id_);
+}
+
RTCError PeerConnection::Rollback(SdpType sdp_type) {
auto state = signaling_state();
if (state != PeerConnectionInterface::kHaveLocalOffer &&
@@ -7574,7 +7666,10 @@
if (sdp_type == SdpType::kRollback) {
UpdateNegotiationNeeded();
if (is_negotiation_needed_) {
+ // Legacy version.
Observer()->OnRenegotiationNeeded();
+ // Spec-compliant version; the event may get invalidated before firing.
+ GenerateNegotiationNeededEvent();
}
}
return RTCError::OK();
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 41cb68c..d33c658 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -331,6 +331,8 @@
// Handler for the "channel closed" signal
void OnSctpDataChannelClosed(DataChannelInterface* channel);
+ bool ShouldFireNegotiationNeededEvent(uint32_t event_id) override;
+
// Functions made public for testing.
void ReturnHistogramVeryQuicklyForTesting() {
RTC_DCHECK_RUN_ON(signaling_thread());
@@ -1134,6 +1136,8 @@
void UpdateNegotiationNeeded();
bool CheckIfNegotiationIsNeeded();
+ void OnOperationsChainEmpty();
+ void GenerateNegotiationNeededEvent();
// | sdp_type | is the type of the SDP that caused the rollback.
RTCError Rollback(SdpType sdp_type);
@@ -1332,6 +1336,9 @@
std::unique_ptr<LocalIceCredentialsToReplace>
local_ice_credentials_to_replace_ RTC_GUARDED_BY(signaling_thread());
bool is_negotiation_needed_ RTC_GUARDED_BY(signaling_thread()) = false;
+ bool update_negotiation_needed_on_empty_chain_
+ RTC_GUARDED_BY(signaling_thread()) = false;
+ uint32_t negotiation_needed_event_id_ = 0;
DataChannelController data_channel_controller_;
rtc::WeakPtrFactory<PeerConnection> weak_ptr_factory_
diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc
index 8502dd4..8c1a764 100644
--- a/pc/peer_connection_ice_unittest.cc
+++ b/pc/peer_connection_ice_unittest.cc
@@ -1041,9 +1041,11 @@
auto callee = CreatePeerConnectionWithAudioVideo();
ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
caller->pc()->RestartIce();
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
}
// In Unified Plan, "onnegotiationneeded" is spec-compliant, including not
@@ -1064,14 +1066,17 @@
ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
// ICE restart becomes needed while an O/A is pending and |caller| is the
// offerer.
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
caller->pc()->RestartIce();
// In Unified Plan, the event should not fire until we are back in the stable
// signaling state.
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
ASSERT_TRUE(
caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
}
TEST_F(PeerConnectionIceTestUnifiedPlan,
@@ -1084,14 +1089,17 @@
ASSERT_TRUE(caller->SetRemoteDescription(callee->CreateOfferAndSetAsLocal()));
// ICE restart becomes needed while an O/A is pending and |caller| is the
// answerer.
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
caller->pc()->RestartIce();
// In Unified Plan, the event should not fire until we are back in the stable
// signaling state.
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
ASSERT_TRUE(
callee->SetRemoteDescription(caller->CreateAnswerAndSetAsLocal()));
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
}
TEST_F(PeerConnectionIceTestUnifiedPlan,
@@ -1102,14 +1110,16 @@
ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
// Local restart.
caller->pc()->RestartIce();
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
// Remote restart and O/A exchange with |caller| as the answerer should
// restart ICE locally as well.
callee->pc()->RestartIce();
ASSERT_TRUE(callee->ExchangeOfferAnswerWith(caller.get()));
// Having restarted ICE by the remote offer, we do not need to renegotiate ICE
// credentials when back in the stable signaling state.
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
}
TEST_F(PeerConnectionIceTestUnifiedPlan,
@@ -1119,10 +1129,13 @@
ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
caller->pc()->RestartIce();
- EXPECT_TRUE(caller->observer()->negotiation_needed());
- caller->observer()->clear_negotiation_needed();
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
caller->pc()->RestartIce();
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
}
// In Plan B, "onnegotiationneeded" is not spec-compliant, firing based on if
@@ -1140,15 +1153,19 @@
auto callee = CreatePeerConnectionWithAudioVideo();
ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
caller->pc()->RestartIce();
- EXPECT_TRUE(caller->observer()->negotiation_needed());
- caller->observer()->clear_negotiation_needed();
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
ASSERT_TRUE(
caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
// In Plan B, the event fired early so we don't expect it to fire now. This is
// not spec-compliant but follows the pattern of existing Plan B behavior.
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
}
TEST_F(PeerConnectionIceTestPlanB,
@@ -1157,15 +1174,19 @@
auto callee = CreatePeerConnectionWithAudioVideo();
ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
caller->pc()->RestartIce();
- EXPECT_TRUE(caller->observer()->negotiation_needed());
- caller->observer()->clear_negotiation_needed();
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
caller->pc()->RestartIce();
// In Plan B, the event fires every time something changed, even if we have
// already fired the event. This is not spec-compliant but follows the same
// pattern of existing Plan B behavior.
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
}
// The following parameterized test verifies that if an offer is sent with a
diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc
index c4b7de1..b7c0759 100644
--- a/pc/peer_connection_jsep_unittest.cc
+++ b/pc/peer_connection_jsep_unittest.cc
@@ -1791,7 +1791,8 @@
EXPECT_EQ(callee->signaling_state(),
PeerConnectionInterface::kHaveRemoteOffer);
EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal());
- EXPECT_FALSE(callee->observer()->negotiation_needed());
+ EXPECT_FALSE(callee->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(callee->observer()->has_negotiation_needed_event());
}
TEST_F(PeerConnectionJsepTest, RollbackImplicitlyNegotatiationNotNeeded) {
@@ -1803,13 +1804,15 @@
caller->AddAudioTrack("a");
callee->AddAudioTrack("b");
EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
- callee->observer()->clear_negotiation_needed();
+ callee->observer()->clear_legacy_renegotiation_needed();
+ callee->observer()->clear_latest_negotiation_needed_event();
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_FALSE(callee->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(callee->observer()->has_negotiation_needed_event());
EXPECT_EQ(callee->observer()->remove_track_events_.size(), 0u);
}
@@ -1821,13 +1824,16 @@
auto callee = CreatePeerConnection(config);
callee->AddAudioTrack("a");
EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
- callee->observer()->clear_negotiation_needed();
+ callee->observer()->clear_legacy_renegotiation_needed();
+ callee->observer()->clear_latest_negotiation_needed_event();
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
EXPECT_EQ(callee->signaling_state(),
PeerConnectionInterface::kHaveRemoteOffer);
- EXPECT_FALSE(callee->observer()->negotiation_needed());
+ EXPECT_FALSE(callee->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(callee->observer()->has_negotiation_needed_event());
EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal());
- EXPECT_TRUE(callee->observer()->negotiation_needed());
+ EXPECT_TRUE(callee->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(callee->observer()->has_negotiation_needed_event());
EXPECT_EQ(callee->observer()->remove_track_events_.size(), 0u);
}
@@ -1938,7 +1944,8 @@
EXPECT_TRUE(
caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
// In stable don't add or remove anything.
- callee->observer()->clear_negotiation_needed();
+ callee->observer()->clear_legacy_renegotiation_needed();
+ callee->observer()->clear_latest_negotiation_needed_event();
size_t transceiver_count = callee->pc()->GetTransceivers().size();
auto mid_0 = callee->pc()->GetTransceivers()[0]->mid();
auto mid_1 = callee->pc()->GetTransceivers()[1]->mid();
@@ -1948,7 +1955,8 @@
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());
+ EXPECT_FALSE(callee->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(callee->observer()->has_negotiation_needed_event());
}
TEST_F(PeerConnectionJsepTest, ImplicitlyRollbackTransceiversWithSameMids) {
@@ -2083,9 +2091,11 @@
EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
callee->AddTransceiver(cricket::MEDIA_TYPE_VIDEO);
EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
- callee->observer()->clear_negotiation_needed();
+ callee->observer()->clear_legacy_renegotiation_needed();
+ callee->observer()->clear_latest_negotiation_needed_event();
EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback()));
- EXPECT_TRUE(callee->observer()->negotiation_needed());
+ EXPECT_TRUE(callee->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(callee->observer()->has_negotiation_needed_event());
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);
@@ -2134,7 +2144,8 @@
EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal());
- EXPECT_TRUE(callee->observer()->negotiation_needed());
+ EXPECT_TRUE(callee->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(callee->observer()->has_negotiation_needed_event());
EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
}
diff --git a/pc/peer_connection_rtp_unittest.cc b/pc/peer_connection_rtp_unittest.cc
index e69a088..152a12e 100644
--- a/pc/peer_connection_rtp_unittest.cc
+++ b/pc/peer_connection_rtp_unittest.cc
@@ -1145,12 +1145,15 @@
RtpTransceiverInit init;
init.direction = RtpTransceiverDirection::kInactive;
auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init);
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
ASSERT_TRUE(caller->AddAudioTrack("a"));
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
EXPECT_EQ(RtpTransceiverDirection::kSendOnly, transceiver->direction());
}
@@ -1165,12 +1168,15 @@
RtpTransceiverInit init;
init.direction = RtpTransceiverDirection::kRecvOnly;
auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, init);
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
ASSERT_TRUE(caller->AddAudioTrack("a"));
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
EXPECT_EQ(RtpTransceiverDirection::kSendRecv, transceiver->direction());
}
@@ -1194,10 +1200,12 @@
auto audio_track = caller->CreateAudioTrack("a");
caller->pc()->Close();
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
auto result = caller->pc()->AddTrack(audio_track, std::vector<std::string>());
EXPECT_EQ(RTCErrorType::INVALID_STATE, result.error().type());
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
}
TEST_F(PeerConnectionRtpTestUnifiedPlan, AddTrackErrorIfTrackAlreadyHasSender) {
@@ -1206,10 +1214,12 @@
auto audio_track = caller->CreateAudioTrack("a");
ASSERT_TRUE(caller->AddTrack(audio_track));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
auto result = caller->pc()->AddTrack(audio_track, std::vector<std::string>());
EXPECT_EQ(RTCErrorType::INVALID_PARAMETER, result.error().type());
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
}
// Unified Plan RemoveTrack tests.
@@ -1236,13 +1246,16 @@
init.direction = RtpTransceiverDirection::kSendRecv;
auto transceiver =
caller->AddTransceiver(caller->CreateAudioTrack("a"), init);
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
ASSERT_TRUE(caller->pc()->RemoveTrack(transceiver->sender()));
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
EXPECT_EQ(RtpTransceiverDirection::kRecvOnly, transceiver->direction());
}
@@ -1258,13 +1271,16 @@
init.direction = RtpTransceiverDirection::kSendOnly;
auto transceiver =
caller->AddTransceiver(caller->CreateAudioTrack("a"), init);
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
ASSERT_TRUE(caller->pc()->RemoveTrack(transceiver->sender()));
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
EXPECT_EQ(RtpTransceiverDirection::kInactive, transceiver->direction());
}
@@ -1278,9 +1294,11 @@
auto transceiver = caller->pc()->GetTransceivers()[0];
ASSERT_TRUE(sender->SetTrack(nullptr));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
ASSERT_TRUE(caller->pc()->RemoveTrack(sender));
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
EXPECT_EQ(RtpTransceiverDirection::kSendRecv, transceiver->direction());
}
@@ -1293,9 +1311,11 @@
auto sender = caller->AddAudioTrack("a");
caller->pc()->Close();
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
EXPECT_FALSE(caller->pc()->RemoveTrack(sender));
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
}
TEST_F(PeerConnectionRtpTestUnifiedPlan,
@@ -1305,9 +1325,11 @@
auto sender = caller->AddAudioTrack("a");
ASSERT_TRUE(caller->pc()->RemoveTrack(sender));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
EXPECT_TRUE(caller->pc()->RemoveTrack(sender));
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
}
// Test that setting offers that add/remove/add a track repeatedly without
@@ -1413,16 +1435,20 @@
RenegotiationNeededAfterTransceiverSetDirection) {
auto caller = CreatePeerConnection();
auto callee = CreatePeerConnection();
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive);
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
}
// Test that OnRenegotiationNeeded is not fired if SetDirection is called on an
@@ -1433,9 +1459,11 @@
auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
transceiver->SetDirectionWithError(transceiver->direction());
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
}
// Test that OnRenegotiationNeeded is not fired if SetDirection is called on a
@@ -1447,9 +1475,11 @@
auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
transceiver->StopInternal();
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
transceiver->SetDirectionWithError(RtpTransceiverDirection::kInactive);
- EXPECT_FALSE(caller->observer()->negotiation_needed());
+ EXPECT_FALSE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
}
// Test that currentDirection returnes "stopped" if the transceiver was stopped.
@@ -1829,13 +1859,16 @@
init.direction = RtpTransceiverDirection::kSendRecv;
auto transceiver =
caller->AddTransceiver(caller->CreateAudioTrack("a"), init);
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
- caller->observer()->clear_negotiation_needed();
+ caller->observer()->clear_legacy_renegotiation_needed();
+ caller->observer()->clear_latest_negotiation_needed_event();
transceiver->sender()->SetStreams({"stream3", "stream4", "stream5"});
- EXPECT_TRUE(caller->observer()->negotiation_needed());
+ EXPECT_TRUE(caller->observer()->legacy_renegotiation_needed());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
auto callee_streams = callee->pc()->GetReceivers()[0]->streams();
diff --git a/pc/peer_connection_signaling_unittest.cc b/pc/peer_connection_signaling_unittest.cc
index 72cbffb..9a89fce 100644
--- a/pc/peer_connection_signaling_unittest.cc
+++ b/pc/peer_connection_signaling_unittest.cc
@@ -976,4 +976,68 @@
ASSERT_EQ(SignalingState::kStable, caller->signaling_state());
}
+TEST_F(PeerConnectionSignalingUnifiedPlanTest,
+ ShouldFireNegotiationNeededWhenNoChangesArePending) {
+ auto caller = CreatePeerConnection();
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
+ auto transceiver =
+ caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, RtpTransceiverInit());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
+ EXPECT_TRUE(caller->pc()->ShouldFireNegotiationNeededEvent(
+ caller->observer()->latest_negotiation_needed_event()));
+}
+
+TEST_F(PeerConnectionSignalingUnifiedPlanTest,
+ SuppressNegotiationNeededWhenOperationChainIsNotEmpty) {
+ auto caller = CreatePeerConnection();
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
+ auto transceiver =
+ caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, RtpTransceiverInit());
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
+
+ rtc::scoped_refptr<MockCreateSessionDescriptionObserver> observer =
+ new rtc::RefCountedObject<MockCreateSessionDescriptionObserver>();
+ caller->pc()->CreateOffer(observer, RTCOfferAnswerOptions());
+ // For this test to work, the operation has to be pending, i.e. the observer
+ // has not yet been invoked.
+ EXPECT_FALSE(observer->called());
+ // Because the Operations Chain is not empty, the event is now suppressed.
+ EXPECT_FALSE(caller->pc()->ShouldFireNegotiationNeededEvent(
+ caller->observer()->latest_negotiation_needed_event()));
+ caller->observer()->clear_latest_negotiation_needed_event();
+
+ // When the Operations Chain becomes empty again, a new negotiation needed
+ // event will be generated that is not suppressed.
+ EXPECT_TRUE_WAIT(observer->called(), kWaitTimeout);
+ EXPECT_TRUE(caller->observer()->has_negotiation_needed_event());
+ EXPECT_TRUE(caller->pc()->ShouldFireNegotiationNeededEvent(
+ caller->observer()->latest_negotiation_needed_event()));
+}
+
+TEST_F(PeerConnectionSignalingUnifiedPlanTest,
+ SuppressNegotiationNeededWhenSignalingStateIsNotStable) {
+ auto caller = CreatePeerConnection();
+ auto callee = CreatePeerConnection();
+ auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+ EXPECT_FALSE(caller->observer()->has_negotiation_needed_event());
+ auto transceiver =
+ callee->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, RtpTransceiverInit());
+ EXPECT_TRUE(callee->observer()->has_negotiation_needed_event());
+
+ // Change signaling state (to "have-remote-offer") by setting a remote offer.
+ callee->SetRemoteDescription(std::move(offer));
+ // Because the signaling state is not "stable", the event is now suppressed.
+ EXPECT_FALSE(callee->pc()->ShouldFireNegotiationNeededEvent(
+ callee->observer()->latest_negotiation_needed_event()));
+ callee->observer()->clear_latest_negotiation_needed_event();
+
+ // Upon rolling back to "stable", a new negotiation needed event will be
+ // generated that is not suppressed.
+ callee->SetLocalDescription(CreateSessionDescription(SdpType::kRollback, ""));
+ EXPECT_TRUE(callee->observer()->has_negotiation_needed_event());
+ EXPECT_TRUE(callee->pc()->ShouldFireNegotiationNeededEvent(
+ callee->observer()->latest_negotiation_needed_event()));
+}
+
} // namespace webrtc
diff --git a/pc/test/mock_peer_connection_observers.h b/pc/test/mock_peer_connection_observers.h
index 0a83571..7766297 100644
--- a/pc/test/mock_peer_connection_observers.h
+++ b/pc/test/mock_peer_connection_observers.h
@@ -85,6 +85,9 @@
remote_streams_->RemoveStream(stream);
}
void OnRenegotiationNeeded() override { renegotiation_needed_ = true; }
+ void OnNegotiationNeededEvent(uint32_t event_id) override {
+ latest_negotiation_needed_event_ = event_id;
+ }
void OnDataChannel(
rtc::scoped_refptr<DataChannelInterface> data_channel) override {
last_datachannel_ = data_channel;
@@ -214,8 +217,18 @@
return candidates;
}
- bool negotiation_needed() const { return renegotiation_needed_; }
- void clear_negotiation_needed() { renegotiation_needed_ = false; }
+ bool legacy_renegotiation_needed() const { return renegotiation_needed_; }
+ void clear_legacy_renegotiation_needed() { renegotiation_needed_ = false; }
+
+ bool has_negotiation_needed_event() {
+ return latest_negotiation_needed_event_.has_value();
+ }
+ uint32_t latest_negotiation_needed_event() {
+ return latest_negotiation_needed_event_.value_or(0u);
+ }
+ void clear_latest_negotiation_needed_event() {
+ latest_negotiation_needed_event_ = absl::nullopt;
+ }
rtc::scoped_refptr<PeerConnectionInterface> pc_;
PeerConnectionInterface::SignalingState state_;
@@ -223,6 +236,7 @@
rtc::scoped_refptr<DataChannelInterface> last_datachannel_;
rtc::scoped_refptr<StreamCollection> remote_streams_;
bool renegotiation_needed_ = false;
+ absl::optional<uint32_t> latest_negotiation_needed_event_;
bool ice_gathering_complete_ = false;
bool ice_connected_ = false;
bool callback_triggered_ = false;
