Use pacing info in ProbeBitrateEstimator to validate probe results.

Use pacing info in ProbeBitrateEstimator to validate probe results.

BUG=webrtc:6822
Review-Url: https://codereview.webrtc.org/2728553007
Cr-Commit-Position: refs/heads/master@{#17147}
Committed: https://chromium.googlesource.com/external/webrtc/+/b5feb2e025c99f0a93eeaa1d73bcbc5c06d27b35

[philipel, 2017-03-02 14:32:48]

philipel@webrtc.org changed reviewers:
+ sergeyu@chromium.org, terelius@webrtc.org

[terelius, 2017-03-08 15:55:12]

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/probe_bitrate_estimator.cc (right):

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:73:
static_cast<int>(packet_info.pacing_info.probe_cluster_min_probes *
This might not be rounded correctly. kMinReceivedProbesRatio is not exactly
representable as a float.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:75: int
min_bytes = static_cast<int>(
This might not be rounded correctly. kMinReceivedBytesRatio is not exactly
representable as a float.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc
(right):

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:63:
AddPacketFeedback(0, 1400, 20, 30);
Could we make this test send 5000 bytes to make it clear that we're really
failing the probe count and not the bytes count limit?

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:68:
TEST_F(TestProbeBitrateEstimator, OneClusterTooFewBytes) {
Could we make this send 5 packets to make it clear that we fail the byte count
and not the probe count limit.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:118:
EXPECT_EQ(kInvalidBitrate, measured_bps_);
Is there a reason for switching order here? Or, more generally, do you prefer
the expected value as first of second argument? Currently there is a mix in this
file. (The style recommendation used to be EXPECT_EQ(expected, actual), but that
is no longer required.)

[Sergey Ulanov, 2017-03-08 22:12:20]

lgtm, when my comments are addressed.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/probe_bitrate_estimator.cc (right):

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:19: // The
minumum number of probes we need for a valid cluster.
Update this comment (e.g. "Portion of successful probes in a cluster for a valid
probe.")?

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:20: constexpr
float kMinReceivedProbesRatio = 4.0 / 5.0;
0.8?

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:22: // The
minumum number of bytes we need for a valid cluster.
rephrase this comment as well please. It's a ratio, not number of bytes.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:73:
static_cast<int>(packet_info.pacing_info.probe_cluster_min_probes *
On 2017/03/08 15:55:11, terelius wrote:
> This might not be rounded correctly. kMinReceivedProbesRatio is not exactly
> representable as a float.

maybe use integer percentage value (4/5 = 80%)? Then this calculation can be
done without float.

[philipel, 2017-03-09 11:47:41]

Also needed to make some changes to DelayBasedBweTest, PTAL.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/probe_bitrate_estimator.cc (right):

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:19: // The
minumum number of probes we need for a valid cluster.
On 2017/03/08 22:12:20, Sergey Ulanov wrote:
> Update this comment (e.g. "Portion of successful probes in a cluster for a
valid
> probe.")?

Done.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:20: constexpr
float kMinReceivedProbesRatio = 4.0 / 5.0;
On 2017/03/08 22:12:20, Sergey Ulanov wrote:
> 0.8?

Changed to int instead.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:22: // The
minumum number of bytes we need for a valid cluster.
On 2017/03/08 22:12:20, Sergey Ulanov wrote:
> rephrase this comment as well please. It's a ratio, not number of bytes.

Done.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:73:
static_cast<int>(packet_info.pacing_info.probe_cluster_min_probes *
On 2017/03/08 15:55:11, terelius wrote:
> This might not be rounded correctly. kMinReceivedProbesRatio is not exactly
> representable as a float.

True, switched to int as Sergey suggested.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:73:
static_cast<int>(packet_info.pacing_info.probe_cluster_min_probes *
On 2017/03/08 22:12:20, Sergey Ulanov wrote:
> On 2017/03/08 15:55:11, terelius wrote:
> > This might not be rounded correctly. kMinReceivedProbesRatio is not exactly
> > representable as a float.
> 
> maybe use integer percentage value (4/5 = 80%)? Then this calculation can be
> done without float.

Good idea, implemented!

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc
(right):

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:63:
AddPacketFeedback(0, 1400, 20, 30);
On 2017/03/08 15:55:11, terelius wrote:
> Could we make this test send 5000 bytes to make it clear that we're really
> failing the probe count and not the bytes count limit?

Done.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:68:
TEST_F(TestProbeBitrateEstimator, OneClusterTooFewBytes) {
On 2017/03/08 15:55:11, terelius wrote:
> Could we make this send 5 packets to make it clear that we fail the byte count
> and not the probe count limit.

Done.

https://codereview.webrtc.org/2728553007/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc:118:
EXPECT_EQ(kInvalidBitrate, measured_bps_);
On 2017/03/08 15:55:11, terelius wrote:
> Is there a reason for switching order here? Or, more generally, do you prefer
> the expected value as first of second argument? Currently there is a mix in
this
> file. (The style recommendation used to be EXPECT_EQ(expected, actual), but
that
> is no longer required.)

No reason, fixed :)

[terelius, 2017-03-09 13:42:57]

lgtm % nit

https://codereview.webrtc.org/2728553007/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right):

https://codereview.webrtc.org/2728553007/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:22: constexpr
int kNumProbes0 = 5;
The names are unclear to me. How should I interpret 0 and 1?

[philipel, 2017-03-09 14:13:44]

https://codereview.webrtc.org/2728553007/diff/20001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc (right):

https://codereview.webrtc.org/2728553007/diff/20001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc:22: constexpr
int kNumProbes0 = 5;
On 2017/03/09 13:42:57, terelius wrote:
> The names are unclear to me. How should I interpret 0 and 1?

Change to kNumProbesCluster0.

[philipel, 2017-03-09 14:13:47]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-03-09 14:13:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, terelius@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2728553007/#ps40001 (title:
"kNumProbes --> kNumProbesCluster")

[commit-bot: I haz the power, 2017-03-09 14:13:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-09 15:01:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489068827928000,
"parent_rev": "5bbf43f9d480f89fa57bbdbff0b028a413202695", "commit_rev":
"b5feb2e025c99f0a93eeaa1d73bcbc5c06d27b35"}

[commit-bot: I haz the power, 2017-03-09 15:02:01]

Description was changed from

==========
Use pacing info in ProbeBitrateEstimator to validate probe results.

BUG=webrtc:6822
==========

to

==========
Use pacing info in ProbeBitrateEstimator to validate probe results.

BUG=webrtc:6822

Review-Url: https://codereview.webrtc.org/2728553007
Cr-Commit-Position: refs/heads/master@{#17147}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b5feb2e025c99f0a93eeaa1d7...
==========

[commit-bot: I haz the power, 2017-03-09 15:02:02]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/b5feb2e025c99f0a93eeaa1d7...