From 9ea46b52864d57d943347b27015f7689f2d00dec Mon Sep 17 00:00:00 2001 From: Stefan Holmer Date: Wed, 15 Mar 2017 12:40:25 +0100 Subject: [PATCH] Ignore packets sent on old network route when receiving feedback. BUG=webrtc:7347 R=philipel@webrtc.org Review-Url: https://codereview.webrtc.org/2755553003 . Cr-Commit-Position: refs/heads/master@{#17243} --- webrtc/call/call.cc | 4 +- .../congestion_controller.cc | 13 +++-- .../congestion_controller_unittest.cc | 17 ++++--- .../include/congestion_controller.h | 8 +-- .../transport_feedback_adapter.cc | 25 +++++++--- .../transport_feedback_adapter.h | 4 +- .../transport_feedback_adapter_unittest.cc | 3 +- .../include/send_time_history.h | 6 +-- .../send_time_history.cc | 18 ++----- .../send_time_history_unittest.cc | 50 +++++++++---------- .../test/estimators/send_side.cc | 9 ++-- .../rtp_rtcp/include/rtp_rtcp_defines.h | 26 ++++++++++ 12 files changed, 109 insertions(+), 74 deletions(-) diff --git a/webrtc/call/call.cc b/webrtc/call/call.cc index bb83b72016..c512d54c73 100644 --- a/webrtc/call/call.cc +++ b/webrtc/call/call.cc @@ -970,8 +970,8 @@ void Call::OnNetworkRouteChanged(const std::string& transport_name, << " bps, max: " << config_.bitrate_config.start_bitrate_bps << " bps."; RTC_DCHECK_GT(config_.bitrate_config.start_bitrate_bps, 0); - congestion_controller_->ResetBweAndBitrates( - config_.bitrate_config.start_bitrate_bps, + congestion_controller_->OnNetworkRouteChanged( + network_route, config_.bitrate_config.start_bitrate_bps, config_.bitrate_config.min_bitrate_bps, config_.bitrate_config.max_bitrate_bps); } diff --git a/webrtc/modules/congestion_controller/congestion_controller.cc b/webrtc/modules/congestion_controller/congestion_controller.cc index 3c349399ce..fdce8da435 100644 --- a/webrtc/modules/congestion_controller/congestion_controller.cc +++ b/webrtc/modules/congestion_controller/congestion_controller.cc @@ -221,9 +221,13 @@ void CongestionController::SetBweBitrates(int min_bitrate_bps, MaybeTriggerOnNetworkChanged(); } -void CongestionController::ResetBweAndBitrates(int bitrate_bps, - int min_bitrate_bps, - int max_bitrate_bps) { +// TODO(holmer): Split this up and use SetBweBitrates in combination with +// OnNetworkRouteChanged. +void CongestionController::OnNetworkRouteChanged( + const rtc::NetworkRoute& network_route, + int bitrate_bps, + int min_bitrate_bps, + int max_bitrate_bps) { ClampBitrates(&bitrate_bps, &min_bitrate_bps, &max_bitrate_bps); // TODO(honghaiz): Recreate this object once the bitrate controller is // no longer exposed outside CongestionController. @@ -235,7 +239,8 @@ void CongestionController::ResetBweAndBitrates(int bitrate_bps, // no longer exposed outside CongestionController. remote_bitrate_estimator_.SetMinBitrate(min_bitrate_bps); - transport_feedback_adapter_.ClearSendTimeHistory(); + transport_feedback_adapter_.SetNetworkIds(network_route.local_network_id, + network_route.remote_network_id); { rtc::CritScope cs(&bwe_lock_); delay_based_bwe_.reset(new DelayBasedBwe(event_log_, clock_)); diff --git a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc index 802f9d1ea4..17d00c86e9 100644 --- a/webrtc/modules/congestion_controller/congestion_controller_unittest.cc +++ b/webrtc/modules/congestion_controller/congestion_controller_unittest.cc @@ -215,12 +215,14 @@ TEST_F(CongestionControllerTest, SignalNetworkState) { controller_->SignalNetworkState(kNetworkDown); } -TEST_F(CongestionControllerTest, ResetBweAndBitrates) { +TEST_F(CongestionControllerTest, OnNetworkRouteChanged) { int new_bitrate = 200000; testing::Mock::VerifyAndClearExpectations(pacer_); EXPECT_CALL(observer_, OnNetworkChanged(new_bitrate, _, _, _)); EXPECT_CALL(*pacer_, SetEstimatedBitrate(new_bitrate)); - controller_->ResetBweAndBitrates(new_bitrate, -1, -1); + rtc::NetworkRoute route; + route.local_network_id = 1; + controller_->OnNetworkRouteChanged(route, new_bitrate, -1, -1); // If the bitrate is reset to -1, the new starting bitrate will be // the minimum default bitrate kMinBitrateBps. @@ -229,7 +231,8 @@ TEST_F(CongestionControllerTest, ResetBweAndBitrates) { OnNetworkChanged(congestion_controller::GetMinBitrateBps(), _, _, _)); EXPECT_CALL(*pacer_, SetEstimatedBitrate(congestion_controller::GetMinBitrateBps())); - controller_->ResetBweAndBitrates(-1, -1, -1); + route.local_network_id = 2; + controller_->OnNetworkRouteChanged(route, -1, -1, -1); } TEST_F(CongestionControllerTest, @@ -316,13 +319,15 @@ TEST_F(CongestionControllerTest, OnReceivedPacketWithAbsSendTime) { EXPECT_EQ(header.ssrc, ssrcs[0]); } -TEST_F(CongestionControllerTest, ProbeOnBweReset) { +TEST_F(CongestionControllerTest, ProbeOnRouteChange) { testing::Mock::VerifyAndClearExpectations(pacer_); EXPECT_CALL(*pacer_, CreateProbeCluster(kInitialBitrateBps * 6)); EXPECT_CALL(*pacer_, CreateProbeCluster(kInitialBitrateBps * 12)); EXPECT_CALL(observer_, OnNetworkChanged(kInitialBitrateBps * 2, _, _, _)); - controller_->ResetBweAndBitrates(2 * kInitialBitrateBps, 0, - 20 * kInitialBitrateBps); + rtc::NetworkRoute route; + route.local_network_id = 1; + controller_->OnNetworkRouteChanged(route, 2 * kInitialBitrateBps, 0, + 20 * kInitialBitrateBps); } // Estimated bitrate reduced when the feedbacks arrive with such a long delay, diff --git a/webrtc/modules/congestion_controller/include/congestion_controller.h b/webrtc/modules/congestion_controller/include/congestion_controller.h index b8ba3b5a2c..18ad17052b 100644 --- a/webrtc/modules/congestion_controller/include/congestion_controller.h +++ b/webrtc/modules/congestion_controller/include/congestion_controller.h @@ -16,6 +16,7 @@ #include "webrtc/base/constructormagic.h" #include "webrtc/base/criticalsection.h" +#include "webrtc/base/networkroute.h" #include "webrtc/base/thread_checker.h" #include "webrtc/common_types.h" #include "webrtc/modules/congestion_controller/delay_based_bwe.h" @@ -81,9 +82,10 @@ class CongestionController : public CallStatsObserver, int max_bitrate_bps); // Resets both the BWE state and the bitrate estimator. Note the first // argument is the bitrate_bps. - virtual void ResetBweAndBitrates(int bitrate_bps, - int min_bitrate_bps, - int max_bitrate_bps); + virtual void OnNetworkRouteChanged(const rtc::NetworkRoute& network_route, + int bitrate_bps, + int min_bitrate_bps, + int max_bitrate_bps); virtual void SignalNetworkState(NetworkState state); virtual void SetTransportOverhead(size_t transport_overhead_bytes_per_packet); diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter.cc b/webrtc/modules/congestion_controller/transport_feedback_adapter.cc index 7780024a86..ddffa72196 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter.cc +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter.cc @@ -32,7 +32,9 @@ TransportFeedbackAdapter::TransportFeedbackAdapter(const Clock* clock) send_time_history_(clock, kSendTimeHistoryWindowMs), clock_(clock), current_offset_ms_(kNoTimestamp), - last_timestamp_us_(kNoTimestamp) {} + last_timestamp_us_(kNoTimestamp), + local_net_id_(0), + remote_net_id_(0) {} TransportFeedbackAdapter::~TransportFeedbackAdapter() {} @@ -43,7 +45,10 @@ void TransportFeedbackAdapter::AddPacket(uint16_t sequence_number, if (send_side_bwe_with_overhead_) { length += transport_overhead_bytes_per_packet_; } - send_time_history_.AddAndRemoveOld(sequence_number, length, pacing_info); + const int64_t creation_time_ms = clock_->TimeInMilliseconds(); + send_time_history_.AddAndRemoveOld( + PacketFeedback(creation_time_ms, sequence_number, length, local_net_id_, + remote_net_id_, pacing_info)); } void TransportFeedbackAdapter::OnSentPacket(uint16_t sequence_number, @@ -58,9 +63,11 @@ void TransportFeedbackAdapter::SetTransportOverhead( transport_overhead_bytes_per_packet_ = transport_overhead_bytes_per_packet; } -void TransportFeedbackAdapter::ClearSendTimeHistory() { +void TransportFeedbackAdapter::SetNetworkIds(uint16_t local_id, + uint16_t remote_id) { rtc::CritScope cs(&lock_); - send_time_history_.Clear(); + local_net_id_ = local_id; + remote_net_id_ = remote_id; } std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( @@ -115,7 +122,10 @@ std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( // as received by another feedback. if (!send_time_history_.GetFeedback(&packet_feedback, false)) ++failed_lookups; - packet_feedback_vector.push_back(packet_feedback); + if (packet_feedback.local_net_id == local_net_id_ && + packet_feedback.remote_net_id == remote_net_id_) { + packet_feedback_vector.push_back(packet_feedback); + } } // Handle this iteration's received packet. @@ -124,7 +134,10 @@ std::vector TransportFeedbackAdapter::GetPacketFeedbackVector( 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); + if (packet_feedback.local_net_id == local_net_id_ && + packet_feedback.remote_net_id == remote_net_id_) { + packet_feedback_vector.push_back(packet_feedback); + } ++seq_num; } diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter.h b/webrtc/modules/congestion_controller/transport_feedback_adapter.h index 628e5ce522..616bebe00a 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter.h +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter.h @@ -43,7 +43,7 @@ class TransportFeedbackAdapter { void SetTransportOverhead(int transport_overhead_bytes_per_packet); - void ClearSendTimeHistory(); + void SetNetworkIds(uint16_t local_id, uint16_t remote_id); private: std::vector GetPacketFeedbackVector( @@ -57,6 +57,8 @@ class TransportFeedbackAdapter { int64_t current_offset_ms_; int64_t last_timestamp_us_; std::vector last_packet_feedback_vector_; + uint16_t local_net_id_; + uint16_t remote_net_id_; }; } // namespace webrtc diff --git a/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc b/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc index bf4dbcae84..22e8b59b95 100644 --- a/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc +++ b/webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc @@ -250,7 +250,8 @@ TEST_F(TransportFeedbackAdapterTest, TimestampDeltas) { rtcp::TransportFeedback::kDeltaScaleFactor * std::numeric_limits::min(); - PacketFeedback packet_feedback(100, 200, 0, 1500, true, PacedPacketInfo()); + PacketFeedback packet_feedback(100, 200, 0, 1500, true, 0, 0, + PacedPacketInfo()); sent_packets.push_back(packet_feedback); packet_feedback.send_time_ms += kSmallDeltaUs / 1000; diff --git a/webrtc/modules/remote_bitrate_estimator/include/send_time_history.h b/webrtc/modules/remote_bitrate_estimator/include/send_time_history.h index d26ea8b657..b3bbcfcb68 100644 --- a/webrtc/modules/remote_bitrate_estimator/include/send_time_history.h +++ b/webrtc/modules/remote_bitrate_estimator/include/send_time_history.h @@ -26,12 +26,8 @@ class SendTimeHistory { SendTimeHistory(const Clock* clock, int64_t packet_age_limit_ms); ~SendTimeHistory(); - void Clear(); - // Cleanup old entries, then add new packet info with provided parameters. - void AddAndRemoveOld(uint16_t sequence_number, - size_t payload_size, - const PacedPacketInfo& pacing_info); + void AddAndRemoveOld(const PacketFeedback& packet); // Updates packet info identified by |sequence_number| with |send_time_ms|. // Return false if not found. diff --git a/webrtc/modules/remote_bitrate_estimator/send_time_history.cc b/webrtc/modules/remote_bitrate_estimator/send_time_history.cc index 1c5ca12f5a..de9246c65d 100644 --- a/webrtc/modules/remote_bitrate_estimator/send_time_history.cc +++ b/webrtc/modules/remote_bitrate_estimator/send_time_history.cc @@ -22,13 +22,7 @@ SendTimeHistory::SendTimeHistory(const Clock* clock, SendTimeHistory::~SendTimeHistory() {} -void SendTimeHistory::Clear() { - history_.clear(); -} - -void SendTimeHistory::AddAndRemoveOld(uint16_t sequence_number, - size_t payload_size, - const PacedPacketInfo& pacing_info) { +void SendTimeHistory::AddAndRemoveOld(const PacketFeedback& packet) { int64_t now_ms = clock_->TimeInMilliseconds(); // Remove old. while (!history_.empty() && @@ -39,14 +33,8 @@ void SendTimeHistory::AddAndRemoveOld(uint16_t sequence_number, } // Add new. - int64_t unwrapped_seq_num = seq_num_unwrapper_.Unwrap(sequence_number); - int64_t creation_time_ms = now_ms; - constexpr int64_t kNoArrivalTimeMs = -1; // Arrival time is ignored. - constexpr int64_t kNoSendTimeMs = -1; // Send time is set by OnSentPacket. - history_.insert(std::make_pair( - unwrapped_seq_num, - PacketFeedback(creation_time_ms, kNoArrivalTimeMs, kNoSendTimeMs, - sequence_number, payload_size, pacing_info))); + int64_t unwrapped_seq_num = seq_num_unwrapper_.Unwrap(packet.sequence_number); + history_.insert(std::make_pair(unwrapped_seq_num, packet)); } bool SendTimeHistory::OnSentPacket(uint16_t sequence_number, diff --git a/webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc b/webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc index 93c7b8f06a..7db1a956bf 100644 --- a/webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc @@ -36,7 +36,9 @@ class SendTimeHistoryTest : public ::testing::Test { size_t length, int64_t send_time_ms, const PacedPacketInfo& pacing_info) { - history_.AddAndRemoveOld(sequence_number, length, pacing_info); + PacketFeedback packet(clock_.TimeInMilliseconds(), sequence_number, length, + 0, 0, pacing_info); + history_.AddAndRemoveOld(packet); history_.OnSentPacket(sequence_number, send_time_ms); } @@ -44,6 +46,22 @@ class SendTimeHistoryTest : public ::testing::Test { SendTimeHistory history_; }; +TEST_F(SendTimeHistoryTest, SaveAndRestoreNetworkId) { + const PacedPacketInfo kPacingInfo(0, 5, 1200); + uint16_t sequence_number = 0; + int64_t now_ms = clock_.TimeInMilliseconds(); + for (int i = 1; i < 5; ++i) { + PacketFeedback packet(now_ms, sequence_number++, 1000, i, i - 1, + kPacingInfo); + history_.AddAndRemoveOld(packet); + history_.OnSentPacket(sequence_number, now_ms); + PacketFeedback restored(now_ms, sequence_number); + EXPECT_TRUE(history_.GetFeedback(&restored, sequence_number)); + EXPECT_EQ(packet.local_net_id, restored.local_net_id); + EXPECT_EQ(packet.remote_net_id, restored.remote_net_id); + } +} + TEST_F(SendTimeHistoryTest, AddRemoveOne) { const uint16_t kSeqNo = 10; // TODO(philipel): Fix PacedPacketInfo constructor? @@ -97,9 +115,10 @@ TEST_F(SendTimeHistoryTest, AddThenRemoveOutOfOrder) { static_cast(i), kPacketSize, PacedPacketInfo())); } for (size_t i = 0; i < num_items; ++i) { - history_.AddAndRemoveOld(sent_packets[i].sequence_number, - sent_packets[i].payload_size, - PacedPacketInfo(1, 2, 200)); + PacketFeedback packet = sent_packets[i]; + packet.arrival_time_ms = -1; + packet.send_time_ms = -1; + history_.AddAndRemoveOld(packet); } for (size_t i = 0; i < num_items; ++i) history_.OnSentPacket(sent_packets[i].sequence_number, @@ -212,28 +231,5 @@ TEST_F(SendTimeHistoryTest, InterlievedGetAndRemove) { EXPECT_TRUE(history_.GetFeedback(&packet3, true)); EXPECT_EQ(packets[2], packet3); } - -TEST_F(SendTimeHistoryTest, Clear) { - const uint16_t kSeqNo = 1; - const int64_t kTimestamp = 2; - const PacedPacketInfo kPacingInfo(0, 5, 1200); - - PacketFeedback packets[] = {{0, kTimestamp, kSeqNo, 0, kPacingInfo}, - {0, kTimestamp + 1, kSeqNo + 1, 0, kPacingInfo}}; - - AddPacketWithSendTime(packets[0].sequence_number, packets[0].payload_size, - packets[0].send_time_ms, kPacingInfo); - AddPacketWithSendTime(packets[1].sequence_number, packets[1].payload_size, - packets[1].send_time_ms, kPacingInfo); - PacketFeedback info(0, 0, packets[0].sequence_number, 0, kPacingInfo); - EXPECT_TRUE(history_.GetFeedback(&info, true)); - EXPECT_EQ(packets[0], info); - - history_.Clear(); - - PacketFeedback info2(0, 0, packets[1].sequence_number, 0, kPacingInfo); - EXPECT_FALSE(history_.GetFeedback(&info2, true)); -} - } // namespace test } // namespace webrtc diff --git a/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc b/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc index 9ad9da5bda..338c4d8e4d 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc @@ -111,9 +111,10 @@ void SendSideBweSender::OnPacketsSent(const Packets& packets) { MediaPacket* media_packet = static_cast(packet); // TODO(philipel): Add probe_cluster_id to Packet class in order // to create tests for probing using cluster ids. - send_time_history_.AddAndRemoveOld(media_packet->header().sequenceNumber, - media_packet->payload_size(), - PacedPacketInfo()); + PacketFeedback packet_feedback( + clock_->TimeInMilliseconds(), media_packet->header().sequenceNumber, + media_packet->payload_size(), 0, 0, PacedPacketInfo()); + send_time_history_.AddAndRemoveOld(packet_feedback); send_time_history_.OnSentPacket(media_packet->header().sequenceNumber, media_packet->sender_timestamp_ms()); } @@ -146,7 +147,7 @@ void SendSideBweReceiver::ReceivePacket(int64_t arrival_time_ms, packet_feedback_vector_.push_back( PacketFeedback(-1, arrival_time_ms, media_packet.sender_timestamp_ms(), media_packet.header().sequenceNumber, - media_packet.payload_size(), PacedPacketInfo())); + media_packet.payload_size(), 0, 0, PacedPacketInfo())); // Log received packet information. BweReceiver::ReceivePacket(arrival_time_ms, media_packet); diff --git a/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 49bdf55479..ab9fc40167 100644 --- a/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -251,6 +251,8 @@ struct PacketFeedback { -1, sequence_number, 0, + 0, + 0, PacedPacketInfo()) {} PacketFeedback(int64_t arrival_time_ms, @@ -263,6 +265,23 @@ struct PacketFeedback { send_time_ms, sequence_number, payload_size, + 0, + 0, + pacing_info) {} + + PacketFeedback(int64_t creation_time_ms, + uint16_t sequence_number, + size_t payload_size, + uint16_t local_net_id, + uint16_t remote_net_id, + const PacedPacketInfo& pacing_info) + : PacketFeedback(creation_time_ms, + -1, + -1, + sequence_number, + payload_size, + local_net_id, + remote_net_id, pacing_info) {} PacketFeedback(int64_t creation_time_ms, @@ -270,12 +289,16 @@ struct PacketFeedback { int64_t send_time_ms, uint16_t sequence_number, size_t payload_size, + uint16_t local_net_id, + uint16_t remote_net_id, const PacedPacketInfo& pacing_info) : creation_time_ms(creation_time_ms), arrival_time_ms(arrival_time_ms), send_time_ms(send_time_ms), sequence_number(sequence_number), payload_size(payload_size), + local_net_id(local_net_id), + remote_net_id(remote_net_id), pacing_info(pacing_info) {} static constexpr int kNotAProbe = -1; @@ -307,6 +330,9 @@ struct PacketFeedback { uint16_t sequence_number; // Size of the packet excluding RTP headers. size_t payload_size; + // The network route ids that this packet is associated with. + uint16_t local_net_id; + uint16_t remote_net_id; // Pacing information about this packet. PacedPacketInfo pacing_info; };