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 <philipel@webrtc.org>
Commit-Queue: Johannes Kron <kron@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27417}diff --git a/modules/remote_bitrate_estimator/BUILD.gn b/modules/remote_bitrate_estimator/BUILD.gn
index 055ae68..91e8a77 100644
--- a/modules/remote_bitrate_estimator/BUILD.gn
+++ b/modules/remote_bitrate_estimator/BUILD.gn
@@ -50,6 +50,7 @@
"../../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 70eef90..009f867 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
@@ -42,7 +42,6 @@
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 @@
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 (window_start_seq_ == -1) {
- window_start_seq_ = sequence_number;
- } else if (seq < window_start_seq_) {
- window_start_seq_ = seq;
+ if (!periodic_window_start_seq_ || seq < *periodic_window_start_seq_) {
+ periodic_window_start_seq_ = seq;
+ }
}
// We are only interested in the first time a packet is received.
@@ -174,16 +160,20 @@
}
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 @@
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 73d89c1..d13e1a1 100644
--- a/modules/remote_bitrate_estimator/remote_estimator_proxy.h
+++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.h
@@ -14,9 +14,9 @@
#include <map>
#include <vector>
-#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 @@
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<uint16_t> unwrapper_ RTC_GUARDED_BY(&lock_);
+ absl::optional<int64_t> periodic_window_start_seq_ RTC_GUARDED_BY(&lock_);
// Map unwrapped seq -> time.
std::map<int64_t, int64_t> 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 71b6659..ab20d5a 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 @@
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;
}));
