Fix issue where the number of packets reported in tests/simulations sometimes are negative.

webrtc/modules/bitrate_controller/bitrate_controller_impl.cc

Index: webrtc/modules/bitrate_controller/bitrate_controller_impl.cc
diff --git a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc
index 8a2464d09c3d5124c8585f6b4996ba141ed12ffd..d283796953b7fc2cf6e171afeb009f09c0d4ff54 100644
--- a/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc
+++ b/webrtc/modules/bitrate_controller/bitrate_controller_impl.cc
@@ -15,6 +15,8 @@
 #include <map>
 #include <utility>
 
+#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<uint32_t, uint32_t>::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) {

[terelius, 2016/08/09 08:31:07]

Can/should this happen, or are we hiding bugs by ignoring bad receiver reports?

[stefan-webrtc, 2016/08/09 08:56:03]

It can happen since a malicious RTCP sender could decide to send us a weird
report.

[terelius, 2016/08/09 11:28:37]

Good point. Is/should there be a unit test for this? (If I understood the rest
of the CL correctly, you changed the tests so that the receiver reports are
sensible, but that also means that this isn't tested.)

[stefan-webrtc, 2016/08/09 11:32:12]

Right. We could add one, but I'm not sure exactly how we would verify that it's
working as intended... Maybe by making sure the BWE isn't affected if we give it
a report with a bogus extendedHighSeqNum and a high fraction lost?

[terelius, 2016/08/10 09:45:12]

Yes, or possibly check that owner_->OnReceivedRtcpReceiverReport() isn't called.
But maybe that is hard to mock?

+      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);
   }