From 3c15f468c618eb3e5b632c9978d06cf7bb6892c1 Mon Sep 17 00:00:00 2001 From: Johannes Kron Date: Tue, 2 Apr 2019 15:54:22 +0200 Subject: [PATCH] Use SeqNumUnwapper to handle reordering of first packets Fixed todo by replacing SequenceNumberUnwrapper with updated class SeqNumUnwrapper that correctly handles reordering of early packets. Bug: webrtc:10263 Change-Id: Iffd93db924fee132d35752996b8d29acbb315d24 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/130498 Reviewed-by: Philip Eliasson Commit-Queue: Johannes Kron Cr-Commit-Position: refs/heads/master@{#27417} --- modules/remote_bitrate_estimator/BUILD.gn | 1 + .../remote_estimator_proxy.cc | 66 ++++++++----------- .../remote_estimator_proxy.h | 6 +- .../remote_estimator_proxy_unittest.cc | 9 +-- 4 files changed, 36 insertions(+), 46 deletions(-) diff --git a/modules/remote_bitrate_estimator/BUILD.gn b/modules/remote_bitrate_estimator/BUILD.gn index 055ae68054..91e8a778ee 100644 --- a/modules/remote_bitrate_estimator/BUILD.gn +++ b/modules/remote_bitrate_estimator/BUILD.gn @@ -50,6 +50,7 @@ rtc_static_library("remote_bitrate_estimator") { "../../modules/rtp_rtcp:rtp_rtcp_format", "../../rtc_base:checks", "../../rtc_base:rtc_base_approved", + "../../rtc_base:rtc_numerics", "../../rtc_base:safe_minmax", "../../rtc_base/experiments:field_trial_parser", "../../system_wrappers", diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc index 70eef90f77..009f867f13 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc @@ -42,7 +42,6 @@ RemoteEstimatorProxy::RemoteEstimatorProxy( last_process_time_ms_(-1), media_ssrc_(0), feedback_packet_count_(0), - window_start_seq_(-1), send_interval_ms_(kDefaultSendIntervalMs), send_feedback_on_request_only_(false) {} @@ -125,40 +124,27 @@ void RemoteEstimatorProxy::OnPacketArrival( return; } - // TODO(holmer): We should handle a backwards wrap here if the first - // sequence number was small and the new sequence number is large. The - // SequenceNumberUnwrapper doesn't do this, so we should replace this with - // calls to IsNewerSequenceNumber instead. int64_t seq = unwrapper_.Unwrap(sequence_number); - if (window_start_seq_ != -1 && seq > window_start_seq_ + 0xFFFF / 2) { - RTC_LOG(LS_WARNING) << "Skipping this sequence number (" << sequence_number - << ") since it likely is reordered, but the unwrapper" - "failed to handle it. Feedback window starts at " - << window_start_seq_ << "."; - return; - } 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 && - arrival_time - it->second >= kBackWindowMs;) { - auto delete_it = it; - ++it; - packet_arrival_times_.erase(delete_it); + } else { + if (periodic_window_start_seq_ && + packet_arrival_times_.lower_bound(*periodic_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 && + arrival_time - it->second >= kBackWindowMs;) { + it = packet_arrival_times_.erase(it); + } + } + if (!periodic_window_start_seq_ || seq < *periodic_window_start_seq_) { + periodic_window_start_seq_ = seq; } - } - - if (window_start_seq_ == -1) { - window_start_seq_ = sequence_number; - } else if (seq < window_start_seq_) { - window_start_seq_ = seq; } // We are only interested in the first time a packet is received. @@ -174,16 +160,20 @@ void RemoteEstimatorProxy::OnPacketArrival( } 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. + // |periodic_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. + if (!periodic_window_start_seq_) + return; + for (auto begin_iterator = - packet_arrival_times_.lower_bound(window_start_seq_); + packet_arrival_times_.lower_bound(*periodic_window_start_seq_); begin_iterator != packet_arrival_times_.cend(); - begin_iterator = packet_arrival_times_.lower_bound(window_start_seq_)) { + begin_iterator = + packet_arrival_times_.lower_bound(*periodic_window_start_seq_)) { rtcp::TransportFeedback feedback_packet; - window_start_seq_ = BuildFeedbackPacket( - feedback_packet_count_++, media_ssrc_, window_start_seq_, + periodic_window_start_seq_ = BuildFeedbackPacket( + feedback_packet_count_++, media_ssrc_, *periodic_window_start_seq_, begin_iterator, packet_arrival_times_.cend(), &feedback_packet); RTC_DCHECK(feedback_sender_ != nullptr); @@ -208,11 +198,9 @@ void RemoteEstimatorProxy::SendFeedbackOnRequest( 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); + 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); diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.h b/modules/remote_bitrate_estimator/remote_estimator_proxy.h index 73d89c159b..d13e1a1123 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.h +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.h @@ -14,9 +14,9 @@ #include #include -#include "modules/include/module_common_types.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "rtc_base/critical_section.h" +#include "rtc_base/numerics/sequence_number_util.h" namespace webrtc { @@ -81,8 +81,8 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator { uint32_t media_ssrc_ 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_); + SeqNumUnwrapper unwrapper_ RTC_GUARDED_BY(&lock_); + absl::optional periodic_window_start_seq_ RTC_GUARDED_BY(&lock_); // Map unwrapped seq -> time. std::map packet_arrival_times_ RTC_GUARDED_BY(&lock_); int64_t send_interval_ms_ RTC_GUARDED_BY(&lock_); diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc index 71b6659993..ab20d5ad61 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc @@ -196,18 +196,19 @@ TEST_F(RemoteEstimatorProxyTest, SendsFragmentedFeedback) { Process(); } -TEST_F(RemoteEstimatorProxyTest, GracefullyHandlesReorderingAndWrap) { +TEST_F(RemoteEstimatorProxyTest, HandlesReorderingAndWrap) { const int64_t kDeltaMs = 1000; const uint16_t kLargeSeq = 62762; IncomingPacket(kBaseSeq, kBaseTimeMs); IncomingPacket(kLargeSeq, kBaseTimeMs + kDeltaMs); EXPECT_CALL(router_, SendTransportFeedback(_)) - .WillOnce(Invoke([](rtcp::TransportFeedback* feedback_packet) { - EXPECT_EQ(kBaseSeq, feedback_packet->GetBaseSequence()); + .WillOnce(Invoke([&](rtcp::TransportFeedback* feedback_packet) { + EXPECT_EQ(kLargeSeq, feedback_packet->GetBaseSequence()); EXPECT_EQ(kMediaSsrc, feedback_packet->media_ssrc()); - EXPECT_THAT(TimestampsMs(*feedback_packet), ElementsAre(kBaseTimeMs)); + EXPECT_THAT(TimestampsMs(*feedback_packet), + ElementsAre(kBaseTimeMs + kDeltaMs, kBaseTimeMs)); return true; }));