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}
diff --git a/audio/audio_receive_stream.h b/audio/audio_receive_stream.h
index 108794c..27e99f0 100644
--- a/audio/audio_receive_stream.h
+++ b/audio/audio_receive_stream.h
@@ -84,6 +84,7 @@
// 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)
diff --git a/call/call.cc b/call/call.cc
index ffe7a25..2737201 100644
--- a/call/call.cc
+++ b/call/call.cc
@@ -81,12 +81,10 @@
return true;
}
-// TODO(nisse): This really begs for a shared context struct.
-bool UseSendSideBwe(const std::vector<RtpExtension>& extensions,
- bool transport_cc) {
- if (!transport_cc)
+bool UseSendSideBwe(const ReceiveStream::RtpConfig& rtp) {
+ if (!rtp.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 @@
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 @@
// 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 @@
// 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 @@
uint32_t ssrc = audio_receive_stream->remote_ssrc();
const AudioReceiveStream::Config& config = audio_receive_stream->config();
- receive_side_cc_
- .GetRemoteBitrateEstimator(
- UseSendSideBwe(config.rtp.extensions, config.rtp.transport_cc))
+ receive_side_cc_.GetRemoteBitrateEstimator(UseSendSideBwe(config.rtp))
->RemoveStream(ssrc);
audio_receive_streams_.erase(audio_receive_stream);
@@ -1177,9 +1137,9 @@
// 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 @@
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 @@
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 @@
receive_stream_impl->UnregisterFromTransport();
RTC_DCHECK(receive_stream != nullptr);
- const FlexfecReceiveStream::Config& config = receive_stream->GetConfig();
- uint32_t ssrc = config.rtp.remote_ssrc;
- receive_rtp_config_.erase(ssrc);
+ const FlexfecReceiveStream::RtpConfig& rtp = receive_stream->rtp_config();
+ receive_rtp_config_.erase(rtp.remote_ssrc);
// Remove all SSRCs pointing to the FlexfecReceiveStreamImpl to be
// destroyed.
- receive_side_cc_.GetRemoteBitrateEstimator(UseSendSideBwe(config))
- ->RemoveStream(ssrc);
+ receive_side_cc_.GetRemoteBitrateEstimator(UseSendSideBwe(rtp))
+ ->RemoveStream(rtp.remote_ssrc);
delete receive_stream;
}
@@ -1591,7 +1550,8 @@
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 @@
// 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::NotifyBweOfReceivedPacket(const RtpPacketReceived& packet,
MediaType media_type) {
auto it = receive_rtp_config_.find(packet.Ssrc());
- bool use_send_side_bwe =
- (it != receive_rtp_config_.end()) && it->second.use_send_side_bwe;
+ bool use_send_side_bwe = (it != receive_rtp_config_.end()) &&
+ UseSendSideBwe(it->second->rtp_config());
RTPHeader header;
packet.GetHeader(&header);
diff --git a/call/flexfec_receive_stream.h b/call/flexfec_receive_stream.h
index 0c9d551..72e544e 100644
--- a/call/flexfec_receive_stream.h
+++ b/call/flexfec_receive_stream.h
@@ -68,8 +68,6 @@
};
virtual Stats GetStats() const = 0;
-
- virtual const Config& GetConfig() const = 0;
};
} // namespace webrtc
diff --git a/call/flexfec_receive_stream_impl.cc b/call/flexfec_receive_stream_impl.cc
index 8368ff4..ab82a6d 100644
--- a/call/flexfec_receive_stream_impl.cc
+++ b/call/flexfec_receive_stream_impl.cc
@@ -205,9 +205,4 @@
return FlexfecReceiveStream::Stats();
}
-const FlexfecReceiveStream::Config& FlexfecReceiveStreamImpl::GetConfig()
- const {
- return config_;
-}
-
} // namespace webrtc
diff --git a/call/flexfec_receive_stream_impl.h b/call/flexfec_receive_stream_impl.h
index 8826aed..12c4b04 100644
--- a/call/flexfec_receive_stream_impl.h
+++ b/call/flexfec_receive_stream_impl.h
@@ -59,7 +59,9 @@
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_;
diff --git a/call/receive_stream.h b/call/receive_stream.h
index 9f1f718..0f59b37 100644
--- a/call/receive_stream.h
+++ b/call/receive_stream.h
@@ -28,21 +28,35 @@
// 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() {}
};
diff --git a/media/engine/fake_webrtc_call.h b/media/engine/fake_webrtc_call.h
index a2273c4..5a14157 100644
--- a/media/engine/fake_webrtc_call.h
+++ b/media/engine/fake_webrtc_call.h
@@ -101,6 +101,9 @@
}
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 @@
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 @@
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;
diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc
index 1e9fb6e..89cc1f4 100644
--- a/media/engine/webrtc_video_engine_unittest.cc
+++ b/media/engine/webrtc_video_engine_unittest.cc
@@ -4164,7 +4164,7 @@
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 @@
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 @@
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]);
diff --git a/modules/rtp_rtcp/source/rtp_packet.cc b/modules/rtp_rtcp/source/rtp_packet.cc
index 1c2331d..dcf7739 100644
--- a/modules/rtp_rtcp/source/rtp_packet.cc
+++ b/modules/rtp_rtcp/source/rtp_packet.cc
@@ -71,8 +71,8 @@
RtpPacket::~RtpPacket() {}
-void RtpPacket::IdentifyExtensions(const ExtensionManager& extensions) {
- extensions_ = extensions;
+void RtpPacket::IdentifyExtensions(ExtensionManager extensions) {
+ extensions_ = std::move(extensions);
}
bool RtpPacket::Parse(const uint8_t* buffer, size_t buffer_size) {
diff --git a/modules/rtp_rtcp/source/rtp_packet.h b/modules/rtp_rtcp/source/rtp_packet.h
index aa854f35..1c31f92 100644
--- a/modules/rtp_rtcp/source/rtp_packet.h
+++ b/modules/rtp_rtcp/source/rtp_packet.h
@@ -51,7 +51,7 @@
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_; }
diff --git a/video/video_receive_stream.h b/video/video_receive_stream.h
index 9bea0b8f..c778d74 100644
--- a/video/video_receive_stream.h
+++ b/video/video_receive_stream.h
@@ -87,6 +87,8 @@
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;
diff --git a/video/video_receive_stream2.h b/video/video_receive_stream2.h
index dbd46cc..8fd9950 100644
--- a/video/video_receive_stream2.h
+++ b/video/video_receive_stream2.h
@@ -127,6 +127,8 @@
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
