From 599d59237abf0dcb6b92774595316bfda84ab7f5 Mon Sep 17 00:00:00 2001 From: Johannes Kron Date: Tue, 19 Feb 2019 14:27:57 +0100 Subject: [PATCH] Extend RemoteEstimatorProxy to support feedback on sender request. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug: webrtc:10263 Change-Id: I85b992eff7d1e7ed66ead4745f346ddad2977e8e Reviewed-on: https://webrtc-review.googlesource.com/c/120800 Commit-Queue: Johannes Kron Reviewed-by: Sebastian Jansson Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#26752} --- .../remote_estimator_proxy.cc | 142 ++++++++++++------ .../remote_estimator_proxy.h | 30 +++- .../remote_estimator_proxy_unittest.cc | 109 +++++++++++++- 3 files changed, 225 insertions(+), 56 deletions(-) diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc index 932304428f..b1d7e70778 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::kBackWindowMs = 500; 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 @@ RemoteEstimatorProxy::RemoteEstimatorProxy( 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 @@ void RemoteEstimatorProxy::IncomingPacket(int64_t arrival_time_ms, } 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* ssrcs, @@ -67,29 +69,26 @@ bool RemoteEstimatorProxy::LatestEstimate(std::vector* ssrcs, } 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 @@ void RemoteEstimatorProxy::SetSendFeedbackOnRequestOnly( 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 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 @@ void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number, 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 @@ void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number, 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::const_iterator begin_iterator, + std::map::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(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(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(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 94b1716004..73d89c159b 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.h +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.h @@ -32,6 +32,10 @@ class TransportFeedback; 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 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { 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 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::const_iterator + begin_iterator, // |begin_iterator| is inclusive. + std::map::const_iterator + end_iterator, // |end_iterator| is exclusive. + rtcp::TransportFeedback* feedback_packet); Clock* const clock_; TransportFeedbackSenderInterface* const feedback_sender_; @@ -66,7 +80,7 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { 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 6c1fc8cd93..9cce167e86 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 @@ class RemoteEstimatorProxyTest : public ::testing::Test { 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 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 @@ TEST_F(RemoteEstimatorProxyTest, TwccReportsUse5PercentOfAvailableBandwidth) { 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