Report an error when trying to set complex Plan B SDP on Unified Plan
This changes the PeerConnection when in Unified Plan mode to reject
SDP applied with SetLocalDescription or SetRemoteDescription if the
SDP has multiple "Plan B tracks" (a=ssrc lines) in a media section.
The error is to inform developers that the given SDP will not be
interpreted as they might expect.
Bug: None
Change-Id: I7a0e11282fbf63dac06038cd22a66683517a87d0
Reviewed-on: https://webrtc-review.googlesource.com/68764
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22829}diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index 1f63874..cce9021 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -2194,6 +2194,11 @@
return;
}
+ if (desc->GetType() == SdpType::kOffer) {
+ // Report to UMA the format of the received offer.
+ ReportSdpFormatReceived(*desc);
+ }
+
RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_REMOTE);
if (!error.ok()) {
std::string error_message = GetSetDescriptionErrorMessage(
@@ -2225,11 +2230,6 @@
}
RTC_DCHECK(remote_description());
- if (type == SdpType::kOffer) {
- // Report to UMA the format of the received offer.
- ReportSdpFormatReceived(*remote_description());
- }
-
if (type == SdpType::kAnswer) {
// TODO(deadbeef): We already had to hop to the network thread for
// MaybeStartGathering...
@@ -5751,6 +5751,25 @@
}
}
+ if (IsUnifiedPlan()) {
+ // Ensure that each audio and video media section has at most one
+ // "StreamParams". This will return an error if receiving a session
+ // description from a "Plan B" endpoint which adds multiple tracks of the
+ // same type. With Unified Plan, there can only be at most one track per
+ // media section.
+ for (const ContentInfo& content : sdesc->description()->contents()) {
+ const MediaContentDescription& desc = *content.description;
+ if ((desc.type() == cricket::MEDIA_TYPE_AUDIO ||
+ desc.type() == cricket::MEDIA_TYPE_VIDEO) &&
+ desc.streams().size() > 1u) {
+ LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER,
+ "Media section has more than one track specified "
+ "with a=ssrc lines which is not supported with "
+ "Unified Plan.");
+ }
+ }
+ }
+
return RTCError::OK();
}
diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc
index f7ac830..b9a0227 100644
--- a/pc/peerconnection_integrationtest.cc
+++ b/pc/peerconnection_integrationtest.cc
@@ -4258,10 +4258,22 @@
ASSERT_TRUE(ExpectNewFrames(media_expectations));
}
-// Test that if one side offers two video tracks then the other side will only
-// see the first one and ignore the second.
-TEST_P(PeerConnectionIntegrationInteropTest, TwoVideoLocalToNoMediaRemote) {
- ASSERT_TRUE(CreatePeerConnectionWrappersWithSemantics());
+INSTANTIATE_TEST_CASE_P(
+ PeerConnectionIntegrationTest,
+ PeerConnectionIntegrationInteropTest,
+ Values(std::make_tuple(SdpSemantics::kPlanB, SdpSemantics::kUnifiedPlan),
+ std::make_tuple(SdpSemantics::kUnifiedPlan, SdpSemantics::kPlanB)));
+
+// Test that if the Unified Plan side offers two video tracks then the Plan B
+// side will only see the first one and ignore the second.
+TEST_F(PeerConnectionIntegrationTestPlanB, TwoVideoUnifiedPlanToNoMediaPlanB) {
+ RTCConfiguration caller_config;
+ caller_config.sdp_semantics = SdpSemantics::kUnifiedPlan;
+ RTCConfiguration callee_config;
+ callee_config.sdp_semantics = SdpSemantics::kPlanB;
+ ASSERT_TRUE(
+ CreatePeerConnectionWrappersWithConfig(caller_config, callee_config));
+
ConnectFakeSignaling();
auto first_sender = caller()->AddVideoTrack();
caller()->AddVideoTrack();
@@ -4281,66 +4293,6 @@
ASSERT_TRUE(ExpectNewFrames(media_expectations));
}
-// Test that in the multi-track case each endpoint only looks at the first track
-// and ignores the second one.
-TEST_P(PeerConnectionIntegrationInteropTest, TwoVideoLocalToTwoVideoRemote) {
- ASSERT_TRUE(CreatePeerConnectionWrappersWithSemantics());
- ConnectFakeSignaling();
- caller()->AddVideoTrack();
- caller()->AddVideoTrack();
- callee()->AddVideoTrack();
- callee()->AddVideoTrack();
-
- caller()->CreateAndSetAndSignalOffer();
- ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout);
-
- PeerConnectionWrapper* plan_b =
- (caller_semantics_ == SdpSemantics::kPlanB ? caller() : callee());
- PeerConnectionWrapper* unified_plan =
- (caller_semantics_ == SdpSemantics::kUnifiedPlan ? caller() : callee());
-
- // Should have two senders each, one for each track.
- EXPECT_EQ(2u, plan_b->pc()->GetSenders().size());
- EXPECT_EQ(2u, unified_plan->pc()->GetSenders().size());
-
- // Plan B will have one receiver since it only looks at the first video
- // section. The receiver should have the same track ID as the sender's first
- // track.
- ASSERT_EQ(1u, plan_b->pc()->GetReceivers().size());
- EXPECT_EQ(unified_plan->pc()->GetSenders()[0]->track()->id(),
- plan_b->pc()->GetReceivers()[0]->track()->id());
-
- // Unified Plan will have two receivers since they were created with the
- // transceivers when the tracks were added.
- ASSERT_EQ(2u, unified_plan->pc()->GetReceivers().size());
-
- if (unified_plan == caller()) {
- // If the Unified Plan endpoint was the caller, then the Plan B endpoint
- // would have rejected the second video media section so we would expect the
- // transceiver to be stopped.
- EXPECT_FALSE(unified_plan->pc()->GetTransceivers()[0]->stopped());
- EXPECT_TRUE(unified_plan->pc()->GetTransceivers()[1]->stopped());
- } else {
- // If the Unified Plan endpoint was the callee, then the Plan B endpoint
- // would have offered only one video section so we would expect the first
- // transceiver to map to the first track and the second transceiver to be
- // missing a mid.
- EXPECT_TRUE(unified_plan->pc()->GetTransceivers()[0]->mid());
- EXPECT_FALSE(unified_plan->pc()->GetTransceivers()[1]->mid());
- }
-
- // Should be exchanging video frames for the first tracks on each endpoint.
- MediaExpectations media_expectations;
- media_expectations.ExpectBidirectionalVideo();
- ASSERT_TRUE(ExpectNewFrames(media_expectations));
-}
-
-INSTANTIATE_TEST_CASE_P(
- PeerConnectionIntegrationTest,
- PeerConnectionIntegrationInteropTest,
- Values(std::make_tuple(SdpSemantics::kPlanB, SdpSemantics::kUnifiedPlan),
- std::make_tuple(SdpSemantics::kUnifiedPlan, SdpSemantics::kPlanB)));
-
} // namespace
#endif // if !defined(THREAD_SANITIZER)
diff --git a/pc/peerconnection_jsep_unittest.cc b/pc/peerconnection_jsep_unittest.cc
index caa0ea0..635591c 100644
--- a/pc/peerconnection_jsep_unittest.cc
+++ b/pc/peerconnection_jsep_unittest.cc
@@ -1263,18 +1263,51 @@
EXPECT_FALSE(transceiver->current_direction());
}
-// Tests that you can't set an answer on a PeerConnection before setting
-// the offer.
+// Test that you can't set an answer on a PeerConnection before setting the
+// offer.
TEST_F(PeerConnectionJsepTest, AnswerBeforeOfferFails) {
auto caller = CreatePeerConnection();
auto callee = CreatePeerConnection();
- RTCError error_out;
caller->AddAudioTrack("audio");
- auto offer = caller->CreateOffer();
- callee->SetRemoteDescription(std::move(offer), &error_out);
- auto answer = callee->CreateAnswer();
- EXPECT_FALSE(caller->SetRemoteDescription(std::move(answer), &error_out));
- EXPECT_EQ(RTCErrorType::INVALID_STATE, error_out.type());
+
+ ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+
+ RTCError error;
+ ASSERT_FALSE(caller->SetRemoteDescription(callee->CreateAnswer(), &error));
+ EXPECT_EQ(RTCErrorType::INVALID_STATE, error.type());
+}
+
+// Test that a Unified Plan PeerConnection fails to set a Plan B offer if it has
+// two video tracks.
+TEST_F(PeerConnectionJsepTest, TwoVideoPlanBToUnifiedPlanFails) {
+ RTCConfiguration config_planb;
+ config_planb.sdp_semantics = SdpSemantics::kPlanB;
+ auto caller = CreatePeerConnection(config_planb);
+ auto callee = CreatePeerConnection();
+ caller->AddVideoTrack("video1");
+ caller->AddVideoTrack("video2");
+
+ RTCError error;
+ ASSERT_FALSE(callee->SetRemoteDescription(caller->CreateOffer(), &error));
+ EXPECT_EQ(RTCErrorType::INVALID_PARAMETER, error.type());
+}
+
+// Test that a Unified Plan PeerConnection fails to set a Plan B answer if it
+// has two video tracks.
+TEST_F(PeerConnectionJsepTest, OneVideoUnifiedPlanToTwoVideoPlanBFails) {
+ auto caller = CreatePeerConnection();
+ RTCConfiguration config_planb;
+ config_planb.sdp_semantics = SdpSemantics::kPlanB;
+ auto callee = CreatePeerConnection(config_planb);
+ caller->AddVideoTrack("video");
+ callee->AddVideoTrack("video1");
+ callee->AddVideoTrack("video2");
+
+ ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+
+ RTCError error;
+ ASSERT_FALSE(caller->SetRemoteDescription(caller->CreateAnswer(), &error));
+ EXPECT_EQ(RTCErrorType::INVALID_PARAMETER, error.type());
}
} // namespace webrtc
diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc
index 5f4f34e..942bd46 100644
--- a/pc/peerconnection_rtp_unittest.cc
+++ b/pc/peerconnection_rtp_unittest.cc
@@ -1294,7 +1294,10 @@
auto callee = CreatePeerConnectionWithUnifiedPlan();
auto callee_metrics = callee->RegisterFakeMetricsObserver();
- ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+ // This fails since Unified Plan cannot set a session description with
+ // multiple "Plan B tracks" in the same media section. But we still expect the
+ // SDP Format to be recorded.
+ ASSERT_FALSE(callee->SetRemoteDescription(caller->CreateOffer()));
EXPECT_TRUE(callee_metrics->ExpectOnlySingleEnumCount(
kEnumCounterSdpFormatReceived, kSdpFormatReceivedComplexPlanB));
