admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix crash which happens when there's reordering in the beginning of a call.

Browse Source 
The added unittest triggers this CHECK:
433ed06800/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}
This commit is contained in:
stefan 2016-07-18 04:14:11 -07:00 committed by Commit bot
parent 433ed06800
commit 159a2fe9da
3 changed files with 35 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc
View File

@ -89,7 +89,18 @@ void RemoteEstimatorProxy::Process() {
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()) {

2
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h
View File

@ -54,7 +54,7 @@ class RemoteEstimatorProxy : public RemoteBitrateEstimator {
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_;

24
webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc
View File

@ -48,7 +48,7 @@ class RemoteEstimatorProxyTest : public ::testing::Test {
}
SimulatedClock clock_;
MockPacketRouter router_;
testing::StrictMock<MockPacketRouter> router_;
RemoteEstimatorProxy proxy_;
const size_t kDefaultPacketSize = 100;
@ -225,6 +225,28 @@ TEST_F(RemoteEstimatorProxyTest, SendsFragmentedFeedback) {
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);