From 601ac2eea89a496ff32923aab0666376464eba8b Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Fri, 8 Dec 2023 09:35:00 +0100 Subject: [PATCH] Reject offer content with no common codecs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Henrik Boström Commit-Queue: Philipp Hancke Reviewed-by: Ilya Nikolaevskiy Reviewed-by: Sam Zackrisson Cr-Commit-Position: refs/heads/main@{#41360} --- media/engine/webrtc_video_engine.cc | 18 ++++++--- media/engine/webrtc_voice_engine.cc | 9 +++-- media/engine/webrtc_voice_engine_unittest.cc | 36 +++++++++++------- pc/sdp_offer_answer_unittest.cc | 39 ++++++++++++++++++++ 4 files changed, 78 insertions(+), 24 deletions(-) diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 8a9d6ff95c..a5b46d3344 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -1038,15 +1038,21 @@ bool WebRtcVideoSendChannel::GetChangedSenderParameters( return false; } - std::vector 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 mapped_codecs = MapCodecs(params.codecs); + if (mapped_codecs.empty()) { + // This suggests a failure in MapCodecs, e.g. inconsistent RTX codecs. return false; } + std::vector 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 adf662074d..f287cb7d4e 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -761,9 +761,9 @@ std::vector WebRtcVoiceEngine::CollectCodecs( 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 @@ bool WebRtcVoiceSendChannel::SetSenderParameters( } } - 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 @@ bool WebRtcVoiceSendChannel::SetSendCodecs( } 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 4d6580631d..8ae441bc69 100644 --- a/media/engine/webrtc_voice_engine_unittest.cc +++ b/media/engine/webrtc_voice_engine_unittest.cc @@ -1702,27 +1702,29 @@ TEST_P(WebRtcVoiceEngineTestFake, DontRecreateSendStream) { // 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 @@ TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecOpusBad0Channels1Stereo) { 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 @@ TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecOpusBad1Channel0Stereo) { 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 @@ TEST_P(WebRtcVoiceEngineTestFake, SetSendCodecOpusBad1Channel1Stereo) { 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_P(WebRtcVoiceEngineTestFake, SetSendCodecsBitrate) { } } -// 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 f158febac7..f178e7bc88 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -1096,4 +1096,43 @@ INSTANTIATE_TEST_SUITE_P(SdpOfferAnswerShuffleMediaTypes, 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