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}
diff --git a/pc/codec_vendor.cc b/pc/codec_vendor.cc
index 0ab676a..529246c 100644
--- a/pc/codec_vendor.cc
+++ b/pc/codec_vendor.cc
@@ -454,6 +454,14 @@
}
}
}
+ // 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 @@
if (found_codec &&
!FindMatchingCodec(supported_codecs, filtered_codecs, codec)) {
// Use the `found_codec` from `codecs` because it has the
- // correctly mapped payload type.
- // This is only done for video since we do not yet have rtx for audio.
+ // correctly mapped payload type (most of the time).
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 @@
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);
}
}
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc
index 20643c2..2e7786d 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -4731,6 +4731,37 @@
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
diff --git a/pc/test/integration_test_helpers.h b/pc/test/integration_test_helpers.h
index 0b7c8b5..9aa7288 100644
--- a/pc/test/integration_test_helpers.h
+++ b/pc/test/integration_test_helpers.h
@@ -746,6 +746,30 @@
});
}
+ // 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 @@
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.
