Reject offer content with no common codecs
instead of throwing an error when trying to pick a send codec.
BUG=webrtc:15145,webrtc:4957
Change-Id: I056b145c093348576e1aeaf5def50d5414f2de70
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/330122
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Reviewed-by: Ilya Nikolaevskiy <ilnik@webrtc.org>
Reviewed-by: Sam Zackrisson <saza@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41360}
diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc
index 8a9d6ff..a5b46d3 100644
--- a/media/engine/webrtc_video_engine.cc
+++ b/media/engine/webrtc_video_engine.cc
@@ -1038,15 +1038,21 @@
return false;
}
- std::vector<VideoCodecSettings> negotiated_codecs =
- SelectSendVideoCodecs(MapCodecs(params.codecs));
-
- // We should only fail here if send direction is enabled.
- if (params.is_stream_active && negotiated_codecs.empty()) {
- RTC_LOG(LS_ERROR) << "No video codecs supported.";
+ std::vector<VideoCodecSettings> mapped_codecs = MapCodecs(params.codecs);
+ if (mapped_codecs.empty()) {
+ // This suggests a failure in MapCodecs, e.g. inconsistent RTX codecs.
return false;
}
+ std::vector<VideoCodecSettings> negotiated_codecs =
+ SelectSendVideoCodecs(mapped_codecs);
+
+ if (params.is_stream_active && negotiated_codecs.empty()) {
+ // This is not a failure but will lead to the answer being rejected.
+ RTC_LOG(LS_ERROR) << "No video codecs in common.";
+ return true;
+ }
+
// Never enable sending FlexFEC, unless we are in the experiment.
if (!IsEnabled(call_->trials(), "WebRTC-FlexFEC-03")) {
for (VideoCodecSettings& codec : negotiated_codecs)
diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc
index adf6620..f287cb7 100644
--- a/media/engine/webrtc_voice_engine.cc
+++ b/media/engine/webrtc_voice_engine.cc
@@ -761,9 +761,9 @@
out.push_back(codec);
if (codec.name == kOpusCodecName) {
- std::string redFmtp =
+ std::string red_fmtp =
rtc::ToString(codec.id) + "/" + rtc::ToString(codec.id);
- map_format({kRedCodecName, 48000, 2, {{"", redFmtp}}}, &out);
+ map_format({kRedCodecName, 48000, 2, {{"", red_fmtp}}}, &out);
}
}
}
@@ -1318,7 +1318,7 @@
}
}
- if (!SetMaxSendBitrate(params.max_bandwidth_bps)) {
+ if (send_codec_spec_ && !SetMaxSendBitrate(params.max_bandwidth_bps)) {
return false;
}
return SetOptions(params.options);
@@ -1402,7 +1402,8 @@
}
if (!send_codec_spec) {
- return false;
+ // No codecs in common, bail out early.
+ return true;
}
RTC_DCHECK(voice_codec_info);
diff --git a/media/engine/webrtc_voice_engine_unittest.cc b/media/engine/webrtc_voice_engine_unittest.cc
index 4d65806..8ae441b 100644
--- a/media/engine/webrtc_voice_engine_unittest.cc
+++ b/media/engine/webrtc_voice_engine_unittest.cc
@@ -1702,27 +1702,29 @@
// TODO(ossu): Revisit if these tests need to be here, now that these kinds of
// tests should be available in AudioEncoderOpusTest.
-// Test that if clockrate is not 48000 for opus, we fail.
+// Test that if clockrate is not 48000 for opus, we do not have a send codec.
TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecOpusBadClockrate) {
EXPECT_TRUE(SetupSendStream());
cricket::AudioSenderParameter parameters;
parameters.codecs.push_back(kOpusCodec);
parameters.codecs[0].bitrate = 0;
parameters.codecs[0].clockrate = 50000;
- EXPECT_FALSE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_TRUE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_EQ(send_channel_->GetSendCodec(), absl::nullopt);
}
-// Test that if channels=0 for opus, we fail.
+// Test that if channels=0 for opus, we do not have a send codec.
TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecOpusBad0ChannelsNoStereo) {
EXPECT_TRUE(SetupSendStream());
cricket::AudioSenderParameter parameters;
parameters.codecs.push_back(kOpusCodec);
parameters.codecs[0].bitrate = 0;
parameters.codecs[0].channels = 0;
- EXPECT_FALSE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_TRUE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_EQ(send_channel_->GetSendCodec(), absl::nullopt);
}
-// Test that if channels=0 for opus, we fail.
+// Test that if channels=0 for opus, we do not have a send codec.
TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecOpusBad0Channels1Stereo) {
EXPECT_TRUE(SetupSendStream());
cricket::AudioSenderParameter parameters;
@@ -1730,20 +1732,23 @@
parameters.codecs[0].bitrate = 0;
parameters.codecs[0].channels = 0;
parameters.codecs[0].params["stereo"] = "1";
- EXPECT_FALSE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_TRUE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_EQ(send_channel_->GetSendCodec(), absl::nullopt);
}
-// Test that if channel is 1 for opus and there's no stereo, we fail.
+// Test that if channel is 1 for opus and there's no stereo, we do not have a
+// send codec.
TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecOpus1ChannelNoStereo) {
EXPECT_TRUE(SetupSendStream());
cricket::AudioSenderParameter parameters;
parameters.codecs.push_back(kOpusCodec);
parameters.codecs[0].bitrate = 0;
parameters.codecs[0].channels = 1;
- EXPECT_FALSE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_TRUE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_EQ(send_channel_->GetSendCodec(), absl::nullopt);
}
-// Test that if channel is 1 for opus and stereo=0, we fail.
+// Test that if channel is 1 for opus and stereo=0, we do not have a send codec.
TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecOpusBad1Channel0Stereo) {
EXPECT_TRUE(SetupSendStream());
cricket::AudioSenderParameter parameters;
@@ -1751,10 +1756,11 @@
parameters.codecs[0].bitrate = 0;
parameters.codecs[0].channels = 1;
parameters.codecs[0].params["stereo"] = "0";
- EXPECT_FALSE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_TRUE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_EQ(send_channel_->GetSendCodec(), absl::nullopt);
}
-// Test that if channel is 1 for opus and stereo=1, we fail.
+// Test that if channel is 1 for opus and stereo=1, we do not have a send codec.
TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecOpusBad1Channel1Stereo) {
EXPECT_TRUE(SetupSendStream());
cricket::AudioSenderParameter parameters;
@@ -1762,7 +1768,8 @@
parameters.codecs[0].bitrate = 0;
parameters.codecs[0].channels = 1;
parameters.codecs[0].params["stereo"] = "1";
- EXPECT_FALSE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_TRUE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_EQ(send_channel_->GetSendCodec(), absl::nullopt);
}
// Test that with bitrate=0 and no stereo, bitrate is 32000.
@@ -2035,11 +2042,12 @@
}
}
-// Test that we fail if no codecs are specified.
+// Test that we do not fail if no codecs are specified.
TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecsNoCodecs) {
EXPECT_TRUE(SetupSendStream());
cricket::AudioSenderParameter parameters;
- EXPECT_FALSE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_TRUE(send_channel_->SetSenderParameters(parameters));
+ EXPECT_EQ(send_channel_->GetSendCodec(), absl::nullopt);
}
// Test that we can set send codecs even with telephone-event codec as the first
diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc
index f158feb..f178e7b 100644
--- a/pc/sdp_offer_answer_unittest.cc
+++ b/pc/sdp_offer_answer_unittest.cc
@@ -1096,4 +1096,43 @@
SdpOfferAnswerShuffleMediaTypes,
::testing::Values(true, false));
+TEST_F(SdpOfferAnswerTest, OfferWithNoCompatibleCodecsIsRejectedInAnswer) {
+ auto pc = CreatePeerConnection();
+ // An offer with no common codecs. This should reject both contents
+ // in the answer without throwing an error.
+ std::string sdp =
+ "v=0\r\n"
+ "o=- 0 3 IN IP4 127.0.0.1\r\n"
+ "s=-\r\n"
+ "t=0 0\r\n"
+ "a=fingerprint:sha-1 "
+ "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n"
+ "a=setup:actpass\r\n"
+ "a=ice-ufrag:ETEn\r\n"
+ "a=ice-pwd:OtSK0WpNtpUjkY4+86js7Z/l\r\n"
+ "m=audio 9 RTP/SAVPF 97\r\n"
+ "c=IN IP4 0.0.0.0\r\n"
+ "a=sendrecv\r\n"
+ "a=rtpmap:97 x-unknown/90000\r\n"
+ "a=rtcp-mux\r\n"
+ "m=video 9 RTP/SAVPF 98\r\n"
+ "c=IN IP4 0.0.0.0\r\n"
+ "a=sendrecv\r\n"
+ "a=rtpmap:98 H263-1998/90000\r\n"
+ "a=fmtp:98 CIF=1;QCIF=1\r\n"
+ "a=rtcp-mux\r\n";
+
+ auto desc = CreateSessionDescription(SdpType::kOffer, sdp);
+ ASSERT_NE(desc, nullptr);
+ RTCError error;
+ pc->SetRemoteDescription(std::move(desc), &error);
+ EXPECT_TRUE(error.ok());
+
+ auto answer = pc->CreateAnswer();
+ auto answer_contents = answer->description()->contents();
+ ASSERT_EQ(answer_contents.size(), 2u);
+ EXPECT_EQ(answer_contents[0].rejected, true);
+ EXPECT_EQ(answer_contents[1].rejected, true);
+}
+
} // namespace webrtc
