[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_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();