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

@@ -694,6 +694,8 @@ void RTCRtpReceiver::UpdateTransport() {
   }
 
   UniquePtr<MediaPipelineFilter> filter;
+  bool signalingStable =
+      (mPc->GetSignalingState() == RTCSignalingState::Stable);
 
   auto const& details = GetJsepTransceiver().mRecvTrack.GetNegotiatedDetails();
   std::vector<webrtc::RtpExtension> 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);
   }
 }
 

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() {

dom/media/webrtc/jsep/JsepTrack.cpp

@@ -731,7 +731,7 @@ void JsepTrack::SetUniqueReceivePayloadTypes(std::vector<JsepTrack*>& tracks,
                                              bool localOffer) {
   // Maps to track details if no other track contains the payload type,
   // otherwise maps to nullptr.
-  std::map<uint16_t, JsepTrack*> payloadTypeToDetailsMap;
+  std::map<uint16_t, std::tuple<JsepTrack*, bool>> payloadTypeToDetailsMap;
 
   for (JsepTrack* track : tracks) {
     if (track->GetMediaType() == SdpMediaSection::kApplication) {
@@ -751,23 +751,24 @@ void JsepTrack::SetUniqueReceivePayloadTypes(std::vector<JsepTrack*>& tracks,
     }
 
     for (uint16_t pt : payloadTypesForTrack) {
-      if (payloadTypeToDetailsMap.count(pt)) {
+      payloadTypeToDetailsMap[pt] =
-        // Found in more than one track, not unique
+          std::make_tuple(track, !payloadTypeToDetailsMap.count(pt));
-        payloadTypeToDetailsMap[pt] = nullptr;
-      } else {
-        payloadTypeToDetailsMap[pt] = track;
-      }
     }
   }
 
   for (auto ptAndDetails : payloadTypeToDetailsMap) {
     uint16_t uniquePt = ptAndDetails.first;
     MOZ_ASSERT(uniquePt <= UINT8_MAX);
-    auto* trackDetails = ptAndDetails.second;
+    auto* trackDetails = std::get<JsepTrack*>(ptAndDetails.second);
 
     if (trackDetails) {
-      trackDetails->mUniqueReceivePayloadTypes.push_back(
+      if (std::get<bool>(ptAndDetails.second)) {
-          static_cast<uint8_t>(uniquePt));
+        trackDetails->mUniqueReceivePayloadTypes.push_back(
+            static_cast<uint8_t>(uniquePt));
+      } else {
+        trackDetails->mDuplicateReceivePayloadTypes.push_back(
+            static_cast<uint8_t>(uniquePt));
+      }
     }
   }
 }

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<uint8_t> GetDuplicateReceivePayloadTypes() const {
+    return mDuplicateReceivePayloadTypes;
+  }
+
  private:
   std::vector<UniquePtr<JsepCodecDescription>> 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<uint8_t> mUniqueReceivePayloadTypes;
   std::vector<uint16_t> mReceivePayloadTypes;
+  // Payload types that are duplicate
+  std::vector<uint8_t> mDuplicateReceivePayloadTypes;
 };
 
 }  // namespace mozilla

dom/media/webrtc/transportbridge/MediaPipeline.cpp

@@ -298,17 +298,21 @@ void MediaPipeline::DetachTransport_s() {
   mReceiverRtcpSendEventListener.DisconnectIfExists();
 }
 
-void MediaPipeline::UpdateTransport_m(
+void MediaPipeline::UpdateTransport_m(const std::string& aTransportId,
-    const std::string& aTransportId, UniquePtr<MediaPipelineFilter>&& aFilter) {
+                                      UniquePtr<MediaPipelineFilter>&& aFilter,
+                                      bool aSignalingStable) {
   mStsThread->Dispatch(NS_NewRunnableFunction(
-      __func__, [aTransportId, filter = std::move(aFilter),
+      __func__,
-                 self = RefPtr<MediaPipeline>(this)]() mutable {
+      [aTransportId, filter = std::move(aFilter),
-        self->UpdateTransport_s(aTransportId, std::move(filter));
+       self = RefPtr<MediaPipeline>(this), aSignalingStable]() mutable {
+        self->UpdateTransport_s(aTransportId, std::move(filter),
+                                aSignalingStable);
       }));
 }
 
-void MediaPipeline::UpdateTransport_s(
+void MediaPipeline::UpdateTransport_s(const std::string& aTransportId,
-    const std::string& aTransportId, UniquePtr<MediaPipelineFilter>&& aFilter) {
+                                      UniquePtr<MediaPipelineFilter>&& 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);
   }

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<MediaPipelineFilter>&& aFilter);
+                         UniquePtr<MediaPipelineFilter>&& aFilter,
+                         bool aSignalingStable);
 
   void UpdateTransport_s(const std::string& aTransportId,
-                         UniquePtr<MediaPipelineFilter>&& aFilter);
+                         UniquePtr<MediaPipelineFilter>&& aFilter,
+                         bool aSignalingStable);
 
   virtual DirectionType Direction() const { return mDirection; }
   size_t Level() const { return mLevel; }

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;

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<webrtc::RtpExtension> GetExtmap() const { return mExtMap; }
 
@@ -77,6 +80,7 @@ class MediaPipelineFilter {
   // for readability.
   std::set<uint32_t> remote_ssrc_set_;
   std::set<uint8_t> receive_payload_type_set_;
+  std::set<uint8_t> duplicate_payload_type_set_;
   Maybe<std::string> mRemoteMid;
   std::set<uint32_t> mRemoteMidBindings;
   // RID extension can be set by tests and is sticky, the rest of

media/webrtc/signaling/gtest/mediapipeline_unittest.cpp

@@ -290,7 +290,7 @@ class TestAgent {
 
   void UpdateTransport_s(const std::string& aTransportId,
                          UniquePtr<MediaPipelineFilter>&& 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<MediaPipelineFilter>&& 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));
 }