Add rtp_config() accessor to ReceiveStream.

This is a consistent way to get to common config parameters for
all receive streams and avoids storing a copy of the extension
headers inside of Call. This is needed to get rid of the need of
keeping config and copies in sync, which currently is part of why
we repeatedly delete and recreate audio receive streams on config
changes.

Bug: webrtc:11993
Change-Id: Ia356b6cac1425c8c6766abd2e52fdeb73c4a4b4f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/222040
Commit-Queue: Tommi <tommi@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34285}

audio/audio_receive_stream.h

@@ -84,6 +84,7 @@ class AudioReceiveStream final : public webrtc::AudioReceiveStream,
   // webrtc::AudioReceiveStream implementation.
   void Start() override;
   void Stop() override;
+  const RtpConfig& rtp_config() const override { return config_.rtp; }
   bool IsRunning() const override;
   void SetDepacketizerToDecoderFrameTransformer(
       rtc::scoped_refptr<webrtc::FrameTransformerInterface> frame_transformer)

call/call.cc

@@ -81,12 +81,10 @@ bool SendPeriodicFeedback(const std::vector<RtpExtension>& extensions) {
   return true;
 }
 
-// TODO(nisse): This really begs for a shared context struct.
+bool UseSendSideBwe(const ReceiveStream::RtpConfig& rtp) {
-bool UseSendSideBwe(const std::vector<RtpExtension>& extensions,
+  if (!rtp.transport_cc)
-                    bool transport_cc) {
-  if (!transport_cc)
     return false;
-  for (const auto& extension : extensions) {
+  for (const auto& extension : rtp.extensions) {
     if (extension.uri == RtpExtension::kTransportSequenceNumberUri ||
         extension.uri == RtpExtension::kTransportSequenceNumberV2Uri)
       return true;
@@ -94,18 +92,6 @@ bool UseSendSideBwe(const std::vector<RtpExtension>& extensions,
   return false;
 }
 
-bool UseSendSideBwe(const VideoReceiveStream::Config& config) {
-  return UseSendSideBwe(config.rtp.extensions, config.rtp.transport_cc);
-}
-
-bool UseSendSideBwe(const AudioReceiveStream::Config& config) {
-  return UseSendSideBwe(config.rtp.extensions, config.rtp.transport_cc);
-}
-
-bool UseSendSideBwe(const FlexfecReceiveStream::Config& config) {
-  return UseSendSideBwe(config.rtp.extensions, config.rtp.transport_cc);
-}
-
 const int* FindKeyByValue(const std::map<int, int>& m, int v) {
   for (const auto& kv : m) {
     if (kv.second == v)
@@ -410,33 +396,9 @@ class Call final : public webrtc::Call,
   // This extra map is used for receive processing which is
   // independent of media type.
 
-  // TODO(nisse): In the RTP transport refactoring, we should have a
-  // single mapping from ssrc to a more abstract receive stream, with
-  // accessor methods for all configuration we need at this level.
-  struct ReceiveRtpConfig {
-    explicit ReceiveRtpConfig(const webrtc::AudioReceiveStream::Config& config)
-        : extensions(config.rtp.extensions),
-          use_send_side_bwe(UseSendSideBwe(config)) {}
-    explicit ReceiveRtpConfig(const webrtc::VideoReceiveStream::Config& config)
-        : extensions(config.rtp.extensions),
-          use_send_side_bwe(UseSendSideBwe(config)) {}
-    explicit ReceiveRtpConfig(const FlexfecReceiveStream::Config& config)
-        : extensions(config.rtp.extensions),
-          use_send_side_bwe(UseSendSideBwe(config)) {}
-
-    // Registered RTP header extensions for each stream. Note that RTP header
-    // extensions are negotiated per track ("m= line") in the SDP, but we have
-    // no notion of tracks at the Call level. We therefore store the RTP header
-    // extensions per SSRC instead, which leads to some storage overhead.
-    const RtpHeaderExtensionMap extensions;
-    // Set if both RTP extension the RTCP feedback message needed for
-    // send side BWE are negotiated.
-    const bool use_send_side_bwe;
-  };
-
   // TODO(bugs.webrtc.org/11993): Move receive_rtp_config_ over to the
   // network thread.
-  std::map<uint32_t, ReceiveRtpConfig> receive_rtp_config_
+  std::map<uint32_t, ReceiveStream*> receive_rtp_config_
       RTC_GUARDED_BY(worker_thread_);
 
   // Audio and Video send streams are owned by the client that creates them.
@@ -995,7 +957,7 @@ webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream(
   // TODO(bugs.webrtc.org/11993): Update the below on the network thread.
   // We could possibly set up the audio_receiver_controller_ association up
   // as part of the async setup.
-  receive_rtp_config_.emplace(config.rtp.remote_ssrc, ReceiveRtpConfig(config));
+  receive_rtp_config_.emplace(config.rtp.remote_ssrc, receive_stream);
 
   ConfigureSync(config.sync_group);
 
@@ -1023,9 +985,7 @@ void Call::DestroyAudioReceiveStream(
 
   uint32_t ssrc = audio_receive_stream->remote_ssrc();
   const AudioReceiveStream::Config& config = audio_receive_stream->config();
-  receive_side_cc_
+  receive_side_cc_.GetRemoteBitrateEstimator(UseSendSideBwe(config.rtp))
-      .GetRemoteBitrateEstimator(
-          UseSendSideBwe(config.rtp.extensions, config.rtp.transport_cc))
       ->RemoveStream(ssrc);
 
   audio_receive_streams_.erase(audio_receive_stream);
@@ -1177,9 +1137,9 @@ webrtc::VideoReceiveStream* Call::CreateVideoReceiveStream(
     // stream. Since the transport_send_cc negotiation is per payload
     // type, we may get an incorrect value for the rtx stream, but
     // that is unlikely to matter in practice.
-    receive_rtp_config_.emplace(config.rtp.rtx_ssrc, ReceiveRtpConfig(config));
+    receive_rtp_config_.emplace(config.rtp.rtx_ssrc, receive_stream);
   }
-  receive_rtp_config_.emplace(config.rtp.remote_ssrc, ReceiveRtpConfig(config));
+  receive_rtp_config_.emplace(config.rtp.remote_ssrc, receive_stream);
   video_receive_streams_.insert(receive_stream);
   ConfigureSync(config.sync_group);
 
@@ -1211,7 +1171,7 @@ void Call::DestroyVideoReceiveStream(
   video_receive_streams_.erase(receive_stream_impl);
   ConfigureSync(config.sync_group);
 
-  receive_side_cc_.GetRemoteBitrateEstimator(UseSendSideBwe(config))
+  receive_side_cc_.GetRemoteBitrateEstimator(UseSendSideBwe(config.rtp))
       ->RemoveStream(config.rtp.remote_ssrc);
 
   UpdateAggregateNetworkState();
@@ -1243,7 +1203,7 @@ FlexfecReceiveStream* Call::CreateFlexfecReceiveStream(
 
   RTC_DCHECK(receive_rtp_config_.find(config.rtp.remote_ssrc) ==
              receive_rtp_config_.end());
-  receive_rtp_config_.emplace(config.rtp.remote_ssrc, ReceiveRtpConfig(config));
+  receive_rtp_config_.emplace(config.rtp.remote_ssrc, receive_stream);
 
   // TODO(brandtr): Store config in RtcEventLog here.
 
@@ -1260,14 +1220,13 @@ void Call::DestroyFlexfecReceiveStream(FlexfecReceiveStream* receive_stream) {
   receive_stream_impl->UnregisterFromTransport();
 
   RTC_DCHECK(receive_stream != nullptr);
-  const FlexfecReceiveStream::Config& config = receive_stream->GetConfig();
+  const FlexfecReceiveStream::RtpConfig& rtp = receive_stream->rtp_config();
-  uint32_t ssrc = config.rtp.remote_ssrc;
+  receive_rtp_config_.erase(rtp.remote_ssrc);
-  receive_rtp_config_.erase(ssrc);
 
   // Remove all SSRCs pointing to the FlexfecReceiveStreamImpl to be
   // destroyed.
-  receive_side_cc_.GetRemoteBitrateEstimator(UseSendSideBwe(config))
+  receive_side_cc_.GetRemoteBitrateEstimator(UseSendSideBwe(rtp))
-      ->RemoveStream(ssrc);
+      ->RemoveStream(rtp.remote_ssrc);
 
   delete receive_stream;
 }
@@ -1591,7 +1550,8 @@ PacketReceiver::DeliveryStatus Call::DeliverRtp(MediaType media_type,
     return DELIVERY_UNKNOWN_SSRC;
   }
 
-  parsed_packet.IdentifyExtensions(it->second.extensions);
+  parsed_packet.IdentifyExtensions(
+      RtpHeaderExtensionMap(it->second->rtp_config().extensions));
 
   NotifyBweOfReceivedPacket(parsed_packet, media_type);
 
@@ -1656,7 +1616,8 @@ void Call::OnRecoveredPacket(const uint8_t* packet, size_t length) {
     // which is being torn down.
     return;
   }
-  parsed_packet.IdentifyExtensions(it->second.extensions);
+  parsed_packet.IdentifyExtensions(
+      RtpHeaderExtensionMap(it->second->rtp_config().extensions));
 
   // TODO(brandtr): Update here when we support protecting audio packets too.
   parsed_packet.set_payload_type_frequency(kVideoPayloadTypeFrequency);
@@ -1667,8 +1628,8 @@ void Call::OnRecoveredPacket(const uint8_t* packet, size_t length) {
 void Call::NotifyBweOfReceivedPacket(const RtpPacketReceived& packet,
                                      MediaType media_type) {
   auto it = receive_rtp_config_.find(packet.Ssrc());
-  bool use_send_side_bwe =
+  bool use_send_side_bwe = (it != receive_rtp_config_.end()) &&
-      (it != receive_rtp_config_.end()) && it->second.use_send_side_bwe;
+                           UseSendSideBwe(it->second->rtp_config());
 
   RTPHeader header;
   packet.GetHeader(&header);

call/flexfec_receive_stream.h

@@ -68,8 +68,6 @@ class FlexfecReceiveStream : public RtpPacketSinkInterface,
   };
 
   virtual Stats GetStats() const = 0;
-
-  virtual const Config& GetConfig() const = 0;
 };
 
 }  // namespace webrtc

call/flexfec_receive_stream_impl.cc

@@ -205,9 +205,4 @@ FlexfecReceiveStreamImpl::Stats FlexfecReceiveStreamImpl::GetStats() const {
   return FlexfecReceiveStream::Stats();
 }
 
-const FlexfecReceiveStream::Config& FlexfecReceiveStreamImpl::GetConfig()
-    const {
-  return config_;
-}
-
 }  // namespace webrtc

call/flexfec_receive_stream_impl.h

@@ -59,7 +59,9 @@ class FlexfecReceiveStreamImpl : public FlexfecReceiveStream {
   void OnRtpPacket(const RtpPacketReceived& packet) override;
 
   Stats GetStats() const override;
-  const Config& GetConfig() const override;
+
+  // ReceiveStream impl.
+  const RtpConfig& rtp_config() const override { return config_.rtp; }
 
  private:
   RTC_NO_UNIQUE_ADDRESS SequenceChecker packet_sequence_checker_;

call/receive_stream.h

@@ -28,21 +28,35 @@ class ReceiveStream {
   // Receive-stream specific RTP settings.
   struct RtpConfig {
     // Synchronization source (stream identifier) to be received.
+    // This member will not change mid-stream and can be assumed to be const
+    // post initialization.
     uint32_t remote_ssrc = 0;
 
     // Sender SSRC used for sending RTCP (such as receiver reports).
+    // This value may change mid-stream and must be done on the same thread
+    // that the value is read on (i.e. packet delivery).
     uint32_t local_ssrc = 0;
 
     // Enable feedback for send side bandwidth estimation.
     // See
     // https://tools.ietf.org/html/draft-holmer-rmcat-transport-wide-cc-extensions
     // for details.
+    // This value may change mid-stream and must be done on the same thread
+    // that the value is read on (i.e. packet delivery).
     bool transport_cc = false;
 
     // RTP header extensions used for the received stream.
+    // This value may change mid-stream and must be done on the same thread
+    // that the value is read on (i.e. packet delivery).
     std::vector<RtpExtension> extensions;
   };
 
+  // Called on the packet delivery thread since some members of the config may
+  // change mid-stream (e.g. the local ssrc). All mutation must also happen on
+  // the packet delivery thread. Return value can be assumed to
+  // only be used in the calling context (on the stack basically).
+  virtual const RtpConfig& rtp_config() const = 0;
+
  protected:
   virtual ~ReceiveStream() {}
 };

media/engine/fake_webrtc_call.h

@@ -101,6 +101,9 @@ class FakeAudioReceiveStream final : public webrtc::AudioReceiveStream {
   }
 
  private:
+  const webrtc::ReceiveStream::RtpConfig& rtp_config() const override {
+    return config_.rtp;
+  }
   void Start() override { started_ = true; }
   void Stop() override { started_ = false; }
   bool IsRunning() const override { return started_; }
@@ -250,6 +253,9 @@ class FakeVideoReceiveStream final : public webrtc::VideoReceiveStream {
 
  private:
   // webrtc::VideoReceiveStream implementation.
+  const webrtc::ReceiveStream::RtpConfig& rtp_config() const override {
+    return config_.rtp;
+  }
   void Start() override;
   void Stop() override;
 
@@ -276,7 +282,11 @@ class FakeFlexfecReceiveStream final : public webrtc::FlexfecReceiveStream {
   explicit FakeFlexfecReceiveStream(
       const webrtc::FlexfecReceiveStream::Config& config);
 
-  const webrtc::FlexfecReceiveStream::Config& GetConfig() const override;
+  const webrtc::ReceiveStream::RtpConfig& rtp_config() const override {
+    return config_.rtp;
+  }
+
+  const webrtc::FlexfecReceiveStream::Config& GetConfig() const;
 
  private:
   webrtc::FlexfecReceiveStream::Stats GetStats() const override;

media/engine/webrtc_video_engine_unittest.cc

@@ -4164,7 +4164,7 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, SetDefaultRecvCodecsWithSsrc) {
   const std::vector<FakeFlexfecReceiveStream*>& streams =
       fake_call_->GetFlexfecReceiveStreams();
   ASSERT_EQ(1U, streams.size());
-  const FakeFlexfecReceiveStream* stream = streams.front();
+  const auto* stream = streams.front();
   const webrtc::FlexfecReceiveStream::Config& config = stream->GetConfig();
   EXPECT_EQ(GetEngineCodec("flexfec-03").id, config.payload_type);
   EXPECT_EQ(kFlexfecSsrc, config.rtp.remote_ssrc);
@@ -4280,7 +4280,7 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, DuplicateFlexfecCodecIsDropped) {
   const std::vector<FakeFlexfecReceiveStream*>& streams =
       fake_call_->GetFlexfecReceiveStreams();
   ASSERT_EQ(1U, streams.size());
-  const FakeFlexfecReceiveStream* stream = streams.front();
+  const auto* stream = streams.front();
   const webrtc::FlexfecReceiveStream::Config& config = stream->GetConfig();
   EXPECT_EQ(GetEngineCodec("flexfec-03").id, config.payload_type);
 }
@@ -5126,7 +5126,7 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, SetRecvParamsWithoutFecDisablesFec) {
   ASSERT_EQ(1U, streams.size());
   const FakeFlexfecReceiveStream* stream = streams.front();
   EXPECT_EQ(GetEngineCodec("flexfec-03").id, stream->GetConfig().payload_type);
-  EXPECT_EQ(kFlexfecSsrc, stream->GetConfig().rtp.remote_ssrc);
+  EXPECT_EQ(kFlexfecSsrc, stream->rtp_config().remote_ssrc);
   ASSERT_EQ(1U, stream->GetConfig().protected_media_ssrcs.size());
   EXPECT_EQ(kSsrcs1[0], stream->GetConfig().protected_media_ssrcs[0]);
 

modules/rtp_rtcp/source/rtp_packet.cc

@@ -71,8 +71,8 @@ RtpPacket::RtpPacket(const ExtensionManager* extensions, size_t capacity)
 
 RtpPacket::~RtpPacket() {}
 
-void RtpPacket::IdentifyExtensions(const ExtensionManager& extensions) {
+void RtpPacket::IdentifyExtensions(ExtensionManager extensions) {
-  extensions_ = extensions;
+  extensions_ = std::move(extensions);
 }
 
 bool RtpPacket::Parse(const uint8_t* buffer, size_t buffer_size) {

modules/rtp_rtcp/source/rtp_packet.h

@@ -51,7 +51,7 @@ class RtpPacket {
   bool Parse(rtc::CopyOnWriteBuffer packet);
 
   // Maps extensions id to their types.
-  void IdentifyExtensions(const ExtensionManager& extensions);
+  void IdentifyExtensions(ExtensionManager extensions);
 
   // Header.
   bool Marker() const { return marker_; }

video/video_receive_stream.h

@@ -87,6 +87,8 @@ class VideoReceiveStream
   void Start() override;
   void Stop() override;
 
+  const RtpConfig& rtp_config() const override { return config_.rtp; }
+
   webrtc::VideoReceiveStream::Stats GetStats() const override;
 
   void AddSecondarySink(RtpPacketSinkInterface* sink) override;

video/video_receive_stream2.h

@@ -127,6 +127,8 @@ class VideoReceiveStream2
   void Start() override;
   void Stop() override;
 
+  const RtpConfig& rtp_config() const override { return config_.rtp; }
+
   webrtc::VideoReceiveStream::Stats GetStats() const override;
 
   // SetBaseMinimumPlayoutDelayMs and GetBaseMinimumPlayoutDelayMs are called