Extend RemoteEstimatorProxy to support feedback on sender request.
Bug: webrtc:10263
Change-Id: I85b992eff7d1e7ed66ead4745f346ddad2977e8e
Reviewed-on: https://webrtc-review.googlesource.com/c/120800
Commit-Queue: Johannes Kron <kron@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26752}diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
index 9323044..b1d7e70 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
@@ -26,6 +26,8 @@
const int RemoteEstimatorProxy::kMinSendIntervalMs = 50;
const int RemoteEstimatorProxy::kMaxSendIntervalMs = 250;
const int RemoteEstimatorProxy::kDefaultSendIntervalMs = 100;
+// Impossible to request feedback older than what can be represented by 15 bits.
+const int RemoteEstimatorProxy::kMaxNumberOfPackets = (1 << 15);
// The maximum allowed value for a timestamp in milliseconds. This is lower
// than the numerical limit since we often convert to microseconds.
@@ -39,7 +41,7 @@
feedback_sender_(feedback_sender),
last_process_time_ms_(-1),
media_ssrc_(0),
- feedback_sequence_(0),
+ feedback_packet_count_(0),
window_start_seq_(-1),
send_interval_ms_(kDefaultSendIntervalMs),
send_feedback_on_request_only_(false) {}
@@ -57,8 +59,8 @@
}
rtc::CritScope cs(&lock_);
media_ssrc_ = header.ssrc;
-
- OnPacketArrival(header.extension.transportSequenceNumber, arrival_time_ms);
+ OnPacketArrival(header.extension.transportSequenceNumber, arrival_time_ms,
+ header.extension.feedback_request);
}
bool RemoteEstimatorProxy::LatestEstimate(std::vector<unsigned int>* ssrcs,
@@ -67,29 +69,26 @@
}
int64_t RemoteEstimatorProxy::TimeUntilNextProcess() {
- int64_t time_until_next = 0;
- if (last_process_time_ms_ != -1) {
- rtc::CritScope cs(&lock_);
+ rtc::CritScope cs(&lock_);
+ if (send_feedback_on_request_only_) {
+ // Wait a day until next process.
+ return 24 * 60 * 60 * 1000;
+ } else if (last_process_time_ms_ != -1) {
int64_t now = clock_->TimeInMilliseconds();
if (now - last_process_time_ms_ < send_interval_ms_)
- time_until_next = (last_process_time_ms_ + send_interval_ms_ - now);
+ return last_process_time_ms_ + send_interval_ms_ - now;
}
- return time_until_next;
+ return 0;
}
void RemoteEstimatorProxy::Process() {
+ rtc::CritScope cs(&lock_);
+ if (send_feedback_on_request_only_) {
+ return;
+ }
last_process_time_ms_ = clock_->TimeInMilliseconds();
- bool more_to_build = true;
- while (more_to_build) {
- rtcp::TransportFeedback feedback_packet;
- if (BuildFeedbackPacket(&feedback_packet)) {
- RTC_DCHECK(feedback_sender_ != nullptr);
- feedback_sender_->SendTransportFeedback(&feedback_packet);
- } else {
- more_to_build = false;
- }
- }
+ SendPeriodicFeedbacks();
}
void RemoteEstimatorProxy::OnBitrateChanged(int bitrate_bps) {
@@ -117,8 +116,10 @@
send_feedback_on_request_only_ = send_feedback_on_request_only;
}
-void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number,
- int64_t arrival_time) {
+void RemoteEstimatorProxy::OnPacketArrival(
+ uint16_t sequence_number,
+ int64_t arrival_time,
+ absl::optional<FeedbackRequest> feedback_request) {
if (arrival_time < 0 || arrival_time > kMaxTimeMs) {
RTC_LOG(LS_WARNING) << "Arrival time out of bounds: " << arrival_time;
return;
@@ -137,8 +138,13 @@
return;
}
- if (packet_arrival_times_.lower_bound(window_start_seq_) ==
- packet_arrival_times_.end()) {
+ if (send_feedback_on_request_only_) {
+ // Remove old packet arrival times.
+ auto clear_to_it =
+ packet_arrival_times_.lower_bound(seq - kMaxNumberOfPackets);
+ packet_arrival_times_.erase(packet_arrival_times_.begin(), clear_to_it);
+ } else if (packet_arrival_times_.lower_bound(window_start_seq_) ==
+ packet_arrival_times_.end()) {
// Start new feedback packet, cull old packets.
for (auto it = packet_arrival_times_.begin();
it != packet_arrival_times_.end() && it->first < seq &&
@@ -160,49 +166,91 @@
return;
packet_arrival_times_[seq] = arrival_time;
+
+ if (feedback_request) {
+ // Send feedback packet immediately.
+ SendFeedbackOnRequest(seq, *feedback_request);
+ }
}
-bool RemoteEstimatorProxy::BuildFeedbackPacket(
- rtcp::TransportFeedback* feedback_packet) {
- // window_start_seq_ is the first sequence number to include in the current
+void RemoteEstimatorProxy::SendPeriodicFeedbacks() {
+ // |window_start_seq_| is the first sequence number to include in the current
// feedback packet. Some older may still be in the map, in case a reordering
// happens and we need to retransmit them.
- rtc::CritScope cs(&lock_);
- auto it = packet_arrival_times_.lower_bound(window_start_seq_);
- if (it == packet_arrival_times_.end()) {
- // Feedback for all packets already sent.
- return false;
+ for (auto begin_iterator =
+ packet_arrival_times_.lower_bound(window_start_seq_);
+ begin_iterator != packet_arrival_times_.cend();
+ begin_iterator = packet_arrival_times_.lower_bound(window_start_seq_)) {
+ rtcp::TransportFeedback feedback_packet;
+ window_start_seq_ = BuildFeedbackPacket(
+ feedback_packet_count_++, media_ssrc_, window_start_seq_,
+ begin_iterator, packet_arrival_times_.cend(), &feedback_packet);
+
+ RTC_DCHECK(feedback_sender_ != nullptr);
+ feedback_sender_->SendTransportFeedback(&feedback_packet);
+ // Note: Don't erase items from packet_arrival_times_ after sending, in case
+ // they need to be re-sent after a reordering. Removal will be handled
+ // by OnPacketArrival once packets are too old.
}
+}
+
+void RemoteEstimatorProxy::SendFeedbackOnRequest(
+ int64_t sequence_number,
+ const FeedbackRequest& feedback_request) {
+ rtcp::TransportFeedback feedback_packet(feedback_request.include_timestamps);
+
+ int64_t first_sequence_number =
+ sequence_number - feedback_request.sequence_count;
+ auto begin_iterator =
+ packet_arrival_times_.lower_bound(first_sequence_number);
+ auto end_iterator = packet_arrival_times_.upper_bound(sequence_number);
+
+ // window_start_seq must be updated to make sure that we detect incorrectly
+ // unwrapped sequence_numbers in OnPacketArrival().
+ window_start_seq_ = BuildFeedbackPacket(feedback_packet_count_++, media_ssrc_,
+ first_sequence_number, begin_iterator,
+ end_iterator, &feedback_packet);
+
+ // Clear up to the first packet that is included in this feedback packet.
+ packet_arrival_times_.erase(packet_arrival_times_.begin(), begin_iterator);
+
+ RTC_DCHECK(feedback_sender_ != nullptr);
+ feedback_sender_->SendTransportFeedback(&feedback_packet);
+}
+
+int64_t RemoteEstimatorProxy::BuildFeedbackPacket(
+ uint8_t feedback_packet_count,
+ uint32_t media_ssrc,
+ int64_t base_sequence_number,
+ std::map<int64_t, int64_t>::const_iterator begin_iterator,
+ std::map<int64_t, int64_t>::const_iterator end_iterator,
+ rtcp::TransportFeedback* feedback_packet) {
+ RTC_DCHECK(begin_iterator != end_iterator);
// TODO(sprang): Measure receive times in microseconds and remove the
// conversions below.
- const int64_t first_sequence = it->first;
- feedback_packet->SetMediaSsrc(media_ssrc_);
- // Base sequence is the expected next (window_start_seq_). This is known, but
- // we might not have actually received it, so the base time shall be the time
- // of the first received packet in the feedback.
- feedback_packet->SetBase(static_cast<uint16_t>(window_start_seq_ & 0xFFFF),
- it->second * 1000);
- feedback_packet->SetFeedbackSequenceNumber(feedback_sequence_++);
- for (; it != packet_arrival_times_.end(); ++it) {
+ feedback_packet->SetMediaSsrc(media_ssrc);
+ // Base sequence number is the expected first sequence number. This is known,
+ // but we might not have actually received it, so the base time shall be the
+ // time of the first received packet in the feedback.
+ feedback_packet->SetBase(static_cast<uint16_t>(base_sequence_number & 0xFFFF),
+ begin_iterator->second * 1000);
+ feedback_packet->SetFeedbackSequenceNumber(feedback_packet_count);
+ int64_t next_sequence_number = base_sequence_number;
+ for (auto it = begin_iterator; it != end_iterator; ++it) {
if (!feedback_packet->AddReceivedPacket(
static_cast<uint16_t>(it->first & 0xFFFF), it->second * 1000)) {
// If we can't even add the first seq to the feedback packet, we won't be
// able to build it at all.
- RTC_CHECK_NE(first_sequence, it->first);
+ RTC_CHECK(begin_iterator != it);
// Could not add timestamp, feedback packet might be full. Return and
// try again with a fresh packet.
break;
}
-
- // Note: Don't erase items from packet_arrival_times_ after sending, in case
- // they need to be re-sent after a reordering. Removal will be handled
- // by OnPacketArrival once packets are too old.
- window_start_seq_ = it->first + 1;
+ next_sequence_number = it->first + 1;
}
-
- return true;
+ return next_sequence_number;
}
} // namespace webrtc
diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.h b/modules/remote_bitrate_estimator/remote_estimator_proxy.h
index 94b1716..73d89c1 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy.h
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.h
@@ -32,6 +32,10 @@
class RemoteEstimatorProxy : public RemoteBitrateEstimator {
public:
+ static const int kMinSendIntervalMs;
+ static const int kMaxSendIntervalMs;
+ static const int kDefaultSendIntervalMs;
+ static const int kBackWindowMs;
RemoteEstimatorProxy(Clock* clock,
TransportFeedbackSenderInterface* feedback_sender);
~RemoteEstimatorProxy() override;
@@ -49,15 +53,25 @@
void OnBitrateChanged(int bitrate);
void SetSendFeedbackOnRequestOnly(bool send_feedback_on_request_only);
- static const int kMinSendIntervalMs;
- static const int kMaxSendIntervalMs;
- static const int kDefaultSendIntervalMs;
- static const int kBackWindowMs;
-
private:
- void OnPacketArrival(uint16_t sequence_number, int64_t arrival_time)
+ static const int kMaxNumberOfPackets;
+ void OnPacketArrival(uint16_t sequence_number,
+ int64_t arrival_time,
+ absl::optional<FeedbackRequest> feedback_request)
RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_);
- bool BuildFeedbackPacket(rtcp::TransportFeedback* feedback_packet);
+ void SendPeriodicFeedbacks() RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_);
+ void SendFeedbackOnRequest(int64_t sequence_number,
+ const FeedbackRequest& feedback_request)
+ RTC_EXCLUSIVE_LOCKS_REQUIRED(&lock_);
+ static int64_t BuildFeedbackPacket(
+ uint8_t feedback_packet_count,
+ uint32_t media_ssrc,
+ int64_t base_sequence_number,
+ std::map<int64_t, int64_t>::const_iterator
+ begin_iterator, // |begin_iterator| is inclusive.
+ std::map<int64_t, int64_t>::const_iterator
+ end_iterator, // |end_iterator| is exclusive.
+ rtcp::TransportFeedback* feedback_packet);
Clock* const clock_;
TransportFeedbackSenderInterface* const feedback_sender_;
@@ -66,7 +80,7 @@
rtc::CriticalSection lock_;
uint32_t media_ssrc_ RTC_GUARDED_BY(&lock_);
- uint8_t feedback_sequence_ RTC_GUARDED_BY(&lock_);
+ uint8_t feedback_packet_count_ RTC_GUARDED_BY(&lock_);
SequenceNumberUnwrapper unwrapper_ RTC_GUARDED_BY(&lock_);
int64_t window_start_seq_ RTC_GUARDED_BY(&lock_);
// Map unwrapped seq -> time.
diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
index 6c1fc8c..9cce167 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
@@ -61,14 +61,21 @@
RemoteEstimatorProxyTest() : clock_(0), proxy_(&clock_, &router_) {}
protected:
- void IncomingPacket(uint16_t seq, int64_t time_ms) {
+ void IncomingPacket(uint16_t seq,
+ int64_t time_ms,
+ absl::optional<FeedbackRequest> feedback_request) {
RTPHeader header;
header.extension.hasTransportSequenceNumber = true;
header.extension.transportSequenceNumber = seq;
+ header.extension.feedback_request = feedback_request;
header.ssrc = kMediaSsrc;
proxy_.IncomingPacket(time_ms, kDefaultPacketSize, header);
}
+ void IncomingPacket(uint16_t seq, int64_t time_ms) {
+ IncomingPacket(seq, time_ms, absl::nullopt);
+ }
+
void Process() {
clock_.AdvanceTimeMilliseconds(
RemoteEstimatorProxy::kDefaultSendIntervalMs);
@@ -334,5 +341,105 @@
EXPECT_EQ(136, proxy_.TimeUntilNextProcess());
}
+//////////////////////////////////////////////////////////////////////////////
+// Tests for the extended protocol where the feedback is explicitly requested
+// by the sender.
+//////////////////////////////////////////////////////////////////////////////
+typedef RemoteEstimatorProxyTest RemoteEstimatorProxyOnRequestTest;
+TEST_F(RemoteEstimatorProxyOnRequestTest, TimeUntilNextProcessIsHigh) {
+ proxy_.SetSendFeedbackOnRequestOnly(true);
+ EXPECT_GE(proxy_.TimeUntilNextProcess(), 60 * 60 * 1000);
+}
+
+TEST_F(RemoteEstimatorProxyOnRequestTest, ProcessDoesNotSendFeedback) {
+ proxy_.SetSendFeedbackOnRequestOnly(true);
+ IncomingPacket(kBaseSeq, kBaseTimeMs);
+ EXPECT_CALL(router_, SendTransportFeedback(_)).Times(0);
+ Process();
+}
+
+TEST_F(RemoteEstimatorProxyOnRequestTest, RequestSinglePacketFeedback) {
+ proxy_.SetSendFeedbackOnRequestOnly(true);
+ IncomingPacket(kBaseSeq, kBaseTimeMs);
+ IncomingPacket(kBaseSeq + 1, kBaseTimeMs + kMaxSmallDeltaMs);
+ IncomingPacket(kBaseSeq + 2, kBaseTimeMs + 2 * kMaxSmallDeltaMs);
+
+ EXPECT_CALL(router_, SendTransportFeedback(_))
+ .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) {
+ EXPECT_EQ(kBaseSeq + 3, feedback_packet->GetBaseSequence());
+ EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc());
+
+ EXPECT_THAT(SequenceNumbers(*feedback_packet),
+ ElementsAre(kBaseSeq + 3));
+ EXPECT_THAT(TimestampsMs(*feedback_packet),
+ ElementsAre(kBaseTimeMs + 3 * kMaxSmallDeltaMs));
+ return true;
+ }));
+
+ constexpr FeedbackRequest kSinglePacketFeedbackRequest = {
+ /*include_timestamps=*/true, /*sequence_count=*/0};
+ IncomingPacket(kBaseSeq + 3, kBaseTimeMs + 3 * kMaxSmallDeltaMs,
+ kSinglePacketFeedbackRequest);
+}
+
+TEST_F(RemoteEstimatorProxyOnRequestTest, RequestLastFivePacketFeedback) {
+ proxy_.SetSendFeedbackOnRequestOnly(true);
+ int i = 0;
+ for (; i < 10; ++i) {
+ IncomingPacket(kBaseSeq + i, kBaseTimeMs + i * kMaxSmallDeltaMs);
+ }
+
+ EXPECT_CALL(router_, SendTransportFeedback(_))
+ .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) {
+ EXPECT_EQ(kBaseSeq + 6, feedback_packet->GetBaseSequence());
+ EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc());
+
+ EXPECT_THAT(SequenceNumbers(*feedback_packet),
+ ElementsAre(kBaseSeq + 6, kBaseSeq + 7, kBaseSeq + 8,
+ kBaseSeq + 9, kBaseSeq + 10));
+ EXPECT_THAT(TimestampsMs(*feedback_packet),
+ ElementsAre(kBaseTimeMs + 6 * kMaxSmallDeltaMs,
+ kBaseTimeMs + 7 * kMaxSmallDeltaMs,
+ kBaseTimeMs + 8 * kMaxSmallDeltaMs,
+ kBaseTimeMs + 9 * kMaxSmallDeltaMs,
+ kBaseTimeMs + 10 * kMaxSmallDeltaMs));
+ return true;
+ }));
+
+ constexpr FeedbackRequest kFivePacketsFeedbackRequest = {
+ /*include_timestamps=*/true, /*sequence_count=*/4};
+ IncomingPacket(kBaseSeq + i, kBaseTimeMs + i * kMaxSmallDeltaMs,
+ kFivePacketsFeedbackRequest);
+}
+
+TEST_F(RemoteEstimatorProxyOnRequestTest,
+ RequestLastFivePacketFeedbackMissingPackets) {
+ proxy_.SetSendFeedbackOnRequestOnly(true);
+ int i = 0;
+ for (; i < 10; ++i) {
+ if (i != 7 && i != 9)
+ IncomingPacket(kBaseSeq + i, kBaseTimeMs + i * kMaxSmallDeltaMs);
+ }
+
+ EXPECT_CALL(router_, SendTransportFeedback(_))
+ .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) {
+ EXPECT_EQ(kBaseSeq + 6, feedback_packet->GetBaseSequence());
+ EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc());
+
+ EXPECT_THAT(SequenceNumbers(*feedback_packet),
+ ElementsAre(kBaseSeq + 6, kBaseSeq + 8, kBaseSeq + 10));
+ EXPECT_THAT(TimestampsMs(*feedback_packet),
+ ElementsAre(kBaseTimeMs + 6 * kMaxSmallDeltaMs,
+ kBaseTimeMs + 8 * kMaxSmallDeltaMs,
+ kBaseTimeMs + 10 * kMaxSmallDeltaMs));
+ return true;
+ }));
+
+ constexpr FeedbackRequest kFivePacketsFeedbackRequest = {
+ /*include_timestamps=*/true, /*sequence_count=*/4};
+ IncomingPacket(kBaseSeq + i, kBaseTimeMs + i * kMaxSmallDeltaMs,
+ kFivePacketsFeedbackRequest);
+}
+
} // namespace
} // namespace webrtc
