From 870eee4b17f3e9d065db7e6cfff58c816698f627 Mon Sep 17 00:00:00 2001 From: stefan Date: Tue, 14 Jul 2015 03:53:57 -0700 Subject: [PATCH] Fix simulator issue where chokes didn't apply to non-congested packets. Review URL: https://codereview.webrtc.org/1235143002 Cr-Commit-Position: refs/heads/master@{#9575} --- webrtc/modules/modules.gyp | 2 + .../remote_bitrate_estimator.gypi | 2 - .../remote_bitrate_estimator/test/bwe.cc | 6 +++ .../remote_bitrate_estimator/test/bwe.h | 1 + .../test/bwe_test_framework.cc | 6 +-- .../test/bwe_test_framework_unittest.cc | 40 ++++++++++++++++--- .../test/estimators/nada.cc | 9 +++-- .../test/estimators/nada.h | 10 ++--- 8 files changed, 56 insertions(+), 20 deletions(-) diff --git a/webrtc/modules/modules.gyp b/webrtc/modules/modules.gyp index 7f3a1e809e..3ecf759c45 100644 --- a/webrtc/modules/modules.gyp +++ b/webrtc/modules/modules.gyp @@ -223,6 +223,8 @@ 'remote_bitrate_estimator/remote_bitrate_estimator_single_stream_unittest.cc', 'remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.cc', 'remote_bitrate_estimator/remote_bitrate_estimator_unittest_helper.h', + 'remote_bitrate_estimator/test/bwe_test_framework_unittest.cc', + 'remote_bitrate_estimator/test/estimators/nada_unittest.cc', 'rtp_rtcp/source/mock/mock_rtp_payload_strategy.h', 'rtp_rtcp/source/byte_io_unittest.cc', 'rtp_rtcp/source/fec_receiver_unittest.cc', diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi index e4b21db375..b313d435e5 100644 --- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi +++ b/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator.gypi @@ -70,7 +70,6 @@ 'test/bwe_test_fileutils.h', 'test/bwe_test_framework.cc', 'test/bwe_test_framework.h', - 'test/bwe_test_framework_unittest.cc', 'test/bwe_test_logging.cc', 'test/bwe_test_logging.h', 'test/packet_receiver.cc', @@ -82,7 +81,6 @@ 'test/random.h', 'test/estimators/nada.cc', 'test/estimators/nada.h', - 'test/estimators/nada_unittest.cc', 'test/estimators/remb.cc', 'test/estimators/remb.h', 'test/estimators/send_side.cc', diff --git a/webrtc/modules/remote_bitrate_estimator/test/bwe.cc b/webrtc/modules/remote_bitrate_estimator/test/bwe.cc index 6e8dbc7c06..5385b9bed8 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/bwe.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/bwe.cc @@ -157,6 +157,11 @@ float BweReceiver::RecentPacketLossRatio() { return static_cast(gap - number_packets_received) / gap; } +LinkedSet::~LinkedSet() { + while (!empty()) + RemoveTail(); +} + void LinkedSet::Insert(uint16_t sequence_number, int64_t send_time_ms, int64_t arrival_time_ms, @@ -181,6 +186,7 @@ void LinkedSet::Insert(uint16_t sequence_number, } void LinkedSet::RemoveTail() { map_.erase(list_.back()->sequence_number); + delete list_.back(); list_.pop_back(); } void LinkedSet::UpdateHead(PacketIdentifierNode* new_head) { diff --git a/webrtc/modules/remote_bitrate_estimator/test/bwe.h b/webrtc/modules/remote_bitrate_estimator/test/bwe.h index 6aa79cac2a..d86331aad7 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/bwe.h +++ b/webrtc/modules/remote_bitrate_estimator/test/bwe.h @@ -46,6 +46,7 @@ typedef std::list::iterator PacketNodeIt; class LinkedSet { public: explicit LinkedSet(int capacity) : capacity_(capacity) {} + ~LinkedSet(); // If the arriving packet (identified by its sequence number) is already // in the LinkedSet, move its Node to the head of the list. Else, create diff --git a/webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc b/webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc index 5e66697215..cac59ae0ad 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework.cc @@ -431,10 +431,10 @@ void ChokeFilter::RunFor(int64_t /*time_ms*/, Packets* in_out) { assert(in_out); for (PacketsIt it = in_out->begin(); it != in_out->end(); ) { int64_t earliest_send_time_us = - last_send_time_us_ + - ((*it)->payload_size() * 8 * 1000 + kbps_ / 2) / kbps_; + std::max(last_send_time_us_, (*it)->send_time_us()); int64_t new_send_time_us = - std::max((*it)->send_time_us(), earliest_send_time_us); + earliest_send_time_us + + ((*it)->payload_size() * 8 * 1000 + kbps_ / 2) / kbps_; if (delay_cap_helper_->ShouldSendPacket(new_send_time_us, (*it)->send_time_us())) { (*it)->set_send_time_us(new_send_time_us); diff --git a/webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc b/webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc index dd7bbfcfeb..01cf7cbe3d 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/bwe_test_framework_unittest.cc @@ -657,13 +657,43 @@ class BweTestFramework_ChokeFilterTest : public ::testing::Test { private: int64_t now_ms_; - uint32_t sequence_number_; + uint16_t sequence_number_; Packets output_packets_; std::vector send_times_us_; DISALLOW_COPY_AND_ASSIGN(BweTestFramework_ChokeFilterTest); }; +TEST_F(BweTestFramework_ChokeFilterTest, NoQueue) { + const int kCapacityKbps = 10; + const size_t kPacketSizeBytes = 125; + const int64_t kExpectedSendTimeUs = + (kPacketSizeBytes * 8 * 1000 + kCapacityKbps / 2) / kCapacityKbps; + uint16_t sequence_number = 0; + int64_t send_time_us = 0; + ChokeFilter filter(NULL, 0); + filter.SetCapacity(10); + Packets packets; + RTPHeader header; + for (int i = 0; i < 2; ++i) { + header.sequenceNumber = sequence_number++; + // Payload is 1000 bits. + packets.push_back( + new MediaPacket(0, send_time_us, kPacketSizeBytes, header)); + // Packets are sent far enough a part plus an extra millisecond so that they + // will never be in the choke queue at the same time. + send_time_us += kExpectedSendTimeUs + 1000; + } + ASSERT_TRUE(IsTimeSorted(packets)); + filter.RunFor(2 * kExpectedSendTimeUs + 1000, &packets); + EXPECT_EQ(kExpectedSendTimeUs, packets.front()->send_time_us()); + delete packets.front(); + packets.pop_front(); + EXPECT_EQ(2 * kExpectedSendTimeUs + 1000, packets.front()->send_time_us()); + delete packets.front(); + packets.pop_front(); +} + TEST_F(BweTestFramework_ChokeFilterTest, Short) { // 100ms, 100 packets, 10 kbps choke -> 1 kbit of data should have propagated. // That is actually just a single packet, since each packet has 1000 bits of @@ -723,8 +753,8 @@ TEST_F(BweTestFramework_ChokeFilterTest, MaxDelay) { // 100 ms delay cap filter.SetMaxDelay(100); - // 10100ms, 50 more packets -> 2 packets or 2 kbit through. - TestChoke(&filter, 100, 50, 2); + // 10100ms, 50 more packets -> 1 packets or 1 kbit through. + TestChoke(&filter, 100, 50, 1); CheckMaxDelay(100); // 20000ms, no input, remaining packets in queue should have been dropped. TestChoke(&filter, 9900, 0, 0); @@ -732,8 +762,8 @@ TEST_F(BweTestFramework_ChokeFilterTest, MaxDelay) { // Reset delay cap (0 is no cap) and verify no packets are dropped. filter.SetCapacity(10); filter.SetMaxDelay(0); - TestChoke(&filter, 100, 100, 2); - TestChoke(&filter, 9900, 0, 98); + TestChoke(&filter, 100, 100, 1); + TestChoke(&filter, 9900, 0, 99); } TEST_F(BweTestFramework_ChokeFilterTest, ShortTrace) { diff --git a/webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc b/webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc index c28749d1d3..ccbc522b7e 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/nada.cc @@ -19,6 +19,7 @@ #include #include +#include "webrtc/base/common.h" #include "webrtc/modules/remote_bitrate_estimator/test/estimators/nada.h" #include "webrtc/modules/remote_bitrate_estimator/test/bwe_test_logging.h" #include "webrtc/modules/rtp_rtcp/interface/receive_statistics.h" @@ -27,10 +28,9 @@ namespace webrtc { namespace testing { namespace bwe { -const int NadaBweReceiver::kMedian; -const int NadaBweSender::kMinRefRateKbps; -const int NadaBweSender::kMaxRefRateKbps; -const int64_t NadaBweReceiver::kReceivingRateTimeWindowMs; +const int NadaBweSender::kMinRefRateKbps = 150; +const int NadaBweSender::kMaxRefRateKbps = 1500; +const int64_t NadaBweReceiver::kReceivingRateTimeWindowMs = 500; NadaBweReceiver::NadaBweReceiver(int flow_id) : BweReceiver(flow_id), @@ -64,6 +64,7 @@ void NadaBweReceiver::ReceivePacket(int64_t arrival_time_ms, baseline_delay_ms_ = std::min(baseline_delay_ms_, delay_ms); } delay_signal_ms_ = delay_ms - baseline_delay_ms_; // Refered as d_n. + const int kMedian = ARRAY_SIZE(last_delays_ms_); last_delays_ms_[(last_delays_index_++) % kMedian] = delay_signal_ms_; int size = std::min(last_delays_index_, kMedian); int64_t median_filtered_delay_ms_ = MedianFilter(last_delays_ms_, size); diff --git a/webrtc/modules/remote_bitrate_estimator/test/estimators/nada.h b/webrtc/modules/remote_bitrate_estimator/test/estimators/nada.h index 6fea624035..75dc94d0a9 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/nada.h +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/nada.h @@ -46,7 +46,7 @@ class NadaBweReceiver : public BweReceiver { int64_t last_smoothed_value, float alpha); - static const int64_t kReceivingRateTimeWindowMs = 500; + static const int64_t kReceivingRateTimeWindowMs; private: SimulatedClock clock_; @@ -58,9 +58,7 @@ class NadaBweReceiver : public BweReceiver { int last_delays_index_; int64_t exp_smoothed_delay_ms_; // Referred as d_hat_n. int64_t est_queuing_delay_signal_ms_; // Referred as d_tilde_n. - - static const int kMedian = 5; // Used for k-points Median Filter. - int64_t last_delays_ms_[kMedian]; // Used for Median Filter. + int64_t last_delays_ms_[5]; // Used for Median Filter. }; class NadaBweSender : public BweSender { @@ -89,8 +87,8 @@ class NadaBweSender : public BweSender { } int64_t NowMs() const { return clock_->TimeInMilliseconds(); } - static const int kMinRefRateKbps = 150; // Referred as R_min. - static const int kMaxRefRateKbps = 1500; // Referred as R_max. + static const int kMinRefRateKbps; // Referred as R_min. + static const int kMaxRefRateKbps; // Referred as R_max. private: Clock* const clock_;