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}

media/base/codec.cc

@@ -400,6 +400,19 @@ const VideoCodec* FindMatchingCodec(
   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(

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<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);
 

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<const VideoCodec*> matched_codecs;
@@ -1045,17 +1050,28 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content,
         continue;
       }
 
-      const VideoCodec* recv_codec =
+      std::vector<const VideoCodec*> recv_codecs =
-          FindMatchingCodec(recv_params.codecs, send_codec);
+          FindAllMatchingCodecs(recv_params.codecs, send_codec);
-      if (recv_codec == nullptr) {
+      if (recv_codecs.empty()) {
         continue;
       }
 
+      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<const VideoCodec*> matched_codecs;
@@ -1145,17 +1166,28 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content,
         continue;
       }
 
-      const VideoCodec* send_codec =
+      std::vector<const VideoCodec*> send_codecs =
-          FindMatchingCodec(send_params.codecs, recv_codec);
+          FindAllMatchingCodecs(send_params.codecs, recv_codec);
-      if (send_codec == nullptr) {
+      if (send_codecs.empty()) {
         continue;
       }
 
+      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);
       }
     }

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();