From dd5f2aa6b4b2f89eb35e4e912be86b5af185b81a Mon Sep 17 00:00:00 2001 From: Dan Baker Date: Thu, 20 Mar 2025 14:57:12 +0000 Subject: [PATCH] Bug 1949282 - Add to known unique PTs instead of replacing when signaling is not stable.;r=bwc Depends on D238836 Differential Revision: https://phabricator.services.mozilla.com/D241311 --- dom/media/webrtc/jsapi/RTCRtpReceiver.cpp | 16 +++++++++++--- dom/media/webrtc/jsapi/RTCRtpSender.cpp | 2 +- dom/media/webrtc/jsep/JsepTrack.cpp | 21 +++++++++--------- dom/media/webrtc/jsep/JsepTrack.h | 7 ++++++ .../webrtc/transportbridge/MediaPipeline.cpp | 20 ++++++++++------- .../webrtc/transportbridge/MediaPipeline.h | 6 +++-- .../transportbridge/MediaPipelineFilter.cpp | 22 +++++++++++++++++-- .../transportbridge/MediaPipelineFilter.h | 6 ++++- .../gtest/mediapipeline_unittest.cpp | 13 ++++++----- 9 files changed, 80 insertions(+), 33 deletions(-) diff --git a/dom/media/webrtc/jsapi/RTCRtpReceiver.cpp b/dom/media/webrtc/jsapi/RTCRtpReceiver.cpp index e9867e2b596b..cac29d80934a 100644 --- a/dom/media/webrtc/jsapi/RTCRtpReceiver.cpp +++ b/dom/media/webrtc/jsapi/RTCRtpReceiver.cpp @@ -694,6 +694,8 @@ void RTCRtpReceiver::UpdateTransport() { } UniquePtr filter; + bool signalingStable = + (mPc->GetSignalingState() == RTCSignalingState::Stable); auto const& details = GetJsepTransceiver().mRecvTrack.GetNegotiatedDetails(); std::vector extmaps; @@ -724,14 +726,22 @@ void RTCRtpReceiver::UpdateTransport() { // Add unique payload types as a last-ditch fallback auto uniquePts = GetJsepTransceiver().mRecvTrack.GetUniqueReceivePayloadTypes(); - for (unsigned char& uniquePt : uniquePts) { + for (auto uniquePt : uniquePts) { filter->AddUniqueReceivePT(uniquePt); } + + // Add duplicate payload types + auto duplicatePts = + GetJsepTransceiver().mRecvTrack.GetDuplicateReceivePayloadTypes(); + + for (auto duplicatePt : duplicatePts) { + filter->AddDuplicateReceivePT(duplicatePt); + } } - if ((mPc->GetSignalingState() == RTCSignalingState::Stable) || filter) { + if (signalingStable || filter) { mPipeline->UpdateTransport_m(GetJsepTransceiver().mTransport.mTransportId, - std::move(filter)); + std::move(filter), signalingStable); } } diff --git a/dom/media/webrtc/jsapi/RTCRtpSender.cpp b/dom/media/webrtc/jsapi/RTCRtpSender.cpp index 4e084ec23e20..44debeb10625 100644 --- a/dom/media/webrtc/jsapi/RTCRtpSender.cpp +++ b/dom/media/webrtc/jsapi/RTCRtpSender.cpp @@ -1393,7 +1393,7 @@ void RTCRtpSender::UpdateTransport() { } mPipeline->UpdateTransport_m(GetJsepTransceiver().mTransport.mTransportId, - nullptr); + nullptr, true); } void RTCRtpSender::MaybeUpdateConduit() { diff --git a/dom/media/webrtc/jsep/JsepTrack.cpp b/dom/media/webrtc/jsep/JsepTrack.cpp index a74578150c4d..13620bf9ed09 100644 --- a/dom/media/webrtc/jsep/JsepTrack.cpp +++ b/dom/media/webrtc/jsep/JsepTrack.cpp @@ -731,7 +731,7 @@ void JsepTrack::SetUniqueReceivePayloadTypes(std::vector& tracks, bool localOffer) { // Maps to track details if no other track contains the payload type, // otherwise maps to nullptr. - std::map payloadTypeToDetailsMap; + std::map> payloadTypeToDetailsMap; for (JsepTrack* track : tracks) { if (track->GetMediaType() == SdpMediaSection::kApplication) { @@ -751,23 +751,24 @@ void JsepTrack::SetUniqueReceivePayloadTypes(std::vector& tracks, } for (uint16_t pt : payloadTypesForTrack) { - if (payloadTypeToDetailsMap.count(pt)) { - // Found in more than one track, not unique - payloadTypeToDetailsMap[pt] = nullptr; - } else { - payloadTypeToDetailsMap[pt] = track; - } + payloadTypeToDetailsMap[pt] = + std::make_tuple(track, !payloadTypeToDetailsMap.count(pt)); } } for (auto ptAndDetails : payloadTypeToDetailsMap) { uint16_t uniquePt = ptAndDetails.first; MOZ_ASSERT(uniquePt <= UINT8_MAX); - auto* trackDetails = ptAndDetails.second; + auto* trackDetails = std::get(ptAndDetails.second); if (trackDetails) { - trackDetails->mUniqueReceivePayloadTypes.push_back( - static_cast(uniquePt)); + if (std::get(ptAndDetails.second)) { + trackDetails->mUniqueReceivePayloadTypes.push_back( + static_cast(uniquePt)); + } else { + trackDetails->mDuplicateReceivePayloadTypes.push_back( + static_cast(uniquePt)); + } } } } diff --git a/dom/media/webrtc/jsep/JsepTrack.h b/dom/media/webrtc/jsep/JsepTrack.h index 37244739a088..eba86eb60ece 100644 --- a/dom/media/webrtc/jsep/JsepTrack.h +++ b/dom/media/webrtc/jsep/JsepTrack.h @@ -136,6 +136,7 @@ class JsepTrack { mVideoPreferredCodec = rhs.mVideoPreferredCodec; mUniqueReceivePayloadTypes = rhs.mUniqueReceivePayloadTypes; mReceivePayloadTypes = rhs.mReceivePayloadTypes; + mDuplicateReceivePayloadTypes = rhs.mDuplicateReceivePayloadTypes; mPrototypeCodecs.clear(); for (const auto& codec : rhs.mPrototypeCodecs) { @@ -264,6 +265,10 @@ class JsepTrack { return mUniqueReceivePayloadTypes; } + std::vector GetDuplicateReceivePayloadTypes() const { + return mDuplicateReceivePayloadTypes; + } + private: std::vector> GetCodecClones() const; static void EnsureNoDuplicatePayloadTypes( @@ -336,6 +341,8 @@ class JsepTrack { // Used for matching SSRC to PT as only unique PTs support for this. std::vector mUniqueReceivePayloadTypes; std::vector mReceivePayloadTypes; + // Payload types that are duplicate + std::vector mDuplicateReceivePayloadTypes; }; } // namespace mozilla diff --git a/dom/media/webrtc/transportbridge/MediaPipeline.cpp b/dom/media/webrtc/transportbridge/MediaPipeline.cpp index d01f70a7f5b9..d1a7b06b39ef 100644 --- a/dom/media/webrtc/transportbridge/MediaPipeline.cpp +++ b/dom/media/webrtc/transportbridge/MediaPipeline.cpp @@ -298,17 +298,21 @@ void MediaPipeline::DetachTransport_s() { mReceiverRtcpSendEventListener.DisconnectIfExists(); } -void MediaPipeline::UpdateTransport_m( - const std::string& aTransportId, UniquePtr&& aFilter) { +void MediaPipeline::UpdateTransport_m(const std::string& aTransportId, + UniquePtr&& aFilter, + bool aSignalingStable) { mStsThread->Dispatch(NS_NewRunnableFunction( - __func__, [aTransportId, filter = std::move(aFilter), - self = RefPtr(this)]() mutable { - self->UpdateTransport_s(aTransportId, std::move(filter)); + __func__, + [aTransportId, filter = std::move(aFilter), + self = RefPtr(this), aSignalingStable]() mutable { + self->UpdateTransport_s(aTransportId, std::move(filter), + aSignalingStable); })); } -void MediaPipeline::UpdateTransport_s( - const std::string& aTransportId, UniquePtr&& aFilter) { +void MediaPipeline::UpdateTransport_s(const std::string& aTransportId, + UniquePtr&& aFilter, + bool aSignalingStable) { ASSERT_ON_THREAD(mStsThread); if (!mSignalsConnected) { mTransportHandler->SignalStateChange.connect( @@ -339,7 +343,7 @@ void MediaPipeline::UpdateTransport_s( if (mFilter && aFilter) { // Use the new filter, but don't forget any remote SSRCs that we've learned // by receiving traffic. - mFilter->Update(*aFilter); + mFilter->Update(*aFilter, aSignalingStable); } else { mFilter = std::move(aFilter); } diff --git a/dom/media/webrtc/transportbridge/MediaPipeline.h b/dom/media/webrtc/transportbridge/MediaPipeline.h index 513680d0fc6a..b8d72a3566ce 100644 --- a/dom/media/webrtc/transportbridge/MediaPipeline.h +++ b/dom/media/webrtc/transportbridge/MediaPipeline.h @@ -112,10 +112,12 @@ class MediaPipeline : public sigslot::has_slots<> { virtual void Shutdown(); void UpdateTransport_m(const std::string& aTransportId, - UniquePtr&& aFilter); + UniquePtr&& aFilter, + bool aSignalingStable); void UpdateTransport_s(const std::string& aTransportId, - UniquePtr&& aFilter); + UniquePtr&& aFilter, + bool aSignalingStable); virtual DirectionType Direction() const { return mDirection; } size_t Level() const { return mLevel; } diff --git a/dom/media/webrtc/transportbridge/MediaPipelineFilter.cpp b/dom/media/webrtc/transportbridge/MediaPipelineFilter.cpp index fce96c1213bf..e8c5662c5890 100644 --- a/dom/media/webrtc/transportbridge/MediaPipelineFilter.cpp +++ b/dom/media/webrtc/transportbridge/MediaPipelineFilter.cpp @@ -130,7 +130,12 @@ void MediaPipelineFilter::AddUniqueReceivePT(uint8_t payload_type) { receive_payload_type_set_.insert(payload_type); } -void MediaPipelineFilter::Update(const MediaPipelineFilter& filter_update) { +void MediaPipelineFilter::AddDuplicateReceivePT(uint8_t payload_type) { + duplicate_payload_type_set_.insert(payload_type); +} + +void MediaPipelineFilter::Update(const MediaPipelineFilter& filter_update, + bool signalingStable) { // We will not stomp the remote_ssrc_set_ if the update has no ssrcs, // because we don't want to unlearn any remote ssrcs unless the other end // has explicitly given us a new set. @@ -144,7 +149,20 @@ void MediaPipelineFilter::Update(const MediaPipelineFilter& filter_update) { mRemoteMid = filter_update.mRemoteMid; mRemoteMidBindings = filter_update.mRemoteMidBindings; } - receive_payload_type_set_ = filter_update.receive_payload_type_set_; + + // If we are signaling is stable replace the filters values otherwise add to + // them. + if (signalingStable) { + receive_payload_type_set_ = filter_update.receive_payload_type_set_; + duplicate_payload_type_set_ = filter_update.duplicate_payload_type_set_; + } else { + for (const auto& uniquePT : filter_update.receive_payload_type_set_) { + if (!receive_payload_type_set_.count(uniquePT) && + !duplicate_payload_type_set_.count(uniquePT)) { + AddUniqueReceivePT(uniquePT); + } + } + } // Use extmapping from new filter mExtMap = filter_update.mExtMap; diff --git a/dom/media/webrtc/transportbridge/MediaPipelineFilter.h b/dom/media/webrtc/transportbridge/MediaPipelineFilter.h index 62d4b63ab850..70d0787577ec 100644 --- a/dom/media/webrtc/transportbridge/MediaPipelineFilter.h +++ b/dom/media/webrtc/transportbridge/MediaPipelineFilter.h @@ -68,7 +68,10 @@ class MediaPipelineFilter { // When a payload type id is unique to our media section, add it here. void AddUniqueReceivePT(uint8_t payload_type); - void Update(const MediaPipelineFilter& filter_update); + // When a payload type id is NOT unique to our media section, add it here. + void AddDuplicateReceivePT(uint8_t payload_type); + + void Update(const MediaPipelineFilter& filter_update, bool signalingStable); std::vector GetExtmap() const { return mExtMap; } @@ -77,6 +80,7 @@ class MediaPipelineFilter { // for readability. std::set remote_ssrc_set_; std::set receive_payload_type_set_; + std::set duplicate_payload_type_set_; Maybe mRemoteMid; std::set mRemoteMidBindings; // RID extension can be set by tests and is sticky, the rest of diff --git a/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp b/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp index 5dd7021e0418..db5ce6c6a3c0 100644 --- a/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp +++ b/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp @@ -290,7 +290,7 @@ class TestAgent { void UpdateTransport_s(const std::string& aTransportId, UniquePtr&& aFilter) { - audio_pipeline_->UpdateTransport_s(aTransportId, std::move(aFilter)); + audio_pipeline_->UpdateTransport_s(aTransportId, std::move(aFilter), false); } void Stop() { @@ -380,7 +380,7 @@ class TestAgentSend : public TestAgent { audio_pipeline->SetSendTrackOverride(audio_track_); control_.Update([](auto& aControl) { aControl.mTransmitting = true; }); - audio_pipeline->UpdateTransport_m(aTransportId, nullptr); + audio_pipeline->UpdateTransport_m(aTransportId, nullptr, true); audio_pipeline_ = audio_pipeline; } }; @@ -411,7 +411,8 @@ class TestAgentReceive : public TestAgent { })); control_.Update([](auto& aControl) { aControl.mReceiving = true; }); - audio_pipeline->UpdateTransport_m(aTransportId, std::move(bundle_filter_)); + audio_pipeline->UpdateTransport_m(aTransportId, std::move(bundle_filter_), + true); audio_pipeline_ = audio_pipeline; } @@ -421,7 +422,7 @@ class TestAgentReceive : public TestAgent { void UpdateTransport_s(const std::string& aTransportId, UniquePtr&& filter) { - audio_pipeline_->UpdateTransport_s(aTransportId, std::move(filter)); + audio_pipeline_->UpdateTransport_s(aTransportId, std::move(filter), false); } private: @@ -684,7 +685,7 @@ TEST_F(MediaPipelineFilterTest, TestRemoteSDPNoSSRCs) { // Update but remember binding./ MediaPipelineFilter filter2; - filter.Update(filter2); + filter.Update(filter2, true); // Ensure that the old SSRC still works. EXPECT_TRUE(Filter(filter, 555, 110)); @@ -692,7 +693,7 @@ TEST_F(MediaPipelineFilterTest, TestRemoteSDPNoSSRCs) { // Forget the previous binding MediaPipelineFilter filter3; filter3.SetRemoteMediaStreamId(Some(std::string("mid1"))); - filter.Update(filter3); + filter.Update(filter3, true); ASSERT_FALSE(Filter(filter, 555, 110)); }