From 572c60f44d8ad8743ba35451b37467ec4527d50e Mon Sep 17 00:00:00 2001 From: Sebastian Jansson Date: Mon, 4 Mar 2019 18:30:41 +0100 Subject: [PATCH] Injecting Clock into video senders. Bug: webrtc:10365 Change-Id: I1dc42345a95929970d4f390e04eff56ca0c6d60b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/125190 Commit-Queue: Sebastian Jansson Reviewed-by: Stefan Holmer Cr-Commit-Position: refs/heads/master@{#26959} --- api/video/video_stream_encoder_create.cc | 6 ++++-- api/video/video_stream_encoder_create.h | 3 +++ call/call.cc | 2 +- call/rtp_transport_controller_send.cc | 2 +- call/rtp_video_sender.cc | 12 +++++++++--- call/rtp_video_sender.h | 1 + call/rtp_video_sender_unittest.cc | 4 ++-- video/video_send_stream.cc | 13 ++++++------- video/video_send_stream.h | 1 + video/video_send_stream_impl.cc | 11 +++++------ video/video_send_stream_impl.h | 3 ++- video/video_send_stream_impl_unittest.cc | 19 ++++++++----------- video/video_stream_encoder.cc | 3 ++- video/video_stream_encoder.h | 3 ++- video/video_stream_encoder_unittest.cc | 3 ++- 15 files changed, 49 insertions(+), 37 deletions(-) diff --git a/api/video/video_stream_encoder_create.cc b/api/video/video_stream_encoder_create.cc index 3225c98a3c..875edd9928 100644 --- a/api/video/video_stream_encoder_create.cc +++ b/api/video/video_stream_encoder_create.cc @@ -20,17 +20,19 @@ std::unique_ptr CreateVideoStreamEncoder( uint32_t number_of_cores, VideoStreamEncoderObserver* encoder_stats_observer, const VideoStreamEncoderSettings& settings) { - return CreateVideoStreamEncoder(&GlobalTaskQueueFactory(), number_of_cores, + return CreateVideoStreamEncoder(Clock::GetRealTimeClock(), + &GlobalTaskQueueFactory(), number_of_cores, encoder_stats_observer, settings); } std::unique_ptr CreateVideoStreamEncoder( + Clock* clock, TaskQueueFactory* task_queue_factory, uint32_t number_of_cores, VideoStreamEncoderObserver* encoder_stats_observer, const VideoStreamEncoderSettings& settings) { return absl::make_unique( - number_of_cores, encoder_stats_observer, settings, + clock, number_of_cores, encoder_stats_observer, settings, absl::make_unique(encoder_stats_observer), task_queue_factory); } diff --git a/api/video/video_stream_encoder_create.h b/api/video/video_stream_encoder_create.h index 3923190239..4241626aa7 100644 --- a/api/video/video_stream_encoder_create.h +++ b/api/video/video_stream_encoder_create.h @@ -22,6 +22,8 @@ #include "api/video/video_stream_encoder_settings.h" namespace webrtc { +// TODO(srte): Find a way to avoid this forward declaration. +class Clock; std::unique_ptr CreateVideoStreamEncoder( uint32_t number_of_cores, @@ -29,6 +31,7 @@ std::unique_ptr CreateVideoStreamEncoder( const VideoStreamEncoderSettings& settings); std::unique_ptr CreateVideoStreamEncoder( + Clock* clock, TaskQueueFactory* task_queue_factory, uint32_t number_of_cores, VideoStreamEncoderObserver* encoder_stats_observer, diff --git a/call/call.cc b/call/call.cc index 35727b3b3d..7c807797ea 100644 --- a/call/call.cc +++ b/call/call.cc @@ -805,7 +805,7 @@ webrtc::VideoSendStream* Call::CreateVideoSendStream( // TODO(srte): VideoSendStream should call GetWorkerQueue directly rather than // having it injected. VideoSendStream* send_stream = new VideoSendStream( - num_cpu_cores_, module_process_thread_.get(), + clock_, num_cpu_cores_, module_process_thread_.get(), transport_send_ptr_->GetWorkerQueue(), task_queue_factory_, call_stats_.get(), transport_send_ptr_, bitrate_allocator_.get(), video_send_delay_stats_.get(), event_log_, std::move(config), diff --git a/call/rtp_transport_controller_send.cc b/call/rtp_transport_controller_send.cc index b07e5ece57..5d0734e10b 100644 --- a/call/rtp_transport_controller_send.cc +++ b/call/rtp_transport_controller_send.cc @@ -109,7 +109,7 @@ RtpVideoSenderInterface* RtpTransportControllerSend::CreateRtpVideoSender( std::unique_ptr fec_controller, const RtpSenderFrameEncryptionConfig& frame_encryption_config) { video_rtp_senders_.push_back(absl::make_unique( - suspended_ssrcs, states, rtp_config, rtcp_report_interval_ms, + clock_, suspended_ssrcs, states, rtp_config, rtcp_report_interval_ms, send_transport, observers, // TODO(holmer): Remove this circular dependency by injecting // the parts of RtpTransportControllerSendInterface that are really used. diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index 6c22120159..e5098dba21 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -58,6 +58,7 @@ static const size_t kPathMTU = 1500; using webrtc_internal_rtp_video_sender::RtpStreamSender; std::vector CreateRtpStreamSenders( + Clock* clock, const RtpConfig& rtp_config, int rtcp_report_interval_ms, Transport* send_transport, @@ -79,6 +80,7 @@ std::vector CreateRtpStreamSenders( RTC_DCHECK_GT(rtp_config.ssrcs.size(), 0); RtpRtcp::Configuration configuration; + configuration.clock = clock; configuration.audio = false; configuration.clock = Clock::GetRealTimeClock(); configuration.receiver_only = false; @@ -147,6 +149,7 @@ bool PayloadTypeSupportsSkippingFecPackets(const std::string& payload_name) { // TODO(brandtr): Update this function when we support multistream protection. std::unique_ptr MaybeCreateFlexfecSender( + Clock* clock, const RtpConfig& rtp, const std::map& suspended_ssrcs) { if (rtp.flexfec.payload_type < 0) { @@ -185,7 +188,7 @@ std::unique_ptr MaybeCreateFlexfecSender( return absl::make_unique( rtp.flexfec.payload_type, rtp.flexfec.ssrc, rtp.flexfec.protected_media_ssrcs[0], rtp.mid, rtp.extensions, - RTPSender::FecExtensionSizes(), rtp_state, Clock::GetRealTimeClock()); + RTPSender::FecExtensionSizes(), rtp_state, clock); } uint32_t CalculateOverheadRateBps(int packets_per_second, @@ -205,6 +208,7 @@ int CalculatePacketRate(uint32_t bitrate_bps, size_t packet_size_bytes) { } // namespace RtpVideoSender::RtpVideoSender( + Clock* clock, std::map suspended_ssrcs, const std::map& states, const RtpConfig& rtp_config, @@ -224,9 +228,11 @@ RtpVideoSender::RtpVideoSender( active_(false), module_process_thread_(nullptr), suspended_ssrcs_(std::move(suspended_ssrcs)), - flexfec_sender_(MaybeCreateFlexfecSender(rtp_config, suspended_ssrcs_)), + flexfec_sender_( + MaybeCreateFlexfecSender(clock, rtp_config, suspended_ssrcs_)), fec_controller_(std::move(fec_controller)), - rtp_streams_(CreateRtpStreamSenders(rtp_config, + rtp_streams_(CreateRtpStreamSenders(clock, + rtp_config, rtcp_report_interval_ms, send_transport, observers.intra_frame_callback, diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index 69187a9439..d50cb7c2d7 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h @@ -70,6 +70,7 @@ class RtpVideoSender : public RtpVideoSenderInterface, public: // Rtp modules are assumed to be sorted in simulcast index order. RtpVideoSender( + Clock* clock, std::map suspended_ssrcs, const std::map& states, const RtpConfig& rtp_config, diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc index 84306420a0..a387932311 100644 --- a/call/rtp_video_sender_unittest.cc +++ b/call/rtp_video_sender_unittest.cc @@ -80,7 +80,7 @@ class RtpVideoSenderTestFixture { int payload_type, const std::map& suspended_payload_states, FrameCountObserver* frame_count_observer) - : clock_(0), + : clock_(1000000), config_(&transport_), send_delay_stats_(&clock_), transport_controller_(&clock_, @@ -101,7 +101,7 @@ class RtpVideoSenderTestFixture { config_.rtp.payload_type = payload_type; std::map suspended_ssrcs; router_ = absl::make_unique( - suspended_ssrcs, suspended_payload_states, config_.rtp, + &clock_, suspended_ssrcs, suspended_payload_states, config_.rtp, config_.rtcp_report_interval_ms, &transport_, CreateObservers(&call_stats_, &encoder_feedback_, &stats_proxy_, &stats_proxy_, &stats_proxy_, frame_count_observer, diff --git a/video/video_send_stream.cc b/video/video_send_stream.cc index 222e6e45b8..978558d5d7 100644 --- a/video/video_send_stream.cc +++ b/video/video_send_stream.cc @@ -65,6 +65,7 @@ size_t CalculateMaxHeaderSize(const RtpConfig& config) { namespace internal { VideoSendStream::VideoSendStream( + Clock* clock, int num_cpu_cores, ProcessThread* module_process_thread, rtc::TaskQueue* worker_queue, @@ -80,26 +81,24 @@ VideoSendStream::VideoSendStream( const std::map& suspended_payload_states, std::unique_ptr fec_controller) : worker_queue_(worker_queue), - stats_proxy_(Clock::GetRealTimeClock(), - config, - encoder_config.content_type), + stats_proxy_(clock, config, encoder_config.content_type), config_(std::move(config)), content_type_(encoder_config.content_type) { RTC_DCHECK(config_.encoder_settings.encoder_factory); RTC_DCHECK(config_.encoder_settings.bitrate_allocator_factory); video_stream_encoder_ = - CreateVideoStreamEncoder(task_queue_factory, num_cpu_cores, &stats_proxy_, - config_.encoder_settings); + CreateVideoStreamEncoder(clock, task_queue_factory, num_cpu_cores, + &stats_proxy_, config_.encoder_settings); // TODO(srte): Initialization should not be done posted on a task queue. // Note that the posted task must not outlive this scope since the closure // references local variables. worker_queue_->PostTask(rtc::NewClosure( - [this, call_stats, transport, bitrate_allocator, send_delay_stats, + [this, clock, call_stats, transport, bitrate_allocator, send_delay_stats, event_log, &suspended_ssrcs, &encoder_config, &suspended_payload_states, &fec_controller]() { send_stream_.reset(new VideoSendStreamImpl( - &stats_proxy_, worker_queue_, call_stats, transport, + clock, &stats_proxy_, worker_queue_, call_stats, transport, bitrate_allocator, send_delay_stats, video_stream_encoder_.get(), event_log, &config_, encoder_config.max_bitrate_bps, encoder_config.bitrate_priority, suspended_ssrcs, diff --git a/video/video_send_stream.h b/video/video_send_stream.h index 92600ce020..240432f09a 100644 --- a/video/video_send_stream.h +++ b/video/video_send_stream.h @@ -53,6 +53,7 @@ class VideoSendStream : public webrtc::VideoSendStream { using RtpPayloadStateMap = std::map; VideoSendStream( + Clock* clock, int num_cpu_cores, ProcessThread* module_process_thread, rtc::TaskQueue* worker_queue, diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index b8088ea8bd..9a563417dc 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc @@ -31,7 +31,6 @@ #include "rtc_base/numerics/safe_conversions.h" #include "rtc_base/sequenced_task_checker.h" #include "rtc_base/thread_checker.h" -#include "rtc_base/time_utils.h" #include "rtc_base/trace_event.h" #include "system_wrappers/include/clock.h" #include "system_wrappers/include/field_trial.h" @@ -195,6 +194,7 @@ PacingConfig::PacingConfig(const PacingConfig&) = default; PacingConfig::~PacingConfig() = default; VideoSendStreamImpl::VideoSendStreamImpl( + Clock* clock, SendStatisticsProxy* stats_proxy, rtc::TaskQueue* worker_queue, CallStats* call_stats, @@ -211,7 +211,8 @@ VideoSendStreamImpl::VideoSendStreamImpl( VideoEncoderConfig::ContentType content_type, std::unique_ptr fec_controller, MediaTransportInterface* media_transport) - : has_alr_probing_(config->periodic_alr_bandwidth_probing || + : clock_(clock), + has_alr_probing_(config->periodic_alr_bandwidth_probing || GetAlrSettings(content_type)), pacing_config_(PacingConfig()), stats_proxy_(stats_proxy), @@ -227,9 +228,7 @@ VideoSendStreamImpl::VideoSendStreamImpl( encoder_bitrate_priority_(initial_encoder_bitrate_priority), has_packet_feedback_(false), video_stream_encoder_(video_stream_encoder), - encoder_feedback_(Clock::GetRealTimeClock(), - config_->rtp.ssrcs, - video_stream_encoder), + encoder_feedback_(clock, config_->rtp.ssrcs, video_stream_encoder), bandwidth_observer_(transport->GetBandwidthObserver()), rtp_video_sender_(transport_->CreateRtpVideoSender( suspended_ssrcs, @@ -454,7 +453,7 @@ void VideoSendStreamImpl::OnBitrateAllocationUpdated( RTC_DCHECK_RUN_ON(worker_queue_); - int64_t now_ms = rtc::TimeMillis(); + int64_t now_ms = clock_->TimeInMilliseconds(); if (encoder_target_rate_bps_ != 0) { if (video_bitrate_allocation_context_) { // If new allocation is within kMaxVbaSizeDifferencePercent larger than diff --git a/video/video_send_stream_impl.h b/video/video_send_stream_impl.h index 1cdad60d2c..c9472fb5fa 100644 --- a/video/video_send_stream_impl.h +++ b/video/video_send_stream_impl.h @@ -70,6 +70,7 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, public VideoBitrateAllocationObserver { public: VideoSendStreamImpl( + Clock* clock, SendStatisticsProxy* stats_proxy, rtc::TaskQueue* worker_queue, CallStats* call_stats, @@ -141,7 +142,7 @@ class VideoSendStreamImpl : public webrtc::BitrateAllocatorObserver, void SignalEncoderActive(); MediaStreamAllocationConfig GetAllocationConfig() const RTC_RUN_ON(worker_queue_); - + Clock* const clock_; const bool has_alr_probing_; const PacingConfig pacing_config_; diff --git a/video/video_send_stream_impl_unittest.cc b/video/video_send_stream_impl_unittest.cc index 1852dc05e9..ea70408b25 100644 --- a/video/video_send_stream_impl_unittest.cc +++ b/video/video_send_stream_impl_unittest.cc @@ -118,11 +118,11 @@ class VideoSendStreamImplTest : public ::testing::Test { std::map suspended_ssrcs; std::map suspended_payload_states; return absl::make_unique( - &stats_proxy_, &test_queue_, &call_stats_, &transport_controller_, - &bitrate_allocator_, &send_delay_stats_, &video_stream_encoder_, - &event_log_, &config_, initial_encoder_max_bitrate, - initial_encoder_bitrate_priority, suspended_ssrcs, - suspended_payload_states, content_type, + &clock_, &stats_proxy_, &test_queue_, &call_stats_, + &transport_controller_, &bitrate_allocator_, &send_delay_stats_, + &video_stream_encoder_, &event_log_, &config_, + initial_encoder_max_bitrate, initial_encoder_bitrate_priority, + suspended_ssrcs, suspended_payload_states, content_type, absl::make_unique(&clock_), /*media_transport=*/nullptr); } @@ -524,9 +524,6 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationOnLayerChange) { TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationAfterTimeout) { test_queue_.SendTask([this] { - rtc::ScopedFakeClock fake_clock; - fake_clock.SetTimeMicros(clock_.TimeInMicroseconds()); - auto vss_impl = CreateVideoSendStreamImpl( kDefaultInitialBitrateBps, kDefaultBitratePriority, VideoEncoderConfig::ContentType::kScreen); @@ -571,7 +568,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationAfterTimeout) { observer->OnBitrateAllocationUpdated(alloc); } - fake_clock.AdvanceTimeMicros(kMaxVbaThrottleTimeMs * 1000); + clock_.AdvanceTimeMicroseconds(kMaxVbaThrottleTimeMs * 1000); { // Sending similar allocation again after timeout, should forward. @@ -598,7 +595,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationAfterTimeout) { { // Advance time and send encoded image, this should wake up and send // cached bitrate allocation. - fake_clock.AdvanceTimeMicros(kMaxVbaThrottleTimeMs * 1000); + clock_.AdvanceTimeMicroseconds(kMaxVbaThrottleTimeMs * 1000); EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc)) .Times(1); static_cast(vss_impl.get()) @@ -608,7 +605,7 @@ TEST_F(VideoSendStreamImplTest, ForwardsVideoBitrateAllocationAfterTimeout) { { // Advance time and send encoded image, there should be no cached // allocation to send. - fake_clock.AdvanceTimeMicros(kMaxVbaThrottleTimeMs * 1000); + clock_.AdvanceTimeMicroseconds(kMaxVbaThrottleTimeMs * 1000); EXPECT_CALL(rtp_video_sender_, OnBitrateAllocationUpdated(alloc)) .Times(0); static_cast(vss_impl.get()) diff --git a/video/video_stream_encoder.cc b/video/video_stream_encoder.cc index f39f77de4b..528a77e24b 100644 --- a/video/video_stream_encoder.cc +++ b/video/video_stream_encoder.cc @@ -419,6 +419,7 @@ class VideoStreamEncoder::VideoSourceProxy { }; VideoStreamEncoder::VideoStreamEncoder( + Clock* clock, uint32_t number_of_cores, VideoStreamEncoderObserver* encoder_stats_observer, const VideoStreamEncoderSettings& settings, @@ -446,7 +447,7 @@ VideoStreamEncoder::VideoStreamEncoder( max_data_payload_length_(0), last_observed_bitrate_bps_(0), encoder_paused_and_dropped_frame_(false), - clock_(Clock::GetRealTimeClock()), + clock_(clock), degradation_preference_(DegradationPreference::DISABLED), posted_frames_waiting_for_encode_(0), last_captured_timestamp_(0), diff --git a/video/video_stream_encoder.h b/video/video_stream_encoder.h index 911627a409..70e2c899ea 100644 --- a/video/video_stream_encoder.h +++ b/video/video_stream_encoder.h @@ -53,7 +53,8 @@ class VideoStreamEncoder : public VideoStreamEncoderInterface, // Protected only to provide access to tests. protected AdaptationObserverInterface { public: - VideoStreamEncoder(uint32_t number_of_cores, + VideoStreamEncoder(Clock* clock, + uint32_t number_of_cores, VideoStreamEncoderObserver* encoder_stats_observer, const VideoStreamEncoderSettings& settings, std::unique_ptr overuse_detector, diff --git a/video/video_stream_encoder_unittest.cc b/video/video_stream_encoder_unittest.cc index 75b47f2803..23984e9f52 100644 --- a/video/video_stream_encoder_unittest.cc +++ b/video/video_stream_encoder_unittest.cc @@ -100,7 +100,8 @@ class VideoStreamEncoderUnderTest : public VideoStreamEncoder { public: VideoStreamEncoderUnderTest(SendStatisticsProxy* stats_proxy, const VideoStreamEncoderSettings& settings) - : VideoStreamEncoder(1 /* number_of_cores */, + : VideoStreamEncoder(Clock::GetRealTimeClock(), + 1 /* number_of_cores */, stats_proxy, settings, std::unique_ptr(