From e2561e17e29e62c02731f1d214d7ee5ffdaeb941 Mon Sep 17 00:00:00 2001 From: Tommi Date: Tue, 8 Jun 2021 16:55:47 +0200 Subject: [PATCH] Remove AudioReceiveStream::Reconfigure() method. Instead, adding specific setters that are needed at runtime: * SetDepacketizerToDecoderFrameTransformer * SetDecoderMap * SetUseTransportCcAndNackHistory The whole config struct is big and much of the state it holds, needs to be considered const. For that reason the Reconfigure() method is too broad of an interface since it overwrites the whole config struct and doesn't actually handle all the potential config changes that might occur when the config changes. Bug: webrtc:11993 Change-Id: Ia5311978f56b2e136781467e44f0d18039f0bb2d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/221363 Reviewed-by: Niels Moller Commit-Queue: Tommi Cr-Commit-Position: refs/heads/master@{#34252} --- audio/audio_receive_stream.cc | 72 +++++++++++++++++--------- audio/audio_receive_stream.h | 24 ++++++++- audio/audio_receive_stream_unittest.cc | 41 ++++++++------- audio/channel_receive.cc | 15 +++--- call/audio_receive_stream.h | 15 +++++- call/call.cc | 27 +++++----- media/engine/fake_webrtc_call.cc | 18 +++++-- media/engine/fake_webrtc_call.h | 8 ++- media/engine/webrtc_voice_engine.cc | 13 ++--- 9 files changed, 155 insertions(+), 78 deletions(-) diff --git a/audio/audio_receive_stream.cc b/audio/audio_receive_stream.cc index aecc246d09..dbb253980c 100644 --- a/audio/audio_receive_stream.cc +++ b/audio/audio_receive_stream.cc @@ -84,8 +84,8 @@ std::unique_ptr CreateChannelReceive( config.jitter_buffer_max_packets, config.jitter_buffer_fast_accelerate, config.jitter_buffer_min_delay_ms, config.jitter_buffer_enable_rtx_handling, config.decoder_factory, - config.codec_pair_id, config.frame_decryptor, config.crypto_options, - std::move(config.frame_transformer)); + config.codec_pair_id, std::move(config.frame_decryptor), + config.crypto_options, std::move(config.frame_transformer)); } } // namespace @@ -143,8 +143,10 @@ AudioReceiveStream::AudioReceiveStream( channel_receive_->SetNACKStatus(config.rtp.nack.rtp_history_ms != 0, config.rtp.nack.rtp_history_ms / 20); channel_receive_->SetReceiveCodecs(config.decoder_map); - channel_receive_->SetDepacketizerToDecoderFrameTransformer( - config.frame_transformer); + // `frame_transformer` and `frame_decryptor` have been given to + // `channel_receive_` already. + RTC_DCHECK(!config.frame_transformer); + RTC_DCHECK(!config.frame_decryptor); } AudioReceiveStream::~AudioReceiveStream() { @@ -168,35 +170,28 @@ void AudioReceiveStream::UnregisterFromTransport() { rtp_stream_receiver_.reset(); } -void AudioReceiveStream::Reconfigure( +void AudioReceiveStream::ReconfigureForTesting( const webrtc::AudioReceiveStream::Config& config) { - RTC_DCHECK(worker_thread_checker_.IsCurrent()); - - // Configuration parameters which cannot be changed. - RTC_DCHECK(config_.rtp.remote_ssrc == config.rtp.remote_ssrc); - RTC_DCHECK(config_.rtcp_send_transport == config.rtcp_send_transport); - // Decoder factory cannot be changed because it is configured at - // voe::Channel construction time. - RTC_DCHECK(config_.decoder_factory == config.decoder_factory); + RTC_DCHECK_RUN_ON(&worker_thread_checker_); // SSRC can't be changed mid-stream. - RTC_DCHECK_EQ(config_.rtp.local_ssrc, config.rtp.local_ssrc); RTC_DCHECK_EQ(config_.rtp.remote_ssrc, config.rtp.remote_ssrc); + RTC_DCHECK_EQ(config_.rtp.local_ssrc, config.rtp.local_ssrc); + + // Configuration parameters which cannot be changed. + RTC_DCHECK_EQ(config_.rtcp_send_transport, config.rtcp_send_transport); + // Decoder factory cannot be changed because it is configured at + // voe::Channel construction time. + RTC_DCHECK_EQ(config_.decoder_factory, config.decoder_factory); // TODO(solenberg): Config NACK history window (which is a packet count), // using the actual packet size for the configured codec. - if (config_.rtp.nack.rtp_history_ms != config.rtp.nack.rtp_history_ms) { - channel_receive_->SetNACKStatus(config.rtp.nack.rtp_history_ms != 0, - config.rtp.nack.rtp_history_ms / 20); - } - if (config_.decoder_map != config.decoder_map) { - channel_receive_->SetReceiveCodecs(config.decoder_map); - } + RTC_DCHECK_EQ(config_.rtp.nack.rtp_history_ms, config.rtp.nack.rtp_history_ms) + << "Use SetUseTransportCcAndNackHistory"; - if (config_.frame_transformer != config.frame_transformer) { - channel_receive_->SetDepacketizerToDecoderFrameTransformer( - config.frame_transformer); - } + RTC_DCHECK(config_.decoder_map == config.decoder_map) << "Use SetDecoderMap"; + RTC_DCHECK_EQ(config_.frame_transformer, config.frame_transformer) + << "Use SetDepacketizerToDecoderFrameTransformer"; config_ = config; } @@ -226,6 +221,33 @@ bool AudioReceiveStream::IsRunning() const { return playing_; } +void AudioReceiveStream::SetDepacketizerToDecoderFrameTransformer( + rtc::scoped_refptr frame_transformer) { + RTC_DCHECK_RUN_ON(&worker_thread_checker_); + channel_receive_->SetDepacketizerToDecoderFrameTransformer( + std::move(frame_transformer)); +} + +void AudioReceiveStream::SetDecoderMap( + std::map decoder_map) { + RTC_DCHECK_RUN_ON(&worker_thread_checker_); + config_.decoder_map = std::move(decoder_map); + channel_receive_->SetReceiveCodecs(config_.decoder_map); +} + +void AudioReceiveStream::SetUseTransportCcAndNackHistory(bool use_transport_cc, + int history_ms) { + RTC_DCHECK_RUN_ON(&worker_thread_checker_); + RTC_DCHECK_GE(history_ms, 0); + config_.rtp.transport_cc = use_transport_cc; + if (config_.rtp.nack.rtp_history_ms != history_ms) { + config_.rtp.nack.rtp_history_ms = history_ms; + // TODO(solenberg): Config NACK history window (which is a packet count), + // using the actual packet size for the configured codec. + channel_receive_->SetNACKStatus(history_ms != 0, history_ms / 20); + } +} + webrtc::AudioReceiveStream::Stats AudioReceiveStream::GetStats( bool get_and_clear_legacy_stats) const { RTC_DCHECK_RUN_ON(&worker_thread_checker_); diff --git a/audio/audio_receive_stream.h b/audio/audio_receive_stream.h index 87c82cce61..4f63155377 100644 --- a/audio/audio_receive_stream.h +++ b/audio/audio_receive_stream.h @@ -11,6 +11,7 @@ #ifndef AUDIO_AUDIO_RECEIVE_STREAM_H_ #define AUDIO_AUDIO_RECEIVE_STREAM_H_ +#include #include #include @@ -81,10 +82,15 @@ class AudioReceiveStream final : public webrtc::AudioReceiveStream, void UnregisterFromTransport(); // webrtc::AudioReceiveStream implementation. - void Reconfigure(const webrtc::AudioReceiveStream::Config& config) override; void Start() override; void Stop() override; bool IsRunning() const override; + void SetDepacketizerToDecoderFrameTransformer( + rtc::scoped_refptr frame_transformer) + override; + void SetDecoderMap(std::map decoder_map) override; + void SetUseTransportCcAndNackHistory(bool use_transport_cc, + int history_ms) override; webrtc::AudioReceiveStream::Stats GetStats( bool get_and_clear_legacy_stats) const override; @@ -111,9 +117,25 @@ class AudioReceiveStream final : public webrtc::AudioReceiveStream, void AssociateSendStream(AudioSendStream* send_stream); void DeliverRtcp(const uint8_t* packet, size_t length); + + uint32_t local_ssrc() const { + // The local_ssrc member variable of config_ will never change and can be + // considered const. + return config_.rtp.local_ssrc; + } + + uint32_t remote_ssrc() const { + // The remote_ssrc member variable of config_ will never change and can be + // considered const. + return config_.rtp.remote_ssrc; + } + const webrtc::AudioReceiveStream::Config& config() const; const AudioSendStream* GetAssociatedSendStreamForTesting() const; + // TODO(tommi): Remove this method. + void ReconfigureForTesting(const webrtc::AudioReceiveStream::Config& config); + private: AudioState* audio_state() const; diff --git a/audio/audio_receive_stream_unittest.cc b/audio/audio_receive_stream_unittest.cc index 59a1f2f5be..fb5f1cb876 100644 --- a/audio/audio_receive_stream_unittest.cc +++ b/audio/audio_receive_stream_unittest.cc @@ -104,8 +104,6 @@ struct ConfigHelper { .WillRepeatedly(Invoke([](const std::map& codecs) { EXPECT_THAT(codecs, ::testing::IsEmpty()); })); - EXPECT_CALL(*channel_receive_, SetDepacketizerToDecoderFrameTransformer(_)) - .Times(1); EXPECT_CALL(*channel_receive_, SetSourceTracker(_)); stream_config_.rtp.local_ssrc = kLocalSsrc; @@ -328,35 +326,37 @@ TEST(AudioReceiveStreamTest, StreamsShouldBeAddedToMixerOnceOnStart) { } } -TEST(AudioReceiveStreamTest, ReconfigureWithSameConfig) { - for (bool use_null_audio_processing : {false, true}) { - ConfigHelper helper(use_null_audio_processing); - auto recv_stream = helper.CreateAudioReceiveStream(); - recv_stream->Reconfigure(helper.config()); - recv_stream->UnregisterFromTransport(); - } -} - TEST(AudioReceiveStreamTest, ReconfigureWithUpdatedConfig) { for (bool use_null_audio_processing : {false, true}) { ConfigHelper helper(use_null_audio_processing); auto recv_stream = helper.CreateAudioReceiveStream(); auto new_config = helper.config(); - new_config.rtp.nack.rtp_history_ms = 300 + 20; + new_config.rtp.extensions.clear(); new_config.rtp.extensions.push_back( RtpExtension(RtpExtension::kAudioLevelUri, kAudioLevelId + 1)); new_config.rtp.extensions.push_back( RtpExtension(RtpExtension::kTransportSequenceNumberUri, kTransportSequenceNumberId + 1)); - new_config.decoder_map.emplace(1, SdpAudioFormat("foo", 8000, 1)); MockChannelReceive& channel_receive = *helper.channel_receive(); - EXPECT_CALL(channel_receive, SetNACKStatus(true, 15 + 1)).Times(1); - EXPECT_CALL(channel_receive, SetReceiveCodecs(new_config.decoder_map)); - recv_stream->Reconfigure(new_config); + // TODO(tommi, nisse): This applies new extensions to the internal config, + // but there's nothing that actually verifies that the changes take effect. + // In fact Call manages the extensions separately in Call::ReceiveRtpConfig + // and changing this config value (there seem to be a few copies), doesn't + // affect that logic. + recv_stream->ReconfigureForTesting(new_config); + + new_config.decoder_map.emplace(1, SdpAudioFormat("foo", 8000, 1)); + EXPECT_CALL(channel_receive, SetReceiveCodecs(new_config.decoder_map)); + recv_stream->SetDecoderMap(new_config.decoder_map); + + EXPECT_CALL(channel_receive, SetNACKStatus(true, 15 + 1)).Times(1); + recv_stream->SetUseTransportCcAndNackHistory(new_config.rtp.transport_cc, + 300 + 20); + recv_stream->UnregisterFromTransport(); } } @@ -371,14 +371,19 @@ TEST(AudioReceiveStreamTest, ReconfigureWithFrameDecryptor) { rtc::make_ref_counted()); new_config_0.frame_decryptor = mock_frame_decryptor_0; - recv_stream->Reconfigure(new_config_0); + // TODO(tommi): While this changes the internal config value, it doesn't + // actually change what frame_decryptor is used. WebRtcAudioReceiveStream + // recreates the whole instance in order to change this value. + // So, it's not clear if changing this post initialization needs to be + // supported. + recv_stream->ReconfigureForTesting(new_config_0); auto new_config_1 = helper.config(); rtc::scoped_refptr mock_frame_decryptor_1( rtc::make_ref_counted()); new_config_1.frame_decryptor = mock_frame_decryptor_1; new_config_1.crypto_options.sframe.require_frame_encryption = true; - recv_stream->Reconfigure(new_config_1); + recv_stream->ReconfigureForTesting(new_config_1); recv_stream->UnregisterFromTransport(); } } diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc index 415da3445b..f221b4e6b5 100644 --- a/audio/channel_receive.cc +++ b/audio/channel_receive.cc @@ -10,8 +10,6 @@ #include "audio/channel_receive.h" -#include - #include #include #include @@ -563,7 +561,7 @@ ChannelReceive::ChannelReceive( } ChannelReceive::~ChannelReceive() { - RTC_DCHECK(construction_thread_.IsCurrent()); + RTC_DCHECK_RUN_ON(&construction_thread_); // Unregister the module before stopping playout etc, to match the order // things were set up in the ctor. @@ -655,7 +653,7 @@ void ChannelReceive::ReceivePacket(const uint8_t* packet, size_t packet_length, const RTPHeader& header) { const uint8_t* payload = packet + header.headerLength; - assert(packet_length >= header.headerLength); + RTC_DCHECK_GE(packet_length, header.headerLength); size_t payload_length = packet_length - header.headerLength; size_t payload_data_length = payload_length - header.paddingLength; @@ -870,8 +868,11 @@ void ChannelReceive::SetDepacketizerToDecoderFrameTransformer( RTC_DCHECK_RUN_ON(&worker_thread_checker_); // Depending on when the channel is created, the transformer might be set // twice. Don't replace the delegate if it was already initialized. - if (!frame_transformer || frame_transformer_delegate_) + if (!frame_transformer || frame_transformer_delegate_) { + RTC_NOTREACHED() << "Not setting the transformer?"; return; + } + InitFrameTransformerDelegate(std::move(frame_transformer)); } @@ -1089,8 +1090,8 @@ std::unique_ptr CreateChannelReceive( rtcp_send_transport, rtc_event_log, local_ssrc, remote_ssrc, jitter_buffer_max_packets, jitter_buffer_fast_playout, jitter_buffer_min_delay_ms, jitter_buffer_enable_rtx_handling, - decoder_factory, codec_pair_id, frame_decryptor, crypto_options, - std::move(frame_transformer)); + decoder_factory, codec_pair_id, std::move(frame_decryptor), + crypto_options, std::move(frame_transformer)); } } // namespace voe diff --git a/call/audio_receive_stream.h b/call/audio_receive_stream.h index 6f74492927..45c318c404 100644 --- a/call/audio_receive_stream.h +++ b/call/audio_receive_stream.h @@ -157,15 +157,26 @@ class AudioReceiveStream { // An optional custom frame decryptor that allows the entire frame to be // decrypted in whatever way the caller choses. This is not required by // default. + // TODO(tommi): Remove this member variable from the struct. It's not + // a part of the AudioReceiveStream state but rather a pass through + // variable. rtc::scoped_refptr frame_decryptor; // An optional frame transformer used by insertable streams to transform // encoded frames. + // TODO(tommi): Remove this member variable from the struct. It's not + // a part of the AudioReceiveStream state but rather a pass through + // variable. rtc::scoped_refptr frame_transformer; }; - // Reconfigure the stream according to the Configuration. - virtual void Reconfigure(const Config& config) = 0; + // Methods that support reconfiguring the stream post initialization. + virtual void SetDepacketizerToDecoderFrameTransformer( + rtc::scoped_refptr + frame_transformer) = 0; + virtual void SetDecoderMap(std::map decoder_map) = 0; + virtual void SetUseTransportCcAndNackHistory(bool use_transport_cc, + int history_ms) = 0; // Starts stream activity. // When a stream is active, it can receive, process and deliver packets. diff --git a/call/call.cc b/call/call.cc index 0b2e4eccaf..52f8f8daf5 100644 --- a/call/call.cc +++ b/call/call.cc @@ -935,7 +935,7 @@ webrtc::AudioSendStream* Call::CreateAudioSendStream( // TODO(bugs.webrtc.org/11993): call AssociateSendStream and // UpdateAggregateNetworkState asynchronously on the network thread. for (AudioReceiveStream* stream : audio_receive_streams_) { - if (stream->config().rtp.local_ssrc == config.rtp.ssrc) { + if (stream->local_ssrc() == config.rtp.ssrc) { stream->AssociateSendStream(send_stream); } } @@ -963,7 +963,7 @@ void Call::DestroyAudioSendStream(webrtc::AudioSendStream* send_stream) { // TODO(bugs.webrtc.org/11993): call AssociateSendStream and // UpdateAggregateNetworkState asynchronously on the network thread. for (AudioReceiveStream* stream : audio_receive_streams_) { - if (stream->config().rtp.local_ssrc == ssrc) { + if (stream->local_ssrc() == ssrc) { stream->AssociateSendStream(nullptr); } } @@ -985,6 +985,7 @@ webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream( clock_, transport_send_->packet_router(), module_process_thread_->process_thread(), config_.neteq_factory, config, config_.audio_state, event_log_); + audio_receive_streams_.insert(receive_stream); // TODO(bugs.webrtc.org/11993): Make the registration on the network thread // (asynchronously). The registration and `audio_receiver_controller_` need @@ -995,7 +996,6 @@ webrtc::AudioReceiveStream* Call::CreateAudioReceiveStream( // 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)); - audio_receive_streams_.insert(receive_stream); ConfigureSync(config.sync_group); @@ -1016,22 +1016,24 @@ void Call::DestroyAudioReceiveStream( webrtc::internal::AudioReceiveStream* audio_receive_stream = static_cast(receive_stream); - const AudioReceiveStream::Config& config = audio_receive_stream->config(); - uint32_t ssrc = config.rtp.remote_ssrc; - receive_side_cc_.GetRemoteBitrateEstimator(UseSendSideBwe(config)) - ->RemoveStream(ssrc); - // TODO(bugs.webrtc.org/11993): Access the map, rtp config, call ConfigureSync // and UpdateAggregateNetworkState on the network thread. The call to // `UnregisterFromTransport` should also happen on the network thread. audio_receive_stream->UnregisterFromTransport(); - audio_receive_streams_.erase(audio_receive_stream); - const std::string& sync_group = audio_receive_stream->config().sync_group; - const auto it = sync_stream_mapping_.find(sync_group); + 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)) + ->RemoveStream(ssrc); + + audio_receive_streams_.erase(audio_receive_stream); + + const auto it = sync_stream_mapping_.find(config.sync_group); if (it != sync_stream_mapping_.end() && it->second == audio_receive_stream) { sync_stream_mapping_.erase(it); - ConfigureSync(sync_group); + ConfigureSync(config.sync_group); } receive_rtp_config_.erase(ssrc); @@ -1444,6 +1446,7 @@ void Call::OnAllocationLimitsChanged(BitrateAllocationLimits limits) { std::memory_order_relaxed); } +// RTC_RUN_ON(worker_thread_) void Call::ConfigureSync(const std::string& sync_group) { // TODO(bugs.webrtc.org/11993): Expect to be called on the network thread. // Set sync only if there was no previous one. diff --git a/media/engine/fake_webrtc_call.cc b/media/engine/fake_webrtc_call.cc index 76a70aaa57..5f484285a5 100644 --- a/media/engine/fake_webrtc_call.cc +++ b/media/engine/fake_webrtc_call.cc @@ -96,9 +96,21 @@ bool FakeAudioReceiveStream::DeliverRtp(const uint8_t* packet, return true; } -void FakeAudioReceiveStream::Reconfigure( - const webrtc::AudioReceiveStream::Config& config) { - config_ = config; +void FakeAudioReceiveStream::SetDepacketizerToDecoderFrameTransformer( + rtc::scoped_refptr frame_transformer) { + config_.frame_transformer = std::move(frame_transformer); +} + +void FakeAudioReceiveStream::SetDecoderMap( + std::map decoder_map) { + config_.decoder_map = std::move(decoder_map); +} + +void FakeAudioReceiveStream::SetUseTransportCcAndNackHistory( + bool use_transport_cc, + int history_ms) { + config_.rtp.transport_cc = use_transport_cc; + config_.rtp.nack.rtp_history_ms = history_ms; } webrtc::AudioReceiveStream::Stats FakeAudioReceiveStream::GetStats( diff --git a/media/engine/fake_webrtc_call.h b/media/engine/fake_webrtc_call.h index fd383dadd1..79f155cd86 100644 --- a/media/engine/fake_webrtc_call.h +++ b/media/engine/fake_webrtc_call.h @@ -102,10 +102,16 @@ class FakeAudioReceiveStream final : public webrtc::AudioReceiveStream { private: // webrtc::AudioReceiveStream implementation. - void Reconfigure(const webrtc::AudioReceiveStream::Config& config) override; void Start() override { started_ = true; } void Stop() override { started_ = false; } bool IsRunning() const override { return started_; } + void SetDepacketizerToDecoderFrameTransformer( + rtc::scoped_refptr frame_transformer) + override; + void SetDecoderMap( + std::map decoder_map) override; + void SetUseTransportCcAndNackHistory(bool use_transport_cc, + int history_ms) override; webrtc::AudioReceiveStream::Stats GetStats( bool get_and_clear_legacy_stats) const override; diff --git a/media/engine/webrtc_voice_engine.cc b/media/engine/webrtc_voice_engine.cc index a23d9ac24c..602d23cf68 100644 --- a/media/engine/webrtc_voice_engine.cc +++ b/media/engine/webrtc_voice_engine.cc @@ -1234,7 +1234,8 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream { RTC_DCHECK_RUN_ON(&worker_thread_checker_); config_.rtp.transport_cc = use_transport_cc; config_.rtp.nack.rtp_history_ms = use_nack ? kNackRtpHistoryMs : 0; - ReconfigureAudioReceiveStream(); + stream_->SetUseTransportCcAndNackHistory(use_transport_cc, + config_.rtp.nack.rtp_history_ms); } void SetRtpExtensionsAndRecreateStream( @@ -1248,7 +1249,7 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream { void SetDecoderMap(const std::map& decoder_map) { RTC_DCHECK_RUN_ON(&worker_thread_checker_); config_.decoder_map = decoder_map; - ReconfigureAudioReceiveStream(); + stream_->SetDecoderMap(decoder_map); } void MaybeRecreateAudioReceiveStream( @@ -1339,8 +1340,8 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream { void SetDepacketizerToDecoderFrameTransformer( rtc::scoped_refptr frame_transformer) { RTC_DCHECK_RUN_ON(&worker_thread_checker_); + stream_->SetDepacketizerToDecoderFrameTransformer(frame_transformer); config_.frame_transformer = std::move(frame_transformer); - ReconfigureAudioReceiveStream(); } private: @@ -1359,12 +1360,6 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream { stream_->SetSink(raw_audio_sink_.get()); } - void ReconfigureAudioReceiveStream() { - RTC_DCHECK_RUN_ON(&worker_thread_checker_); - RTC_DCHECK(stream_); - stream_->Reconfigure(config_); - } - webrtc::SequenceChecker worker_thread_checker_; webrtc::Call* call_ = nullptr; webrtc::AudioReceiveStream::Config config_;