From ec8262788b51e69ea1ac7a8802e9b59b954e81e0 Mon Sep 17 00:00:00 2001 From: Emil Lundmark Date: Wed, 20 Sep 2023 12:22:39 +0200 Subject: [PATCH] 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 Reviewed-by: Stefan Holmer Auto-Submit: Emil Lundmark Reviewed-by: Sergey Sukhanov Commit-Queue: Stefan Holmer Cr-Commit-Position: refs/heads/main@{#40775} --- media/base/codec.cc | 13 +++++++++ media/base/codec.h | 6 ++++ pc/channel.cc | 60 ++++++++++++++++++++++++++++++--------- pc/channel_unittest.cc | 64 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 129 insertions(+), 14 deletions(-) diff --git a/media/base/codec.cc b/media/base/codec.cc index 7ecf383d9f..9ada18f956 100644 --- a/media/base/codec.cc +++ b/media/base/codec.cc @@ -400,6 +400,19 @@ const VideoCodec* FindMatchingCodec( return nullptr; } +std::vector FindAllMatchingCodecs( + const std::vector& supported_codecs, + const VideoCodec& codec) { + std::vector 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 5595708cfa..b8ef22e4fb 100644 --- a/media/base/codec.h +++ b/media/base/codec.h @@ -214,12 +214,18 @@ bool HasNack(const Codec& codec); 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& supported_codecs, const VideoCodec& codec); +// Returns all codecs in `supported_codecs` that matches `codec`. +std::vector FindAllMatchingCodecs( + const std::vector& supported_codecs, + const VideoCodec& codec); + RTC_EXPORT void AddH264ConstrainedBaselineProfileToSupportedFormats( std::vector* supported_formats); diff --git a/pc/channel.cc b/pc/channel.cc index c763b6bea8..4b8959cb50 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -1035,6 +1035,11 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, 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 matched_codecs; @@ -1045,17 +1050,28 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, continue; } - const VideoCodec* recv_codec = - FindMatchingCodec(recv_params.codecs, send_codec); - if (recv_codec == nullptr) { + std::vector 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 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content, return false; } - if (recv_codec->packetization == send_codec.packetization) { + if (has_matching_packetization) { matched_codecs.insert(&send_codec); } } @@ -1135,6 +1151,11 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, 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 matched_codecs; @@ -1145,17 +1166,28 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, continue; } - const VideoCodec* send_codec = - FindMatchingCodec(send_params.codecs, recv_codec); - if (send_codec == nullptr) { + std::vector 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 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content, 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 c86f2cc71e..7d60376141 100644 --- a/pc/channel_unittest.cc +++ b/pc/channel_unittest.cc @@ -2349,6 +2349,70 @@ TEST_F(VideoChannelSingleThreadTest, 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();