Propagate packet pacing information to SenTimeHistory

webrtc/modules/pacing/bitrate_prober.h

Index: webrtc/modules/pacing/bitrate_prober.h
diff --git a/webrtc/modules/pacing/bitrate_prober.h b/webrtc/modules/pacing/bitrate_prober.h
index 225ee9e157bed7d3cfe16861ebbe3c59de93e198..0f80e0588020bf6ff6a927047d90d66b8899a8f9 100644
--- a/webrtc/modules/pacing/bitrate_prober.h
+++ b/webrtc/modules/pacing/bitrate_prober.h
@@ -18,12 +18,26 @@
 
 namespace webrtc {
 
+class ProbeClusterCreatedObserver {
+ public:
+  virtual ~ProbeClusterCreatedObserver() {}
+  // Called whenever a probing cluster is created by the BitrateProber.
+  // |cluster_id| = the id of the cluster.

[nisse-webrtc, 2017/01/11 10:47:40]

I think I understand the purpose of this now. The thing is, that the cluster id
is sent over the wire and included in the feedback messages. And this interface
is used to provide information about the probe associated with the cluster id
from the code generating the probe to the code processing the feedback.

I think a comment briefly explaining the signalling flow and the purpose of this
interface would be helpful.

Also consider if the values really are the ones needed by the callee. There are
two very related decisions: How much data to send, and how much data to receive
feedback for before using the probe as a valid measurement. It would make sense
to have the same component be responsible for both decisions.

[philipel, 2017/01/11 12:08:04]

Actually, the cluster id is not sent over the wire, it is saved on the sender
side in the SendTimeHistory. When we receive feedback we use the sequence number
to match the saved history with the incoming feedback.
Adding comments where we make assumptions about how the rest of the code work is
very hard to maintain.
I agree that the two decisions are very related, but I think adding another
class that will be used by both the BitrateProber and the ProbeBitrateEstimator
will just add more complexity to a rather straight forward design. I don't think
it's worth it.

[nisse-webrtc, 2017/01/11 12:33:39]

In that case, I think information about the probe really should be added to
SendTimeHistory.

So you look up the sequence number in the SendTimeHistory, and that should
return *all* information needed to act on the feedback. Is this interface
required to get there? 

I would prefer if you extended this cl to actually store the information in the
right place; without that, the interface looks a bit premature to me and hard to
review properly.

[nisse-webrtc, 2017/01/11 12:33:39]

The class still needs a comment explaining its purpose. If you feel this isn't
the right place for that explanation, consider moving the declaration of the
interface to some other place where the explanation fits better.

[philipel, 2017/01/12 10:39:29]

I think it makes more sense to store this information in the
ProbeBitrateEstimator rather than the SendTimeHistory.

+  // |min_bytes|  = the minimum number of bytes that will be used.
+  // |min_probes| = the minimum number of probing packets that will be used.
+  virtual void OnProbingClusterCreated(int cluster_id,
+                                       int min_bytes,
+                                       int min_probes) = 0;

[nisse-webrtc, 2017/01/11 10:47:40]

I think min_packets is a better name than min_probes.

[philipel, 2017/01/11 12:08:04]

We have used "probes" to refer to packets being parts of a probe cluster
throughout the rest of the code, so if we want to change that then I guess that
should be done in a refactoring CL.

+};
+
 // Note that this class isn't thread-safe by itself and therefore relies
 // on being protected by the caller.
 class BitrateProber {
  public:
   BitrateProber();
 
+  explicit BitrateProber(ProbeClusterCreatedObserver* cluster_created_observer);
+
   void SetEnabled(bool enable);
 
   // Returns true if the prober is in a probing session, i.e., it currently
@@ -100,6 +114,8 @@ class BitrateProber {
   int64_t next_probe_time_ms_;
 
   int next_cluster_id_;
+
+  ProbeClusterCreatedObserver* cluster_created_observer_;
 };
 
 }  // namespace webrtc