diff --git a/call/call.cc b/call/call.cc index 5752be7e5c..f676b5f51a 100644 --- a/call/call.cc +++ b/call/call.cc @@ -231,15 +231,6 @@ class Call final : public webrtc::Call, uint32_t max_padding_bitrate_bps, uint32_t total_bitrate_bps) override; - // This method is invoked when the media transport is created and when the - // media transport is being destructed. - // We only allow one media transport per connection. - // - // It should be called with non-null argument at most once, and if it was - // called with non-null argument, it has to be called with a null argument - // at least once after that. - void MediaTransportChange(MediaTransportInterface* media_transport) override; - void SetClientBitratePreferences(const BitrateSettings& preferences) override; private: @@ -262,29 +253,24 @@ class Call final : public webrtc::Call, void UpdateHistograms(); void UpdateAggregateNetworkState(); - // If |media_transport| is not null, it registers the rate observer for the - // media transport. - void RegisterRateObserver() RTC_LOCKS_EXCLUDED(target_observer_crit_); - - // Intended for DCHECKs, to avoid locking in production builds. - MediaTransportInterface* media_transport() - RTC_LOCKS_EXCLUDED(target_observer_crit_); + void RegisterRateObserver(); Clock* const clock_; TaskQueueFactory* const task_queue_factory_; - // Caching the last SetBitrate for media transport. - absl::optional last_set_bitrate_ - RTC_GUARDED_BY(&target_observer_crit_); const int num_cpu_cores_; const std::unique_ptr module_process_thread_; const std::unique_ptr call_stats_; const std::unique_ptr bitrate_allocator_; Call::Config config_; SequenceChecker configuration_sequence_checker_; + SequenceChecker worker_sequence_checker_; NetworkState audio_network_state_; NetworkState video_network_state_; + // TODO(tommi): Once tests have been fixed to not call GetStats() on the wrong + // thread, remove this lock and protect aggregate_network_up_crit_ with the + // configuration_sequence_checker_. rtc::CriticalSection aggregate_network_up_crit_; bool aggregate_network_up_ RTC_GUARDED_BY(aggregate_network_up_crit_); @@ -370,7 +356,8 @@ class Call final : public webrtc::Call, // TODO(holmer): Remove this lock once BitrateController no longer calls // OnNetworkChanged from multiple threads. rtc::CriticalSection bitrate_crit_; - uint32_t min_allocated_send_bitrate_bps_ RTC_GUARDED_BY(&bitrate_crit_); + uint32_t min_allocated_send_bitrate_bps_ + RTC_GUARDED_BY(&worker_sequence_checker_); uint32_t configured_max_padding_bitrate_bps_ RTC_GUARDED_BY(&bitrate_crit_); AvgCounter estimated_send_bitrate_kbps_counter_ RTC_GUARDED_BY(&bitrate_crit_); @@ -387,18 +374,13 @@ class Call final : public webrtc::Call, // Note that this is declared before transport_send_ to ensure that it is not // invalidated until no more tasks can be running on the transport_send_ task // queue. - RtpTransportControllerSendInterface* transport_send_ptr_; + RtpTransportControllerSendInterface* const transport_send_ptr_; // Declared last since it will issue callbacks from a task queue. Declaring it // last ensures that it is destroyed first and any running tasks are finished. std::unique_ptr transport_send_; - // This is a precaution, since |MediaTransportChange| is not guaranteed to be - // invoked on a particular thread. - rtc::CriticalSection target_observer_crit_; bool is_target_rate_observer_registered_ - RTC_GUARDED_BY(&target_observer_crit_) = false; - MediaTransportInterface* media_transport_ - RTC_GUARDED_BY(&target_observer_crit_) = nullptr; + RTC_GUARDED_BY(&configuration_sequence_checker_) = false; RTC_DISALLOW_COPY_AND_ASSIGN(Call); }; @@ -479,10 +461,11 @@ Call::Call(Clock* clock, receive_side_cc_(clock_, transport_send->packet_router()), receive_time_calculator_(ReceiveTimeCalculator::CreateFromFieldTrial()), video_send_delay_stats_(new SendDelayStats(clock_)), - start_ms_(clock_->TimeInMilliseconds()) { + start_ms_(clock_->TimeInMilliseconds()), + transport_send_ptr_(transport_send.get()), + transport_send_(std::move(transport_send)) { RTC_DCHECK(config.event_log != nullptr); - transport_send_ = std::move(transport_send); - transport_send_ptr_ = transport_send_.get(); + worker_sequence_checker_.Detach(); } Call::~Call() { @@ -494,14 +477,12 @@ Call::~Call() { RTC_CHECK(audio_receive_streams_.empty()); RTC_CHECK(video_receive_streams_.empty()); - if (!media_transport_) { - module_process_thread_->DeRegisterModule( - receive_side_cc_.GetRemoteBitrateEstimator(true)); - module_process_thread_->DeRegisterModule(&receive_side_cc_); - module_process_thread_->DeRegisterModule(call_stats_.get()); - module_process_thread_->Stop(); - call_stats_->DeregisterStatsObserver(&receive_side_cc_); - } + module_process_thread_->DeRegisterModule( + receive_side_cc_.GetRemoteBitrateEstimator(true)); + module_process_thread_->DeRegisterModule(&receive_side_cc_); + module_process_thread_->DeRegisterModule(call_stats_.get()); + module_process_thread_->Stop(); + call_stats_->DeregisterStatsObserver(&receive_side_cc_); absl::optional first_sent_packet_ms = transport_send_->GetFirstPacketTime(); @@ -515,102 +496,33 @@ Call::~Call() { UpdateHistograms(); } +// TODO(tommi): Most of this work could be done when Call gets created. +// Starting the process thread itself could be done on demand when streams +// are created and in that case, calling Start() multiple times is harmless +// so holding an extra state variable, |is_target_rate_observer_registered_| +// also shouldn't be necessary. void Call::RegisterRateObserver() { - rtc::CritScope lock(&target_observer_crit_); + RTC_DCHECK_RUN_ON(&configuration_sequence_checker_); - if (is_target_rate_observer_registered_) { + if (is_target_rate_observer_registered_) return; - } is_target_rate_observer_registered_ = true; - if (media_transport_) { - // TODO(bugs.webrtc.org/9719): We should report call_stats_ from - // media transport (at least Rtt). We should extend media transport - // interface to include "receive_side bwe" if needed. - media_transport_->AddTargetTransferRateObserver(this); - } else { - transport_send_ptr_->RegisterTargetTransferRateObserver(this); + transport_send_ptr_->RegisterTargetTransferRateObserver(this); - call_stats_->RegisterStatsObserver(&receive_side_cc_); + call_stats_->RegisterStatsObserver(&receive_side_cc_); - module_process_thread_->RegisterModule( - receive_side_cc_.GetRemoteBitrateEstimator(true), RTC_FROM_HERE); - module_process_thread_->RegisterModule(call_stats_.get(), RTC_FROM_HERE); - module_process_thread_->RegisterModule(&receive_side_cc_, RTC_FROM_HERE); - module_process_thread_->Start(); - } -} - -MediaTransportInterface* Call::media_transport() { - rtc::CritScope lock(&target_observer_crit_); - return media_transport_; -} - -void Call::MediaTransportChange(MediaTransportInterface* media_transport) { - rtc::CritScope lock(&target_observer_crit_); - - if (is_target_rate_observer_registered_) { - // Only used to unregister rate observer from media transport. Registration - // happens when the stream is created. - if (!media_transport && media_transport_) { - media_transport_->RemoveTargetTransferRateObserver(this); - media_transport_ = nullptr; - is_target_rate_observer_registered_ = false; - } - } else if (media_transport) { - RTC_DCHECK(media_transport_ == nullptr || - media_transport_ == media_transport) - << "media_transport_=" << (media_transport_ != nullptr) - << ", (media_transport_==media_transport)=" - << (media_transport_ == media_transport); - media_transport_ = media_transport; - MediaTransportTargetRateConstraints constraints; - if (config_.bitrate_config.start_bitrate_bps > 0) { - constraints.starting_bitrate = - DataRate::bps(config_.bitrate_config.start_bitrate_bps); - } - if (config_.bitrate_config.max_bitrate_bps > 0) { - constraints.max_bitrate = - DataRate::bps(config_.bitrate_config.max_bitrate_bps); - } - if (config_.bitrate_config.min_bitrate_bps > 0) { - constraints.min_bitrate = - DataRate::bps(config_.bitrate_config.min_bitrate_bps); - } - - // User called ::SetBitrate on peer connection before - // media transport was created. - if (last_set_bitrate_) { - media_transport_->SetTargetBitrateLimits(*last_set_bitrate_); - } else { - media_transport_->SetTargetBitrateLimits(constraints); - } - } + module_process_thread_->RegisterModule( + receive_side_cc_.GetRemoteBitrateEstimator(true), RTC_FROM_HERE); + module_process_thread_->RegisterModule(call_stats_.get(), RTC_FROM_HERE); + module_process_thread_->RegisterModule(&receive_side_cc_, RTC_FROM_HERE); + module_process_thread_->Start(); } void Call::SetClientBitratePreferences(const BitrateSettings& preferences) { + RTC_DCHECK_RUN_ON(&configuration_sequence_checker_); GetTransportControllerSend()->SetClientBitratePreferences(preferences); - // Can the client code invoke 'SetBitrate' before media transport is created? - // It's probably possible :/ - MediaTransportTargetRateConstraints constraints; - if (preferences.start_bitrate_bps.has_value()) { - constraints.starting_bitrate = - webrtc::DataRate::bps(*preferences.start_bitrate_bps); - } - if (preferences.max_bitrate_bps.has_value()) { - constraints.max_bitrate = - webrtc::DataRate::bps(*preferences.max_bitrate_bps); - } - if (preferences.min_bitrate_bps.has_value()) { - constraints.min_bitrate = - webrtc::DataRate::bps(*preferences.min_bitrate_bps); - } - rtc::CritScope lock(&target_observer_crit_); - last_set_bitrate_ = constraints; - if (media_transport_) { - media_transport_->SetTargetBitrateLimits(constraints); - } } void Call::UpdateHistograms() { @@ -699,9 +611,6 @@ webrtc::AudioSendStream* Call::CreateAudioSendStream( TRACE_EVENT0("webrtc", "Call::CreateAudioSendStream"); RTC_DCHECK_RUN_ON(&configuration_sequence_checker_); - RTC_DCHECK_EQ(media_transport(), - config.media_transport_config.media_transport); - RegisterRateObserver(); // Stream config is logged in AudioSendStream::ConfigureStream, as it may @@ -831,8 +740,6 @@ webrtc::VideoSendStream* Call::CreateVideoSendStream( TRACE_EVENT0("webrtc", "Call::CreateVideoSendStream"); RTC_DCHECK_RUN_ON(&configuration_sequence_checker_); - RTC_DCHECK(media_transport() == config.media_transport); - RegisterRateObserver(); video_send_delay_stats_->AddSsrcs(config); @@ -1164,17 +1071,8 @@ void Call::OnStartRateUpdate(DataRate start_rate) { } void Call::OnTargetTransferRate(TargetTransferRate msg) { - // TODO(bugs.webrtc.org/9719) - // Call::OnTargetTransferRate requires that on target transfer rate is invoked - // from the worker queue (because bitrate_allocator_ requires it). Media - // transport does not guarantee the callback on the worker queue. - // When the threading model for MediaTransportInterface is update, reconsider - // changing this implementation. - if (!transport_send_ptr_->GetWorkerQueue()->IsCurrent()) { - transport_send_ptr_->GetWorkerQueue()->PostTask( - [this, msg] { this->OnTargetTransferRate(msg); }); - return; - } + RTC_DCHECK(transport_send_ptr_->GetWorkerQueue()->IsCurrent()); + RTC_DCHECK_RUN_ON(&worker_sequence_checker_); uint32_t target_bitrate_bps = msg.target_rate.bps(); int loss_ratio_255 = msg.network_estimate.loss_rate_ratio * 255; @@ -1224,22 +1122,13 @@ void Call::OnTargetTransferRate(TargetTransferRate msg) { void Call::OnAllocationLimitsChanged(uint32_t min_send_bitrate_bps, uint32_t max_padding_bitrate_bps, uint32_t total_bitrate_bps) { + RTC_DCHECK_RUN_ON(&worker_sequence_checker_); transport_send_ptr_->SetAllocatedSendBitrateLimits( min_send_bitrate_bps, max_padding_bitrate_bps, total_bitrate_bps); - { - rtc::CritScope lock(&target_observer_crit_); - if (media_transport_) { - MediaTransportAllocatedBitrateLimits limits; - limits.min_pacing_rate = DataRate::bps(min_send_bitrate_bps); - limits.max_padding_bitrate = DataRate::bps(max_padding_bitrate_bps); - limits.max_total_allocated_bitrate = DataRate::bps(total_bitrate_bps); - media_transport_->SetAllocatedBitrateLimits(limits); - } - } + min_allocated_send_bitrate_bps_ = min_send_bitrate_bps; rtc::CritScope lock(&bitrate_crit_); - min_allocated_send_bitrate_bps_ = min_send_bitrate_bps; configured_max_padding_bitrate_bps_ = max_padding_bitrate_bps; } diff --git a/call/call.h b/call/call.h index 2c5aca2a77..77cd3d2690 100644 --- a/call/call.h +++ b/call/call.h @@ -57,10 +57,6 @@ class Call { virtual AudioSendStream* CreateAudioSendStream( const AudioSendStream::Config& config) = 0; - // Gets called when media transport is created or removed. - virtual void MediaTransportChange( - MediaTransportInterface* media_transport_interface) = 0; - virtual void DestroyAudioSendStream(AudioSendStream* send_stream) = 0; virtual AudioReceiveStream* CreateAudioReceiveStream( diff --git a/call/call_unittest.cc b/call/call_unittest.cc index e5bc6c0c16..248a96a19d 100644 --- a/call/call_unittest.cc +++ b/call/call_unittest.cc @@ -295,30 +295,4 @@ TEST(CallTest, RecreatingAudioStreamWithSameSsrcReusesRtpState) { EXPECT_EQ(rtp_state1.media_has_been_sent, rtp_state2.media_has_been_sent); } -TEST(CallTest, RegisterMediaTransportBitrateCallbacksInCreateStream) { - CallHelper call; - MediaTransportSettings settings; - webrtc::FakeMediaTransport fake_media_transport(settings); - - EXPECT_EQ(0, fake_media_transport.target_rate_observers_size()); - // TODO(solenberg): This test shouldn't require a Transport, but currently - // RTCPSender requires one. - MockTransport send_transport; - AudioSendStream::Config config(&send_transport, - MediaTransportConfig(&fake_media_transport)); - - call->MediaTransportChange(&fake_media_transport); - AudioSendStream* stream = call->CreateAudioSendStream(config); - - // We get 2 subscribers: one subscriber from call.cc, and one from - // ChannelSend. - EXPECT_EQ(2, fake_media_transport.target_rate_observers_size()); - - call->DestroyAudioSendStream(stream); - EXPECT_EQ(1, fake_media_transport.target_rate_observers_size()); - - call->MediaTransportChange(nullptr); - EXPECT_EQ(0, fake_media_transport.target_rate_observers_size()); -} - } // namespace webrtc diff --git a/call/degraded_call.cc b/call/degraded_call.cc index 5a185d5665..61102a6abe 100644 --- a/call/degraded_call.cc +++ b/call/degraded_call.cc @@ -242,10 +242,4 @@ PacketReceiver::DeliveryStatus DegradedCall::DeliverPacket( return status; } -void DegradedCall::MediaTransportChange( - MediaTransportInterface* media_transport) { - // TODO(bugs.webrtc.org/9719) We should add support for media transport here - // at some point. -} - } // namespace webrtc diff --git a/call/degraded_call.h b/call/degraded_call.h index 400450ea08..609049333f 100644 --- a/call/degraded_call.h +++ b/call/degraded_call.h @@ -129,7 +129,6 @@ class DegradedCall : public Call, private Transport, private PacketReceiver { const std::unique_ptr call_; TaskQueueFactory* const task_queue_factory_; - void MediaTransportChange(MediaTransportInterface* media_transport) override; void SetClientBitratePreferences( const webrtc::BitrateSettings& preferences) override {} const absl::optional send_config_; diff --git a/media/engine/fake_webrtc_call.cc b/media/engine/fake_webrtc_call.cc index dcab48b008..78d4ba41e0 100644 --- a/media/engine/fake_webrtc_call.cc +++ b/media/engine/fake_webrtc_call.cc @@ -644,7 +644,4 @@ void FakeCall::OnSentPacket(const rtc::SentPacket& sent_packet) { } } -void FakeCall::MediaTransportChange( - webrtc::MediaTransportInterface* media_transport_interface) {} - } // namespace cricket diff --git a/media/engine/fake_webrtc_call.h b/media/engine/fake_webrtc_call.h index e87c24f905..9441e99ece 100644 --- a/media/engine/fake_webrtc_call.h +++ b/media/engine/fake_webrtc_call.h @@ -303,9 +303,6 @@ class FakeCall final : public webrtc::Call, public webrtc::PacketReceiver { int GetNumCreatedReceiveStreams() const; void SetStats(const webrtc::Call::Stats& stats); - void MediaTransportChange( - webrtc::MediaTransportInterface* media_transport_interface) override; - void SetClientBitratePreferences( const webrtc::BitrateSettings& preferences) override {} diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index b321d5c82d..4953494d88 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -7305,9 +7305,7 @@ bool PeerConnection::OnTransportChanged( } if (use_media_transport_) { - // Only pass media transport to call object if media transport is used - // for media (and not data channel). - call_ptr_->MediaTransportChange(media_transport); + RTC_LOG(LS_ERROR) << "Media transport isn't supported."; } return ret;