From 4aeab708bb9bd41f5626b05965bfd13fe8a6c458 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 17 Feb 2025 18:06:32 +0000 Subject: [PATCH] Reland "Fix codec collision on reoffer after munged codec on offer." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 20bd702ebeb13a709832463fe5aadd623b7dc71b. Reason for revert: Fixed test to not fail when AV1 is missing Original change's description: > Revert "Fix codec collision on reoffer after munged codec on offer." > > This reverts commit b9ddaa154b91b5d1cbe38bf38fce544a87e00d1a. > > Reason for revert: Downstream failure. > > Original change's description: > > 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 > > Commit-Queue: Harald Alvestrand > > Cr-Commit-Position: refs/heads/main@{#43889} > > Bug: chromium:395077842 > Change-Id: I10184a0d521add123838861a5c5e7929864537bb > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/377500 > Bot-Commit: rubber-stamper@appspot.gserviceaccount.com > Reviewed-by: Jeremy Leconte > Commit-Queue: Harald Alvestrand > Reviewed-by: Henrik Boström > Cr-Commit-Position: refs/heads/main@{#43901} Bug: chromium:395077842 Change-Id: I3ba5cacebc7eb608edffea2de54cf1c1e9355a81 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/377281 Reviewed-by: Henrik Boström Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#43907} --- pc/codec_vendor.cc | 14 ++++++-- pc/peer_connection_integrationtest.cc | 44 +++++++++++++++++++++++++ pc/test/integration_test_helpers.h | 47 ++++++++++++++------------- 3 files changed, 80 insertions(+), 25 deletions(-) diff --git a/pc/codec_vendor.cc b/pc/codec_vendor.cc index 0ab676a9cd..529246c354 100644 --- a/pc/codec_vendor.cc +++ b/pc/codec_vendor.cc @@ -454,6 +454,14 @@ webrtc::RTCErrorOr> 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 found_codec = @@ -461,12 +469,12 @@ webrtc::RTCErrorOr> 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. - // 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 @@ webrtc::RTCErrorOr> 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); } } diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 20643c2f29..ce02e516e8 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -4731,6 +4731,50 @@ 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& sdp) { + auto video = GetFirstVideoContentDescription(sdp->description()); + auto codecs = video->codecs(); + std::optional replacement_codec; + for (auto&& codec : codecs) { + if (codec.name == "AV1") { + replacement_codec = codec; + break; + } + } + if (replacement_codec) { + for (auto&& codec : codecs) { + if (codec.name == "VP9") { + RTC_LOG(LS_INFO) << "Remapping VP9 codec " << codec << " to AV1"; + codec.name = replacement_codec->name; + codec.params = replacement_codec->params; + break; + } + } + video->set_codecs(codecs); + } else { + RTC_LOG(LS_INFO) << "Skipping munge, no AV1 codec found"; + } + }; + 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 0b7c8b5de0..9aa72883d6 100644 --- a/pc/test/integration_test_helpers.h +++ b/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 desc) { + auto observer = rtc::make_ref_counted(); + 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 desc) { - auto observer = rtc::make_ref_counted(); - 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.