diff --git a/audio/audio_receive_stream.cc b/audio/audio_receive_stream.cc index aecc246d09..cc53a746ff 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,8 @@ 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. } AudioReceiveStream::~AudioReceiveStream() { @@ -168,35 +168,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 +219,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_;