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
