From daeb892a6472db9b591a4d64f02589b44a3f4d8b Mon Sep 17 00:00:00 2001 From: stefan Date: Wed, 27 Apr 2016 09:49:31 -0700 Subject: [PATCH] Reland: Deliver reordered packets in arrival time order after parsing feedback message. This is what the remote bitrate estimator expects. BUG=5823 R=sprang@webrtc.org Review-Url: https://codereview.webrtc.org/1925643002 Cr-Commit-Position: refs/heads/master@{#12535} --- .../transport_feedback_adapter.cc | 14 ++++++ .../transport_feedback_adapter_unittest.cc | 49 +++++++++++++++++-- 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc b/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc index f7e07a5dc5..ed8b6e6d3a 100644 --- a/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc +++ b/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.cc @@ -10,6 +10,7 @@ #include "webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter.h" +#include #include #include "webrtc/base/checks.h" @@ -26,6 +27,17 @@ const int64_t kBaseTimestampScaleFactor = rtcp::TransportFeedback::kDeltaScaleFactor * (1 << 8); const int64_t kBaseTimestampRangeSizeUs = kBaseTimestampScaleFactor * (1 << 24); +class PacketInfoComparator { + public: + inline bool operator()(const PacketInfo& lhs, const PacketInfo& rhs) { + if (lhs.arrival_time_ms != rhs.arrival_time_ms) + return lhs.arrival_time_ms < rhs.arrival_time_ms; + if (lhs.send_time_ms != rhs.send_time_ms) + return lhs.send_time_ms < rhs.send_time_ms; + return lhs.sequence_number < rhs.sequence_number; + } +}; + TransportFeedbackAdapter::TransportFeedbackAdapter( BitrateController* bitrate_controller, Clock* clock) @@ -104,6 +116,8 @@ void TransportFeedbackAdapter::OnTransportFeedback( } ++sequence_number; } + std::sort(packet_feedback_vector.begin(), packet_feedback_vector.end(), + PacketInfoComparator()); RTC_DCHECK(delta_it == delta_vec.end()); if (failed_lookups > 0) { LOG(LS_WARNING) << "Failed to lookup send time for " << failed_lookups diff --git a/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc b/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc index 0111571262..4f7d83c9eb 100644 --- a/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/transport_feedback_adapter_unittest.cc @@ -223,6 +223,39 @@ TEST_F(TransportFeedbackAdapterTest, SendTimeWrapsBothWays) { } } +TEST_F(TransportFeedbackAdapterTest, HandlesReordering) { + std::vector packets; + packets.push_back(PacketInfo(120, 200, 0, 1500, true)); + packets.push_back(PacketInfo(110, 210, 1, 1500, true)); + packets.push_back(PacketInfo(100, 220, 2, 1500, true)); + std::vector expected_packets; + expected_packets.push_back(packets[2]); + expected_packets.push_back(packets[1]); + expected_packets.push_back(packets[0]); + + for (const PacketInfo& packet : packets) + OnSentPacket(packet); + + rtcp::TransportFeedback feedback; + feedback.WithBase(packets[0].sequence_number, + packets[0].arrival_time_ms * 1000); + + for (const PacketInfo& packet : packets) { + EXPECT_TRUE(feedback.WithReceivedPacket(packet.sequence_number, + packet.arrival_time_ms * 1000)); + } + + feedback.Build(); + + EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) + .Times(1) + .WillOnce(Invoke([expected_packets, + this](const std::vector& feedback_vector) { + ComparePacketVectors(expected_packets, feedback_vector); + })); + adapter_->OnTransportFeedback(feedback); +} + TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { std::vector sent_packets; const int64_t kSmallDeltaUs = @@ -257,6 +290,14 @@ TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { info.arrival_time_ms += (kLargePositiveDeltaUs + 1000) / 1000; ++info.sequence_number; + // Expected to be ordered on arrival time when the feedback message has been + // parsed. + std::vector expected_packets; + expected_packets.push_back(sent_packets[0]); + expected_packets.push_back(sent_packets[3]); + expected_packets.push_back(sent_packets[1]); + expected_packets.push_back(sent_packets[2]); + // Packets will be added to send history. for (const PacketInfo& packet : sent_packets) OnSentPacket(packet); @@ -284,9 +325,9 @@ TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { EXPECT_TRUE(feedback.get() != nullptr); EXPECT_CALL(*bitrate_estimator_, IncomingPacketFeedbackVector(_)) .Times(1) - .WillOnce(Invoke([sent_packets, &received_feedback]( + .WillOnce(Invoke([expected_packets, &received_feedback]( const std::vector& feedback_vector) { - EXPECT_EQ(sent_packets.size(), feedback_vector.size()); + EXPECT_EQ(expected_packets.size(), feedback_vector.size()); received_feedback = feedback_vector; })); adapter_->OnTransportFeedback(*feedback.get()); @@ -310,9 +351,9 @@ TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { })); adapter_->OnTransportFeedback(*feedback.get()); - sent_packets.push_back(info); + expected_packets.push_back(info); - ComparePacketVectors(sent_packets, received_feedback); + ComparePacketVectors(expected_packets, received_feedback); } } // namespace test