Fix codec collision on reoffer after munged codec on offer.

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

pc/codec_vendor.cc

@@ -454,6 +454,14 @@ webrtc::RTCErrorOr<std::vector<Codec>> CodecVendor::GetNegotiatedCodecsForOffer(
         }
       }
     }
+    // Note what PTs are already in use.
+    UsedPayloadTypes
+        used_pltypes;  // Used to avoid pt collisions in filtered_codecs
+    for (auto& codec : filtered_codecs) {
+      // Note: This may change PTs. Doing so woud indicate an error, but
+      // UsedPayloadTypes doesn't offer a means to make the distinction.
+      used_pltypes.FindAndSetIdUsed(&codec);
+    }
     // Add other supported codecs.
     for (const Codec& codec : supported_codecs) {
       std::optional<Codec> found_codec =
@@ -461,12 +469,12 @@ webrtc::RTCErrorOr<std::vector<Codec>> CodecVendor::GetNegotiatedCodecsForOffer(
       if (found_codec &&
           !FindMatchingCodec(supported_codecs, filtered_codecs, codec)) {
         // Use the `found_codec` from `codecs` because it has the
-        // correctly mapped payload type.
+        // correctly mapped payload type (most of the time).
-        // This is only done for video since we do not yet have rtx for audio.
         if (media_description_options.type == MEDIA_TYPE_VIDEO &&
             found_codec->GetResiliencyType() == Codec::ResiliencyType::kRtx) {
           // For RTX we might need to adjust the apt parameter if we got a
           // remote offer without RTX for a codec for which we support RTX.
+          // This is only done for video since we do not yet have rtx for audio.
           auto referenced_codec =
               GetAssociatedCodecForRtx(supported_codecs, codec);
           RTC_DCHECK(referenced_codec);
@@ -479,6 +487,8 @@ webrtc::RTCErrorOr<std::vector<Codec>> CodecVendor::GetNegotiatedCodecsForOffer(
                                   changed_referenced_codec->id);
           }
         }
+        // Quick fix for b/395077842: Remap the codec if it collides.
+        used_pltypes.FindAndSetIdUsed(&(*found_codec));
         filtered_codecs.push_back(*found_codec);
       }
     }

pc/peer_connection_integrationtest.cc

@@ -4731,6 +4731,37 @@ TEST_F(PeerConnectionIntegrationTestUnifiedPlan,
   EXPECT_LT(metrics::MinSample("WebRTC.Call.AbsCapture.Offset"), 2);
 }
 
+// Test that when SDP is munged to use a PT for a different codec,
+// the old codec is added to a subsequent offer with a different PT
+// Regression test for https://issues.chromium.org/395077824
+TEST_F(PeerConnectionIntegrationTestUnifiedPlan,
+       MungeOfferCodecAndReOfferWorks) {
+  ASSERT_TRUE(CreatePeerConnectionWrappers());
+  ConnectFakeSignaling();
+  caller()->AddVideoTrack();
+  auto munger = [](std::unique_ptr<SessionDescriptionInterface>& sdp) {
+    auto video = GetFirstVideoContentDescription(sdp->description());
+    auto codecs = video->codecs();
+    for (auto&& codec : codecs) {
+      if (codec.name == "VP9") {
+        RTC_LOG(LS_ERROR) << "Remapping VP9 codec " << codec << " to AV1";
+        codec.name = "AV1";
+      }
+    }
+    video->set_codecs(codecs);
+  };
+  caller()->SetGeneratedSdpMunger(munger);
+  caller()->CreateAndSetAndSignalOffer();
+  ASSERT_THAT(
+      WaitUntil([&] { return SignalingStateStable(); }, ::testing::IsTrue()),
+      IsRtcOk());
+  caller()->SetGeneratedSdpMunger(nullptr);
+  auto offer = caller()->CreateOfferAndWait();
+  ASSERT_NE(nullptr, offer);
+  // The offer should be acceptable.
+  EXPECT_TRUE(caller()->SetLocalDescriptionAndSendSdpMessage(std::move(offer)));
+}
+
 }  // namespace
 
 }  // namespace webrtc

pc/test/integration_test_helpers.h

@@ -746,6 +746,30 @@ class PeerConnectionIntegrationWrapper : public PeerConnectionObserver,
     });
   }
 
+  // Setting the local description and sending the SDP message over the fake
+  // signaling channel are combined into the same method because the SDP
+  // message needs to be sent as soon as SetLocalDescription finishes, without
+  // waiting for the observer to be called. This ensures that ICE candidates
+  // don't outrace the description.
+  bool SetLocalDescriptionAndSendSdpMessage(
+      std::unique_ptr<SessionDescriptionInterface> desc) {
+    auto observer = rtc::make_ref_counted<MockSetSessionDescriptionObserver>();
+    RTC_LOG(LS_INFO) << debug_name_ << ": SetLocalDescriptionAndSendSdpMessage";
+    SdpType type = desc->GetType();
+    std::string sdp;
+    EXPECT_TRUE(desc->ToString(&sdp));
+    RTC_LOG(LS_INFO) << debug_name_ << ": local SDP contents=\n" << sdp;
+    pc()->SetLocalDescription(observer.get(), desc.release());
+    RemoveUnusedVideoRenderers();
+    // As mentioned above, we need to send the message immediately after
+    // SetLocalDescription.
+    SendSdpMessage(type, sdp);
+    EXPECT_THAT(
+        WaitUntil([&] { return observer->called(); }, ::testing::IsTrue()),
+        IsRtcOk());
+    return true;
+  }
+
  private:
   // Constructor used by friend class PeerConnectionIntegrationBaseTest.
   explicit PeerConnectionIntegrationWrapper(const std::string& debug_name)
@@ -868,29 +892,6 @@ class PeerConnectionIntegrationWrapper : public PeerConnectionObserver,
     return description;
   }
 
-  // Setting the local description and sending the SDP message over the fake
-  // signaling channel are combined into the same method because the SDP
-  // message needs to be sent as soon as SetLocalDescription finishes, without
-  // waiting for the observer to be called. This ensures that ICE candidates
-  // don't outrace the description.
-  bool SetLocalDescriptionAndSendSdpMessage(
-      std::unique_ptr<SessionDescriptionInterface> desc) {
-    auto observer = rtc::make_ref_counted<MockSetSessionDescriptionObserver>();
-    RTC_LOG(LS_INFO) << debug_name_ << ": SetLocalDescriptionAndSendSdpMessage";
-    SdpType type = desc->GetType();
-    std::string sdp;
-    EXPECT_TRUE(desc->ToString(&sdp));
-    RTC_LOG(LS_INFO) << debug_name_ << ": local SDP contents=\n" << sdp;
-    pc()->SetLocalDescription(observer.get(), desc.release());
-    RemoveUnusedVideoRenderers();
-    // As mentioned above, we need to send the message immediately after
-    // SetLocalDescription.
-    SendSdpMessage(type, sdp);
-    EXPECT_THAT(
-        WaitUntil([&] { return observer->called(); }, ::testing::IsTrue()),
-        IsRtcOk());
-    return true;
-  }
 
   // This is a work around to remove unused fake_video_renderers from
   // transceivers that have either stopped or are no longer receiving.