From ec304f96b30ef679708971889b2fcab15481fb1b Mon Sep 17 00:00:00 2001 From: "elad.alon" Date: Wed, 8 Mar 2017 05:03:53 -0800 Subject: [PATCH] GetTransportFeedbackVector return vector with lost packets too, sorted by seq-num 1. GetTransportFeedbackVector will now return a vector which also explicitly states lost packets. 2. The returned vector is unsorted (uses default order - by sequence number). It's up to the users to sort otherwise, if they need a different order. BUG=None Review-Url: https://codereview.webrtc.org/2707383006 Cr-Commit-Position: refs/heads/master@{#17114} --- .../congestion_controller/delay_based_bwe.cc | 33 +++++++++- .../transport_feedback_adapter.cc | 42 +++++++----- .../transport_feedback_adapter_unittest.cc | 66 +++++++++++++------ .../rtp_rtcp/include/rtp_rtcp_defines.h | 4 +- webrtc/tools/event_log_visualizer/analyzer.cc | 22 +++++++ 5 files changed, 128 insertions(+), 39 deletions(-) diff --git a/webrtc/modules/congestion_controller/delay_based_bwe.cc b/webrtc/modules/congestion_controller/delay_based_bwe.cc index 43bf592423..b8fa341c22 100644 --- a/webrtc/modules/congestion_controller/delay_based_bwe.cc +++ b/webrtc/modules/congestion_controller/delay_based_bwe.cc @@ -122,6 +122,28 @@ bool ReadMedianSlopeFilterExperimentParameters(size_t* window_size, *threshold_gain = kDefaultMedianSlopeThresholdGain; return false; } + +class PacketFeedbackComparator { + public: + inline bool operator()(const webrtc::PacketFeedback& lhs, + const webrtc::PacketFeedback& 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; + } +}; + +void SortPacketFeedbackVector(const std::vector& input, + std::vector* output) { + auto pred = [](const webrtc::PacketFeedback& packet_feedback) { + return packet_feedback.arrival_time_ms != + webrtc::PacketFeedback::kNotReceived; + }; + std::copy_if(input.begin(), input.end(), std::back_inserter(*output), pred); + std::sort(output->begin(), output->end(), PacketFeedbackComparator()); +} } // namespace namespace webrtc { @@ -255,6 +277,11 @@ DelayBasedBwe::DelayBasedBwe(RtcEventLog* event_log, Clock* clock) DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector( const std::vector& packet_feedback_vector) { RTC_DCHECK(network_thread_.CalledOnValidThread()); + + std::vector sorted_packet_feedback_vector; + SortPacketFeedbackVector(packet_feedback_vector, + &sorted_packet_feedback_vector); + if (!uma_recorded_) { RTC_HISTOGRAM_ENUMERATION(kBweTypeHistogram, BweNames::kSendSideTransportSeqNum, @@ -263,7 +290,7 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector( } Result aggregated_result; bool delayed_feedback = true; - for (const auto& packet_feedback : packet_feedback_vector) { + for (const auto& packet_feedback : sorted_packet_feedback_vector) { if (packet_feedback.send_time_ms < 0) continue; delayed_feedback = false; @@ -277,8 +304,8 @@ DelayBasedBwe::Result DelayBasedBwe::IncomingPacketFeedbackVector( consecutive_delayed_feedbacks_ = 0; } if (consecutive_delayed_feedbacks_ >= kMaxConsecutiveFailedLookups) { - aggregated_result = - OnLongFeedbackDelay(packet_feedback_vector.back().arrival_time_ms); + aggregated_result = OnLongFeedbackDelay( + sorted_packet_feedback_vector.back().arrival_time_ms); consecutive_delayed_feedbacks_ = 0; } return aggregated_result; diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter.cc b/webrtc/modules/congestion_controller/transport_feedback_adapter.cc index 2d173391f2..d158eb97a1 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter.cc +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter.cc @@ -15,6 +15,7 @@ #include "webrtc/base/checks.h" #include "webrtc/base/logging.h" +#include "webrtc/base/mod_ops.h" #include "webrtc/logging/rtc_event_log/rtc_event_log.h" #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h" #include "webrtc/modules/congestion_controller/delay_based_bwe.h" @@ -30,17 +31,6 @@ const int64_t kBaseTimestampScaleFactor = rtcp::TransportFeedback::kDeltaScaleFactor * (1 << 8); const int64_t kBaseTimestampRangeSizeUs = kBaseTimestampScaleFactor * (1 << 24); -class PacketFeedbackComparator { - public: - inline bool operator()(const PacketFeedback& lhs, const PacketFeedback& 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( RtcEventLog* event_log, Clock* clock, @@ -128,26 +118,48 @@ std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( auto received_packets = feedback.GetReceivedPackets(); std::vector packet_feedback_vector; - packet_feedback_vector.reserve(received_packets.size()); if (received_packets.empty()) { LOG(LS_INFO) << "Empty transport feedback packet received."; return packet_feedback_vector; } + const uint16_t last_sequence_number = + received_packets.back().sequence_number(); + const size_t packet_count = + 1 + ForwardDiff(feedback.GetBaseSequence(), last_sequence_number); + packet_feedback_vector.reserve(packet_count); + // feedback.GetStatusVector().size() is a less efficient way to reach what + // should be the same value. + RTC_DCHECK_EQ(packet_count, feedback.GetStatusVector().size()); + { rtc::CritScope cs(&lock_); size_t failed_lookups = 0; int64_t offset_us = 0; int64_t timestamp_ms = 0; - for (const auto& packet : feedback.GetReceivedPackets()) { + uint16_t seq_num = feedback.GetBaseSequence(); + for (const auto& packet : received_packets) { + // Insert into the vector those unreceived packets which precede this + // iteration's received packet. + for (; seq_num != packet.sequence_number(); ++seq_num) { + PacketFeedback packet_feedback(PacketFeedback::kNotReceived, seq_num); + // Note: Element not removed from history because it might be reported + // as received by another feedback. + if (!send_time_history_.GetFeedback(&packet_feedback, false)) + ++failed_lookups; + packet_feedback_vector.push_back(packet_feedback); + } + + // Handle this iteration's received packet. offset_us += packet.delta_us(); timestamp_ms = current_offset_ms_ + (offset_us / 1000); PacketFeedback packet_feedback(timestamp_ms, packet.sequence_number()); if (!send_time_history_.GetFeedback(&packet_feedback, true)) ++failed_lookups; packet_feedback_vector.push_back(packet_feedback); + + ++seq_num; } - std::sort(packet_feedback_vector.begin(), packet_feedback_vector.end(), - PacketFeedbackComparator()); + if (failed_lookups > 0) { LOG(LS_WARNING) << "Failed to lookup send time for " << failed_lookups << " packet" << (failed_lookups > 1 ? "s" : "") diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc b/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc index a41926bbc8..42effebcf9 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc @@ -90,8 +90,11 @@ class TransportFeedbackAdapterTest : public ::testing::Test { int64_t arrival_time_delta = truth[0].arrival_time_ms - input[0].arrival_time_ms; for (size_t i = 0; i < len; ++i) { - EXPECT_EQ(truth[i].arrival_time_ms, - input[i].arrival_time_ms + arrival_time_delta); + RTC_CHECK(truth[i].arrival_time_ms != PacketFeedback::kNotReceived); + if (input[i].arrival_time_ms != PacketFeedback::kNotReceived) { + EXPECT_EQ(truth[i].arrival_time_ms, + input[i].arrival_time_ms + arrival_time_delta); + } EXPECT_EQ(truth[i].send_time_ms, input[i].send_time_ms); EXPECT_EQ(truth[i].sequence_number, input[i].sequence_number); EXPECT_EQ(truth[i].payload_size, input[i].payload_size); @@ -140,6 +143,41 @@ TEST_F(TransportFeedbackAdapterTest, AdaptsFeedbackAndPopulatesSendTimes) { ComparePacketVectors(packets, adapter_->GetTransportFeedbackVector()); } +TEST_F(TransportFeedbackAdapterTest, FeedbackVectorReportsUnreceived) { + std::vector sent_packets = { + PacketFeedback(100, 220, 0, 1500, kPacingInfo0), + PacketFeedback(110, 210, 1, 1500, kPacingInfo0), + PacketFeedback(120, 220, 2, 1500, kPacingInfo0), + PacketFeedback(130, 230, 3, 1500, kPacingInfo0), + PacketFeedback(140, 240, 4, 1500, kPacingInfo0), + PacketFeedback(150, 250, 5, 1500, kPacingInfo0), + PacketFeedback(160, 260, 6, 1500, kPacingInfo0) + }; + + for (const PacketFeedback& packet : sent_packets) + OnSentPacket(packet); + + // Note: Important to include the last packet, as only unreceived packets in + // between received packets can be inferred. + std::vector received_packets = { + sent_packets[0], sent_packets[2], sent_packets[6] + }; + + rtcp::TransportFeedback feedback; + feedback.SetBase(received_packets[0].sequence_number, + received_packets[0].arrival_time_ms * 1000); + + for (const PacketFeedback& packet : received_packets) { + EXPECT_TRUE(feedback.AddReceivedPacket(packet.sequence_number, + packet.arrival_time_ms * 1000)); + } + + feedback.Build(); + + adapter_->OnTransportFeedback(feedback); + ComparePacketVectors(sent_packets, adapter_->GetTransportFeedbackVector()); +} + TEST_F(TransportFeedbackAdapterTest, LongFeedbackDelays) { const int64_t kFeedbackTimeoutMs = 60001; const int kMaxConsecutiveFailedLookups = 5; @@ -305,15 +343,11 @@ TEST_F(TransportFeedbackAdapterTest, SendTimeWrapsBothWays) { } } -TEST_F(TransportFeedbackAdapterTest, HandlesReordering) { +TEST_F(TransportFeedbackAdapterTest, HandlesArrivalReordering) { std::vector packets; packets.push_back(PacketFeedback(120, 200, 0, 1500, kPacingInfo0)); packets.push_back(PacketFeedback(110, 210, 1, 1500, kPacingInfo0)); packets.push_back(PacketFeedback(100, 220, 2, 1500, kPacingInfo0)); - std::vector expected_packets; - expected_packets.push_back(packets[2]); - expected_packets.push_back(packets[1]); - expected_packets.push_back(packets[0]); for (const PacketFeedback& packet : packets) OnSentPacket(packet); @@ -329,9 +363,11 @@ TEST_F(TransportFeedbackAdapterTest, HandlesReordering) { feedback.Build(); + // Adapter keeps the packets ordered by sequence number (which is itself + // assigned by the order of transmission). Reordering by some other criteria, + // eg. arrival time, is up to the observers. adapter_->OnTransportFeedback(feedback); - ComparePacketVectors(expected_packets, - adapter_->GetTransportFeedbackVector()); + ComparePacketVectors(packets, adapter_->GetTransportFeedbackVector()); } TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { @@ -394,17 +430,7 @@ TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { EXPECT_TRUE(feedback.get() != nullptr); adapter_->OnTransportFeedback(*feedback.get()); - { - // 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]); - ComparePacketVectors(expected_packets, - adapter_->GetTransportFeedbackVector()); - } + ComparePacketVectors(sent_packets, adapter_->GetTransportFeedbackVector()); // Create a new feedback message and add the trailing item. feedback.reset(new rtcp::TransportFeedback()); diff --git a/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h index f11949033a..49bdf55479 100644 --- a/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -279,6 +279,7 @@ struct PacketFeedback { pacing_info(pacing_info) {} static constexpr int kNotAProbe = -1; + static constexpr int64_t kNotReceived = -1; // NOTE! The variable |creation_time_ms| is not used when testing equality. // This is due to |creation_time_ms| only being used by SendTimeHistory @@ -295,7 +296,8 @@ struct PacketFeedback { // Time corresponding to when this object was created. int64_t creation_time_ms; // Time corresponding to when the packet was received. Timestamped with the - // receiver's clock. + // receiver's clock. For unreceived packet, the sentinel value kNotReceived + // is used. int64_t arrival_time_ms; // Time corresponding to when the packet was sent, timestamped with the // sender's clock. diff --git a/webrtc/tools/event_log_visualizer/analyzer.cc b/webrtc/tools/event_log_visualizer/analyzer.cc index e29ef32838..15b3e821b8 100644 --- a/webrtc/tools/event_log_visualizer/analyzer.cc +++ b/webrtc/tools/event_log_visualizer/analyzer.cc @@ -43,6 +43,26 @@ namespace plotting { namespace { +class PacketFeedbackComparator { + public: + inline bool operator()(const webrtc::PacketFeedback& lhs, + const webrtc::PacketFeedback& 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; + } +}; + +void SortPacketFeedbackVector(std::vector* vec) { + auto pred = [](const PacketFeedback& packet_feedback) { + return packet_feedback.arrival_time_ms == PacketFeedback::kNotReceived; + }; + vec->erase(std::remove_if(vec->begin(), vec->end(), pred), vec->end()); + std::sort(vec->begin(), vec->end(), PacketFeedbackComparator()); +} + std::string SsrcToString(uint32_t ssrc) { std::stringstream ss; ss << "SSRC " << ssrc; @@ -1057,6 +1077,7 @@ void EventLogAnalyzer::CreateBweSimulationGraph(Plot* plot) { rtcp.packet.get())); std::vector feedback = observer->GetTransportFeedbackVector(); + SortPacketFeedbackVector(&feedback); rtc::Optional bitrate_bps; if (!feedback.empty()) { for (const PacketFeedback& packet : feedback) @@ -1192,6 +1213,7 @@ void EventLogAnalyzer::CreateNetworkDelayFeedbackGraph(Plot* plot) { *static_cast(rtcp.packet.get())); std::vector feedback = feedback_adapter.GetTransportFeedbackVector(); + SortPacketFeedbackVector(&feedback); for (const PacketFeedback& packet : feedback) { int64_t y = packet.arrival_time_ms - packet.send_time_ms; float x =