Structure of BBR's implementation,some main classes and functions which are going to be used

webrtc/modules/remote_bitrate_estimator/test/estimators/bbr.cc

+/*
+ *  Copyright (c) 2017 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/remote_bitrate_estimator/test/estimators/bbr.h"
+
+#include <time.h>
+#include <algorithm>
+#include <utility>
+
+#include "webrtc/base/logging.h"
+#include "webrtc/modules/congestion_controller/delay_based_bwe.h"
+#include "webrtc/modules/remote_bitrate_estimator/test/bwe_test_logging.h"
+
+namespace webrtc {
+namespace testing {
+namespace bwe {
+namespace {
+const float kHighGain = 2.885f;

[stefan-webrtc, 2017/05/30 09:45:13]

Could you point to a reference which recommends this value, or describe how we
came to conclude it was a good value?

[gnish2, 2017/05/30 15:13:20]

Done.

+const float kDrainGain = 1.f / kHighGain;
+const int kFeedbackIntervals = 100;

[stefan-webrtc, 2017/05/30 09:45:13]

Unit?

[gnish2, 2017/05/30 15:13:20]

Done.

+}
+
+BbrBweSender::BbrBweSender(int kbps, BitrateObserver* observer, Clock* clock)
+    : BweSender(kbps),
+      clock_(clock),
+      observer_(observer),
+      send_time_(),
+      mode_(STARTUP),
+      stats_(),
+      round_trip_end_(0),
+      rt_count_(0),
+      last_packet_sent_(0) {
+  // Initially enter Startup mode.
+  EnterStartup();
+}
+
+BbrBweSender::~BbrBweSender() {}
+
+int BbrBweSender::GetFeedbackIntervalMs() const {
+  return kFeedbackIntervals;
+}
+
+int BbrBweSender::GetPacketSize(uint64_t sequence_number) {

[philipel, 2017/05/30 09:43:38]

Is this function necessary, wont it be trivial when implemented? Why is it
public?

[gnish2, 2017/05/30 15:13:20]

Done.

+  return 0;
+}
+
+void BbrBweSender::GiveFeedback(const FeedbackPacket& feedback) {
+  bool min_rtt_expired = false;

[stefan-webrtc, 2017/05/30 09:45:13]

Move declaration down to where it's used the first time.

[gnish2, 2017/05/30 15:13:20]

Done.

+  int now = clock_->TimeInMilliseconds();
+  int64_t bytes_acked = 0;

[stefan-webrtc, 2017/05/30 09:45:13]

Same with these two

[gnish2, 2017/05/30 15:13:20]

Done.

+  const BbrBweFeedback& fb = static_cast<const BbrBweFeedback&>(feedback);
+  const std::vector<std::pair<uint64_t, int64_t>>& feedback_vector =

[stefan-webrtc, 2017/05/30 09:45:12]

Here I'd probably use const auto&

[gnish2, 2017/05/30 15:13:20]

Done.

+      fb.packet_feedback_vector();
+  // Calculate number of bytes acknowledged after the feedback.

[philipel, 2017/05/30 09:43:38]

For trivial pieces of code comments are not useful, remove this comment.

[gnish2, 2017/05/30 15:13:21]

Done.

+  for (const auto& feedback : feedback_vector) {

[philipel, 2017/05/30 09:43:38]

remove {}

[gnish2, 2017/05/30 15:13:20]

Done.

+    bytes_acked += GetPacketSize(feedback.first);
+  }
+  // Update bandwidth and min_rtt estimates,also

[stefan-webrtc, 2017/05/30 09:45:12]

space after ,

[gnish2, 2017/05/30 15:13:21]

Done.

+  // check if current min_rtt estimate expired or not.

[philipel, 2017/05/30 09:43:38]

Since you choose descriptive names (good! :)) this comment is not necessary
either.

[gnish2, 2017/05/30 15:13:21]

Done.

+  min_rtt_expired = UpdateBandwidthAndMinRtt(now, feedback_vector, bytes_acked);
+  // If it did enter ProbeRtt mode.

[philipel, 2017/05/30 09:43:38]

Remove comment

[stefan-webrtc, 2017/05/30 09:45:13]

I don't understand this comment

[gnish2, 2017/05/30 15:13:20]

Done.

[gnish2, 2017/05/30 15:13:21]

Done.

+  if (min_rtt_expired) {

[philipel, 2017/05/30 09:43:38]

remove {}

[gnish2, 2017/05/30 15:13:20]

Done.

+    EnterProbeRtt(now);
+  }
+  // This switch checks whether it is time or not to exit
+  // the current mode we are in.
+  switch (mode_) {
+    case STARTUP:
+      TryExitingStartup();
+      break;
+    case DRAIN:
+      TryExitingDrain(now);
+      break;
+    case PROBE_BW:
+      TryUpdatingCyclePhase(now);
+      break;
+    case PROBE_RTT:
+      TryExitingProbeRtt(now);
+      break;
+  }
+
+  // Update pacing rate and congestion window values.
+  // TODO(gnish): Implement CalculatePacingRate() and
+  // CalculateCongestionWindow() functions.
+  stats_->CalculatePacingRate();
+  stats_->CalculateCongestionWindow();
+}
+
+bool BbrBweSender::UpdateBandwidthAndMinRtt(
+    int64_t now,
+    const std::vector<std::pair<uint64_t, int64_t>>& feedback_vector,
+    int64_t bytes_acked) {
+  return false;
+}
+
+void BbrBweSender::EnterStartup() {
+  mode_ = STARTUP;
+  stats_->set_pacing_gain(kHighGain);

[stefan-webrtc, 2017/05/30 09:45:12]

What is the purpose of stats_? Seems odd that it's responsible for pacing?

[gnish2, 2017/05/30 15:13:21]

stats_ is responsible to basically store all the data about current connection
state,estimated bandwidth,estimated rtt,the pacing gain which the connection
should use,congestion window which connection should use and etc.It just sets
pacing gain to some value,I believe it will not be responsible for the pacing.

+}
+
+void BbrBweSender::TryExitingStartup() {
+  // If delivery rate hasn't grown
+  // switch to Drain mode.

[stefan-webrtc, 2017/05/30 09:45:13]

Merge these lines.

[gnish2, 2017/05/30 15:13:20]

Done.

+  // TODO(gnish): Implement DeliveryRateGrows()
+  if (!stats_->DeliveryRateGrows()) {
+    mode_ = DRAIN;
+    stats_->set_pacing_gain(kDrainGain);
+  }
+}
+
+void BbrBweSender::TryExitingDrain(int64_t now) {}
+
+void BbrBweSender::EnterProbeBw(int64_t now) {}
+
+void BbrBweSender::TryUpdatingCyclePhase(int64_t now) {}
+
+void BbrBweSender::EnterProbeRtt(int64_t now) {}
+
+void BbrBweSender::TryExitingProbeRtt(int64_t now) {}
+
+int64_t BbrBweSender::TimeUntilNextProcess() {
+  return 100;
+}
+
+void BbrBweSender::OnPacketsSent(const Packets& packets) {}
+
+void BbrBweSender::Process() {}
+
+BbrBweReceiver::BbrBweReceiver(int flow_id)
+    : BweReceiver(flow_id, kReceivingRateTimeWindowMs),
+      last_feedback_ms_(0),
+      clock_(0),
+      packet_feedbacks_() {}
+
+BbrBweReceiver::~BbrBweReceiver() {}
+
+void BbrBweReceiver::ReceivePacket(int64_t arrival_time_ms,
+                                   const MediaPacket& media_packet) {
+  packet_feedbacks_.push_back(

[philipel, 2017/05/30 09:43:38]

Instead of 
packet_feedbacks_.push_back(
       {media_packet.sequence_number(), arrival_time_ms});
use
packet_feedbacks_.emplace_back(
       media_packet.sequence_number(), arrival_time_ms);

[gnish2, 2017/05/30 15:13:21]

Done.

+      {media_packet.sequence_number(), arrival_time_ms});
+  BweReceiver::ReceivePacket(arrival_time_ms, media_packet);
+}
+
+FeedbackPacket* BbrBweReceiver::GetFeedback(int64_t now_ms) {

[philipel, 2017/05/30 09:43:38]

I think this function should be as bare bone as possible since it is not
relevant for this CL.

[gnish2, 2017/05/30 15:13:21]

Done.

+  if (now_ms - last_feedback_ms_ < kFeedbackIntervals)
+    return NULL;

[philipel, 2017/05/30 09:43:38]

NULL --> nullptr

[gnish2, 2017/05/30 15:13:20]

Done.

+  last_feedback_ms_ = now_ms;
+  int64_t corrected_send_time_ms = 0L;
+  if (!received_packets_.empty()) {
+    PacketIdentifierNode* latest = *(received_packets_.begin());

[stefan-webrtc, 2017/05/30 09:45:13]

Remove parentheses.

[gnish2, 2017/05/30 15:13:20]

Done.

+    corrected_send_time_ms =
+        latest->send_time_ms + now_ms - latest->arrival_time_ms;
+  }
+  FeedbackPacket* fb = new BbrBweFeedback(
+      flow_id_, now_ms * 1000, corrected_send_time_ms, packet_feedbacks_);
+  packet_feedbacks_.clear();
+  return fb;
+}
+}  // namespace bwe
+}  // namespace testing
+}  // namespace webrtc