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 <tommi@webrtc.org>
Reviewed-by: Markus Handell <handellm@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34214}
diff --git a/video/BUILD.gn b/video/BUILD.gn
index f446a53..7743aba 100644
--- a/video/BUILD.gn
+++ b/video/BUILD.gn
@@ -67,6 +67,7 @@
"../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 b81ff61..17095a0 100644
--- a/video/encoder_rtcp_feedback.cc
+++ b/video/encoder_rtcp_feedback.cc
@@ -10,6 +10,9 @@
#include "video/encoder_rtcp_feedback.h"
+#include <algorithm>
+#include <utility>
+
#include "absl/types/optional.h"
#include "api/video_codecs/video_encoder.h"
#include "rtc_base/checks.h"
@@ -21,47 +24,36 @@
constexpr int kMinKeyframeSendIntervalMs = 300;
} // namespace
-EncoderRtcpFeedback::EncoderRtcpFeedback(Clock* clock,
- const std::vector<uint32_t>& ssrcs,
- VideoStreamEncoderInterface* encoder)
+EncoderRtcpFeedback::EncoderRtcpFeedback(
+ Clock* clock,
+ const std::vector<uint32_t>& ssrcs,
+ VideoStreamEncoderInterface* encoder,
+ std::function<std::vector<RtpSequenceNumberMap::Info>(
+ uint32_t ssrc,
+ const std::vector<uint16_t>& 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 @@
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<uint16_t> seq_nums = {seq_num_of_last_decodable,
seq_num_of_last_received};
const std::vector<RtpSequenceNumberMap::Info> 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 3bd1cb9..2aadcc3 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 <functional>
#include <vector>
+#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 EncoderRtcpFeedback : public RtcpIntraFrameObserver,
public RtcpLossNotificationObserver {
public:
- EncoderRtcpFeedback(Clock* clock,
- const std::vector<uint32_t>& ssrcs,
- VideoStreamEncoderInterface* encoder);
+ EncoderRtcpFeedback(
+ Clock* clock,
+ const std::vector<uint32_t>& ssrcs,
+ VideoStreamEncoderInterface* encoder,
+ std::function<std::vector<RtpSequenceNumberMap::Info>(
+ uint32_t ssrc,
+ const std::vector<uint16_t>& 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 @@
bool decodability_flag) override;
private:
- bool HasSsrc(uint32_t ssrc);
-
Clock* const clock_;
const std::vector<uint32_t> ssrcs_;
- const RtpVideoSenderInterface* rtp_video_sender_;
+ const std::function<std::vector<RtpSequenceNumberMap::Info>(
+ uint32_t ssrc,
+ const std::vector<uint16_t>& 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 81ac22b..4cbb747 100644
--- a/video/encoder_rtcp_feedback_unittest.cc
+++ b/video/encoder_rtcp_feedback_unittest.cc
@@ -26,7 +26,8 @@
encoder_rtcp_feedback_(
&simulated_clock_,
std::vector<uint32_t>(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 ebd4445..8f369d1 100644
--- a/video/video_send_stream_impl.cc
+++ b/video/video_send_stream_impl.cc
@@ -223,7 +223,13 @@
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<uint16_t>& 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 @@
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);
