From d350006b70927b19afa43e9c11e0bf3fdb0cdf15 Mon Sep 17 00:00:00 2001 From: Tommi Date: Mon, 14 Jun 2021 19:39:45 +0200 Subject: [PATCH] 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 Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/master@{#34285} --- audio/audio_receive_stream.h | 1 + call/call.cc | 79 +++++--------------- call/flexfec_receive_stream.h | 2 - call/flexfec_receive_stream_impl.cc | 5 -- call/flexfec_receive_stream_impl.h | 4 +- call/receive_stream.h | 14 ++++ media/engine/fake_webrtc_call.h | 12 ++- media/engine/webrtc_video_engine_unittest.cc | 6 +- modules/rtp_rtcp/source/rtp_packet.cc | 4 +- modules/rtp_rtcp/source/rtp_packet.h | 2 +- video/video_receive_stream.h | 2 + video/video_receive_stream2.h | 2 + 12 files changed, 59 insertions(+), 74 deletions(-) diff --git a/audio/audio_receive_stream.h b/audio/audio_receive_stream.h index 108794cd92..27e99f0ff2 100644 --- a/audio/audio_receive_stream.h +++ b/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 frame_transformer) diff --git a/call/call.cc b/call/call.cc index ffe7a25e7f..273720189f 100644 --- a/call/call.cc +++ b/call/call.cc @@ -81,12 +81,10 @@ bool SendPeriodicFeedback(const std::vector& extensions) { return true; } -// TODO(nisse): This really begs for a shared context struct. -bool UseSendSideBwe(const std::vector& 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 @@ bool UseSendSideBwe(const std::vector& 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& 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 receive_rtp_config_ + std::map 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_ - .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 @@ 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(); - 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 @@ 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 = - (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 0c9d5519e5..72e544e7ec 100644 --- a/call/flexfec_receive_stream.h +++ b/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 diff --git a/call/flexfec_receive_stream_impl.cc b/call/flexfec_receive_stream_impl.cc index 8368ff495f..ab82a6d71a 100644 --- a/call/flexfec_receive_stream_impl.cc +++ b/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 diff --git a/call/flexfec_receive_stream_impl.h b/call/flexfec_receive_stream_impl.h index 8826aedad9..12c4b04332 100644 --- a/call/flexfec_receive_stream_impl.h +++ b/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_; diff --git a/call/receive_stream.h b/call/receive_stream.h index 9f1f718dec..0f59b37ae3 100644 --- a/call/receive_stream.h +++ b/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 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 a2273c4002..5a14157082 100644 --- a/media/engine/fake_webrtc_call.h +++ b/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; diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 1e9fb6edfe..89cc1f4d9f 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -4164,7 +4164,7 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, SetDefaultRecvCodecsWithSsrc) { const std::vector& 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& 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]); diff --git a/modules/rtp_rtcp/source/rtp_packet.cc b/modules/rtp_rtcp/source/rtp_packet.cc index 1c2331dec9..dcf773952b 100644 --- a/modules/rtp_rtcp/source/rtp_packet.cc +++ b/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) { - 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 aa854f35ab..1c31f92ce9 100644 --- a/modules/rtp_rtcp/source/rtp_packet.h +++ b/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_; } diff --git a/video/video_receive_stream.h b/video/video_receive_stream.h index 9bea0b8fb9..c778d74558 100644 --- a/video/video_receive_stream.h +++ b/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; diff --git a/video/video_receive_stream2.h b/video/video_receive_stream2.h index dbd46cc66e..8fd995084e 100644 --- a/video/video_receive_stream2.h +++ b/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