Fix crash which happens when there's reordering in the beginning of a call.
The added unittest triggers this CHECK:
https://chromium.googlesource.com/external/webrtc/+/433ed0680083d4f5315f5ec5e8122ef34901519a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc#146
It happens because the unwrap of the sequence number fails if the unwrappers last sequence number is small, but the newly added sequence number is large (greater than last seq num + 2^15), and therefore should have been interpreted as a reordering and a backwards wrap. Since that would mean the sequence number returned from the unwrapper would be negative, it simply returns the original sequence number instead. This causes problems later where the wrap is correctly handled, and everything breaks.
The real solution should be to correctly handle wraps, but to prevent the crash this is a reasonable workaround for now.
BUG=
Review-Url: https://codereview.webrtc.org/2157843002
Cr-Commit-Position: refs/heads/master@{#13496}
diff --git a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
index 524985c..194ef56 100644
--- a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
+++ b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
@@ -89,7 +89,18 @@
void RemoteEstimatorProxy::OnPacketArrival(uint16_t sequence_number,
int64_t arrival_time) {
+ // 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 (seq > window_start_seq_ + 0xFFFF / 2) {
+ 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 (packet_arrival_times_.lower_bound(window_start_seq_) ==
packet_arrival_times_.end()) {
diff --git a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h
index 66373e2..1278863 100644
--- a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h
+++ b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h
@@ -54,7 +54,7 @@
private:
void OnPacketArrival(uint16_t sequence_number, int64_t arrival_time)
EXCLUSIVE_LOCKS_REQUIRED(&lock_);
- bool BuildFeedbackPacket(rtcp::TransportFeedback* feedback_packetket);
+ bool BuildFeedbackPacket(rtcp::TransportFeedback* feedback_packet);
Clock* const clock_;
PacketRouter* const packet_router_;
diff --git a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
index 62dd2c3..d999525 100644
--- a/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
+++ b/webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
@@ -48,7 +48,7 @@
}
SimulatedClock clock_;
- MockPacketRouter router_;
+ testing::StrictMock<MockPacketRouter> router_;
RemoteEstimatorProxy proxy_;
const size_t kDefaultPacketSize = 100;
@@ -225,6 +225,28 @@
Process();
}
+TEST_F(RemoteEstimatorProxyTest, GracefullyHandlesReorderingAndWrap) {
+ const int64_t kDeltaMs = 1000;
+ const uint16_t kLargeSeq = 62762;
+ IncomingPacket(kBaseSeq, kBaseTimeMs);
+ IncomingPacket(kLargeSeq, kBaseTimeMs + kDeltaMs);
+
+ EXPECT_CALL(router_, SendFeedback(_))
+ .Times(1)
+ .WillOnce(Invoke([this](rtcp::TransportFeedback* packet) {
+ packet->Build();
+ EXPECT_EQ(kBaseSeq, packet->GetBaseSequence());
+ EXPECT_EQ(kMediaSsrc, packet->GetMediaSourceSsrc());
+
+ std::vector<int64_t> delta_vec = packet->GetReceiveDeltasUs();
+ EXPECT_EQ(1u, delta_vec.size());
+ EXPECT_EQ(kBaseTimeMs, (packet->GetBaseTimeUs() + delta_vec[0]) / 1000);
+ return true;
+ }));
+
+ Process();
+}
+
TEST_F(RemoteEstimatorProxyTest, ResendsTimestampsOnReordering) {
IncomingPacket(kBaseSeq, kBaseTimeMs);
IncomingPacket(kBaseSeq + 2, kBaseTimeMs + 2);
