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

webrtc/modules/bitrate_controller/bitrate_controller_impl.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  *
  */
 
 #include "webrtc/modules/bitrate_controller/bitrate_controller_impl.h"
 
 #include <algorithm>
 #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 {
 
 class BitrateControllerImpl::RtcpBandwidthObserverImpl
     : public RtcpBandwidthObserver {
  public:
   explicit RtcpBandwidthObserverImpl(BitrateControllerImpl* owner)
       : owner_(owner) {
   }
   virtual ~RtcpBandwidthObserverImpl() {
   }
   // Received RTCP REMB or TMMBR.
   void OnReceivedEstimatedBitrate(uint32_t bitrate) override {
     owner_->OnReceivedEstimatedBitrate(bitrate);
   }
   // Received RTCP receiver block.
   void OnReceivedRtcpReceiverReport(const ReportBlockList& report_blocks,
                                     int64_t rtt,
                                     int64_t now_ms) override {
     if (report_blocks.empty())
       return;
 
     int fraction_lost_aggregate = 0;
     int total_number_of_packets = 0;
 
     // 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;
     else
       fraction_lost_aggregate  = (fraction_lost_aggregate +
           total_number_of_packets / 2) / total_number_of_packets;
     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);
   }
 
  private:
   std::map<uint32_t, uint32_t> ssrc_to_last_received_extended_high_seq_num_;
   BitrateControllerImpl* owner_;
 };
 
 BitrateController* BitrateController::CreateBitrateController(
@@ skipping 177 matching lines @@
   bandwidth_estimation_.CurrentEstimate(&bitrate, &fraction_loss, &rtt);
   if (bitrate > 0) {
     bitrate = bitrate - std::min<int>(bitrate, reserved_bitrate_bps_);
     bitrate = std::max(bitrate, bandwidth_estimation_.GetMinBitrate());
     *bandwidth = bitrate;
     return true;
   }
   return false;
 }
 }  // namespace webrtc