diff --git a/call/call.cc b/call/call.cc index 1e8d3a9309..38a4dd5a6d 100644 --- a/call/call.cc +++ b/call/call.cc @@ -69,12 +69,12 @@ namespace webrtc { namespace { -bool SendFeedbackOnRequestOnly(const std::vector& extensions) { +bool SendPeriodicFeedback(const std::vector& extensions) { for (const auto& extension : extensions) { if (extension.uri == RtpExtension::kTransportSequenceNumberV2Uri) - return true; + return false; } - return false; + return true; } // TODO(nisse): This really begs for a shared context struct. @@ -931,8 +931,8 @@ webrtc::VideoReceiveStream* Call::CreateVideoReceiveStream( TRACE_EVENT0("webrtc", "Call::CreateVideoReceiveStream"); RTC_DCHECK_CALLED_SEQUENTIALLY(&configuration_sequence_checker_); - receive_side_cc_.SetSendFeedbackOnRequestOnly( - SendFeedbackOnRequestOnly(configuration.rtp.extensions)); + receive_side_cc_.SetSendPeriodicFeedback( + SendPeriodicFeedback(configuration.rtp.extensions)); RegisterRateObserver(); diff --git a/modules/congestion_controller/include/receive_side_congestion_controller.h b/modules/congestion_controller/include/receive_side_congestion_controller.h index 5532b3c60f..fdf210f3f6 100644 --- a/modules/congestion_controller/include/receive_side_congestion_controller.h +++ b/modules/congestion_controller/include/receive_side_congestion_controller.h @@ -38,7 +38,7 @@ class ReceiveSideCongestionController : public CallStatsObserver, size_t payload_size, const RTPHeader& header); - void SetSendFeedbackOnRequestOnly(bool send_feedback_on_request_only); + void SetSendPeriodicFeedback(bool send_periodic_feedback); // TODO(nisse): Delete these methods, design a more specific interface. virtual RemoteBitrateEstimator* GetRemoteBitrateEstimator(bool send_side_bwe); virtual const RemoteBitrateEstimator* GetRemoteBitrateEstimator( diff --git a/modules/congestion_controller/receive_side_congestion_controller.cc b/modules/congestion_controller/receive_side_congestion_controller.cc index 1a95f8ce0f..f886b8e019 100644 --- a/modules/congestion_controller/receive_side_congestion_controller.cc +++ b/modules/congestion_controller/receive_side_congestion_controller.cc @@ -139,10 +139,9 @@ void ReceiveSideCongestionController::OnReceivedPacket( } } -void ReceiveSideCongestionController::SetSendFeedbackOnRequestOnly( - bool send_feedback_on_request_only) { - remote_estimator_proxy_.SetSendFeedbackOnRequestOnly( - send_feedback_on_request_only); +void ReceiveSideCongestionController::SetSendPeriodicFeedback( + bool send_periodic_feedback) { + remote_estimator_proxy_.SetSendPeriodicFeedback(send_periodic_feedback); } RemoteBitrateEstimator* diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc index 009f867f13..5220d15f88 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc @@ -43,7 +43,7 @@ RemoteEstimatorProxy::RemoteEstimatorProxy( media_ssrc_(0), feedback_packet_count_(0), send_interval_ms_(kDefaultSendIntervalMs), - send_feedback_on_request_only_(false) {} + send_periodic_feedback_(true) {} RemoteEstimatorProxy::~RemoteEstimatorProxy() {} @@ -69,7 +69,7 @@ bool RemoteEstimatorProxy::LatestEstimate(std::vector* ssrcs, int64_t RemoteEstimatorProxy::TimeUntilNextProcess() { rtc::CritScope cs(&lock_); - if (send_feedback_on_request_only_) { + if (!send_periodic_feedback_) { // Wait a day until next process. return 24 * 60 * 60 * 1000; } else if (last_process_time_ms_ != -1) { @@ -82,7 +82,7 @@ int64_t RemoteEstimatorProxy::TimeUntilNextProcess() { void RemoteEstimatorProxy::Process() { rtc::CritScope cs(&lock_); - if (send_feedback_on_request_only_) { + if (!send_periodic_feedback_) { return; } last_process_time_ms_ = clock_->TimeInMilliseconds(); @@ -109,10 +109,10 @@ void RemoteEstimatorProxy::OnBitrateChanged(int bitrate_bps) { rtc::SafeClamp(0.05 * bitrate_bps, kMinTwccRate, kMaxTwccRate)); } -void RemoteEstimatorProxy::SetSendFeedbackOnRequestOnly( - bool send_feedback_on_request_only) { +void RemoteEstimatorProxy::SetSendPeriodicFeedback( + bool send_periodic_feedback) { rtc::CritScope cs(&lock_); - send_feedback_on_request_only_ = send_feedback_on_request_only; + send_periodic_feedback_ = send_periodic_feedback; } void RemoteEstimatorProxy::OnPacketArrival( @@ -126,12 +126,7 @@ void RemoteEstimatorProxy::OnPacketArrival( int64_t seq = unwrapper_.Unwrap(sequence_number); - 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 (send_periodic_feedback_) { if (periodic_window_start_seq_ && packet_arrival_times_.lower_bound(*periodic_window_start_seq_) == packet_arrival_times_.end()) { @@ -153,6 +148,20 @@ void RemoteEstimatorProxy::OnPacketArrival( packet_arrival_times_[seq] = arrival_time; + // Limit the range of sequence numbers to send feedback for. + auto first_arrival_time_to_keep = packet_arrival_times_.lower_bound( + packet_arrival_times_.rbegin()->first - kMaxNumberOfPackets); + if (first_arrival_time_to_keep != packet_arrival_times_.begin()) { + packet_arrival_times_.erase(packet_arrival_times_.begin(), + first_arrival_time_to_keep); + if (send_periodic_feedback_) { + // |packet_arrival_times_| cannot be empty since we just added one element + // and the last element is not deleted. + RTC_DCHECK(!packet_arrival_times_.empty()); + periodic_window_start_seq_ = packet_arrival_times_.begin()->first; + } + } + if (feedback_request) { // Send feedback packet immediately. SendFeedbackOnRequest(seq, *feedback_request); diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.h b/modules/remote_bitrate_estimator/remote_estimator_proxy.h index d13e1a1123..fa9f7c8e91 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.h +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.h @@ -51,7 +51,7 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { int64_t TimeUntilNextProcess() override; void Process() override; void OnBitrateChanged(int bitrate); - void SetSendFeedbackOnRequestOnly(bool send_feedback_on_request_only); + void SetSendPeriodicFeedback(bool send_periodic_feedback); private: static const int kMaxNumberOfPackets; @@ -86,7 +86,7 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { // Map unwrapped seq -> time. std::map packet_arrival_times_ RTC_GUARDED_BY(&lock_); int64_t send_interval_ms_ RTC_GUARDED_BY(&lock_); - bool send_feedback_on_request_only_ RTC_GUARDED_BY(&lock_); + bool send_periodic_feedback_ RTC_GUARDED_BY(&lock_); }; } // namespace webrtc diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc index ab20d5ad61..e66b9c943c 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc @@ -215,6 +215,60 @@ TEST_F(RemoteEstimatorProxyTest, HandlesReorderingAndWrap) { Process(); } +TEST_F(RemoteEstimatorProxyTest, HandlesMalformedSequenceNumbers) { + // This test generates incoming packets with large jumps in sequence numbers. + // When unwrapped, the sequeunce numbers of these 30 incoming packets, will + // span a range of roughly 650k packets. Test that we only send feedback for + // the last packets. Test for regression found in chromium:949020. + const int64_t kDeltaMs = 1000; + for (int i = 0; i < 10; ++i) { + IncomingPacket(kBaseSeq + i, kBaseTimeMs + 3 * i * kDeltaMs); + IncomingPacket(kBaseSeq + 20000 + i, kBaseTimeMs + (3 * i + 1) * kDeltaMs); + IncomingPacket(kBaseSeq + 40000 + i, kBaseTimeMs + (3 * i + 2) * kDeltaMs); + } + + // Only expect feedback for the last two packets. + EXPECT_CALL(router_, SendTransportFeedback(_)) + .WillOnce(Invoke([&](rtcp::TransportFeedback* feedback_packet) { + EXPECT_EQ(kBaseSeq + 20000 + 9, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq + 20009, kBaseSeq + 40009)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + 28 * kDeltaMs, + kBaseTimeMs + 29 * kDeltaMs)); + return true; + })); + + Process(); +} + +TEST_F(RemoteEstimatorProxyTest, HandlesBackwardsWrappingSequenceNumbers) { + // This test is like HandlesMalformedSequenceNumbers but for negative wrap + // arounds. Test that we only send feedback for the packets with highest + // sequence numbers. Test for regression found in chromium:949020. + const int64_t kDeltaMs = 1000; + for (int i = 0; i < 10; ++i) { + IncomingPacket(kBaseSeq + i, kBaseTimeMs + 3 * i * kDeltaMs); + IncomingPacket(kBaseSeq + 40000 + i, kBaseTimeMs + (3 * i + 1) * kDeltaMs); + IncomingPacket(kBaseSeq + 20000 + i, kBaseTimeMs + (3 * i + 2) * kDeltaMs); + } + + // Only expect feedback for the first two packets. + EXPECT_CALL(router_, SendTransportFeedback(_)) + .WillOnce(Invoke([&](rtcp::TransportFeedback* feedback_packet) { + EXPECT_EQ(kBaseSeq + 40000, feedback_packet->GetBaseSequence()); + EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); + EXPECT_THAT(SequenceNumbers(*feedback_packet), + ElementsAre(kBaseSeq + 40000, kBaseSeq)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + kDeltaMs, kBaseTimeMs)); + return true; + })); + + Process(); +} + TEST_F(RemoteEstimatorProxyTest, ResendsTimestampsOnReordering) { IncomingPacket(kBaseSeq, kBaseTimeMs); IncomingPacket(kBaseSeq + 2, kBaseTimeMs + 2); @@ -348,19 +402,19 @@ TEST_F(RemoteEstimatorProxyTest, TwccReportsUse5PercentOfAvailableBandwidth) { ////////////////////////////////////////////////////////////////////////////// typedef RemoteEstimatorProxyTest RemoteEstimatorProxyOnRequestTest; TEST_F(RemoteEstimatorProxyOnRequestTest, TimeUntilNextProcessIsHigh) { - proxy_.SetSendFeedbackOnRequestOnly(true); + proxy_.SetSendPeriodicFeedback(false); EXPECT_GE(proxy_.TimeUntilNextProcess(), 60 * 60 * 1000); } TEST_F(RemoteEstimatorProxyOnRequestTest, ProcessDoesNotSendFeedback) { - proxy_.SetSendFeedbackOnRequestOnly(true); + proxy_.SetSendPeriodicFeedback(false); IncomingPacket(kBaseSeq, kBaseTimeMs); EXPECT_CALL(router_, SendTransportFeedback(_)).Times(0); Process(); } TEST_F(RemoteEstimatorProxyOnRequestTest, RequestSinglePacketFeedback) { - proxy_.SetSendFeedbackOnRequestOnly(true); + proxy_.SetSendPeriodicFeedback(false); IncomingPacket(kBaseSeq, kBaseTimeMs); IncomingPacket(kBaseSeq + 1, kBaseTimeMs + kMaxSmallDeltaMs); IncomingPacket(kBaseSeq + 2, kBaseTimeMs + 2 * kMaxSmallDeltaMs); @@ -384,7 +438,7 @@ TEST_F(RemoteEstimatorProxyOnRequestTest, RequestSinglePacketFeedback) { } TEST_F(RemoteEstimatorProxyOnRequestTest, RequestLastFivePacketFeedback) { - proxy_.SetSendFeedbackOnRequestOnly(true); + proxy_.SetSendPeriodicFeedback(false); int i = 0; for (; i < 10; ++i) { IncomingPacket(kBaseSeq + i, kBaseTimeMs + i * kMaxSmallDeltaMs); @@ -415,7 +469,7 @@ TEST_F(RemoteEstimatorProxyOnRequestTest, RequestLastFivePacketFeedback) { TEST_F(RemoteEstimatorProxyOnRequestTest, RequestLastFivePacketFeedbackMissingPackets) { - proxy_.SetSendFeedbackOnRequestOnly(true); + proxy_.SetSendPeriodicFeedback(false); int i = 0; for (; i < 10; ++i) { if (i != 7 && i != 9)