diff --git a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc index 2e0e4a0a94..63d3ed3e42 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.cc @@ -52,37 +52,6 @@ bool IsValid(Timestamp timestamp) { double ToKiloBytes(DataSize datasize) { return datasize.bytes() / 1000.0; } -struct PacketResultsSummary { - int num_packets = 0; - int num_lost_packets = 0; - DataSize total_size = DataSize::Zero(); - DataSize lost_size = DataSize::Zero(); - Timestamp first_send_time = Timestamp::PlusInfinity(); - Timestamp last_send_time = Timestamp::MinusInfinity(); -}; - -// Returns a `PacketResultsSummary` where `first_send_time` is `PlusInfinity, -// and `last_send_time` is `MinusInfinity`, if `packet_results` is empty. -PacketResultsSummary GetPacketResultsSummary( - rtc::ArrayView packet_results) { - PacketResultsSummary packet_results_summary; - - packet_results_summary.num_packets = packet_results.size(); - for (const PacketResult& packet : packet_results) { - if (!packet.IsReceived()) { - packet_results_summary.num_lost_packets++; - packet_results_summary.lost_size += packet.sent_packet.size; - } - packet_results_summary.total_size += packet.sent_packet.size; - packet_results_summary.first_send_time = std::min( - packet_results_summary.first_send_time, packet.sent_packet.send_time); - packet_results_summary.last_send_time = std::max( - packet_results_summary.last_send_time, packet.sent_packet.send_time); - } - - return packet_results_summary; -} - double GetLossProbability(double inherent_loss, DataRate loss_limited_bandwidth, DataRate sending_rate) { @@ -173,7 +142,6 @@ LossBasedBweV2::Result LossBasedBweV2::GetLossBasedResult() const { : DataRate::PlusInfinity(), .state = LossBasedState::kDelayBasedEstimate}; } - return loss_based_result_; } @@ -1140,22 +1108,27 @@ bool LossBasedBweV2::PushBackObservation( return false; } - PacketResultsSummary packet_results_summary = - GetPacketResultsSummary(packet_results); - - partial_observation_.num_packets += packet_results_summary.num_packets; - partial_observation_.num_lost_packets += - packet_results_summary.num_lost_packets; - partial_observation_.size += packet_results_summary.total_size; - partial_observation_.lost_size += packet_results_summary.lost_size; + partial_observation_.num_packets += packet_results.size(); + Timestamp last_send_time = Timestamp::MinusInfinity(); + Timestamp first_send_time = Timestamp::PlusInfinity(); + for (const PacketResult& packet : packet_results) { + if (packet.IsReceived()) { + partial_observation_.lost_packets.erase( + packet.sent_packet.sequence_number); + } else { + partial_observation_.lost_packets.emplace( + packet.sent_packet.sequence_number, packet.sent_packet.size); + } + partial_observation_.size += packet.sent_packet.size; + last_send_time = std::max(last_send_time, packet.sent_packet.send_time); + first_send_time = std::min(first_send_time, packet.sent_packet.send_time); + } // This is the first packet report we have received. if (!IsValid(last_send_time_most_recent_observation_)) { - last_send_time_most_recent_observation_ = - packet_results_summary.first_send_time; + last_send_time_most_recent_observation_ = first_send_time; } - const Timestamp last_send_time = packet_results_summary.last_send_time; const TimeDelta observation_duration = last_send_time - last_send_time_most_recent_observation_; // Too small to be meaningful. @@ -1168,12 +1141,14 @@ bool LossBasedBweV2::PushBackObservation( Observation observation; observation.num_packets = partial_observation_.num_packets; - observation.num_lost_packets = partial_observation_.num_lost_packets; + observation.num_lost_packets = partial_observation_.lost_packets.size(); observation.num_received_packets = observation.num_packets - observation.num_lost_packets; observation.sending_rate = GetSendingRate(partial_observation_.size / observation_duration); - observation.lost_size = partial_observation_.lost_size; + for (auto const& [key, packet_size] : partial_observation_.lost_packets) { + observation.lost_size += packet_size; + } observation.size = partial_observation_.size; observation.id = num_observations_++; observations_[observation.id % config_->observation_window_size] = diff --git a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h index 34c96c66d9..528a2328c1 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2.h @@ -11,6 +11,8 @@ #ifndef MODULES_CONGESTION_CONTROLLER_GOOG_CC_LOSS_BASED_BWE_V2_H_ #define MODULES_CONGESTION_CONTROLLER_GOOG_CC_LOSS_BASED_BWE_V2_H_ +#include +#include #include #include "absl/types/optional.h" @@ -147,9 +149,8 @@ class LossBasedBweV2 { struct PartialObservation { int num_packets = 0; - int num_lost_packets = 0; + std::unordered_map lost_packets; DataSize size = DataSize::Zero(); - DataSize lost_size = DataSize::Zero(); }; struct PaddingInfo { diff --git a/modules/congestion_controller/goog_cc/loss_based_bwe_v2_test.cc b/modules/congestion_controller/goog_cc/loss_based_bwe_v2_test.cc index bb867f4fb0..40c1b4ec25 100644 --- a/modules/congestion_controller/goog_cc/loss_based_bwe_v2_test.cc +++ b/modules/congestion_controller/goog_cc/loss_based_bwe_v2_test.cc @@ -10,6 +10,7 @@ #include "modules/congestion_controller/goog_cc/loss_based_bwe_v2.h" +#include #include #include @@ -91,6 +92,10 @@ class LossBasedBweV2Test : public ::testing::TestWithParam { std::vector CreatePacketResultsWithReceivedPackets( Timestamp first_packet_timestamp) { std::vector enough_feedback(2); + enough_feedback[0].sent_packet.sequence_number = + transport_sequence_number_++; + enough_feedback[1].sent_packet.sequence_number = + transport_sequence_number_++; enough_feedback[0].sent_packet.size = DataSize::Bytes(kPacketSize); enough_feedback[1].sent_packet.size = DataSize::Bytes(kPacketSize); enough_feedback[0].sent_packet.send_time = first_packet_timestamp; @@ -107,8 +112,9 @@ class LossBasedBweV2Test : public ::testing::TestWithParam { Timestamp first_packet_timestamp, DataSize lost_packet_size = DataSize::Bytes(kPacketSize)) { std::vector enough_feedback(10); - enough_feedback[0].sent_packet.size = DataSize::Bytes(kPacketSize); for (unsigned i = 0; i < enough_feedback.size(); ++i) { + enough_feedback[i].sent_packet.sequence_number = + transport_sequence_number_++; enough_feedback[i].sent_packet.size = DataSize::Bytes(kPacketSize); enough_feedback[i].sent_packet.send_time = first_packet_timestamp + @@ -125,6 +131,10 @@ class LossBasedBweV2Test : public ::testing::TestWithParam { std::vector CreatePacketResultsWith50pPacketLossRate( Timestamp first_packet_timestamp) { std::vector enough_feedback(2); + enough_feedback[0].sent_packet.sequence_number = + transport_sequence_number_++; + enough_feedback[1].sent_packet.sequence_number = + transport_sequence_number_++; enough_feedback[0].sent_packet.size = DataSize::Bytes(kPacketSize); enough_feedback[1].sent_packet.size = DataSize::Bytes(kPacketSize); enough_feedback[0].sent_packet.send_time = first_packet_timestamp; @@ -139,6 +149,10 @@ class LossBasedBweV2Test : public ::testing::TestWithParam { std::vector CreatePacketResultsWith100pLossRate( Timestamp first_packet_timestamp) { std::vector enough_feedback(2); + enough_feedback[0].sent_packet.sequence_number = + transport_sequence_number_++; + enough_feedback[1].sent_packet.sequence_number = + transport_sequence_number_++; enough_feedback[0].sent_packet.size = DataSize::Bytes(kPacketSize); enough_feedback[1].sent_packet.size = DataSize::Bytes(kPacketSize); enough_feedback[0].sent_packet.send_time = first_packet_timestamp; @@ -148,6 +162,9 @@ class LossBasedBweV2Test : public ::testing::TestWithParam { enough_feedback[1].receive_time = Timestamp::PlusInfinity(); return enough_feedback; } + + private: + int64_t transport_sequence_number_ = 0; }; TEST_F(LossBasedBweV2Test, EnabledWhenGivenValidConfigurationValues) { @@ -1812,5 +1829,61 @@ TEST_F(LossBasedBweV2Test, PaceAtLossBasedEstimate) { EXPECT_TRUE(loss_based_bandwidth_estimator.PaceAtLossBasedEstimate()); } +TEST_F(LossBasedBweV2Test, + EstimateDoesNotBackOffDueToPacketReorderingBetweenFeedback) { + ExplicitKeyValueConfig key_value_config(ShortObservationConfig("")); + LossBasedBweV2 loss_based_bandwidth_estimator(&key_value_config); + const DataRate kStartBitrate = DataRate::KilobitsPerSec(2500); + loss_based_bandwidth_estimator.SetBandwidthEstimate(kStartBitrate); + + std::vector feedback_1(3); + feedback_1[0].sent_packet.sequence_number = 1; + feedback_1[0].sent_packet.size = DataSize::Bytes(kPacketSize); + feedback_1[0].sent_packet.send_time = Timestamp::Zero(); + feedback_1[0].receive_time = + feedback_1[0].sent_packet.send_time + TimeDelta::Millis(10); + feedback_1[1].sent_packet.sequence_number = 2; + feedback_1[1].sent_packet.size = DataSize::Bytes(kPacketSize); + feedback_1[1].sent_packet.send_time = Timestamp::Zero(); + // Lost or reordered + feedback_1[1].receive_time = Timestamp::PlusInfinity(); + + feedback_1[2].sent_packet.sequence_number = 3; + feedback_1[2].sent_packet.size = DataSize::Bytes(kPacketSize); + feedback_1[2].sent_packet.send_time = Timestamp::Zero(); + feedback_1[2].receive_time = + feedback_1[2].sent_packet.send_time + TimeDelta::Millis(10); + + std::vector feedback_2(3); + feedback_2[0].sent_packet.sequence_number = 2; + feedback_2[0].sent_packet.size = DataSize::Bytes(kPacketSize); + feedback_2[0].sent_packet.send_time = Timestamp::Zero(); + feedback_2[0].receive_time = + feedback_1[0].sent_packet.send_time + TimeDelta::Millis(10); + feedback_2[1].sent_packet.sequence_number = 4; + feedback_2[1].sent_packet.size = DataSize::Bytes(kPacketSize); + feedback_2[1].sent_packet.send_time = + Timestamp::Zero() + kObservationDurationLowerBound; + feedback_2[1].receive_time = + feedback_2[1].sent_packet.send_time + TimeDelta::Millis(10); + feedback_2[2].sent_packet.sequence_number = 5; + feedback_2[2].sent_packet.size = DataSize::Bytes(kPacketSize); + feedback_2[2].sent_packet.send_time = Timestamp::Zero(); + feedback_2[2].receive_time = + feedback_2[2].sent_packet.send_time + TimeDelta::Millis(10); + + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + feedback_1, + /*delay_based_estimate=*/kStartBitrate, + /*in_alr=*/false); + loss_based_bandwidth_estimator.UpdateBandwidthEstimate( + feedback_2, + /*delay_based_estimate=*/kStartBitrate, + /*in_alr=*/false); + EXPECT_EQ( + loss_based_bandwidth_estimator.GetLossBasedResult().bandwidth_estimate, + kStartBitrate); +} + } // namespace } // namespace webrtc diff --git a/modules/congestion_controller/rtp/transport_feedback_adapter.cc b/modules/congestion_controller/rtp/transport_feedback_adapter.cc index e81f579c50..081e016b03 100644 --- a/modules/congestion_controller/rtp/transport_feedback_adapter.cc +++ b/modules/congestion_controller/rtp/transport_feedback_adapter.cc @@ -253,9 +253,10 @@ TransportFeedbackAdapter::ProcessTransportFeedbackInner( }); if (failed_lookups > 0) { - RTC_LOG(LS_WARNING) << "Failed to lookup send time for " << failed_lookups - << " packet" << (failed_lookups > 1 ? "s" : "") - << ". Send time history too small?"; + RTC_LOG(LS_WARNING) + << "Failed to lookup send time for " << failed_lookups << " packet" + << (failed_lookups > 1 ? "s" : "") + << ". Packets reordered or send time history too small?"; } if (ignored > 0) { RTC_LOG(LS_INFO) << "Ignoring " << ignored