Reland "Parameterize PeerConnection signaling tests for Unified Plan"
Original change's description:
> Parameterize PeerConnection signaling tests for Unified Plan
>
> This also changes the behavior of CreateAnswer to fail unless
> the signaling state is kHaveRemoteOffer or kHaveLocalPranswer,
> as per the WebRTC specification.
>
> Bug: webrtc:8765
> Change-Id: I60ac67cd92b17fcbff964afc14d049481e816a28
> Reviewed-on: https://webrtc-review.googlesource.com/41042
> Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
> Commit-Queue: Steve Anton <steveanton@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#21779}
Bug: webrtc:8813
Change-Id: I9f608fcd0b7aca00b4c1092e271dbd9cd710c38a
Reviewed-on: https://webrtc-review.googlesource.com/46861
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21860}diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index ca97e81..b4908f5 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -1674,20 +1674,17 @@
return;
}
- if (IsClosed()) {
- std::string error = "CreateAnswer called when PeerConnection is closed.";
+ if (!(signaling_state_ == kHaveRemoteOffer ||
+ signaling_state_ == kHaveLocalPrAnswer)) {
+ std::string error =
+ "CreateAnswer called when PeerConnection is in a wrong state.";
RTC_LOG(LS_ERROR) << error;
PostCreateSessionDescriptionFailure(observer, error);
return;
}
- if (remote_description() &&
- remote_description()->GetType() != SdpType::kOffer) {
- std::string error = "CreateAnswer called without remote offer.";
- RTC_LOG(LS_ERROR) << error;
- PostCreateSessionDescriptionFailure(observer, error);
- return;
- }
+ // The remote description should be set if we're in the right state.
+ RTC_DCHECK(remote_description());
if (IsUnifiedPlan()) {
if (options.offer_to_receive_audio != RTCOfferAnswerOptions::kUndefined) {
@@ -3486,18 +3483,15 @@
rtc::Optional<size_t> audio_index;
rtc::Optional<size_t> video_index;
rtc::Optional<size_t> data_index;
- if (remote_description()) {
- // The pending remote description should be an offer.
- RTC_DCHECK(remote_description()->GetType() == SdpType::kOffer);
- // Generate m= sections that match those in the offer.
- // Note that mediasession.cc will handle intersection our preferred
- // direction with the offered direction.
- GenerateMediaDescriptionOptions(
- remote_description(),
- RtpTransceiverDirectionFromSendRecv(send_audio, recv_audio),
- RtpTransceiverDirectionFromSendRecv(send_video, recv_video),
- &audio_index, &video_index, &data_index, session_options);
- }
+
+ // Generate m= sections that match those in the offer.
+ // Note that mediasession.cc will handle intersection our preferred
+ // direction with the offered direction.
+ GenerateMediaDescriptionOptions(
+ remote_description(),
+ RtpTransceiverDirectionFromSendRecv(send_audio, recv_audio),
+ RtpTransceiverDirectionFromSendRecv(send_video, recv_video), &audio_index,
+ &video_index, &data_index, session_options);
cricket::MediaDescriptionOptions* audio_media_description_options =
!audio_index ? nullptr
diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc
index b6a8124..6f6bd73 100644
--- a/pc/peerconnection_ice_unittest.cc
+++ b/pc/peerconnection_ice_unittest.cc
@@ -303,7 +303,7 @@
EXPECT_TRUE_WAIT(callee->IsIceGatheringDone(), kIceCandidatesTimeout);
- auto answer = callee->CreateAnswer();
+ auto* answer = callee->pc()->local_description();
EXPECT_LT(0u, caller->observer()->GetCandidatesByMline(0).size());
EXPECT_EQ(callee->observer()->GetCandidatesByMline(0).size(),
answer->candidates(0)->count());
diff --git a/pc/peerconnection_signaling_unittest.cc b/pc/peerconnection_signaling_unittest.cc
index cd26f8b..2cb2fc7 100644
--- a/pc/peerconnection_signaling_unittest.cc
+++ b/pc/peerconnection_signaling_unittest.cc
@@ -55,12 +55,14 @@
}
};
-class PeerConnectionSignalingTest : public ::testing::Test {
+class PeerConnectionSignalingBaseTest : public ::testing::Test {
protected:
typedef std::unique_ptr<PeerConnectionWrapperForSignalingTest> WrapperPtr;
- PeerConnectionSignalingTest()
- : vss_(new rtc::VirtualSocketServer()), main_(vss_.get()) {
+ explicit PeerConnectionSignalingBaseTest(SdpSemantics sdp_semantics)
+ : vss_(new rtc::VirtualSocketServer()),
+ main_(vss_.get()),
+ sdp_semantics_(sdp_semantics) {
#ifdef WEBRTC_ANDROID
InitializeAndroidObjects();
#endif
@@ -76,8 +78,10 @@
WrapperPtr CreatePeerConnection(const RTCConfiguration& config) {
auto observer = rtc::MakeUnique<MockPeerConnectionObserver>();
- auto pc = pc_factory_->CreatePeerConnection(config, nullptr, nullptr,
- observer.get());
+ RTCConfiguration modified_config = config;
+ modified_config.sdp_semantics = sdp_semantics_;
+ auto pc = pc_factory_->CreatePeerConnection(modified_config, nullptr,
+ nullptr, observer.get());
if (!pc) {
return nullptr;
}
@@ -102,16 +106,24 @@
std::unique_ptr<rtc::VirtualSocketServer> vss_;
rtc::AutoSocketServerThread main_;
rtc::scoped_refptr<PeerConnectionFactoryInterface> pc_factory_;
+ const SdpSemantics sdp_semantics_;
};
-TEST_F(PeerConnectionSignalingTest, SetLocalOfferTwiceWorks) {
+class PeerConnectionSignalingTest
+ : public PeerConnectionSignalingBaseTest,
+ public ::testing::WithParamInterface<SdpSemantics> {
+ protected:
+ PeerConnectionSignalingTest() : PeerConnectionSignalingBaseTest(GetParam()) {}
+};
+
+TEST_P(PeerConnectionSignalingTest, SetLocalOfferTwiceWorks) {
auto caller = CreatePeerConnection();
EXPECT_TRUE(caller->SetLocalDescription(caller->CreateOffer()));
EXPECT_TRUE(caller->SetLocalDescription(caller->CreateOffer()));
}
-TEST_F(PeerConnectionSignalingTest, SetRemoteOfferTwiceWorks) {
+TEST_P(PeerConnectionSignalingTest, SetRemoteOfferTwiceWorks) {
auto caller = CreatePeerConnection();
auto callee = CreatePeerConnection();
@@ -119,14 +131,14 @@
EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
}
-TEST_F(PeerConnectionSignalingTest, FailToSetNullLocalDescription) {
+TEST_P(PeerConnectionSignalingTest, FailToSetNullLocalDescription) {
auto caller = CreatePeerConnection();
std::string error;
ASSERT_FALSE(caller->SetLocalDescription(nullptr, &error));
EXPECT_EQ("SessionDescription is NULL.", error);
}
-TEST_F(PeerConnectionSignalingTest, FailToSetNullRemoteDescription) {
+TEST_P(PeerConnectionSignalingTest, FailToSetNullRemoteDescription) {
auto caller = CreatePeerConnection();
std::string error;
ASSERT_FALSE(caller->SetRemoteDescription(nullptr, &error));
@@ -142,9 +154,15 @@
// state the PeerConnection was created in before it was closed.
class PeerConnectionSignalingStateTest
- : public PeerConnectionSignalingTest,
- public ::testing::WithParamInterface<std::tuple<SignalingState, bool>> {
+ : public PeerConnectionSignalingBaseTest,
+ public ::testing::WithParamInterface<
+ std::tuple<SdpSemantics, SignalingState, bool>> {
protected:
+ PeerConnectionSignalingStateTest()
+ : PeerConnectionSignalingBaseTest(std::get<0>(GetParam())),
+ state_under_test_(std::make_tuple(std::get<1>(GetParam()),
+ std::get<2>(GetParam()))) {}
+
RTCConfiguration GetConfig() {
RTCConfiguration config;
config.certificates.push_back(
@@ -152,6 +170,10 @@
return config;
}
+ WrapperPtr CreatePeerConnectionUnderTest() {
+ return CreatePeerConnectionInState(state_under_test_);
+ }
+
WrapperPtr CreatePeerConnectionInState(SignalingState state) {
return CreatePeerConnectionInState(std::make_tuple(state, false));
}
@@ -207,10 +229,12 @@
return wrapper;
}
+
+ std::tuple<SignalingState, bool> state_under_test_;
};
TEST_P(PeerConnectionSignalingStateTest, CreateOffer) {
- auto wrapper = CreatePeerConnectionInState(GetParam());
+ auto wrapper = CreatePeerConnectionUnderTest();
if (wrapper->signaling_state() != SignalingState::kClosed) {
EXPECT_TRUE(wrapper->CreateOffer());
} else {
@@ -222,30 +246,21 @@
}
TEST_P(PeerConnectionSignalingStateTest, CreateAnswer) {
- auto wrapper = CreatePeerConnectionInState(GetParam());
+ auto wrapper = CreatePeerConnectionUnderTest();
if (wrapper->signaling_state() == SignalingState::kHaveLocalPrAnswer ||
wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) {
EXPECT_TRUE(wrapper->CreateAnswer());
} else {
std::string error;
ASSERT_FALSE(wrapper->CreateAnswer(RTCOfferAnswerOptions(), &error));
- if (wrapper->signaling_state() == SignalingState::kClosed) {
- EXPECT_PRED_FORMAT2(AssertStartsWith, error,
- "CreateAnswer called when PeerConnection is closed.");
- } else if (wrapper->signaling_state() ==
- SignalingState::kHaveRemotePrAnswer) {
- EXPECT_PRED_FORMAT2(AssertStartsWith, error,
- "CreateAnswer called without remote offer.");
- } else {
- EXPECT_PRED_FORMAT2(
- AssertStartsWith, error,
- "CreateAnswer can't be called before SetRemoteDescription.");
- }
+ EXPECT_PRED_FORMAT2(
+ AssertStartsWith, error,
+ "CreateAnswer called when PeerConnection is in a wrong state.");
}
}
TEST_P(PeerConnectionSignalingStateTest, SetLocalOffer) {
- auto wrapper = CreatePeerConnectionInState(GetParam());
+ auto wrapper = CreatePeerConnectionUnderTest();
if (wrapper->signaling_state() == SignalingState::kStable ||
wrapper->signaling_state() == SignalingState::kHaveLocalOffer) {
// Need to call CreateOffer on the PeerConnection under test, otherwise when
@@ -273,7 +288,7 @@
auto pranswer =
CloneSessionDescription(wrapper_for_pranswer->pc()->local_description());
- auto wrapper = CreatePeerConnectionInState(GetParam());
+ auto wrapper = CreatePeerConnectionUnderTest();
if (wrapper->signaling_state() == SignalingState::kHaveLocalPrAnswer ||
wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) {
EXPECT_TRUE(wrapper->SetLocalDescription(std::move(pranswer)));
@@ -291,7 +306,7 @@
CreatePeerConnectionInState(SignalingState::kHaveRemoteOffer);
auto answer = wrapper_for_answer->CreateAnswer();
- auto wrapper = CreatePeerConnectionInState(GetParam());
+ auto wrapper = CreatePeerConnectionUnderTest();
if (wrapper->signaling_state() == SignalingState::kHaveLocalPrAnswer ||
wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) {
EXPECT_TRUE(wrapper->SetLocalDescription(std::move(answer)));
@@ -310,7 +325,7 @@
auto offer =
CloneSessionDescription(wrapper_for_offer->pc()->remote_description());
- auto wrapper = CreatePeerConnectionInState(GetParam());
+ auto wrapper = CreatePeerConnectionUnderTest();
if (wrapper->signaling_state() == SignalingState::kStable ||
wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) {
EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(offer)));
@@ -329,7 +344,7 @@
auto pranswer =
CloneSessionDescription(wrapper_for_pranswer->pc()->remote_description());
- auto wrapper = CreatePeerConnectionInState(GetParam());
+ auto wrapper = CreatePeerConnectionUnderTest();
if (wrapper->signaling_state() == SignalingState::kHaveLocalOffer ||
wrapper->signaling_state() == SignalingState::kHaveRemotePrAnswer) {
EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(pranswer)));
@@ -347,7 +362,7 @@
CreatePeerConnectionInState(SignalingState::kHaveRemoteOffer);
auto answer = wrapper_for_answer->CreateAnswer();
- auto wrapper = CreatePeerConnectionInState(GetParam());
+ auto wrapper = CreatePeerConnectionUnderTest();
if (wrapper->signaling_state() == SignalingState::kHaveLocalOffer ||
wrapper->signaling_state() == SignalingState::kHaveRemotePrAnswer) {
EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(answer)));
@@ -362,46 +377,35 @@
INSTANTIATE_TEST_CASE_P(PeerConnectionSignalingTest,
PeerConnectionSignalingStateTest,
- Combine(Values(SignalingState::kStable,
+ Combine(Values(SdpSemantics::kPlanB,
+ SdpSemantics::kUnifiedPlan),
+ Values(SignalingState::kStable,
SignalingState::kHaveLocalOffer,
SignalingState::kHaveLocalPrAnswer,
SignalingState::kHaveRemoteOffer,
SignalingState::kHaveRemotePrAnswer),
Bool()));
-TEST_F(PeerConnectionSignalingTest,
- CreateAnswerSucceedsIfStableAndRemoteDescriptionIsOffer) {
+// Test that CreateAnswer fails if a round of offer/answer has been done and
+// the PeerConnection is in the stable state.
+TEST_P(PeerConnectionSignalingTest, CreateAnswerFailsIfStable) {
auto caller = CreatePeerConnection();
auto callee = CreatePeerConnection();
- ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
- ASSERT_TRUE(
- caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
-
- ASSERT_EQ(SignalingState::kStable, callee->signaling_state());
- EXPECT_TRUE(callee->CreateAnswer());
-}
-
-TEST_F(PeerConnectionSignalingTest,
- CreateAnswerFailsIfStableButRemoteDescriptionIsAnswer) {
- auto caller = CreatePeerConnection();
- auto callee = CreatePeerConnection();
-
- ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
- ASSERT_TRUE(
- caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
+ ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
ASSERT_EQ(SignalingState::kStable, caller->signaling_state());
- std::string error;
- ASSERT_FALSE(caller->CreateAnswer(RTCOfferAnswerOptions(), &error));
- EXPECT_EQ("CreateAnswer called without remote offer.", error);
+ EXPECT_FALSE(caller->CreateAnswer());
+
+ ASSERT_EQ(SignalingState::kStable, callee->signaling_state());
+ EXPECT_FALSE(callee->CreateAnswer());
}
// According to https://tools.ietf.org/html/rfc3264#section-8, the session id
// stays the same but the version must be incremented if a later, different
// session description is generated. These two tests verify that is the case for
// both offers and answers.
-TEST_F(PeerConnectionSignalingTest,
+TEST_P(PeerConnectionSignalingTest,
SessionVersionIncrementedInSubsequentDifferentOffer) {
auto caller = CreatePeerConnection();
auto callee = CreatePeerConnection();
@@ -422,14 +426,14 @@
EXPECT_LT(rtc::FromString<uint64_t>(original_version),
rtc::FromString<uint64_t>(later_offer->session_version()));
}
-TEST_F(PeerConnectionSignalingTest,
+TEST_P(PeerConnectionSignalingTest,
SessionVersionIncrementedInSubsequentDifferentAnswer) {
auto caller = CreatePeerConnection();
auto callee = CreatePeerConnection();
ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
- auto original_answer = callee->CreateAnswerAndSetAsLocal();
+ auto original_answer = callee->CreateAnswer();
const std::string original_id = original_answer->session_id();
const std::string original_version = original_answer->session_version();
@@ -443,7 +447,7 @@
rtc::FromString<uint64_t>(later_answer->session_version()));
}
-TEST_F(PeerConnectionSignalingTest, InitiatorFlagSetOnCallerAndNotOnCallee) {
+TEST_P(PeerConnectionSignalingTest, InitiatorFlagSetOnCallerAndNotOnCallee) {
auto caller = CreatePeerConnectionWithAudioVideo();
auto callee = CreatePeerConnectionWithAudioVideo();
@@ -466,7 +470,7 @@
// PeerConnection and make sure we get success/failure callbacks for all of the
// requests.
// Background: crbug.com/507307
-TEST_F(PeerConnectionSignalingTest, CreateOffersAndShutdown) {
+TEST_P(PeerConnectionSignalingTest, CreateOffersAndShutdown) {
auto caller = CreatePeerConnection();
RTCOfferAnswerOptions options;
@@ -491,4 +495,9 @@
}
}
+INSTANTIATE_TEST_CASE_P(PeerConnectionSignalingTest,
+ PeerConnectionSignalingTest,
+ Values(SdpSemantics::kPlanB,
+ SdpSemantics::kUnifiedPlan));
+
} // namespace webrtc
