From cb7c7366d0bea44c8b9eadab043263a1e02e1928 Mon Sep 17 00:00:00 2001 From: Tommi Date: Mon, 9 May 2022 14:49:37 +0000 Subject: [PATCH] Separate reading remote_ssrc from using the rtp_config() getter. `remote_ssrc` can be considered const while some other state represented by rtp_config() can not and also is tied to a specific thread. Separating access to these variables, makes moving things around easier. Bug: webrtc:11993 Change-Id: I70aa000daab6174a401e01dca163213174e8f284 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261316 Commit-Queue: Tomas Gunnarsson Reviewed-by: Niels Moller Cr-Commit-Position: refs/heads/main@{#36818} --- audio/audio_receive_stream.h | 2 +- call/audio_receive_stream.h | 5 +++++ call/call.cc | 13 +++++++------ call/flexfec_receive_stream_impl.cc | 5 ++++- call/flexfec_receive_stream_impl.h | 7 +++++-- media/engine/fake_webrtc_call.h | 3 +++ media/engine/webrtc_video_engine_unittest.cc | 2 +- media/engine/webrtc_voice_engine.cc | 4 ++-- 8 files changed, 28 insertions(+), 13 deletions(-) diff --git a/audio/audio_receive_stream.h b/audio/audio_receive_stream.h index 6fc9f7fe62..252ab13f39 100644 --- a/audio/audio_receive_stream.h +++ b/audio/audio_receive_stream.h @@ -128,7 +128,7 @@ class AudioReceiveStream final : public webrtc::AudioReceiveStream, uint32_t local_ssrc() const; - uint32_t remote_ssrc() const { + uint32_t remote_ssrc() const override { // The remote_ssrc member variable of config_ will never change and can be // considered const. return config_.rtp.remote_ssrc; diff --git a/call/audio_receive_stream.h b/call/audio_receive_stream.h index 17691f7021..b18e076530 100644 --- a/call/audio_receive_stream.h +++ b/call/audio_receive_stream.h @@ -194,6 +194,11 @@ class AudioReceiveStream : public MediaReceiveStream { // Returns current value of base minimum delay in milliseconds. virtual int GetBaseMinimumPlayoutDelayMs() const = 0; + // Synchronization source (stream identifier) to be received. + // This member will not change mid-stream and can be assumed to be const + // post initialization. + virtual uint32_t remote_ssrc() const = 0; + protected: virtual ~AudioReceiveStream() {} }; diff --git a/call/call.cc b/call/call.cc index 1a0357fc75..0703fd4d29 100644 --- a/call/call.cc +++ b/call/call.cc @@ -1245,16 +1245,17 @@ void Call::DestroyFlexfecReceiveStream(FlexfecReceiveStream* receive_stream) { // TODO(bugs.webrtc.org/11993): Unregister on the network thread. receive_stream_impl->UnregisterFromTransport(); - RTC_DCHECK(receive_stream != nullptr); - const FlexfecReceiveStream::RtpConfig& rtp = receive_stream->rtp_config(); - UnregisterReceiveStream(rtp.remote_ssrc); + auto ssrc = receive_stream_impl->remote_ssrc(); + UnregisterReceiveStream(ssrc); // Remove all SSRCs pointing to the FlexfecReceiveStreamImpl to be // destroyed. - receive_side_cc_.GetRemoteBitrateEstimator(UseSendSideBwe(rtp)) - ->RemoveStream(rtp.remote_ssrc); + receive_side_cc_ + .GetRemoteBitrateEstimator( + UseSendSideBwe(receive_stream_impl->rtp_config())) + ->RemoveStream(ssrc); - delete receive_stream; + delete receive_stream_impl; } void Call::AddAdaptationResource(rtc::scoped_refptr resource) { diff --git a/call/flexfec_receive_stream_impl.cc b/call/flexfec_receive_stream_impl.cc index a78448170f..6c8378d4b2 100644 --- a/call/flexfec_receive_stream_impl.cc +++ b/call/flexfec_receive_stream_impl.cc @@ -204,7 +204,10 @@ FlexfecReceiveStreamImpl::Stats FlexfecReceiveStreamImpl::GetStats() const { void FlexfecReceiveStreamImpl::SetRtpExtensions( std::vector extensions) { RTC_DCHECK_RUN_ON(&packet_sequence_checker_); - config_.rtp.extensions = std::move(extensions); + // TODO(tommi): Remove this cast once header extensions are managed outside + // of the config struct. + const_cast&>(config_.rtp.extensions) = + std::move(extensions); } } // namespace webrtc diff --git a/call/flexfec_receive_stream_impl.h b/call/flexfec_receive_stream_impl.h index c2407cd419..c640ac6879 100644 --- a/call/flexfec_receive_stream_impl.h +++ b/call/flexfec_receive_stream_impl.h @@ -61,12 +61,15 @@ class FlexfecReceiveStreamImpl : public FlexfecReceiveStream { // ReceiveStream impl. void SetRtpExtensions(std::vector extensions) override; const RtpConfig& rtp_config() const override { return config_.rtp; } + uint32_t remote_ssrc() const { return config_.rtp.remote_ssrc; } private: RTC_NO_UNIQUE_ADDRESS SequenceChecker packet_sequence_checker_; - // Config. Mostly const, header extensions may change. - Config config_ RTC_GUARDED_BY(packet_sequence_checker_); + // Config. Mostly const, header extensions may change, which is an exception + // case that's specifically handled in `SetRtpExtensions`, which must be + // called on the `packet_sequence_checker` thread. + const Config config_; // Erasure code interfacing. const std::unique_ptr receiver_; diff --git a/media/engine/fake_webrtc_call.h b/media/engine/fake_webrtc_call.h index d2a867fde0..338996a625 100644 --- a/media/engine/fake_webrtc_call.h +++ b/media/engine/fake_webrtc_call.h @@ -113,6 +113,7 @@ class FakeAudioReceiveStream final : public webrtc::AudioReceiveStream { const webrtc::ReceiveStream::RtpConfig& rtp_config() const override { return config_.rtp; } + uint32_t remote_ssrc() const override { return config_.rtp.remote_ssrc; } void Start() override { started_ = true; } void Stop() override { started_ = false; } bool IsRunning() const override { return started_; } @@ -304,6 +305,8 @@ class FakeFlexfecReceiveStream final : public webrtc::FlexfecReceiveStream { const webrtc::FlexfecReceiveStream::Config& GetConfig() const; + uint32_t remote_ssrc() const { return config_.rtp.remote_ssrc; } + 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 377ea232c4..70fb43f8f1 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -5041,7 +5041,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->rtp_config().remote_ssrc); + EXPECT_EQ(kFlexfecSsrc, stream->remote_ssrc()); ASSERT_EQ(1U, stream->GetConfig().protected_media_ssrcs.size()); EXPECT_EQ(kSsrcs1[0], stream->GetConfig().protected_media_ssrcs[0]); diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index b042c70a0f..1672a718b1 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -1244,7 +1244,7 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream { RTC_LOG(LS_ERROR) << "Failed to SetBaseMinimumPlayoutDelayMs" " on AudioReceiveStream on SSRC=" - << stream_->rtp_config().remote_ssrc + << stream_->remote_ssrc() << " with delay_ms=" << delay_ms; return false; } @@ -1263,7 +1263,7 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream { webrtc::RtpParameters rtp_parameters; rtp_parameters.encodings.emplace_back(); const auto& config = stream_->rtp_config(); - rtp_parameters.encodings[0].ssrc = config.remote_ssrc; + rtp_parameters.encodings[0].ssrc = stream_->remote_ssrc(); rtp_parameters.header_extensions = config.extensions; return rtp_parameters; }