From 17304c3bf83490bb0e6f8a7a2c6f3941c5f2f59d Mon Sep 17 00:00:00 2001 From: Emil Lundmark Date: Fri, 15 Sep 2023 16:33:03 +0200 Subject: [PATCH] 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 Reviewed-by: Stefan Holmer Auto-Submit: Emil Lundmark Reviewed-by: Sergey Sukhanov Cr-Commit-Position: refs/heads/main@{#40760} --- pc/BUILD.gn | 1 + pc/channel.cc | 83 +++++++++++++++++++++++++----------- pc/channel_unittest.cc | 97 +++++++++++++++++++++++++++++++++++++----- 3 files changed, 144 insertions(+), 37 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index e8d1e8622b..9176cf9735 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -105,6 +105,7 @@ rtc_source_set("channel") { "../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 82ca1a389e..c763b6bea8 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -16,6 +16,7 @@ #include #include +#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 VideoChannel::SetLocalContent_w(const MediaContentDescription* content, 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 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 VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, 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 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 0d7f0b0cd0..c86f2cc71e 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 kAudioPts[] = {0, 8}; const int kVideoPts[] = {97, 99}; enum class NetworkIsWorker { Yes, No }; -} // namespace template 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 @@ TEST_F(VideoChannelDoubleThreadTest, SocketOptionsMergedOnSetTransport) { Base::SocketOptionsMergedOnSetTransport(); } -// TODO(pthatcher): TestSetReceiver? +} // namespace