Fix for payload type id collision

This collision can occur when we have
asymetrical send and receive codecs. This is the case in the current
code base with the VP9 codec familly but is not visible untill more
codecs are added.

Added Nutanix Inc. to AUTHORS.

Bug: chromium:1291956
Change-Id: I09d3f76161d984d2a3edf721639753bffd4947b9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/250034
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35944}

AUTHORS

@@ -138,6 +138,7 @@ Microsoft Corporation <*@microsoft.com>
 MIPS Technologies <*@mips.com>
 Mozilla Foundation <*@mozilla.com>
 Netgem S.A. <*@netgem.com>
+Nutanix Inc. <*@nutanix.com>
 NVIDIA Corporation <*@nvidia.com>
 Opera Software ASA <*@opera.com>
 Optical Tone Ltd <*@opticaltone.com>

media/engine/webrtc_video_engine.cc

@@ -135,8 +135,15 @@ bool IsCodecValidForLowerRange(const VideoCodec& codec) {
     return true;
   } else if (absl::EqualsIgnoreCase(codec.name, kH264CodecName)) {
     std::string profileLevelId;
-    // H264 with YUV444.
+    std::string packetizationMode;
+
     if (codec.GetParam(kH264FmtpProfileLevelId, &profileLevelId)) {
+      if (absl::StartsWithIgnoreCase(profileLevelId, "4d00")) {
+        if (codec.GetParam(kH264FmtpPacketizationMode, &packetizationMode)) {
+          return packetizationMode == "0";
+        }
+      }
+      // H264 with YUV444.
       return absl::StartsWithIgnoreCase(profileLevelId, "f400");
     }
   }

pc/media_session.cc

@@ -1111,6 +1111,25 @@ static Codecs MatchCodecPreference(
   return filtered_codecs;
 }
 
+// Compute the union of `codecs1` and `codecs2`.
+template <class C>
+std::vector<C> ComputeCodecsUnion(const std::vector<C>& codecs1,
+                                  const std::vector<C>& codecs2) {
+  std::vector<C> all_codecs;
+  UsedPayloadTypes used_payload_types;
+  for (const C& codec : codecs1) {
+    C codec_mutable = codec;
+    used_payload_types.FindAndSetIdUsed(&codec_mutable);
+    all_codecs.push_back(codec_mutable);
+  }
+
+  // Use MergeCodecs to merge the second half of our list as it already checks
+  // and fixes problems with duplicate payload types.
+  MergeCodecs<C>(codecs2, &all_codecs, &used_payload_types);
+
+  return all_codecs;
+}
+
 // Adds all extensions from `reference_extensions` to `offered_extensions` that
 // don't already exist in `offered_extensions` and ensure the IDs don't
 // collide. If an extension is added, it's also added to `regular_extensions` or
@@ -2696,7 +2715,9 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer(
         }
       }
     }
+
     // Add other supported video codecs.
+    VideoCodecs other_video_codecs;
     for (const VideoCodec& codec : supported_video_codecs) {
       if (FindMatchingCodec<VideoCodec>(supported_video_codecs, video_codecs,
                                         codec, nullptr) &&
@@ -2704,9 +2725,13 @@ bool MediaSessionDescriptionFactory::AddVideoContentForAnswer(
                                          filtered_codecs, codec, nullptr)) {
         // We should use the local codec with local parameters and the codec id
         // would be correctly mapped in `NegotiateCodecs`.
-        filtered_codecs.push_back(codec);
+        other_video_codecs.push_back(codec);
       }
     }
+
+    // Use ComputeCodecsUnion to avoid having duplicate payload IDs
+    filtered_codecs =
+        ComputeCodecsUnion<VideoCodec>(filtered_codecs, other_video_codecs);
   }
 
   if (session_options.raw_packetization_for_video) {
@@ -2900,27 +2925,11 @@ void MediaSessionDescriptionFactory::ComputeAudioCodecsIntersectionAndUnion() {
 
 void MediaSessionDescriptionFactory::ComputeVideoCodecsIntersectionAndUnion() {
   video_sendrecv_codecs_.clear();
-  all_video_codecs_.clear();
-  // Compute the video codecs union.
-  for (const VideoCodec& send : video_send_codecs_) {
-    all_video_codecs_.push_back(send);
-    if (!FindMatchingCodec<VideoCodec>(video_send_codecs_, video_recv_codecs_,
-                                       send, nullptr)) {
-      // TODO(kron): This check is violated by the unit test:
-      // MediaSessionDescriptionFactoryTest.RtxWithoutApt
-      // Remove either the test or the check.
 
-      // It doesn't make sense to have an RTX codec we support sending but not
-      // receiving.
-      // RTC_DCHECK(!IsRtxCodec(send));
-    }
-  }
-  for (const VideoCodec& recv : video_recv_codecs_) {
-    if (!FindMatchingCodec<VideoCodec>(video_recv_codecs_, video_send_codecs_,
-                                       recv, nullptr)) {
-      all_video_codecs_.push_back(recv);
-    }
-  }
+  // Use ComputeCodecsUnion to avoid having duplicate payload IDs
+  all_video_codecs_ =
+      ComputeCodecsUnion(video_recv_codecs_, video_send_codecs_);
+
   // Use NegotiateCodecs to merge our codec lists, since the operation is
   // essentially the same. Put send_codecs as the offered_codecs, which is the
   // order we'd like to follow. The reasoning is that encoding is usually more

pc/peer_connection_jsep_unittest.cc

@@ -505,6 +505,42 @@ TEST_F(PeerConnectionJsepTest, SetRemoteAnswerUpdatesCurrentDirection) {
             transceivers[0]->current_direction());
 }
 
+TEST_F(PeerConnectionJsepTest,
+       ChangeDirectionFromRecvOnlyToSendRecvDoesNotBreakVideoNegotiation) {
+  auto caller = CreatePeerConnection();
+  auto caller_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO);
+  auto callee = CreatePeerConnection();
+  caller_transceiver->SetDirectionWithError(RtpTransceiverDirection::kRecvOnly);
+
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  ASSERT_TRUE(
+      caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
+
+  caller_transceiver->SetDirectionWithError(RtpTransceiverDirection::kSendRecv);
+
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  ASSERT_TRUE(
+      caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
+}
+
+TEST_F(PeerConnectionJsepTest,
+       ChangeDirectionFromRecvOnlyToSendRecvDoesNotBreakAudioNegotiation) {
+  auto caller = CreatePeerConnection();
+  auto caller_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO);
+  auto callee = CreatePeerConnection();
+  caller_transceiver->SetDirectionWithError(RtpTransceiverDirection::kRecvOnly);
+
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  ASSERT_TRUE(
+      caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
+
+  caller_transceiver->SetDirectionWithError(RtpTransceiverDirection::kSendRecv);
+
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
+  ASSERT_TRUE(
+      caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
+}
+
 // Tests for multiple round trips.
 
 // Test that setting a transceiver with the inactive direction does not stop it