From 28e9653b1f3bf7900bffdfdee05e17ad432d37a4 Mon Sep 17 00:00:00 2001 From: Tommi Date: Thu, 3 Jun 2021 11:52:15 +0200 Subject: [PATCH] Remove dependency on RtpVideoSenderInterface from EncoderRtcpFeedback. This removes the two step initialization and explicit circular dependency between the sender and the observer that complicates construction and making members const that should be. Moving forward the encoder feedback instance will move to a different class, so this CL is one part of making that change possible. Also removing an unnecessary mutex and replacing with a checker. Bug: webrtc:12840 Change-Id: I21694806b122592de0cd1e1d96f241d339a0860f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/221108 Commit-Queue: Tommi Reviewed-by: Markus Handell Cr-Commit-Position: refs/heads/master@{#34214} --- video/BUILD.gn | 1 + video/encoder_rtcp_feedback.cc | 64 +++++++++++-------------- video/encoder_rtcp_feedback.h | 32 ++++++++----- video/encoder_rtcp_feedback_unittest.cc | 3 +- video/video_send_stream_impl.cc | 10 ++-- 5 files changed, 58 insertions(+), 52 deletions(-) diff --git a/video/BUILD.gn b/video/BUILD.gn index f446a537d5..7743aba944 100644 --- a/video/BUILD.gn +++ b/video/BUILD.gn @@ -67,6 +67,7 @@ rtc_library("video") { "../api/crypto:options", "../api/rtc_event_log", "../api/task_queue", + "../api/units:time_delta", "../api/units:timestamp", "../api/video:encoded_image", "../api/video:recordable_encoded_frame", diff --git a/video/encoder_rtcp_feedback.cc b/video/encoder_rtcp_feedback.cc index b81ff6120f..17095a0a0c 100644 --- a/video/encoder_rtcp_feedback.cc +++ b/video/encoder_rtcp_feedback.cc @@ -10,6 +10,9 @@ #include "video/encoder_rtcp_feedback.h" +#include +#include + #include "absl/types/optional.h" #include "api/video_codecs/video_encoder.h" #include "rtc_base/checks.h" @@ -21,47 +24,36 @@ namespace { constexpr int kMinKeyframeSendIntervalMs = 300; } // namespace -EncoderRtcpFeedback::EncoderRtcpFeedback(Clock* clock, - const std::vector& ssrcs, - VideoStreamEncoderInterface* encoder) +EncoderRtcpFeedback::EncoderRtcpFeedback( + Clock* clock, + const std::vector& ssrcs, + VideoStreamEncoderInterface* encoder, + std::function( + uint32_t ssrc, + const std::vector& seq_nums)> get_packet_infos) : clock_(clock), ssrcs_(ssrcs), - rtp_video_sender_(nullptr), + get_packet_infos_(std::move(get_packet_infos)), video_stream_encoder_(encoder), - time_last_intra_request_ms_(-1), - min_keyframe_send_interval_ms_( - KeyframeIntervalSettings::ParseFromFieldTrials() - .MinKeyframeSendIntervalMs() - .value_or(kMinKeyframeSendIntervalMs)) { + time_last_packet_delivery_queue_(Timestamp::Millis(0)), + min_keyframe_send_interval_( + TimeDelta::Millis(KeyframeIntervalSettings::ParseFromFieldTrials() + .MinKeyframeSendIntervalMs() + .value_or(kMinKeyframeSendIntervalMs))) { RTC_DCHECK(!ssrcs.empty()); + packet_delivery_queue_.Detach(); } -void EncoderRtcpFeedback::SetRtpVideoSender( - const RtpVideoSenderInterface* rtp_video_sender) { - RTC_DCHECK(rtp_video_sender); - RTC_DCHECK(!rtp_video_sender_); - rtp_video_sender_ = rtp_video_sender; -} - -bool EncoderRtcpFeedback::HasSsrc(uint32_t ssrc) { - for (uint32_t registered_ssrc : ssrcs_) { - if (registered_ssrc == ssrc) { - return true; - } - } - return false; -} - +// Called via Call::DeliverRtcp. void EncoderRtcpFeedback::OnReceivedIntraFrameRequest(uint32_t ssrc) { - RTC_DCHECK(HasSsrc(ssrc)); - { - int64_t now_ms = clock_->TimeInMilliseconds(); - MutexLock lock(&mutex_); - if (time_last_intra_request_ms_ + min_keyframe_send_interval_ms_ > now_ms) { - return; - } - time_last_intra_request_ms_ = now_ms; - } + RTC_DCHECK_RUN_ON(&packet_delivery_queue_); + RTC_DCHECK(std::find(ssrcs_.begin(), ssrcs_.end(), ssrc) != ssrcs_.end()); + + const Timestamp now = clock_->CurrentTime(); + if (time_last_packet_delivery_queue_ + min_keyframe_send_interval_ > now) + return; + + time_last_packet_delivery_queue_ = now; // Always produce key frame for all streams. video_stream_encoder_->SendKeyFrame(); @@ -72,12 +64,12 @@ void EncoderRtcpFeedback::OnReceivedLossNotification( uint16_t seq_num_of_last_decodable, uint16_t seq_num_of_last_received, bool decodability_flag) { - RTC_DCHECK(rtp_video_sender_) << "Object initialization incomplete."; + RTC_DCHECK(get_packet_infos_) << "Object initialization incomplete."; const std::vector seq_nums = {seq_num_of_last_decodable, seq_num_of_last_received}; const std::vector infos = - rtp_video_sender_->GetSentRtpPacketInfos(ssrc, seq_nums); + get_packet_infos_(ssrc, seq_nums); if (infos.empty()) { return; } diff --git a/video/encoder_rtcp_feedback.h b/video/encoder_rtcp_feedback.h index 3bd1cb91f0..2aadcc34e7 100644 --- a/video/encoder_rtcp_feedback.h +++ b/video/encoder_rtcp_feedback.h @@ -10,12 +10,16 @@ #ifndef VIDEO_ENCODER_RTCP_FEEDBACK_H_ #define VIDEO_ENCODER_RTCP_FEEDBACK_H_ +#include #include +#include "api/sequence_checker.h" +#include "api/units/time_delta.h" +#include "api/units/timestamp.h" #include "api/video/video_stream_encoder_interface.h" #include "call/rtp_video_sender_interface.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" -#include "rtc_base/synchronization/mutex.h" +#include "rtc_base/system/no_unique_address.h" #include "system_wrappers/include/clock.h" namespace webrtc { @@ -27,13 +31,15 @@ class VideoStreamEncoderInterface; class EncoderRtcpFeedback : public RtcpIntraFrameObserver, public RtcpLossNotificationObserver { public: - EncoderRtcpFeedback(Clock* clock, - const std::vector& ssrcs, - VideoStreamEncoderInterface* encoder); + EncoderRtcpFeedback( + Clock* clock, + const std::vector& ssrcs, + VideoStreamEncoderInterface* encoder, + std::function( + uint32_t ssrc, + const std::vector& seq_nums)> get_packet_infos); ~EncoderRtcpFeedback() override = default; - void SetRtpVideoSender(const RtpVideoSenderInterface* rtp_video_sender); - void OnReceivedIntraFrameRequest(uint32_t ssrc) override; // Implements RtcpLossNotificationObserver. @@ -43,17 +49,19 @@ class EncoderRtcpFeedback : public RtcpIntraFrameObserver, bool decodability_flag) override; private: - bool HasSsrc(uint32_t ssrc); - Clock* const clock_; const std::vector ssrcs_; - const RtpVideoSenderInterface* rtp_video_sender_; + const std::function( + uint32_t ssrc, + const std::vector& seq_nums)> + get_packet_infos_; VideoStreamEncoderInterface* const video_stream_encoder_; - Mutex mutex_; - int64_t time_last_intra_request_ms_ RTC_GUARDED_BY(mutex_); + RTC_NO_UNIQUE_ADDRESS SequenceChecker packet_delivery_queue_; + Timestamp time_last_packet_delivery_queue_ + RTC_GUARDED_BY(packet_delivery_queue_); - const int min_keyframe_send_interval_ms_; + const TimeDelta min_keyframe_send_interval_; }; } // namespace webrtc diff --git a/video/encoder_rtcp_feedback_unittest.cc b/video/encoder_rtcp_feedback_unittest.cc index 81ac22b6c6..4cbb747e51 100644 --- a/video/encoder_rtcp_feedback_unittest.cc +++ b/video/encoder_rtcp_feedback_unittest.cc @@ -26,7 +26,8 @@ class VieKeyRequestTest : public ::testing::Test { encoder_rtcp_feedback_( &simulated_clock_, std::vector(1, VieKeyRequestTest::kSsrc), - &encoder_) {} + &encoder_, + nullptr) {} protected: const uint32_t kSsrc = 1234; diff --git a/video/video_send_stream_impl.cc b/video/video_send_stream_impl.cc index ebd4445004..8f369d103d 100644 --- a/video/video_send_stream_impl.cc +++ b/video/video_send_stream_impl.cc @@ -223,7 +223,13 @@ VideoSendStreamImpl::VideoSendStreamImpl( encoder_bitrate_priority_(initial_encoder_bitrate_priority), has_packet_feedback_(false), video_stream_encoder_(video_stream_encoder), - encoder_feedback_(clock, config_->rtp.ssrcs, video_stream_encoder), + encoder_feedback_( + clock, + config_->rtp.ssrcs, + video_stream_encoder, + [this](uint32_t ssrc, const std::vector& seq_nums) { + return rtp_video_sender_->GetSentRtpPacketInfos(ssrc, seq_nums); + }), bandwidth_observer_(transport->GetBandwidthObserver()), rtp_video_sender_( transport_->CreateRtpVideoSender(suspended_ssrcs, @@ -245,8 +251,6 @@ VideoSendStreamImpl::VideoSendStreamImpl( RTC_LOG(LS_INFO) << "VideoSendStreamInternal: " << config_->ToString(); weak_ptr_ = weak_ptr_factory_.GetWeakPtr(); - encoder_feedback_.SetRtpVideoSender(rtp_video_sender_); - RTC_DCHECK(!config_->rtp.ssrcs.empty()); RTC_DCHECK(transport_); RTC_DCHECK_NE(initial_encoder_max_bitrate, 0);