Look through all candidates before falling back to default packetization
It's possible that a peer can signal the same payload with multiple
packetization options. As such, we shouldn't try to fall back to default
packetization until we have considered all the alternatives.
Bug: webrtc:15473
Change-Id: I21772b4d8c53819d1c3105988551ebdbea0df045
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/320241
Commit-Queue: Emil Lundmark <lndmrk@webrtc.org>
Reviewed-by: Stefan Holmer <stefan@webrtc.org>
Auto-Submit: Emil Lundmark <lndmrk@webrtc.org>
Reviewed-by: Sergey Sukhanov <sergeysu@webrtc.org>
Commit-Queue: Stefan Holmer <stefan@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40775}
diff --git a/media/base/codec.cc b/media/base/codec.cc
index 7ecf383..9ada18f 100644
--- a/media/base/codec.cc
+++ b/media/base/codec.cc
@@ -400,6 +400,19 @@
return nullptr;
}
+std::vector<const VideoCodec*> FindAllMatchingCodecs(
+ const std::vector<VideoCodec>& supported_codecs,
+ const VideoCodec& codec) {
+ std::vector<const VideoCodec*> result;
+ webrtc::SdpVideoFormat sdp(codec.name, codec.params);
+ for (const VideoCodec& supported_codec : supported_codecs) {
+ if (sdp.IsSameCodec({supported_codec.name, supported_codec.params})) {
+ result.push_back(&supported_codec);
+ }
+ }
+ return result;
+}
+
// If a decoder supports any H264 profile, it is implicitly assumed to also
// support constrained base line even though it's not explicitly listed.
void AddH264ConstrainedBaselineProfileToSupportedFormats(
diff --git a/media/base/codec.h b/media/base/codec.h
index 5595708..b8ef22e 100644
--- a/media/base/codec.h
+++ b/media/base/codec.h
@@ -214,12 +214,18 @@
bool HasRemb(const Codec& codec);
bool HasRrtr(const Codec& codec);
bool HasTransportCc(const Codec& codec);
+
// Returns the first codec in `supported_codecs` that matches `codec`, or
// nullptr if no codec matches.
const VideoCodec* FindMatchingCodec(
const std::vector<VideoCodec>& supported_codecs,
const VideoCodec& codec);
+// Returns all codecs in `supported_codecs` that matches `codec`.
+std::vector<const VideoCodec*> FindAllMatchingCodecs(
+ const std::vector<VideoCodec>& supported_codecs,
+ const VideoCodec& codec);
+
RTC_EXPORT void AddH264ConstrainedBaselineProfileToSupportedFormats(
std::vector<webrtc::SdpVideoFormat>* supported_formats);
diff --git a/pc/channel.cc b/pc/channel.cc
index c763b6b..4b8959c 100644
--- a/pc/channel.cc
+++ b/pc/channel.cc
@@ -1035,6 +1035,11 @@
VideoSenderParameters send_params = last_send_params_;
+ // Ensure that there is a matching packetization for each send codec. If the
+ // other peer offered to exclusively send non-standard packetization but we
+ // only accept to receive standard packetization we effectively amend their
+ // offer by ignoring the packetiztion and fall back to standard packetization
+ // instead.
bool needs_send_params_update = false;
if (type == SdpType::kAnswer || type == SdpType::kPrAnswer) {
webrtc::flat_set<const VideoCodec*> matched_codecs;
@@ -1045,17 +1050,28 @@
continue;
}
- const VideoCodec* recv_codec =
- FindMatchingCodec(recv_params.codecs, send_codec);
- if (recv_codec == nullptr) {
+ std::vector<const VideoCodec*> recv_codecs =
+ FindAllMatchingCodecs(recv_params.codecs, send_codec);
+ if (recv_codecs.empty()) {
continue;
}
- if (!recv_codec->packetization.has_value() &&
- send_codec.packetization.has_value()) {
+ bool may_ignore_packetization = false;
+ bool has_matching_packetization = false;
+ for (const VideoCodec* recv_codec : recv_codecs) {
+ if (!recv_codec->packetization.has_value() &&
+ send_codec.packetization.has_value()) {
+ may_ignore_packetization = true;
+ } else if (recv_codec->packetization == send_codec.packetization) {
+ has_matching_packetization = true;
+ break;
+ }
+ }
+
+ if (may_ignore_packetization) {
send_codec.packetization = absl::nullopt;
needs_send_params_update = true;
- } else if (recv_codec->packetization != send_codec.packetization) {
+ } else if (!has_matching_packetization) {
error_desc = StringFormat(
"Failed to set local answer due to incompatible codec "
"packetization for pt='%d' specified in m-section with mid='%s'.",
@@ -1063,7 +1079,7 @@
return false;
}
- if (recv_codec->packetization == send_codec.packetization) {
+ if (has_matching_packetization) {
matched_codecs.insert(&send_codec);
}
}
@@ -1135,6 +1151,11 @@
VideoReceiverParameters recv_params = last_recv_params_;
+ // Ensure that there is a matching packetization for each receive codec. If we
+ // offered to exclusively receive a non-standard packetization but the other
+ // peer only accepts to send standard packetization we effectively amend our
+ // offer by ignoring the packetiztion and fall back to standard packetization
+ // instead.
bool needs_recv_params_update = false;
if (type == SdpType::kAnswer || type == SdpType::kPrAnswer) {
webrtc::flat_set<const VideoCodec*> matched_codecs;
@@ -1145,17 +1166,28 @@
continue;
}
- const VideoCodec* send_codec =
- FindMatchingCodec(send_params.codecs, recv_codec);
- if (send_codec == nullptr) {
+ std::vector<const VideoCodec*> send_codecs =
+ FindAllMatchingCodecs(send_params.codecs, recv_codec);
+ if (send_codecs.empty()) {
continue;
}
- if (!send_codec->packetization.has_value() &&
- recv_codec.packetization.has_value()) {
+ bool may_ignore_packetization = false;
+ bool has_matching_packetization = false;
+ for (const VideoCodec* send_codec : send_codecs) {
+ if (!send_codec->packetization.has_value() &&
+ recv_codec.packetization.has_value()) {
+ may_ignore_packetization = true;
+ } else if (send_codec->packetization == recv_codec.packetization) {
+ has_matching_packetization = true;
+ break;
+ }
+ }
+
+ if (may_ignore_packetization) {
recv_codec.packetization = absl::nullopt;
needs_recv_params_update = true;
- } else if (send_codec->packetization != recv_codec.packetization) {
+ } else if (!has_matching_packetization) {
error_desc = StringFormat(
"Failed to set remote answer due to incompatible codec "
"packetization for pt='%d' specified in m-section with mid='%s'.",
@@ -1163,7 +1195,7 @@
return false;
}
- if (send_codec->packetization == recv_codec.packetization) {
+ if (has_matching_packetization) {
matched_codecs.insert(&recv_codec);
}
}
diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc
index c86f2cc..7d60376 100644
--- a/pc/channel_unittest.cc
+++ b/pc/channel_unittest.cc
@@ -2349,6 +2349,70 @@
Field(&cricket::Codec::packetization, absl::nullopt))));
}
+TEST_F(VideoChannelSingleThreadTest,
+ ConsidersAllCodecsWithDiffrentPacketizationsInRemoteAnswer) {
+ cricket::VideoCodec vp8 = cricket::CreateVideoCodec(96, "VP8");
+ cricket::VideoCodec vp8_raw = cricket::CreateVideoCodec(97, "VP8");
+ vp8_raw.packetization = cricket::kPacketizationParamRaw;
+ cricket::VideoContentDescription local;
+ local.set_codecs({vp8, vp8_raw});
+ cricket::VideoContentDescription remote;
+ remote.set_codecs({vp8_raw, vp8});
+
+ 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, absl::nullopt)),
+ AllOf(Field(&cricket::Codec::id, 97),
+ Field(&cricket::Codec::packetization,
+ cricket::kPacketizationParamRaw))));
+ EXPECT_THAT(
+ media_send_channel1_impl()->send_codecs(),
+ ElementsAre(AllOf(Field(&cricket::Codec::id, 97),
+ Field(&cricket::Codec::packetization,
+ cricket::kPacketizationParamRaw)),
+ AllOf(Field(&cricket::Codec::id, 96),
+ Field(&cricket::Codec::packetization, absl::nullopt))));
+}
+
+TEST_F(VideoChannelSingleThreadTest,
+ ConsidersAllCodecsWithDiffrentPacketizationsInLocalAnswer) {
+ cricket::VideoCodec vp8 = cricket::CreateVideoCodec(96, "VP8");
+ cricket::VideoCodec vp8_raw = cricket::CreateVideoCodec(97, "VP8");
+ vp8_raw.packetization = cricket::kPacketizationParamRaw;
+ cricket::VideoContentDescription local;
+ local.set_codecs({vp8_raw, vp8});
+ cricket::VideoContentDescription remote;
+ remote.set_codecs({vp8, vp8_raw});
+
+ 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, 97),
+ Field(&cricket::Codec::packetization,
+ cricket::kPacketizationParamRaw)),
+ AllOf(Field(&cricket::Codec::id, 96),
+ 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, absl::nullopt)),
+ AllOf(Field(&cricket::Codec::id, 97),
+ Field(&cricket::Codec::packetization,
+ cricket::kPacketizationParamRaw))));
+}
+
// VideoChannelDoubleThreadTest
TEST_F(VideoChannelDoubleThreadTest, TestInit) {
Base::TestInit();
