Perform packetization verification until a match is found If there happens to be an asymmetry between local and remote codecs we shouldn't validate that there's a 1:1 packetization mapping for every codec. It's sufficient to check that there's at least one matching packetization per codec. Bug: webrtc:15473 Change-Id: Ib4fc8fdd54bb4dccf96f0c802746c848e2deed83 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/320440 Commit-Queue: Stefan Holmer <stefan@webrtc.org> Reviewed-by: Stefan Holmer <stefan@webrtc.org> Auto-Submit: Emil Lundmark <lndmrk@webrtc.org> Reviewed-by: Sergey Sukhanov <sergeysu@webrtc.org> Cr-Commit-Position: refs/heads/main@{#40760}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn index e8d1e86..9176cf9 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn
@@ -105,6 +105,7 @@ "../rtc_base/third_party/sigslot", ] absl_deps = [ + "//third_party/abseil-cpp/absl/algorithm:container", "//third_party/abseil-cpp/absl/strings", "//third_party/abseil-cpp/absl/types:optional", ]
diff --git a/pc/channel.cc b/pc/channel.cc index 82ca1a3..c763b6b 100644 --- a/pc/channel.cc +++ b/pc/channel.cc
@@ -16,6 +16,7 @@ #include <type_traits> #include <utility> +#include "absl/algorithm/container.h" #include "absl/strings/string_view.h" #include "api/rtp_parameters.h" #include "api/sequence_checker.h" @@ -1036,19 +1037,34 @@ bool needs_send_params_update = false; if (type == SdpType::kAnswer || type == SdpType::kPrAnswer) { - for (auto& send_codec : send_params.codecs) { - auto* recv_codec = FindMatchingCodec(recv_params.codecs, send_codec); - if (recv_codec) { - if (!recv_codec->packetization && send_codec.packetization) { - send_codec.packetization.reset(); - needs_send_params_update = true; - } else if (recv_codec->packetization != send_codec.packetization) { - error_desc = StringFormat( - "Failed to set local answer due to invalid codec packetization " - "specified in m-section with mid='%s'.", - mid().c_str()); - return false; - } + webrtc::flat_set<const VideoCodec*> matched_codecs; + for (VideoCodec& send_codec : send_params.codecs) { + if (absl::c_any_of(matched_codecs, [&](const VideoCodec* c) { + return send_codec.Matches(*c); + })) { + continue; + } + + const VideoCodec* recv_codec = + FindMatchingCodec(recv_params.codecs, send_codec); + if (recv_codec == nullptr) { + continue; + } + + if (!recv_codec->packetization.has_value() && + send_codec.packetization.has_value()) { + send_codec.packetization = absl::nullopt; + needs_send_params_update = true; + } else if (recv_codec->packetization != send_codec.packetization) { + error_desc = StringFormat( + "Failed to set local answer due to incompatible codec " + "packetization for pt='%d' specified in m-section with mid='%s'.", + send_codec.id, mid().c_str()); + return false; + } + + if (recv_codec->packetization == send_codec.packetization) { + matched_codecs.insert(&send_codec); } } } @@ -1121,19 +1137,34 @@ bool needs_recv_params_update = false; if (type == SdpType::kAnswer || type == SdpType::kPrAnswer) { - for (auto& recv_codec : recv_params.codecs) { - auto* send_codec = FindMatchingCodec(send_params.codecs, recv_codec); - if (send_codec) { - if (!send_codec->packetization && recv_codec.packetization) { - recv_codec.packetization.reset(); - needs_recv_params_update = true; - } else if (send_codec->packetization != recv_codec.packetization) { - error_desc = StringFormat( - "Failed to set remote answer due to invalid codec packetization " - "specifid in m-section with mid='%s'.", - mid().c_str()); - return false; - } + webrtc::flat_set<const VideoCodec*> matched_codecs; + for (VideoCodec& recv_codec : recv_params.codecs) { + if (absl::c_any_of(matched_codecs, [&](const VideoCodec* c) { + return recv_codec.Matches(*c); + })) { + continue; + } + + const VideoCodec* send_codec = + FindMatchingCodec(send_params.codecs, recv_codec); + if (send_codec == nullptr) { + continue; + } + + if (!send_codec->packetization.has_value() && + recv_codec.packetization.has_value()) { + recv_codec.packetization = absl::nullopt; + needs_recv_params_update = true; + } else if (send_codec->packetization != recv_codec.packetization) { + error_desc = StringFormat( + "Failed to set remote answer due to incompatible codec " + "packetization for pt='%d' specified in m-section with mid='%s'.", + recv_codec.id, mid().c_str()); + return false; + } + + if (send_codec->packetization == recv_codec.packetization) { + matched_codecs.insert(&recv_codec); } } }
diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index 0d7f0b0..c86f2cc 100644 --- a/pc/channel_unittest.cc +++ b/pc/channel_unittest.cc
@@ -48,16 +48,20 @@ #include "test/gtest.h" #include "test/scoped_key_value_config.h" -using cricket::DtlsTransportInternal; -using cricket::FakeVoiceMediaReceiveChannel; -using cricket::FakeVoiceMediaSendChannel; -using cricket::RidDescription; -using cricket::RidDirection; -using cricket::StreamParams; -using webrtc::RtpTransceiverDirection; -using webrtc::SdpType; - namespace { + +using ::cricket::DtlsTransportInternal; +using ::cricket::FakeVoiceMediaReceiveChannel; +using ::cricket::FakeVoiceMediaSendChannel; +using ::cricket::RidDescription; +using ::cricket::RidDirection; +using ::cricket::StreamParams; +using ::testing::AllOf; +using ::testing::ElementsAre; +using ::testing::Field; +using ::webrtc::RtpTransceiverDirection; +using ::webrtc::SdpType; + const cricket::AudioCodec kPcmuCodec = cricket::CreateAudioCodec(0, "PCMU", 64000, 1); const cricket::AudioCodec kPcmaCodec = @@ -75,7 +79,6 @@ const int kVideoPts[] = {97, 99}; enum class NetworkIsWorker { Yes, No }; -} // namespace template <class ChannelT, class MediaSendChannelT, @@ -2274,6 +2277,78 @@ absl::nullopt); } +TEST_F(VideoChannelSingleThreadTest, + StopsPacketizationVerificationWhenMatchIsFoundInRemoteAnswer) { + cricket::VideoCodec vp8_foo = cricket::CreateVideoCodec(96, "VP8"); + vp8_foo.packetization = "foo"; + cricket::VideoCodec vp8_bar = cricket::CreateVideoCodec(97, "VP8"); + vp8_bar.packetization = "bar"; + cricket::VideoCodec vp9 = cricket::CreateVideoCodec(98, "VP9"); + cricket::VideoCodec vp9_foo = cricket::CreateVideoCodec(99, "VP9"); + vp9_foo.packetization = "bar"; + cricket::VideoContentDescription local; + local.set_codecs({vp8_foo, vp8_bar, vp9_foo}); + cricket::VideoContentDescription remote; + remote.set_codecs({vp8_foo, vp9}); + + CreateChannels(0, 0); + std::string err; + ASSERT_TRUE(channel1_->SetLocalContent(&local, SdpType::kOffer, err)) << err; + ASSERT_TRUE(channel1_->SetRemoteContent(&remote, SdpType::kAnswer, err)) + << err; + + EXPECT_THAT( + media_receive_channel1_impl()->recv_codecs(), + ElementsAre(AllOf(Field(&cricket::Codec::id, 96), + Field(&cricket::Codec::packetization, "foo")), + AllOf(Field(&cricket::Codec::id, 97), + Field(&cricket::Codec::packetization, "bar")), + AllOf(Field(&cricket::Codec::id, 99), + Field(&cricket::Codec::packetization, absl::nullopt)))); + EXPECT_THAT( + media_send_channel1_impl()->send_codecs(), + ElementsAre(AllOf(Field(&cricket::Codec::id, 96), + Field(&cricket::Codec::packetization, "foo")), + AllOf(Field(&cricket::Codec::id, 98), + Field(&cricket::Codec::packetization, absl::nullopt)))); +} + +TEST_F(VideoChannelSingleThreadTest, + StopsPacketizationVerificationWhenMatchIsFoundInLocalAnswer) { + cricket::VideoCodec vp8_foo = cricket::CreateVideoCodec(96, "VP8"); + vp8_foo.packetization = "foo"; + cricket::VideoCodec vp8_bar = cricket::CreateVideoCodec(97, "VP8"); + vp8_bar.packetization = "bar"; + cricket::VideoCodec vp9 = cricket::CreateVideoCodec(98, "VP9"); + cricket::VideoCodec vp9_foo = cricket::CreateVideoCodec(99, "VP9"); + vp9_foo.packetization = "bar"; + cricket::VideoContentDescription local; + local.set_codecs({vp8_foo, vp9}); + cricket::VideoContentDescription remote; + remote.set_codecs({vp8_foo, vp8_bar, vp9_foo}); + + CreateChannels(0, 0); + std::string err; + ASSERT_TRUE(channel1_->SetRemoteContent(&remote, SdpType::kOffer, err)) + << err; + ASSERT_TRUE(channel1_->SetLocalContent(&local, SdpType::kAnswer, err)) << err; + + EXPECT_THAT( + media_receive_channel1_impl()->recv_codecs(), + ElementsAre(AllOf(Field(&cricket::Codec::id, 96), + Field(&cricket::Codec::packetization, "foo")), + AllOf(Field(&cricket::Codec::id, 98), + Field(&cricket::Codec::packetization, absl::nullopt)))); + EXPECT_THAT( + media_send_channel1_impl()->send_codecs(), + ElementsAre(AllOf(Field(&cricket::Codec::id, 96), + Field(&cricket::Codec::packetization, "foo")), + AllOf(Field(&cricket::Codec::id, 97), + Field(&cricket::Codec::packetization, "bar")), + AllOf(Field(&cricket::Codec::id, 99), + Field(&cricket::Codec::packetization, absl::nullopt)))); +} + // VideoChannelDoubleThreadTest TEST_F(VideoChannelDoubleThreadTest, TestInit) { Base::TestInit(); @@ -2409,4 +2484,4 @@ Base::SocketOptionsMergedOnSetTransport(); } -// TODO(pthatcher): TestSetReceiver? +} // namespace