From d48717b45515283e432f4a49abcc050ef5a47085 Mon Sep 17 00:00:00 2001 From: stefan Date: Mon, 22 Aug 2016 08:50:31 -0700 Subject: [PATCH] Fix issue where the number of packets reported in tests/simulations sometimes are negative. BUG=webrtc:6159 Review-Url: https://codereview.webrtc.org/2223033002 Cr-Commit-Position: refs/heads/master@{#13850} --- .../bitrate_controller_impl.cc | 28 +++++++++++++------ .../bitrate_controller_unittest.cc | 4 +-- .../test/estimators/remb.cc | 7 ++--- .../test/estimators/remb.h | 1 + .../test/estimators/send_side.cc | 6 +++- .../test/estimators/send_side.h | 1 + 6 files changed, 31 insertions(+), 16 deletions(-) diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc index 8a2464d09c..d283796953 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc +++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc @@ -15,6 +15,8 @@ #include #include +#include "webrtc/base/checks.h" +#include "webrtc/base/logging.h" #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h" namespace webrtc { @@ -43,22 +45,28 @@ class BitrateControllerImpl::RtcpBandwidthObserverImpl // Compute the a weighted average of the fraction loss from all report // blocks. - for (ReportBlockList::const_iterator it = report_blocks.begin(); - it != report_blocks.end(); ++it) { + for (const RTCPReportBlock& report_block : report_blocks) { std::map::iterator seq_num_it = - ssrc_to_last_received_extended_high_seq_num_.find(it->sourceSSRC); + ssrc_to_last_received_extended_high_seq_num_.find( + report_block.sourceSSRC); int number_of_packets = 0; - if (seq_num_it != ssrc_to_last_received_extended_high_seq_num_.end()) - number_of_packets = it->extendedHighSeqNum - - seq_num_it->second; + if (seq_num_it != ssrc_to_last_received_extended_high_seq_num_.end()) { + number_of_packets = + report_block.extendedHighSeqNum - seq_num_it->second; + } - fraction_lost_aggregate += number_of_packets * it->fractionLost; + fraction_lost_aggregate += number_of_packets * report_block.fractionLost; total_number_of_packets += number_of_packets; // Update last received for this SSRC. - ssrc_to_last_received_extended_high_seq_num_[it->sourceSSRC] = - it->extendedHighSeqNum; + ssrc_to_last_received_extended_high_seq_num_[report_block.sourceSSRC] = + report_block.extendedHighSeqNum; + } + if (total_number_of_packets < 0) { + LOG(LS_WARNING) << "Received report block where extended high sequence " + "number goes backwards, ignoring."; + return; } if (total_number_of_packets == 0) fraction_lost_aggregate = 0; @@ -68,6 +76,8 @@ class BitrateControllerImpl::RtcpBandwidthObserverImpl if (fraction_lost_aggregate > 255) return; + RTC_DCHECK_GE(total_number_of_packets, 0); + owner_->OnReceivedRtcpReceiverReport(fraction_lost_aggregate, rtt, total_number_of_packets, now_ms); } diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc b/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc index b40332e5db..a70eecf241 100644 --- a/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc +++ b/webrtc/modules/bitrate_controller/bitrate_controller_unittest.cc @@ -152,14 +152,14 @@ TEST_F(BitrateControllerTest, OneBitrateObserverOneRtcpObserver) { time_ms += 1000; report_blocks.clear(); - report_blocks.push_back(CreateReportBlock(1, 2, 0, 801)); + report_blocks.push_back(CreateReportBlock(1, 2, 0, 101)); bandwidth_observer_->OnReceivedRtcpReceiverReport(report_blocks, 50, time_ms); EXPECT_EQ(299732, bitrate_observer_.last_bitrate_); time_ms += 1000; // Reach max cap. report_blocks.clear(); - report_blocks.push_back(CreateReportBlock(1, 2, 0, 101)); + report_blocks.push_back(CreateReportBlock(1, 2, 0, 121)); bandwidth_observer_->OnReceivedRtcpReceiverReport(report_blocks, 50, time_ms); EXPECT_EQ(300000, bitrate_observer_.last_bitrate_); time_ms += 1000; diff --git a/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc b/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc index b1f364b451..c12b269099 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.cc @@ -113,11 +113,11 @@ FeedbackPacket* RembReceiver::GetFeedback(int64_t now_ms) { StatisticianMap statisticians = recv_stats_->GetActiveStatisticians(); RTCPReportBlock report_block; if (!statisticians.empty()) { - report_block = BuildReportBlock(statisticians.begin()->second); + latest_report_block_ = BuildReportBlock(statisticians.begin()->second); } feedback = new RembFeedback(flow_id_, now_ms * 1000, last_feedback_ms_, - estimated_bps, report_block); + estimated_bps, latest_report_block_); last_feedback_ms_ = now_ms; double estimated_kbps = static_cast(estimated_bps) / 1000.0; @@ -137,8 +137,7 @@ RTCPReportBlock RembReceiver::BuildReportBlock( StreamStatistician* statistician) { RTCPReportBlock report_block; RtcpStatistics stats; - if (!statistician->GetStatistics(&stats, true)) - return report_block; + RTC_DCHECK(statistician->GetStatistics(&stats, true)); report_block.fractionLost = stats.fraction_lost; report_block.cumulativeLost = stats.cumulative_lost; report_block.extendedHighSeqNum = stats.extended_max_sequence_number; diff --git a/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.h b/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.h index d9e4f571f3..6bf5721d70 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.h +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/remb.h @@ -76,6 +76,7 @@ class RembReceiver : public BweReceiver, public RemoteBitrateObserver { int64_t latest_estimate_bps_; int64_t last_feedback_ms_; std::unique_ptr estimator_; + RTCPReportBlock latest_report_block_; RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(RembReceiver); }; 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 777761a03a..0b9312890f 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc @@ -10,6 +10,8 @@ #include "webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h" +#include + #include "webrtc/base/logging.h" #include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h" #include "webrtc/modules/remote_bitrate_estimator/test/bwe_test_logging.h" @@ -78,8 +80,10 @@ void FullBweSender::GiveFeedback(const FeedbackPacket& feedback) { static_cast(fb.packet_feedback_vector().size()); report_block_.fractionLost = (lost_packets << 8) / expected_packets; report_block_.cumulativeLost += lost_packets; + uint32_t unwrapped = seq_num_unwrapper_.Unwrap( + packet_feedback_vector.back().sequence_number); report_block_.extendedHighSeqNum = - packet_feedback_vector.back().sequence_number; + std::max(unwrapped, report_block_.extendedHighSeqNum); ReportBlockList report_blocks; report_blocks.push_back(report_block_); feedback_observer_->OnReceivedRtcpReceiverReport( diff --git a/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h b/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h index bbe3eac01c..aa7914ef1a 100644 --- a/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h +++ b/webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.h @@ -47,6 +47,7 @@ class FullBweSender : public BweSender, public RemoteBitrateObserver { bool has_received_ack_; uint16_t last_acked_seq_num_; int64_t last_log_time_ms_; + SequenceNumberUnwrapper seq_num_unwrapper_; ::testing::NiceMock event_log_; RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(FullBweSender);